[PATCH] leds: lm3532: propagate enable GPIO errors

[PATCH] leds: lm3532: propagate enable GPIO errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

devm_gpiod_get_optional() returns NULL when the optional GPIO is not
present, but may return -EPROBE_DEFER or other errors.

lm3532_parse_node() currently treats GPIO lookup errors as if the GPIO
were not present.

Check the return value and propagate errors from the enable GPIO lookup.

Fixes: bc1b8492c764 ("leds: lm3532: Introduce the lm3532 LED driver")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/leds/leds-lm3532.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index b51496910b08..a3bc9680dc4c 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -560,7 +560,8 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 	priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
 						   "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->enable_gpio))
-		priv->enable_gpio = NULL;
+		return dev_err_probe(&priv->client->dev, PTR_ERR(priv->enable_gpio),
+				     "failed to get enable GPIO\n");
 
 	if (priv->enable_gpio) {
 		ret = devm_add_action(&priv->client->dev, gpio_set_low_action, priv);
-- 
2.43.0

Re: [PATCH] leds: lm3532: propagate enable GPIO errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] leds: lm3532: unconditional dev_err() in caller defeats dev_err_probe() log suppression
- [Low] leds: lm3532: commit message description capitalization violation
--

commit 8b48de70c8c717d07cec849b9e0b416a17a8bb23
Author: bui duc phuc <phucduc.bui@gmail.com>

leds: lm3532: propagate enable GPIO errors

This commit modifies the lm3532 driver to check the return value of
devm_gpiod_get_optional() and propagate errors, rather than treating them
as if the GPIO were not present.

> leds: lm3532: propagate enable GPIO errors

[Severity: Low]
This isn't a bug, but does the commit message subject violate the LED
subsystem guidelines? The guidelines require the commit message description
following the subsystem prefix to be capitalized. Should the description
start with a capital letter?

> diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
> index b51496910b082..a3bc9680dc4cf 100644
> --- a/drivers/leds/leds-lm3532.c
> +++ b/drivers/leds/leds-lm3532.c
> @@ -560,7 +560,8 @@ static int lm3532_parse_node(struct lm3532_data *priv)
>  	priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
>  						   "enable", GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->enable_gpio))
> -		priv->enable_gpio = NULL;
> +		return dev_err_probe(&priv->client->dev, PTR_ERR(priv->enable_gpio),
> +				     "failed to get enable GPIO\n");

[Severity: Medium]
Will this change result in unconditional error logging during deferred probes
anyway?

When devm_gpiod_get_optional() returns a deferred probe error, dev_err_probe()
correctly suppresses the log message here. However, the caller lm3532_probe()
does not use dev_err_probe() for its return check:

lm3532_probe() {
    ...
    ret = lm3532_parse_node(drvdata);
    if (ret) {
        dev_err(&client->dev, "Failed to parse node\n");
        return ret;
    }
    ...
}

Because the deferred probe error now bubbles up to lm3532_probe(), won't the
caller log a hard error message to the kernel log and negate the suppression
benefit of this patch?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007104915.507801-1-phucduc.bui@gmail.com?part=1

Re: [PATCH] leds: lm3532: propagate enable GPIO errors

[Bui Duc Phuc]

>
> [Severity: Low]
> This isn't a bug, but does the commit message subject violate the LED
> subsystem guidelines? The guidelines require the commit message description
> following the subsystem prefix to be capitalized. Should the description
> start with a capital letter?
>

I will update the commit message subject to:
leds: lm3532: Propagate enable GPIO errors


> [Severity: Medium]
> Will this change result in unconditional error logging during deferred probes
> anyway?
>
> When devm_gpiod_get_optional() returns a deferred probe error, dev_err_probe()
> correctly suppresses the log message here. However, the caller lm3532_probe()
> does not use dev_err_probe() for its return check:
>
> lm3532_probe() {
>     ...
>     ret = lm3532_parse_node(drvdata);
>     if (ret) {
>         dev_err(&client->dev, "Failed to parse node\n");
>         return ret;
>     }
>     ...
> }

I will also add this code to the current patch.

--------------------------------------------------------------------------------------------
ret = lm3532_parse_node(drvdata);
if (ret)
        return dev_err_probe(&client->dev, ret, "Failed to parse node\n");
---------------------------------------------------------------------------------------------

Re: [PATCH] leds: lm3532: propagate enable GPIO errors

[Bui Duc Phuc]

> > [Severity: Medium]
> > Will this change result in unconditional error logging during deferred probes
> > anyway?
> >
> > When devm_gpiod_get_optional() returns a deferred probe error, dev_err_probe()
> > correctly suppresses the log message here. However, the caller lm3532_probe()
> > does not use dev_err_probe() for its return check:
> >
> > lm3532_probe() {
> >     ...
> >     ret = lm3532_parse_node(drvdata);
> >     if (ret) {
> >         dev_err(&client->dev, "Failed to parse node\n");
> >         return ret;
> >     }
> >     ...
> > }
>
> I will also add this code to the current patch.
>
> --------------------------------------------------------------------------------------------
> ret = lm3532_parse_node(drvdata);
> if (ret)
>         return dev_err_probe(&client->dev, ret, "Failed to parse node\n");
> ---------------------------------------------------------------------------------------------

After looking at the error handling in lm3532_parse_node(),
I think it would be better to log errors there and return the error
directly from lm3532_probe(),
avoiding duplicate error messages.
I'll update the patch accordingly.