Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Andreas Kemnade]

On Mon, 8 Jul 2024 17:45:43 +0200
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:

> On 08/07/2024 10:14, Javier Carrasco wrote:
> > On 07/07/2024 18:57, Jonathan Cameron wrote:  
> >> On Sat, 06 Jul 2024 17:23:35 +0200
> >> Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
> >>  
> >>> The iterated nodes are direct children of the device node, and the
> >>> `device_for_each_child_node()` macro accounts for child node
> >>> availability.
> >>>
> >>> `fwnode_for_each_available_child_node()` is meant to access the
> >>> child nodes of an fwnode, and therefore not direct child nodes of
> >>> the device node.
> >>>
> >>> Use `device_for_each_child_node()` to indicate device's direct
> >>> child nodes.
> >>>
> >>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>  
> >> Why not the scoped variant?
> >> There look to be two error paths in there which would be
> >> simplified. 
> > 
> > I did not use the scoped variant because "child" is used outside
> > the loop.
> > 
> > On the other hand, I think an fwnode_handle_get() is missing for
> > every "led_fwnodes[reg] = child" because a simple assignment does
> > not increment the refcount.
> > 
> > After adding fwnode_handle_get(), the scoped variant could be used,
> > and the call to fwnode_handle_put() would act on led_fwnodes[reg]
> > instead. 
> 
> Actually, the whole trouble comes from doing the processing in two
> consecutive loops, where the second loop accesses a child node that
> gets released at the end of the first one. It seems that some code
> got moved from one loop to a new one between two versions of the
> patchset.
> 
> @Andreas Kemnade: I see that you had a single loop until v4 (the
> driver got applied with v6), and then you split it into two loops,
> but I don't see any mention to this modification in the change log.
> 
> What was the reason for this modification? Apparently, similar drivers
> do everything in one loop to avoid such issues.
> 
The reason for two loops is that we check in the first loop whether
broghtness can be individually controlled so we can set max_brightness
in the second loop. I had the assumption that max_brightness should be
set before registering leds.

Some LEDs share brightness register, in the case where leds are defined
with a shared register, we revert to on-off.

> Maybe refactoring to have a single loop again (if possible) would be
> the cleanest solution. Otherwise a get/put mechanism might be
> necessary.
> 
I had no idea how to do it the time I wrote the patch.

Regards,
Andreas