From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 17158C6FD1C for ; Fri, 24 Mar 2023 15:36:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231346AbjCXPgb (ORCPT ); Fri, 24 Mar 2023 11:36:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41016 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231127AbjCXPga (ORCPT ); Fri, 24 Mar 2023 11:36:30 -0400 Received: from smtp.smtpout.orange.fr (smtp-12.smtpout.orange.fr [80.12.242.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 78EC52069D for ; Fri, 24 Mar 2023 08:36:04 -0700 (PDT) Received: from [192.168.1.18] ([86.243.2.178]) by smtp.orange.fr with ESMTPA id fjSTpBhedqHjrfjSTp1Ery; Fri, 24 Mar 2023 16:36:02 +0100 X-ME-Helo: [192.168.1.18] X-ME-Auth: Y2hyaXN0b3BoZS5qYWlsbGV0QHdhbmFkb28uZnI= X-ME-Date: Fri, 24 Mar 2023 16:36:02 +0100 X-ME-IP: 86.243.2.178 Message-ID: Date: Fri, 24 Mar 2023 16:36:00 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH v2 2/2] leds: max597x: Add support for max597x To: Naresh Solanki , Lee Jones , Pavel Machek Cc: Patrick Rudolph , linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org References: <20230323194550.1914725-1-Naresh.Solanki@9elements.com> <20230323194550.1914725-2-Naresh.Solanki@9elements.com> <688423c6-ba98-002c-efe5-7b0997d6af73@9elements.com> Content-Language: fr From: Christophe JAILLET In-Reply-To: <688423c6-ba98-002c-efe5-7b0997d6af73@9elements.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-leds@vger.kernel.org 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 >>> >>> 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? >> "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; >>> +} >>> + [...]