* [PATCH v2 1/5] leds: lm3692x: Print error value on dev_err
2019-09-20 5:27 [PATCH v2 0/5] leds: lm3692x: Probing and flag fixes Guido Günther
@ 2019-09-20 5:27 ` Guido Günther
2019-09-20 5:27 ` [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path Guido Günther
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Guido Günther @ 2019-09-20 5:27 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Dan Murphy, linux-leds, linux-kernel
This gives a way better idea what is going on.
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Reviewed-by: Dan Murphy <dmurphy@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
drivers/leds/leds-lm3692x.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 3d381f2f73d0..487228c2bed2 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -174,19 +174,20 @@ static int lm3692x_brightness_set(struct led_classdev *led_cdev,
ret = lm3692x_fault_check(led);
if (ret) {
- dev_err(&led->client->dev, "Cannot read/clear faults\n");
+ dev_err(&led->client->dev, "Cannot read/clear faults: %d\n",
+ ret);
goto out;
}
ret = regmap_write(led->regmap, LM3692X_BRT_MSB, brt_val);
if (ret) {
- dev_err(&led->client->dev, "Cannot write MSB\n");
+ dev_err(&led->client->dev, "Cannot write MSB: %d\n", ret);
goto out;
}
ret = regmap_write(led->regmap, LM3692X_BRT_LSB, led_brightness_lsb);
if (ret) {
- dev_err(&led->client->dev, "Cannot write LSB\n");
+ dev_err(&led->client->dev, "Cannot write LSB: %d\n", ret);
goto out;
}
out:
@@ -203,7 +204,7 @@ static int lm3692x_init(struct lm3692x_led *led)
ret = regulator_enable(led->regulator);
if (ret) {
dev_err(&led->client->dev,
- "Failed to enable regulator\n");
+ "Failed to enable regulator: %d\n", ret);
return ret;
}
}
@@ -213,7 +214,8 @@ static int lm3692x_init(struct lm3692x_led *led)
ret = lm3692x_fault_check(led);
if (ret) {
- dev_err(&led->client->dev, "Cannot read/clear faults\n");
+ dev_err(&led->client->dev, "Cannot read/clear faults: %d\n",
+ ret);
goto out;
}
@@ -409,7 +411,8 @@ static int lm3692x_remove(struct i2c_client *client)
ret = regmap_update_bits(led->regmap, LM3692X_EN, LM3692X_DEVICE_EN, 0);
if (ret) {
- dev_err(&led->client->dev, "Failed to disable regulator\n");
+ dev_err(&led->client->dev, "Failed to disable regulator: %d\n",
+ ret);
return ret;
}
@@ -420,7 +423,7 @@ static int lm3692x_remove(struct i2c_client *client)
ret = regulator_disable(led->regulator);
if (ret)
dev_err(&led->client->dev,
- "Failed to disable regulator\n");
+ "Failed to disable regulator: %d\n", ret);
}
mutex_destroy(&led->lock);
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path
2019-09-20 5:27 [PATCH v2 0/5] leds: lm3692x: Probing and flag fixes Guido Günther
2019-09-20 5:27 ` [PATCH v2 1/5] leds: lm3692x: Print error value on dev_err Guido Günther
@ 2019-09-20 5:27 ` Guido Günther
2019-09-20 11:47 ` Pavel Machek
2019-09-20 11:48 ` Dan Murphy
2019-09-20 5:27 ` [PATCH v2 3/5] leds: lm3692x: Handle failure to probe the regulator Guido Günther
` (2 subsequent siblings)
4 siblings, 2 replies; 9+ messages in thread
From: Guido Günther @ 2019-09-20 5:27 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Dan Murphy, linux-leds, linux-kernel
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 <agx@sigxcpu.org>
---
drivers/leds/leds-lm3692x.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 487228c2bed2..31115655f97b 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -198,7 +198,7 @@ static int lm3692x_brightness_set(struct led_classdev *led_cdev,
static int lm3692x_init(struct lm3692x_led *led)
{
int enable_state;
- int ret;
+ int ret, ret2;
if (led->regulator) {
ret = regulator_enable(led->regulator);
@@ -313,14 +313,15 @@ static int lm3692x_init(struct lm3692x_led *led)
gpiod_direction_output(led->enable_gpio, 0);
if (led->regulator) {
- ret = regulator_disable(led->regulator);
- if (ret)
+ ret2 = regulator_disable(led->regulator);
+ if (ret2)
dev_err(&led->client->dev,
"Failed to disable regulator\n");
}
return ret;
}
+
static int lm3692x_probe_dt(struct lm3692x_led *led)
{
struct fwnode_handle *child = NULL;
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path
2019-09-20 5:27 ` [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path Guido Günther
@ 2019-09-20 11:47 ` Pavel Machek
2019-09-20 11:48 ` Dan Murphy
1 sibling, 0 replies; 9+ messages in thread
From: Pavel Machek @ 2019-09-20 11:47 UTC (permalink / raw)
To: Guido Günther; +Cc: Jacek Anaszewski, Dan Murphy, linux-leds, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 495 bytes --]
On Thu 2019-09-19 22:27:04, 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 <agx@sigxcpu.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path
2019-09-20 5:27 ` [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path Guido Günther
2019-09-20 11:47 ` Pavel Machek
@ 2019-09-20 11:48 ` Dan Murphy
2019-09-21 21:15 ` Guido Günther
1 sibling, 1 reply; 9+ messages in thread
From: Dan Murphy @ 2019-09-20 11:48 UTC (permalink / raw)
To: Guido Günther, Jacek Anaszewski, Pavel Machek, linux-leds,
linux-kernel
Guido
On 9/20/19 12:27 AM, 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 <agx@sigxcpu.org>
> ---
> drivers/leds/leds-lm3692x.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
> index 487228c2bed2..31115655f97b 100644
> --- a/drivers/leds/leds-lm3692x.c
> +++ b/drivers/leds/leds-lm3692x.c
> @@ -198,7 +198,7 @@ static int lm3692x_brightness_set(struct led_classdev *led_cdev,
> static int lm3692x_init(struct lm3692x_led *led)
> {
> int enable_state;
> - int ret;
> + int ret, ret2;
>
> if (led->regulator) {
> ret = regulator_enable(led->regulator);
> @@ -313,14 +313,15 @@ static int lm3692x_init(struct lm3692x_led *led)
> gpiod_direction_output(led->enable_gpio, 0);
>
> if (led->regulator) {
> - ret = regulator_disable(led->regulator);
> - if (ret)
> + ret2 = regulator_disable(led->regulator);
> + if (ret2)
> dev_err(&led->client->dev,
> "Failed to disable regulator\n");
s/ret2/reg_ret
Like you did in patch 1 log the error code as well.
If regulator_disabled failed you might want to send that error code but
either error returned is fine.
Dan
> }
>
> return ret;
> }
> +
> static int lm3692x_probe_dt(struct lm3692x_led *led)
> {
> struct fwnode_handle *child = NULL;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path
2019-09-20 11:48 ` Dan Murphy
@ 2019-09-21 21:15 ` Guido Günther
0 siblings, 0 replies; 9+ messages in thread
From: Guido Günther @ 2019-09-21 21:15 UTC (permalink / raw)
To: Dan Murphy; +Cc: Jacek Anaszewski, Pavel Machek, linux-leds, linux-kernel
Hi Dan,
On Fri, Sep 20, 2019 at 06:48:59AM -0500, Dan Murphy wrote:
> Guido
>
> On 9/20/19 12:27 AM, 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 <agx@sigxcpu.org>
> > ---
> > drivers/leds/leds-lm3692x.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
> > index 487228c2bed2..31115655f97b 100644
> > --- a/drivers/leds/leds-lm3692x.c
> > +++ b/drivers/leds/leds-lm3692x.c
> > @@ -198,7 +198,7 @@ static int lm3692x_brightness_set(struct led_classdev *led_cdev,
> > static int lm3692x_init(struct lm3692x_led *led)
> > {
> > int enable_state;
> > - int ret;
> > + int ret, ret2;
> > if (led->regulator) {
> > ret = regulator_enable(led->regulator);
> > @@ -313,14 +313,15 @@ static int lm3692x_init(struct lm3692x_led *led)
> > gpiod_direction_output(led->enable_gpio, 0);
> > if (led->regulator) {
> > - ret = regulator_disable(led->regulator);
> > - if (ret)
> > + ret2 = regulator_disable(led->regulator);
> > + if (ret2)
> > dev_err(&led->client->dev,
> > "Failed to disable regulator\n");
>
> s/ret2/reg_ret
>
> Like you did in patch 1 log the error code as well.
>
> If regulator_disabled failed you might want to send that error code but
> either error returned is fine.
Changed in v3 - i opted to return the original error code since this is
likely what upper layers care about and only printk the regulator
failure one.
Thanks for the review,
-- Guido
>
> Dan
>
>
> > }
> > return ret;
> > }
> > +
> > static int lm3692x_probe_dt(struct lm3692x_led *led)
> > {
> > struct fwnode_handle *child = NULL;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/5] leds: lm3692x: Handle failure to probe the regulator
2019-09-20 5:27 [PATCH v2 0/5] leds: lm3692x: Probing and flag fixes Guido Günther
2019-09-20 5:27 ` [PATCH v2 1/5] leds: lm3692x: Print error value on dev_err Guido Günther
2019-09-20 5:27 ` [PATCH v2 2/5] leds: lm3692x: Don't overwrite return value in error path Guido Günther
@ 2019-09-20 5:27 ` Guido Günther
2019-09-20 5:27 ` [PATCH v2 4/5] leds: lm3692x: Use flags from LM3692X_BOOST_CTRL Guido Günther
2019-09-20 5:27 ` [PATCH v2 5/5] leds: lm3692x: Use flags from LM3692X_BRT_CTRL Guido Günther
4 siblings, 0 replies; 9+ messages in thread
From: Guido Günther @ 2019-09-20 5:27 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Dan Murphy, linux-leds, linux-kernel
Instead use devm_regulator_get_optional since the regulator
is optional and check for errors.
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
drivers/leds/leds-lm3692x.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 31115655f97b..6f6a17ec7c9f 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -337,9 +337,18 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
return ret;
}
- led->regulator = devm_regulator_get(&led->client->dev, "vled");
- if (IS_ERR(led->regulator))
+ led->regulator = devm_regulator_get_optional(&led->client->dev, "vled");
+ if (IS_ERR(led->regulator)) {
+ ret = PTR_ERR(led->regulator);
+ if (ret != -ENODEV) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&led->client->dev,
+ "Failed to get vled regulator: %d\n",
+ ret);
+ return ret;
+ }
led->regulator = NULL;
+ }
child = device_get_next_child_node(&led->client->dev, child);
if (!child) {
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 4/5] leds: lm3692x: Use flags from LM3692X_BOOST_CTRL
2019-09-20 5:27 [PATCH v2 0/5] leds: lm3692x: Probing and flag fixes Guido Günther
` (2 preceding siblings ...)
2019-09-20 5:27 ` [PATCH v2 3/5] leds: lm3692x: Handle failure to probe the regulator Guido Günther
@ 2019-09-20 5:27 ` Guido Günther
2019-09-20 5:27 ` [PATCH v2 5/5] leds: lm3692x: Use flags from LM3692X_BRT_CTRL Guido Günther
4 siblings, 0 replies; 9+ messages in thread
From: Guido Günther @ 2019-09-20 5:27 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Dan Murphy, linux-leds, linux-kernel
The current setup of LM3692X_BOOST_CTRL uses flags from LM3692X_BRT_CTRL.
Use flags from LM3692X_BOOST_CTRL but leave the resulting register value
unchanged.
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Reviewed-by: Dan Murphy <dmurphy@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
drivers/leds/leds-lm3692x.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 6f6a17ec7c9f..d8a60c7c8077 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -250,9 +250,9 @@ static int lm3692x_init(struct lm3692x_led *led)
goto out;
ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,
- LM3692X_BRHT_MODE_RAMP_MULTI |
- LM3692X_BL_ADJ_POL |
- LM3692X_RAMP_RATE_250us);
+ LM3692X_BOOST_SW_1MHZ |
+ LM3692X_BOOST_SW_NO_SHIFT |
+ LM3692X_OCP_PROT_1_5A);
if (ret)
goto out;
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 5/5] leds: lm3692x: Use flags from LM3692X_BRT_CTRL
2019-09-20 5:27 [PATCH v2 0/5] leds: lm3692x: Probing and flag fixes Guido Günther
` (3 preceding siblings ...)
2019-09-20 5:27 ` [PATCH v2 4/5] leds: lm3692x: Use flags from LM3692X_BOOST_CTRL Guido Günther
@ 2019-09-20 5:27 ` Guido Günther
4 siblings, 0 replies; 9+ messages in thread
From: Guido Günther @ 2019-09-20 5:27 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Dan Murphy, linux-leds, linux-kernel
Use LM3692X_RAMP_EN instead of LM3692X_PWM_HYSTER_4LSB
since the later is a flag for the PWM register. The
actual register value remains unchanged.
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Reviewed-by: Dan Murphy <dmurphy@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
drivers/leds/leds-lm3692x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index d8a60c7c8077..a404b66b31e5 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -269,7 +269,7 @@ static int lm3692x_init(struct lm3692x_led *led)
goto out;
ret = regmap_write(led->regmap, LM3692X_BRT_CTRL,
- LM3692X_BL_ADJ_POL | LM3692X_PWM_HYSTER_4LSB);
+ LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN);
if (ret)
goto out;
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 9+ messages in thread