mbox series

[v2,0/6] use device_for_each_child_node() to access device child nodes

Message ID 20240721-device_for_each_child_node-available-v2-0-f33748fd8b2d@gmail.com (mailing list archive)
Headers show
Series use device_for_each_child_node() to access device child nodes | expand

Message

Javier Carrasco July 21, 2024, 3:19 p.m. UTC
This series aims to clarify the use cases of:

- device_for_each_child_node[_scoped]()
- fwnode_for_each_available_child_node[_scoped]()

to access firmware nodes.

There have been multiple discussions [1][2] about what the first macro
implies in the sense of availability, and a number of users have opted
for the second macro in cases where the first one should have been
preferred.

The second macro is intended to be used over child nodes of a firmware
node, not direct child nodes of the device node. Instead, those users
retrieve the fwnode member from the device struct just to have access to
a macro that explicitly indicates node availability.

That workaround is not necessary because `device_for_each_child_node()`
implies availability for the existing backends (ACPI, DT, swnode).

This series does not cover other points discussed in [2] like addressing
uses of `fwnode_for_each_child_node()` where `device_*` should have been
used, using the `_avaialble_` variant of the fwnode loop whenever
possible, or adding new `_scoped` macros. Such points will be covered by
subsequent series to keep focus on the "availability" issue.

The conversion has been validated with an LTC2992 hwmon sensor, which is
one of the affected drivers. The rest of the drivers could only be
compiled and checked with static-analysis tools.

Link: https://lore.kernel.org/all/20211205190101.26de4a57@jic23-huawei/ [1]
Link: https://lore.kernel.org/all/20240523-fwnode_for_each_available_child_node_scoped-v2-0-701f3a03f2fb@gmail.com/ [2]

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v2:
- [1/6] property.h: drop "if found" from the description of
  device_for_each_child_node()
- [3/6] bd2607mvv.c: fix child node usage.
- Link to v1: https://lore.kernel.org/r/20240706-device_for_each_child_node-available-v1-0-8a3f7615e41c@gmail.com

---
Javier Carrasco (6):
      device property: document device_for_each_child_node macro
      hwmon: (ltc2992) use device_for_each_child_node_scoped() to access child nodes
      leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
      leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
      leds: pca995x: use device_for_each_child_node() to access device child nodes
      net: mvpp2: use device_for_each_child_node() to access device child nodes

 drivers/hwmon/ltc2992.c                         | 19 ++++----------
 drivers/leds/leds-bd2606mvv.c                   | 23 ++++++++---------
 drivers/leds/leds-is31fl319x.c                  | 34 ++++++++-----------------
 drivers/leds/leds-pca995x.c                     | 15 ++++-------
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 +++-------
 include/linux/property.h                        | 10 ++++++++
 6 files changed, 45 insertions(+), 69 deletions(-)
---
base-commit: 41c196e567fb1ea97f68a2ffb7faab451cd90854
change-id: 20240701-device_for_each_child_node-available-1c1eca4b6495

Best regards,

Comments

Lee Jones July 25, 2024, 3:48 p.m. UTC | #1
On Sun, 21 Jul 2024, Javier Carrasco wrote:

> This series aims to clarify the use cases of:
> 
> - device_for_each_child_node[_scoped]()
> - fwnode_for_each_available_child_node[_scoped]()
> 
> to access firmware nodes.
> 
> There have been multiple discussions [1][2] about what the first macro
> implies in the sense of availability, and a number of users have opted
> for the second macro in cases where the first one should have been
> preferred.
> 
> The second macro is intended to be used over child nodes of a firmware
> node, not direct child nodes of the device node. Instead, those users
> retrieve the fwnode member from the device struct just to have access to
> a macro that explicitly indicates node availability.
> 
> That workaround is not necessary because `device_for_each_child_node()`
> implies availability for the existing backends (ACPI, DT, swnode).
> 
> This series does not cover other points discussed in [2] like addressing
> uses of `fwnode_for_each_child_node()` where `device_*` should have been
> used, using the `_avaialble_` variant of the fwnode loop whenever
> possible, or adding new `_scoped` macros. Such points will be covered by
> subsequent series to keep focus on the "availability" issue.
> 
> The conversion has been validated with an LTC2992 hwmon sensor, which is
> one of the affected drivers. The rest of the drivers could only be
> compiled and checked with static-analysis tools.
> 
> Link: https://lore.kernel.org/all/20211205190101.26de4a57@jic23-huawei/ [1]
> Link: https://lore.kernel.org/all/20240523-fwnode_for_each_available_child_node_scoped-v2-0-701f3a03f2fb@gmail.com/ [2]
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> Changes in v2:
> - [1/6] property.h: drop "if found" from the description of
>   device_for_each_child_node()
> - [3/6] bd2607mvv.c: fix child node usage.
> - Link to v1: https://lore.kernel.org/r/20240706-device_for_each_child_node-available-v1-0-8a3f7615e41c@gmail.com
> 
> ---
> Javier Carrasco (6):
>       device property: document device_for_each_child_node macro
>       hwmon: (ltc2992) use device_for_each_child_node_scoped() to access child nodes
>       leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
>       leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
>       leds: pca995x: use device_for_each_child_node() to access device child nodes
>       net: mvpp2: use device_for_each_child_node() to access device child nodes
> 
>  drivers/hwmon/ltc2992.c                         | 19 ++++----------
>  drivers/leds/leds-bd2606mvv.c                   | 23 ++++++++---------
>  drivers/leds/leds-is31fl319x.c                  | 34 ++++++++-----------------
>  drivers/leds/leds-pca995x.c                     | 15 ++++-------
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 +++-------
>  include/linux/property.h                        | 10 ++++++++
>  6 files changed, 45 insertions(+), 69 deletions(-)
> ---
> base-commit: 41c196e567fb1ea97f68a2ffb7faab451cd90854
> change-id: 20240701-device_for_each_child_node-available-1c1eca4b6495

