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
