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 <[email protected]>
> >> Reviewed-by: Andrew Lunn <[email protected]>
> >
> > 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.
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 <[email protected]>
>>>> Reviewed-by: Andrew Lunn <[email protected]>
>>>
>>> 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.
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 <[email protected]>
> >>>> Reviewed-by: Andrew Lunn <[email protected]>
> >>>
> >>> 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.
On Wed, 26 Jul 2023 09:42:17 +0200 Simon Horman wrote:
> Thanks for splitting things up into multiple patches.
> I know that is a lot of work. But it is very helpful.
+1, definitely much easier to review now!
On 7/26/2023 1:12 PM, Simon Horman wrote:
> 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 <[email protected]>
>>>>>> Reviewed-by: Andrew Lunn <[email protected]>
>>>>>
>>>>> 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.
>
I will be adding __maybe_unused tags to all the helper APIs introduced
before the main driver patch. In the main driver patch I will be
removing all those __maybe_unused tags and all the helper APIs will be
back to their original name (without __maybe_unused tags)
>> 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.
--
Thanks and Regards,
Md Danish Anwar