Messages in this thread | | | Date | Mon, 27 Mar 2023 23:19:49 +0530 | Subject | Re: [PATCH v2 2/2] leds: max597x: Add support for max597x | From | Naresh Solanki <> |
| |
Hi,
On 27-03-2023 10:50 pm, Christophe JAILLET wrote: > Le 27/03/2023 à 17:47, Naresh Solanki a écrit : >> 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. > By reference, I was speaking of reference taken by a of_node_get() call > and released by a of_node_put() call. > > Anyway, I do agree with leds-sc27xx-bltc.c. > There is a of_node_put() because for_each_available_child_of_node() > won't be able to do it by itself *in case of early return* ("return err;") > > In all other paths (when the loop goes to the end), the reference taken > by for_each_available_child_of_node() is also released, on the next > iteration, by for_each_available_child_of_node(). > > In *your* case, if you don't break or return, there is no need to call > of_node_put() explicitly. It would lead to a double put. (yours and the > one that will be done by for_each_available_child_of_node()). > > Have a look at for_each_available_child_of_node() and more specifically > at of_get_next_available_child(). > > At the first call 'child' is NULL. A ref is taken [1]. Nothing is released. > For following calls, a new ref is taken on a new node [1], and the > previous reference is released [2]. > On the last call, the 'for' loop will not be executed because there is > nothing to scan anymore. No new reference is taken, and the previous > (and last) refence is finally released [2]. Yes you are right. That of_node_put would be duplicate as it is already taken care by the for loop. Will remove that in next revision. > > > [1]: > https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L808 > [2]: > https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L811 > >> >>>>> "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. > > No, sorry, I won't be of any help on what design is the best. > > CJ > Regards, Naresh
| |