Re: [PATCH 2/5] leds: lm3692x: Don't overwrite return value in error path

[Pavel Machek]

On Tue 2019-09-17 19:19:55, Guido Günther wrote:
> The driver currently reports successful initialization on every failure
> as long as it's able to power off the regulator. Don't check the return
> value of regulator_disable to avoid that.
> 
> Signed-off-by: Guido Günther 
> ---
>  drivers/leds/leds-lm3692x.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
> index 487228c2bed2..f394669ad8f2 100644
> --- a/drivers/leds/leds-lm3692x.c
> +++ b/drivers/leds/leds-lm3692x.c
> @@ -312,15 +312,12 @@ static int lm3692x_init(struct lm3692x_led *led)
>   if (led->enable_gpio)
>   gpiod_direction_output(led->enable_gpio, 0);
>  
> - if (led->regulator) {
> - ret = regulator_disable(led->regulator);
> - if (ret)
> - dev_err(>client->dev,
> - "Failed to disable regulator\n");
> - }
> + if (led->regulator)
> + regulator_disable(led->regulator);
>  
>   return ret;

Overwriting return value is bad, but we should still print some kind
of error message.

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 2/5] leds: lm3692x: Don't overwrite return value in error path

[Dan Murphy]

Guido

On 9/17/19 9:19 PM, Guido Günther wrote:

The driver currently reports successful initialization on every failure
as long as it's able to power off the regulator. Don't check the return
value of regulator_disable to avoid that.

Signed-off-by: Guido Günther 
---
  drivers/leds/leds-lm3692x.c | 9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 487228c2bed2..f394669ad8f2 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -312,15 +312,12 @@ static int lm3692x_init(struct lm3692x_led *led)
if (led->enable_gpio)
gpiod_direction_output(led->enable_gpio, 0);
  
-	if (led->regulator) {

-   ret = regulator_disable(led->regulator);
-   if (ret)
-   dev_err(>client->dev,
-   "Failed to disable regulator\n");
-   }
+   if (led->regulator)
+   regulator_disable(led->regulator);


The change is ok and makes sense but I believe that if the regulator was 
not properly disabled there needs to be some error message t0o.


If the code got here then there is either a fault or an I/O issue not a 
regulator issue.


The regulator failing to disable should be logged.

Dan


  
  	return ret;

  }
+
  static int lm3692x_probe_dt(struct lm3692x_led *led)
  {
struct fwnode_handle *child = NULL;

[PATCH 2/5] leds: lm3692x: Don't overwrite return value in error path

[Guido Günther]

The driver currently reports successful initialization on every failure
as long as it's able to power off the regulator. Don't check the return
value of regulator_disable to avoid that.

Signed-off-by: Guido Günther 
---
 drivers/leds/leds-lm3692x.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 487228c2bed2..f394669ad8f2 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -312,15 +312,12 @@ static int lm3692x_init(struct lm3692x_led *led)
if (led->enable_gpio)
gpiod_direction_output(led->enable_gpio, 0);
 
-   if (led->regulator) {
-   ret = regulator_disable(led->regulator);
-   if (ret)
-   dev_err(>client->dev,
-   "Failed to disable regulator\n");
-   }
+   if (led->regulator)
+   regulator_disable(led->regulator);
 
return ret;
 }
+
 static int lm3692x_probe_dt(struct lm3692x_led *led)
 {
struct fwnode_handle *child = NULL;
-- 
2.23.0.rc1