Re: [PATCH] net/nfp: improve readability NFP HWINFO header

[Ferruh Yigit <ferruh.yigit@xilinx.com>]

On 9/20/2022 6:51 PM, Niklas Söderlund wrote:
> Hi Ferruh,
> 
> Thanks for your feedback.
> 
> On 2022-09-20 18:33:02 +0100, Ferruh Yigit wrote:
>> On 8/26/2022 6:39 AM, Chaoyong He wrote:
>>> From: James Hershaw <james.hershaw@corigine.com>
>>>
>>> Prepend `0x` to the NFP HWINFO header value that is printed to improve
>>> the readability of the printed statement.
>>>
>>> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>> ---
>>>    drivers/net/nfp/nfpcore/nfp_hwinfo.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/nfp/nfpcore/nfp_hwinfo.c b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
>>> index c0516bf..9f848bd 100644
>>> --- a/drivers/net/nfp/nfpcore/nfp_hwinfo.c
>>> +++ b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
>>> @@ -108,7 +108,7 @@
>>>    		goto exit_free;
>>>    	header = (void *)db;
>>> -	printf("NFP HWINFO header: %08x\n", *(uint32_t *)header);
>>> +	printf("NFP HWINFO header: %#08x\n", *(uint32_t *)header);
>>
>> Why driver is directly using 'printf', but not rte_log APIs?
>>
>> I can see there are already 'PMD_INIT_LOG' & 'PMD_DRV_LOG' macros for this.
> 
> We have a series ready to convert all printf style logging into rte_log
> APIs as well as fix some other style issues.
> 
> We also have a few other things in our internal patch queue waiting to
> be sent out. To reduce conflicts in patchwork we are sending them out in
> the order as some of them depends on each other. And the one cleaning up
> log messages are at the end of the pile unfortunately.
> 
> Do you think it's acceptable to take this fix as-is and then a patch
> that convert all printf on one go, or would you prefers we move touch
> this line only once and create a v2 of this fix while also moving it to
> the rte_log APIs?
> 

Hi Niklas,

Good to hear that you have patch to convert them to rte_log.

Instead of changing the log content and API with same patch, it is 
better to have them separate.

I prefer to convert them to proper log API first, and later fix the 
content of the log (to not update a line with wrong call).
But order of patch preference is a soft one, if somehow other-way around 
(first fix the log content, later the API) makes your life easier, I am 
OK to go with that too (as long as both issues are fixed).