Re: [PATCH v2 2/2] leds: max597x: Add support for max597x

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 24/03/2023 à 11:54, Naresh Solanki a écrit :
> Hi,
> 
> On 24-03-2023 01:48 am, Christophe JAILLET wrote:
>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>
>>> max597x is hot swap controller with indicator LED support.
>>> This driver uses DT property to configure led during boot time &
>>> also provide the LED control in sysfs.
>>>

[...]


>>> +static int max597x_led_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device_node *np = dev_of_node(pdev->dev.parent);
>>> +    struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>>> +    struct device_node *led_node;
>>> +    struct device_node *child;
>>> +    int ret = 0;
>>> +
>>> +    if (!regmap)
>>> +        return -EPROBE_DEFER;
>>> +
>>> +    led_node = of_get_child_by_name(np, "leds");
>>> +    if (!led_node)
>>> +        return -ENODEV;
>>> +
>>> +    for_each_available_child_of_node(led_node, child) {
>>> +        u32 reg;
>>> +
>>> +        if (of_property_read_u32(child, "reg", &reg))
>>> +            continue;
>>> +
>>> +        if (reg >= MAX597X_NUM_LEDS) {
>>> +            dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
>>> +                MAX597X_NUM_LEDS);
>>> +            continue;
>>> +        }
>>> +
>>> +        ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
>>> +        if (ret < 0)
>>> +            of_node_put(child);
>>
>> This of_node_put() looks odd to me.
> Not sure if I get this right but if led setup fails of_node_put should 
> be called.

My understanding is that this of_node_put() is there in case of error, 
to release what would otherwise never be released by 
for_each_available_child_of_node() if we exit early from the loop.

If the purpose is to release a reference taken in max597x_setup_led() in 
case of error:
    - this should be done IMHO within max597x_setup_led() directly
    - there should be a of_node_get() somewhere in max597x_setup_led()
      (after:
	if (of_property_read_string(nc, "label", &led->led.name))
		led->led.name = nc->name;
       + error handling path,  *or*
      just before the final return ret; when we know that everything is 
fine,
      if I understand correctly the code)

Is the reference taken elsewhere?
Did I miss something obvious?


>> "return ret;" or "break;" missing ?
>>
> Didn't add a break so that it can continue initializing remaining led if 
> any.

Ok. Don't know the code enough to see if correct or not, but based on my 
comment above, I think that something is missing in max597x_setup_led() 
and that errors should be silently ignored here.

CJ

>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +

[...]