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

[Simon Horman <simon.horman@corigine.com>]

On Tue, Jul 25, 2023 at 01:28:21PM +0530, Md Danish Anwar wrote:
> 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@ti.com>
> >>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> >>>
> >>> 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.

Sure, that sounds reasonable.

> 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.

Yes, I understand.
Thanks for splitting things up into multiple patches.
I know that is a lot of work. But it is very helpful.

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

Will do, but I nothing to add at this time.