leds: leds-is31fl32xx: fix an error in is31fl32xx_parse_dt()

Message ID YLtyxq0Ty/7Uo4NS@mwanda
State New
Headers show
Series
  • leds: leds-is31fl32xx: fix an error in is31fl32xx_parse_dt()
Related show

Commit Message

Dan Carpenter June 5, 2021, 12:49 p.m.
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(+)

Comments

David Rivshin June 7, 2021, 6:08 p.m. | #1
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;

>  		}

>

Patch

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;
 		}