No
On 8 July 2024 09:14:44 BST, Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> 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.
Ah missed that. Good sign that things are wrong...
>
>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.
Yes. Looks like a bug to me as well.
>
>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.
There looks to be another bug as it only frees one handle on error. Right now it shouldnt free any but once you fix that you will need to free any not freed otherwise.
Can it be squashed into one loop?
J
>
>>> ---
>>> 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", ®);
>>>
>>
>
