Message-ID: <20240712230656.67e89eb2@akphone>
Date: Fri, 12 Jul 2024 23:06:56 +0200
From: Andreas Kemnade <andreas@...nade.info>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Andy
 Shevchenko <andriy.shevchenko@...ux.intel.com>, Rob Herring
 <robh@...nel.org>, Daniel Scally <djrscally@...il.com>, Heikki Krogerus
 <heikki.krogerus@...ux.intel.com>, Sakari Ailus
 <sakari.ailus@...ux.intel.com>, Jean Delvare <jdelvare@...e.com>, Guenter
 Roeck <linux@...ck-us.net>, Pavel Machek <pavel@....cz>, Lee Jones
 <lee@...nel.org>, Marcin Wojtas <marcin.s.wojtas@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, linux-acpi@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-leds@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node()
 to access device child nodes

On Mon, 8 Jul 2024 17:45:43 +0200
Javier Carrasco <javier.carrasco.cruz@...il.com> 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@...il.com> 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@...il.com>  
> >> 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

Powered by blists - more mailing lists