Messages in this thread | | | Date | Mon, 27 Mar 2023 21:17:16 +0530 | Subject | Re: [PATCH v2 2/2] leds: max597x: Add support for max597x | From | Naresh Solanki <> |
| |
Hi,
On 24-03-2023 09:06 pm, Christophe JAILLET wrote: > 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", ®)) >>>> + 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? > > One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311 Please do let me know if I have to do anything about it.
>>> "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. In my implementation, I have chosen to continue with the next LED if an error occurs, rather than aborting the 'for loop' with an error. I have seen other implementations also done in a similar way. Do you have any further inputs or suggestions on this approach.
> > CJ > >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + > > [...] > Regards, Naresh
| |