[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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