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

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

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@gmail.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@gmail.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.

Maybe refactoring to have a single loop again (if possible) would be the
cleanest solution. Otherwise a get/put mechanism might be necessary.

>>> ---
>>>  drivers/leds/leds-bd2606mvv.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c
>>> index 3fda712d2f80..4f38b7b4d9d1 100644
>>> --- a/drivers/leds/leds-bd2606mvv.c
>>> +++ b/drivers/leds/leds-bd2606mvv.c
>>> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = {
>>>  
>>>  static int bd2606mvv_probe(struct i2c_client *client)
>>>  {
>>> -	struct fwnode_handle *np, *child;
>>> +	struct fwnode_handle *child;
>>>  	struct device *dev = &client->dev;
>>>  	struct bd2606mvv_priv *priv;
>>>  	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
>>> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>>  	int err, reg;
>>>  	int i;
>>>  
>>> -	np = dev_fwnode(dev);
>>> -	if (!np)
>>> +	if (!dev_fwnode(dev))
>>>  		return -ENODEV;
>>>  
>>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>>  
>>>  	i2c_set_clientdata(client, priv);
>>>  
>>> -	fwnode_for_each_available_child_node(np, child) {
>>> +	device_for_each_child_node(dev, child) {
>>>  		struct bd2606mvv_led *led;
>>>  
>>>  		err = fwnode_property_read_u32(child, "reg", &reg);
>>>
>>
>