* [PATCH] leds: leds-is31fl32xx: fix an error in is31fl32xx_parse_dt()
@ 2021-06-05 12:49 Dan Carpenter
2021-06-07 18:08 ` David Rivshin
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2021-06-05 12:49 UTC (permalink / raw)
To: Pavel Machek; +Cc: David Rivshin, Jacek Anaszewski, linux-leds, kernel-janitors
Return -EBUSY if the data is already in use (instead of returning
success).
Fixes: 9d7cffaf99f5 ("leds: Add driver for the ISSI IS31FL32xx family of LED controllers")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Not tested. It prints an error so the intention seems clear, but
sometimes making stuff a failure instead of a success can lead to
unexpected problems in production. On the other hand, if this is not
an error then shouldn't we do a continue instead of a goto err?
drivers/leds/leds-is31fl32xx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 3b55af9a8c58..e6f34464914f 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -386,6 +386,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
dev_err(dev,
"Node %pOF 'reg' conflicts with another LED\n",
child);
+ ret = -EBUSY;
goto err;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] leds: leds-is31fl32xx: fix an error in is31fl32xx_parse_dt()
2021-06-05 12:49 [PATCH] leds: leds-is31fl32xx: fix an error in is31fl32xx_parse_dt() Dan Carpenter
@ 2021-06-07 18:08 ` David Rivshin
0 siblings, 0 replies; 2+ messages in thread
From: David Rivshin @ 2021-06-07 18:08 UTC (permalink / raw)
To: Dan Carpenter
Cc: Pavel Machek, David Rivshin, Jacek Anaszewski, linux-leds,
kernel-janitors
On Sat, 5 Jun 2021 15:49:10 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Return -EBUSY if the data is already in use (instead of returning
> success).
>
> Fixes: 9d7cffaf99f5 ("leds: Add driver for the ISSI IS31FL32xx family of LED controllers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Not tested. It prints an error so the intention seems clear, but
> sometimes making stuff a failure instead of a success can lead to
> unexpected problems in production. On the other hand, if this is not
> an error then shouldn't we do a continue instead of a goto err?
I originally had it continue (and ignore that LED), but during review
Jacek commented [1] that he preferred it to be an error condition
instead. It should be noted that this condition involves an invalid
devicetree: two (or more) LED sub-nodes on the same controller that
specify the same controller channel number. FYI, the error message
originally had a bit more detail which might make that more obvious,
but it was simplified as part of another cleanup [2].
In any event, the end result is that the whole controller will fail to
probe, and none of it's LEDs will be available. Because it's not a
temporary condition, I don't think -EBUSY is the appropriate return
value.
There happened to be another patch recently that used -EINVAL [3],
which I think is more appropriate.
[1] https://lore.kernel.org/linux-leds/56CDD4AA.5030801@samsung.com/
[2] https://lore.kernel.org/linux-leds/20200917223338.14164-25-marek.behun@nic.cz/
[2] https://lore.kernel.org/linux-leds/1622545743-21240-1-git-send-email-jiapeng.chong@linux.alibaba.com/
>
> drivers/leds/leds-is31fl32xx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 3b55af9a8c58..e6f34464914f 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -386,6 +386,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
> dev_err(dev,
> "Node %pOF 'reg' conflicts with another LED\n",
> child);
> + ret = -EBUSY;
> goto err;
> }
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-06-07 18:08 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-05 12:49 [PATCH] leds: leds-is31fl32xx: fix an error in is31fl32xx_parse_dt() Dan Carpenter
2021-06-07 18:08 ` David Rivshin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).