fatal: bad object 41c196e567fb1ea97f68a2ffb7faab451cd90854

And the LED patches do not apply to LED.

Please rebase onto -next.
Lee Jones July 25, 2024, 4:28 p.m. UTC | #2
On Sun, 21 Jul 2024 17:19:00 +0200, Javier Carrasco wrote:
> This series aims to clarify the use cases of:
> 
> - device_for_each_child_node[_scoped]()
> - fwnode_for_each_available_child_node[_scoped]()
> 
> to access firmware nodes.
> 
> [...]

Applied, thanks!

[3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
      commit: 75d2a77327c4917bb66163eea0374bb749428e9c
[4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
      commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
[5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
      (no commit info)

--
Lee Jones [李琼斯]
Javier Carrasco July 29, 2024, 6:12 p.m. UTC | #3
On 25/07/2024 18:28, Lee Jones wrote:
> On Sun, 21 Jul 2024 17:19:00 +0200, Javier Carrasco wrote:
>> This series aims to clarify the use cases of:
>>
>> - device_for_each_child_node[_scoped]()
>> - fwnode_for_each_available_child_node[_scoped]()
>>
>> to access firmware nodes.
>>
>> [...]
> 
> Applied, thanks!
> 
> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
>       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
>       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
> [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
>       (no commit info)
> 
> --
> Lee Jones [李琼斯]
> 

Hi Lee,

could you please tell me where you applied them? I rebased onto
linux-next to prepare for v3, and these patches are still added on top
of it. Can I find them in some leds/ branch? Thank you.

Best regards,
Javier Carrasco
Lee Jones Aug. 1, 2024, 12:39 p.m. UTC | #4
On Mon, 29 Jul 2024, Javier Carrasco wrote:

> On 25/07/2024 18:28, Lee Jones wrote:
> > On Sun, 21 Jul 2024 17:19:00 +0200, Javier Carrasco wrote:
> >> This series aims to clarify the use cases of:
> >>
> >> - device_for_each_child_node[_scoped]()
> >> - fwnode_for_each_available_child_node[_scoped]()
> >>
> >> to access firmware nodes.
> >>
> >> [...]
> > 
> > Applied, thanks!
> > 
> > [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
> >       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
> > [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
> >       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
> > [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
> >       (no commit info)
> > 
> > --
> > Lee Jones [李琼斯]
> > 
> 
> Hi Lee,
> 
> could you please tell me where you applied them? I rebased onto
> linux-next to prepare for v3, and these patches are still added on top
> of it. Can I find them in some leds/ branch? Thank you.

Sorry, I was side-tracked before pushing.

Pushed now.  They should be in -next tomorrow.
Javier Carrasco Aug. 2, 2024, 7:37 a.m. UTC | #5
On 01/08/2024 14:39, Lee Jones wrote:
> On Mon, 29 Jul 2024, Javier Carrasco wrote:
> 
>> On 25/07/2024 18:28, Lee Jones wrote:
>>> On Sun, 21 Jul 2024 17:19:00 +0200, Javier Carrasco wrote:
>>>> This series aims to clarify the use cases of:
>>>>
>>>> - device_for_each_child_node[_scoped]()
>>>> - fwnode_for_each_available_child_node[_scoped]()
>>>>
>>>> to access firmware nodes.
>>>>
>>>> [...]
>>>
>>> Applied, thanks!
>>>
>>> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
>>>       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
>>> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
>>>       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
>>> [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
>>>       (no commit info)
>>>
>>> --
>>> Lee Jones [李琼斯]
>>>
>>
>> Hi Lee,
>>
>> could you please tell me where you applied them? I rebased onto
>> linux-next to prepare for v3, and these patches are still added on top
>> of it. Can I find them in some leds/ branch? Thank you.
> 
> Sorry, I was side-tracked before pushing.
> 
> Pushed now.  They should be in -next tomorrow.
> 

Thanks, I see

[3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()

[4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to
access child nodes

applied to -next, but

[5/6] leds: pca995x: use device_for_each_child_node() to access device
child nodes

has not been applied yet.
Lee Jones Aug. 5, 2024, 2:32 p.m. UTC | #6
On Fri, 02 Aug 2024, Javier Carrasco wrote:

> On 01/08/2024 14:39, Lee Jones wrote:
> > On Mon, 29 Jul 2024, Javier Carrasco wrote:
> > 
> >> On 25/07/2024 18:28, Lee Jones wrote:
> >>> On Sun, 21 Jul 2024 17:19:00 +0200, Javier Carrasco wrote:
> >>>> This series aims to clarify the use cases of:
> >>>>
> >>>> - device_for_each_child_node[_scoped]()
> >>>> - fwnode_for_each_available_child_node[_scoped]()
> >>>>
> >>>> to access firmware nodes.
> >>>>
> >>>> [...]
> >>>
> >>> Applied, thanks!
> >>>
> >>> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
> >>>       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
> >>> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
> >>>       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
> >>> [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
> >>>       (no commit info)
> >>>
> >>> --
> >>> Lee Jones [李琼斯]
> >>>
> >>
> >> Hi Lee,
> >>
> >> could you please tell me where you applied them? I rebased onto
> >> linux-next to prepare for v3, and these patches are still added on top
> >> of it. Can I find them in some leds/ branch? Thank you.
> > 
> > Sorry, I was side-tracked before pushing.
> > 
> > Pushed now.  They should be in -next tomorrow.
> > 
> 
> Thanks, I see
> 
> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
> 
> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to
> access child nodes
> 
> applied to -next, but
> 
> [5/6] leds: pca995x: use device_for_each_child_node() to access device
> child nodes
> 
> has not been applied yet.

Yep, looks like b4 didn't like that one:

[3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
      commit: 75d2a77327c4917bb66163eea0374bb749428e9c
[4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
      commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
[5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
      (no commit info)

I'll try again and see if it can be pulled in.

If not you'll have to resubmit it.
Lee Jones Aug. 5, 2024, 2:33 p.m. UTC | #7
On Mon, 05 Aug 2024, Lee Jones wrote:

> On Fri, 02 Aug 2024, Javier Carrasco wrote:
> 
> > On 01/08/2024 14:39, Lee Jones wrote:
> > > On Mon, 29 Jul 2024, Javier Carrasco wrote:
> > > 
> > >> On 25/07/2024 18:28, Lee Jones wrote:
> > >>> On Sun, 21 Jul 2024 17:19:00 +0200, Javier Carrasco wrote:
> > >>>> This series aims to clarify the use cases of:
> > >>>>
> > >>>> - device_for_each_child_node[_scoped]()
> > >>>> - fwnode_for_each_available_child_node[_scoped]()
> > >>>>
> > >>>> to access firmware nodes.
> > >>>>
> > >>>> [...]
> > >>>
> > >>> Applied, thanks!
> > >>>
> > >>> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
> > >>>       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
> > >>> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
> > >>>       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
> > >>> [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
> > >>>       (no commit info)
> > >>>
> > >>> --
> > >>> Lee Jones [李琼斯]
> > >>>
> > >>
> > >> Hi Lee,
> > >>
> > >> could you please tell me where you applied them? I rebased onto
> > >> linux-next to prepare for v3, and these patches are still added on top
> > >> of it. Can I find them in some leds/ branch? Thank you.
> > > 
> > > Sorry, I was side-tracked before pushing.
> > > 
> > > Pushed now.  They should be in -next tomorrow.
> > > 
> > 
> > Thanks, I see
> > 
> > [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
> > 
> > [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to
> > access child nodes
> > 
> > applied to -next, but
> > 
> > [5/6] leds: pca995x: use device_for_each_child_node() to access device
> > child nodes
> > 
> > has not been applied yet.
> 
> Yep, looks like b4 didn't like that one:
> 
> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
>       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
>       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
> [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
>       (no commit info)
> 
> I'll try again and see if it can be pulled in.
> 
> If not you'll have to resubmit it.

Now results in conflict:

    Applying patch(es)
    Applying: leds: pca995x: use device_for_each_child_node() to access device child nodes
    Using index info to reconstruct a base tree...
    M	drivers/leds/leds-pca995x.c
    Checking patch drivers/leds/leds-pca995x.c...
    Applied patch drivers/leds/leds-pca995x.c cleanly.
    Falling back to patching base and 3-way merge...
    error: Your local changes to the following files would be overwritten by merge:
    	drivers/leds/leds-pca995x.c
    Please commit your changes or stash them before you merge.
    Aborting
    error: Failed to merge in the changes.
    Patch failed at 0001 leds: pca995x: use device_for_each_child_node() to access device child nodes
    hint: Use 'git am --show-current-patch=diff' to see the failed patch
    hint: When you have resolved this problem, run "git am --continue".
    hint: If you prefer to skip this patch, run "git am --skip" instead.
    hint: To restore the original branch and stop patching, run "git am --abort".
    hint: Disable this message with "git config advice.mergeConflict false"
    
    Failed to apply patches (fix and either hit return to continue or Ctrl+c to exit)

Please rebase and resubmit.
Javier Carrasco Aug. 5, 2024, 2:41 p.m. UTC | #8
On 05/08/2024 16:33, Lee Jones wrote:
> On Mon, 05 Aug 2024, Lee Jones wrote:
> 
>> On Fri, 02 Aug 2024, Javier Carrasco wrote:
>>
>>> On 01/08/2024 14:39, Lee Jones wrote:
>>>> On Mon, 29 Jul 2024, Javier Carrasco wrote:
>>>>
>>>>> On 25/07/2024 18:28, Lee Jones wrote:
>>>>>> On Sun, 21 Jul 2024 17:19:00 +0200, Javier Carrasco wrote:
>>>>>>> This series aims to clarify the use cases of:
>>>>>>>
>>>>>>> - device_for_each_child_node[_scoped]()
>>>>>>> - fwnode_for_each_available_child_node[_scoped]()
>>>>>>>
>>>>>>> to access firmware nodes.
>>>>>>>
>>>>>>> [...]
>>>>>>
>>>>>> Applied, thanks!
>>>>>>
>>>>>> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
>>>>>>       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
>>>>>> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
>>>>>>       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
>>>>>> [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
>>>>>>       (no commit info)
>>>>>>
>>>>>> --
>>>>>> Lee Jones [李琼斯]
>>>>>>
>>>>>
>>>>> Hi Lee,
>>>>>
>>>>> could you please tell me where you applied them? I rebased onto
>>>>> linux-next to prepare for v3, and these patches are still added on top
>>>>> of it. Can I find them in some leds/ branch? Thank you.
>>>>
>>>> Sorry, I was side-tracked before pushing.
>>>>
>>>> Pushed now.  They should be in -next tomorrow.
>>>>
>>>
>>> Thanks, I see
>>>
>>> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
>>>
>>> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to
>>> access child nodes
>>>
>>> applied to -next, but
>>>
>>> [5/6] leds: pca995x: use device_for_each_child_node() to access device
>>> child nodes
>>>
>>> has not been applied yet.
>>
>> Yep, looks like b4 didn't like that one:
>>
>> [3/6] leds: bd2606mvv: fix device child node usage in bd2606mvv_probe()
>>       commit: 75d2a77327c4917bb66163eea0374bb749428e9c
>> [4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
>>       commit: 0f5a3feb60aba5d74f0b655cdff9c35aca03e81b
>> [5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes
>>       (no commit info)
>>
>> I'll try again and see if it can be pulled in.
>>
>> If not you'll have to resubmit it.
> 
> Now results in conflict:
> 
>     Applying patch(es)
>     Applying: leds: pca995x: use device_for_each_child_node() to access device child nodes
>     Using index info to reconstruct a base tree...
>     M	drivers/leds/leds-pca995x.c
>     Checking patch drivers/leds/leds-pca995x.c...
>     Applied patch drivers/leds/leds-pca995x.c cleanly.
>     Falling back to patching base and 3-way merge...
>     error: Your local changes to the following files would be overwritten by merge:
>     	drivers/leds/leds-pca995x.c
>     Please commit your changes or stash them before you merge.
>     Aborting
>     error: Failed to merge in the changes.
>     Patch failed at 0001 leds: pca995x: use device_for_each_child_node() to access device child nodes
>     hint: Use 'git am --show-current-patch=diff' to see the failed patch
>     hint: When you have resolved this problem, run "git am --continue".
>     hint: If you prefer to skip this patch, run "git am --skip" instead.
>     hint: To restore the original branch and stop patching, run "git am --abort".
>     hint: Disable this message with "git config advice.mergeConflict false"
>     
>     Failed to apply patches (fix and either hit return to continue or Ctrl+c to exit)
> 
> Please rebase and resubmit.
> 

Thank you for making the effort anyway, I will resubmit the patch.

Best regards,
Javier Carrasco