Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v11 03/10] net: ti: icssg-prueth: Add Firmware config and classification APIs.

[Md Danish Anwar]

On 25/07/23 1:14 pm, Simon Horman wrote:
> On Tue, Jul 25, 2023 at 01:10:30PM +0530, Md Danish Anwar wrote:
>> Hi Simon,
>>
>> On 25/07/23 12:55 pm, Simon Horman wrote:
>>> On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote:
>>>> Add icssg_config.h / .c and icssg_classifier.c files. These are firmware
>>>> configuration and classification related files. These will be used by
>>>> ICSSG ethernet driver.
>>>>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
>>>> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
>>>
>>> Hi Danish,
>>>
>>> some feedback from my side.
>>>
>>
>> Thanks for the feedback.
>>
>>> ...
>>>
>>>> diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c
>>>
>>> ...
>>>
>>>> +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac)
>>>
>>> This function appears to be unused.
>>> Perhaps it would be better placed in a later patch?
>>>
>>> Or perhaps not, if it makes it hard to split up the patches nicely.
>>> In which case, perhaps the __maybe_unused annotation could be added,
>>> temporarily.
>>>
>>
>> Due to splitting the patch into 8-9 patches, I had to introduce these helper
>> APIs earlier. All these APIs are helper APIs, they will be used in patch 6
>> (Introduce ICSSG Prueth driver).
>>
>> I had this concern that some APIs which will be used later but introduced
>> earlier can create some warnings, before splitting the patches.
>>
>> I had raised this concern in [1] and asked Jakub if it would be OK to introduce
>> these APIs earlier. Jakub said it would be fine [2], so I went ahead with this
>> approach.
>>
>> It will make very hard to break patches if these APIs are introduced and used
>> in same patch.
> 
> Thanks, I understand.
> 
> In that case my suggestion is to, temporarily, add __maybe_unused,
> which will allow static analysis tools to work more cleanly over the
> series. It is just a suggestion, not a hard requirement.
> 
> Probably something along those lines applies to all the
> review I provided in my previous email. Please use your discretion here.

For now I think I will leave it as it is. Let reviewers review all other
patches. Let's see if there are any other comments on all the patches in this
series. If there are any more comments on other patches, then while re-spinning
next revision I will keep this in mind and try to add __maybe_unused tags in
all APIs that are used later.

The idea behind splitting the patches was to get them reviewed individually as
it is quite difficult to get one big patch reviewed as explained by Jakub. And
these warnings were expected. If there are any other comments on this series, I
will try to address all of them together in next revision.

Meanwhile, Please let me know if you have any comments on other patches in this
series.

-- 
Thanks and Regards,
Danish.