leds: lm3692x: Disable chip on brightness 0

PATCH In review Guido Günther 2 participants 9 messages Dec 25, 2019 – Dec 27, 2019 LED

Share & tools

Guido Günther

author

2019-12-25 11:16 UTC #0 lore raw

cover leds: lm3692x: Disable chip on brightness 0

Otherwise there's a hardly noticeable glow even with brightness 0. Also turning off the regulator can save additional power.

This is on top of the "leds: lm3692x: Allow to set ovp and brigthness mode". Without that applied there's a minor merge conflict since this introduces a new variable in struct lm3692x_led as well. I did not want to lump it in there as well since it's not related to any DT properties at all.

Guido Günther (3):
leds: lm3692x: Move lm3692x_init and rename to lm3692x_leds_enable
leds: lm3692x: Split out lm3692x_leds_disable
leds: lm3692x: Disable chip on brightness 0

 drivers/leds/leds-lm3692x.c | 128 ++++++++++++++++++++++--------------
 1 file changed, 77 insertions(+), 51 deletions(-)

Signature · 1 line

2.23.0

Guido Günther

author

2019-12-25 11:16 UTC #1 lore raw

PATCH 1/3 leds: lm3692x: Move lm3692x_init and rename to lm3692x_leds_enable

This moves lm3692x_init so it can be used from lm3692x_brightness_set. Rename to lm3692_leds_enable to pair up with lm3692x_leds_disable. No functional change.

Signed-off-by Guido Günther <[email protected]>

---
 drivers/leds/leds-lm3692x.c | 70 ++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

drivers/leds/leds-lm3692x.c +35 ~~-35~~

index d7e5de8fe8db..7bf97ee6aa92 100644
@@ -165,40 +165,7 @@ static int lm3692x_fault_check(struct lm3692x_led *led)
 	return read_buf;
 }
 
-static int lm3692x_brightness_set(struct led_classdev *led_cdev,
-				enum led_brightness brt_val)
-{
-	struct lm3692x_led *led =
-			container_of(led_cdev, struct lm3692x_led, led_dev);
-	int ret;
-	int led_brightness_lsb = (brt_val >> 5);
-
-	mutex_lock(&led->lock);
-
-	ret = lm3692x_fault_check(led);
-	if (ret) {
-		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: %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: %d\n", ret);
-		goto out;
-	}
-out:
-	mutex_unlock(&led->lock);
-	return ret;
-}
-
-static int lm3692x_init(struct lm3692x_led *led)
+static int lm3692x_leds_enable(struct lm3692x_led *led)
 {
 	int enable_state;
 	int ret, reg_ret;

@@ -321,6 +288,39 @@ static int lm3692x_init(struct lm3692x_led *led)
 	return ret;
 }
 
+static int lm3692x_brightness_set(struct led_classdev *led_cdev,
+				enum led_brightness brt_val)
+{
+	struct lm3692x_led *led =
+			container_of(led_cdev, struct lm3692x_led, led_dev);
+	int ret;
+	int led_brightness_lsb = (brt_val >> 5);
+
+	mutex_lock(&led->lock);
+
+	ret = lm3692x_fault_check(led);
+	if (ret) {
+		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: %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: %d\n", ret);
+		goto out;
+	}
+out:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
 static enum led_brightness lm3692x_max_brightness(struct lm3692x_led *led,
 						  u32 max_cur)
 {

@@ -462,7 +462,7 @@ static int lm3692x_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	ret = lm3692x_init(led);
+	ret = lm3692x_leds_enable(led);
 	if (ret)
 		return ret;

Signature · 1 line

2.23.0

Guido Günther

author

2019-12-25 11:16 UTC #3 lore raw

PATCH 2/3 leds: lm3692x: Split out lm3692x_leds_disable

In reply to Guido Günther

Move the relevant parts out of lm3692x_remove() and call it from there. No functional change.

Signed-off-by Guido Günther <[email protected]>

---
 drivers/leds/leds-lm3692x.c | 42 +++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 16 deletions(-)

drivers/leds/leds-lm3692x.c +26 ~~-16~~

index 7bf97ee6aa92..d1bd9ae4e7ab 100644
@@ -288,6 +288,30 @@ static int lm3692x_leds_enable(struct lm3692x_led *led)
 	return ret;
 }
 
+static int lm3692x_leds_disable(struct lm3692x_led *led)
+{
+	int ret;
+
+	ret = regmap_update_bits(led->regmap, LM3692X_EN, LM3692X_DEVICE_EN, 0);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed to disable regulator: %d\n",
+			ret);
+		return ret;
+	}
+
+	if (led->enable_gpio)
+		gpiod_direction_output(led->enable_gpio, 0);
+
+	if (led->regulator) {
+		ret = regulator_disable(led->regulator);
+		if (ret)
+			dev_err(&led->client->dev,
+				"Failed to disable regulator: %d\n", ret);
+	}
+
+	return ret;
+}
+
 static int lm3692x_brightness_set(struct led_classdev *led_cdev,
 				enum led_brightness brt_val)
 {

@@ -474,23 +498,9 @@ static int lm3692x_remove(struct i2c_client *client)
 	struct lm3692x_led *led = i2c_get_clientdata(client);
 	int ret;
 
-	ret = regmap_update_bits(led->regmap, LM3692X_EN, LM3692X_DEVICE_EN, 0);
-	if (ret) {
-		dev_err(&led->client->dev, "Failed to disable regulator: %d\n",
-			ret);
+	ret = lm3692x_leds_disable(led);
+	if (ret)
 		return ret;
-	}
-
-	if (led->enable_gpio)
-		gpiod_direction_output(led->enable_gpio, 0);
-
-	if (led->regulator) {
-		ret = regulator_disable(led->regulator);
-		if (ret)
-			dev_err(&led->client->dev,
-				"Failed to disable regulator: %d\n", ret);
-	}
-
 	mutex_destroy(&led->lock);
 
 	return 0;

Signature · 1 line

2.23.0

Guido Günther

author

2019-12-25 11:16 UTC #5 lore raw

PATCH 3/3 leds: lm3692x: Disable chip on brightness 0

In reply to Guido Günther

Otherwise there's a noticeable glow even with brightness 0. Also turning off the regulator can save additional power.

Signed-off-by Guido Günther <[email protected]>

---
 drivers/leds/leds-lm3692x.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/leds/leds-lm3692x.c +16 -0

index d1bd9ae4e7ab..183614744b92 100644
@@ -117,6 +117,7 @@ struct lm3692x_led {
 	int model_id;
 
 	u8 boost_ctrl, brightness_ctrl;
+	bool enabled;
 };
 
 static const struct reg_default lm3692x_reg_defs[] = {

@@ -170,6 +171,9 @@ static int lm3692x_leds_enable(struct lm3692x_led *led)
 	int enable_state;
 	int ret, reg_ret;
 
+	if (led->enabled)
+		return 0;
+
 	if (led->regulator) {
 		ret = regulator_enable(led->regulator);
 		if (ret) {

@@ -271,6 +275,7 @@ static int lm3692x_leds_enable(struct lm3692x_led *led)
 	ret = regmap_update_bits(led->regmap, LM3692X_EN, LM3692X_ENABLE_MASK,
 				 enable_state | LM3692X_DEVICE_EN);
 
+	led->enabled = true;
 	return ret;
 out:
 	dev_err(&led->client->dev, "Fail writing initialization values\n");

@@ -292,6 +297,9 @@ static int lm3692x_leds_disable(struct lm3692x_led *led)
 {
 	int ret;
 
+	if (!led->enabled)
+		return 0;
+
 	ret = regmap_update_bits(led->regmap, LM3692X_EN, LM3692X_DEVICE_EN, 0);
 	if (ret) {
 		dev_err(&led->client->dev, "Failed to disable regulator: %d\n",

@@ -309,6 +317,7 @@ static int lm3692x_leds_disable(struct lm3692x_led *led)
 				"Failed to disable regulator: %d\n", ret);
 	}
 
+	led->enabled = false;
 	return ret;
 }

@@ -322,6 +331,13 @@ static int lm3692x_brightness_set(struct led_classdev *led_cdev,
 
 	mutex_lock(&led->lock);
 
+	if (brt_val == 0) {
+		ret = lm3692x_leds_disable(led);
+		goto out;
+	} else {
+		lm3692x_leds_enable(led);
+	}
+
 	ret = lm3692x_fault_check(led);
 	if (ret) {
 		dev_err(&led->client->dev, "Cannot read/clear faults: %d\n",

Signature · 1 line

2.23.0

Pavel Machek

2019-12-26 10:14 UTC #8 lore raw

In reply to Guido Günther

Quoted 2 lines · Guido Günther

Otherwise there's a hardly noticeable glow even with brightness 0. Also turning off the regulator can save additional power.

I guess it is of too low intensity to be used, for example in completely dark room?

Best regards,
Pavel

Signature · 2 lines

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

Pavel Machek

Acked-by on v1 1/3

2019-12-26 10:15 UTC #2 lore raw

Re: [PATCH 1/3] leds: lm3692x: Move lm3692x_init and rename to lm3692x_leds_enable

In reply to Guido Günther

Quoted 5 lines · Guido Günther

This moves lm3692x_init so it can be used from lm3692x_brightness_set. Rename to lm3692_leds_enable to pair up with lm3692x_leds_disable. No functional change.
Signed-off-by Guido Günther <[email protected]>

Acked-by Pavel Machek <[email protected]>

Signature · 2 lines

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

Pavel Machek

Acked-by on v1 2/3

2019-12-26 10:16 UTC #4 lore raw

Re: [PATCH 2/3] leds: lm3692x: Split out lm3692x_leds_disable

In reply to Guido Günther

Quoted 4 lines · Guido Günther

Move the relevant parts out of lm3692x_remove() and call it from there. No functional change.
Signed-off-by Guido Günther <[email protected]>

Acked-by Pavel Machek <[email protected]>

Pavel

Signature · 2 lines

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

Pavel Machek

2019-12-26 10:30 UTC #6 lore raw

In reply to Guido Günther

Quoted 2 lines · Guido Günther

Otherwise there's a noticeable glow even with brightness 0. Also turning off the regulator can save additional power.

Ok, this will make set brightness slower, but I guess it makes sense.

Can you try to toggling brightness quickly from userspace, maybe even from two threads, to ensure nothing really goes wrong there?

Hmm. And if this is independend of the other series, tell me and I can apply v2.

Best regards,
Pavel

Signature · 2 lines

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

Guido Günther

author

2019-12-27 12:56 UTC #7 lore raw

Hi Pavel,

Quoted 8 lines · Pavel Machek

Quoted 2 lines · Guido Günther
Otherwise there's a noticeable glow even with brightness 0. Also turning off the regulator can save additional power.
Ok, this will make set brightness slower, but I guess it makes sense.
Can you try to toggling brightness quickly from userspace, maybe even from two threads, to ensure nothing really goes wrong there?

I ran brightness setting in a tight loop from 0 to max from two threads 100 times without issues.

Quoted 3 lines

Hmm. And if this is independend of the other series, tell me and I can apply v2.

There's minor merge conflict so to make sure this does not get dependent on the order i've folded this to the end of the

"leds: lm3692x: Allow to set ovp and brigthness mode"

series with your Acked-by's folded in. I'll wait a couple of days and then send out a v3.

Cheers and thanks for having a look,
-- Guido

Quoted 6 lines

Best regards,
Pavel

Signature · 2 lines

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