leds: lm3692x: Allow to set ovp and brigthness mode

PATCH In review Guido Günther 2 participants 9 messages Jan 6, 2020 – Jan 8, 2020

Subsystems (2)

Device tree bindings LED

lore

Share & tools

lore

Guido Günther

author

2020-01-06 15:49 UTC #0 lore raw

v4 cover leds: lm3692x: Allow to set ovp and brigthness mode

Overvoltage protection and brightness mode are currently hardcoded as 29V and disabled in the driver. Make these configurable via DT.

This v4 moves the exponential brightness mode to the back of the series as per Pavel's request:

https://lore.kernel.org/linux-next/20200106103233.GA32426@amd/T/#m93270a9bf10b88e060f4e4cf5701c527476de985

The end result is identical and i've tested everything still works when dropping the last to patches and checked compiltion via

git rebase -i ... -exec 'make ... Image dtbs'

Patches are against linux-leds-next.

Changes from v3
- Move exponential mode patches to the back of the series
https://lore.kernel.org/linux-next/20200106103233.GA32426@amd/T/#m93270a9bf10b88e060f4e4cf5701c527476de985
- Add Rob's Reviewed-by:, thanks!

Changes from v2
- As per review comment from Pavel Machek
https://lore.kernel.org/linux-leds/20191226100615.GA4033@amd/T/#u
- Use default value in DT example
https://lore.kernel.org/linux-leds/20191226100842.GC4033@amd/T/#u
- Use uppercase LED in commit message
https://lore.kernel.org/linux-leds/20191226101336.GD4033@amd/T/#u
- Fix typo in commit message
- Use correct return value when checking if property is present
- Fold in

    https://lore.kernel.org/linux-leds/20191226101419.GE4033@amd/T/#t

- Add Acked-By's from Pavel Machek, thanks!

Changes from v1
- As per review comments by Dan Murphy
https://lore.kernel.org/linux-leds/[email protected]/
- Split commits per propoerty
- Add new properties to DT example too
- Drop dev_dbg() statements
- ovp: fix 21V value parsing
- ovp: Set correct default value if DT parsing fails
- As per review comments by Pavel Machek
https://lore.kernel.org/linux-leds/20191221191515.GF32732@amd/
- Fix defaults (which is 29V)
- Use uV as Unit for ovp property
- Change property name to 'ti,ovp-microvolt' to make it shorter
- Honor led-max-microamp to not exceed the maximum led current

Guido Günther (6):
leds: lm3692x: Make sure we don't exceed the maximum led current
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
dt: bindings: lm3692x: Add ti,brightness-mapping-exponential property
leds: lm3692x: Allow to configure brigthness mode

 .../devicetree/bindings/leds/leds-lm3692x.txt |   3 +
 drivers/leds/leds-lm3692x.c                   | 165 ++++++++++++------
 2 files changed, 113 insertions(+), 55 deletions(-)

Signature · 1 line

2.23.0

Guido Günther

author

2020-01-06 15:49 UTC #1 lore raw

PATCH v4 1/6 leds: lm3692x: Make sure we don't exceed the maximum led current

The current is given by the formular from page 12 of
https://www.ti.com/lit/ds/symlink/lm36922.pdf. We use this to limit the led's max_brightness using the led-max-microamp DT property.

The formula for the lm36923 is identical according to the data sheet.

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

Acked-by Pavel Machek <[email protected]>

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

drivers/leds/leds-lm3692x.c +19 -1

index 28973cc5a6cc..1b056af60bd6 100644
@@ -6,6 +6,7 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/leds.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>

@@ -321,11 +322,24 @@ static int lm3692x_init(struct lm3692x_led *led)
 	return ret;
 }
 
+static enum led_brightness lm3692x_max_brightness(struct lm3692x_led *led,
+						  u32 max_cur)
+{
+	u32 max_code;
+
+	/* see p.12 of LM36922 data sheet for brightness formula */
+	max_code = ((max_cur * 1000) - 37806) / 12195;
+	if (max_code > 0x7FF)
+		max_code = 0x7FF;
+
+	return max_code >> 3;
+}
+
 static int lm3692x_probe_dt(struct lm3692x_led *led)
 {
 	struct fwnode_handle *child = NULL;
 	struct led_init_data init_data = {};
-	u32 ovp;
+	u32 ovp, max_cur;
 	int ret;
 
 	led->enable_gpio = devm_gpiod_get_optional(&led->client->dev,

@@ -391,6 +405,10 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
 		return ret;
 	}
 
+	ret = fwnode_property_read_u32(child, "led-max-microamp", &max_cur);
+	led->led_dev.max_brightness = ret ? LED_FULL :
+		lm3692x_max_brightness(led, max_cur);
+
 	init_data.fwnode = child;
 	init_data.devicename = led->client->name;
 	init_data.default_label = ":";

Signature · 1 line

2.23.0

Guido Günther

author

2020-01-06 15:49 UTC #2 lore raw

PATCH v4 2/6 leds: lm3692x: Move lm3692x_init and rename to lm3692x_leds_enable

In reply to 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]>

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

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

index 1b056af60bd6..f7fdfaee5ac5 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;

@@ -322,6 +289,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)
 {

@@ -451,7 +451,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

2020-01-06 15:49 UTC #3 lore raw

PATCH v4 3/6 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]>

Acked-by Pavel Machek <[email protected]>

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

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

index f7fdfaee5ac5..1254695d7e94 100644
@@ -289,6 +289,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)
 {

@@ -463,23 +487,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

2020-01-06 15:49 UTC #4 lore raw

PATCH v4 4/6 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 | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

drivers/leds/leds-lm3692x.c +17 -1

index 1254695d7e94..28a51aeb28de 100644
@@ -116,7 +116,8 @@ struct lm3692x_led {
 	int led_enable;
 	int model_id;
 
-	u8 boost_ctrl;
+	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) {

@@ -272,6 +276,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");

@@ -293,6 +298,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",

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

@@ -323,6 +332,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

Guido Günther

author

2020-01-06 15:49 UTC #5 lore raw

PATCH v4 5/6 dt: bindings: lm3692x: Add ti,brightness-mapping-exponential property

In reply to Guido Günther

This allows to select exponential brightness mode instead of the default linear mode.

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

Reviewed-by Rob Herring <[email protected]>

---
 Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 3 +++
 1 file changed, 3 insertions(+)

Documentation/devicetree/bindings/leds/leds-lm3692x.txt +3 -0

index 501468aa4d38..b8939fdd19d6 100644
@@ -18,6 +18,8 @@ Required properties:
 Optional properties:
 	- enable-gpios : gpio pin to enable/disable the device.
 	- vled-supply : LED supply
+	- ti,brightness-mapping-exponential: Whether to use exponential
+	    brightness mapping
 	- ti,ovp-microvolt: Overvoltage protection in
 	    micro-volt, can be 17000000, 21000000, 25000000 or
 	    29000000. If ti,ovp-microvolt is not specified it

@@ -51,6 +53,7 @@ led-controller@36 {
 	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
 	vled-supply = <&vbatt>;
 	ti,ovp-microvolt = <29000000>;
+	ti,brightness-mapping-exponential;
 
 	led@0 {
 		reg = <0>;

Signature · 1 line

2.23.0

Guido Günther

author

2020-01-06 15:49 UTC #6 lore raw

PATCH v4 6/6 leds: lm3692x: Allow to configure brigthness mode

In reply to Guido Günther

Brightness mode is currently hardcoded as linear in the driver. Make exponential mode configurable via DT.

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

---
 drivers/leds/leds-lm3692x.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

drivers/leds/leds-lm3692x.c +14 -3

index 28a51aeb28de..933b786cfaec 100644
@@ -239,8 +239,7 @@ static int lm3692x_leds_enable(struct lm3692x_led *led)
 	if (ret)
 		goto out;
 
-	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL,
-			LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN);
+	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, led->brightness_ctrl);
 	if (ret)
 		goto out;

@@ -368,7 +367,12 @@ static enum led_brightness lm3692x_max_brightness(struct lm3692x_led *led,
 	u32 max_code;
 
 	/* see p.12 of LM36922 data sheet for brightness formula */
-	max_code = ((max_cur * 1000) - 37806) / 12195;
+	if (led->brightness_ctrl & LM3692X_MAP_MODE_EXP) {
+		/*  228 =~ 1.0 / log2(1.003040572) */
+		max_code = ilog2(max_cur/50) * 228;
+	} else {
+		max_code = ((max_cur * 1000) - 37806) / 12195;
+	}
 	if (max_code > 0x7FF)
 		max_code = 0x7FF;

@@ -380,6 +384,7 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
 	struct fwnode_handle *child = NULL;
 	struct led_init_data init_data = {};
 	u32 ovp, max_cur;
+	bool exp_mode;
 	int ret;
 
 	led->enable_gpio = devm_gpiod_get_optional(&led->client->dev,

@@ -430,6 +435,12 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
 		}
 	}
 
+	led->brightness_ctrl = LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN;
+	exp_mode = device_property_read_bool(&led->client->dev,
+				     "ti,brightness-mapping-exponential");
+	if (exp_mode)
+		led->brightness_ctrl |= LM3692X_MAP_MODE_EXP;
+
 	child = device_get_next_child_node(&led->client->dev, child);
 	if (!child) {
 		dev_err(&led->client->dev, "No LED Child node\n");

Signature · 1 line

2.23.0

Pavel Machek

2020-01-07 13:31 UTC #7 lore raw

Exponential LED brightness Re: [PATCH v4 0/6] leds: lm3692x: Allow to set ovp and brigthness mode

In reply to Guido Günther

Hi!

Quoted 10 lines

Overvoltage protection and brightness mode are currently hardcoded as 29V and disabled in the driver. Make these configurable via DT.
This v4 moves the exponential brightness mode to the back of the series as per Pavel's request:
https://lore.kernel.org/linux-next/20200106103233.GA32426@amd/T/#m93270a9bf10b88e060f4e4cf5701c527476de985
The end result is identical and i've tested everything still works when dropping the last to patches and checked compiltion via

Thank you. Applied 1-4 (with some reformatting of changelog, and led->LED).

Exponential mode:

We should decide if LEDs should be linear or not. Most LEDs are linear now, and we may want to make it part of the API. Additional advantage is that linear is "well defined". It is actually quite important for RGB LEDs, because you get wrong colors otherwise.

(Non-linear can have advantages, too... like needing less bits.)

So, my suggestion is to document LEDs as linear, and leave exponential->linear conversion to someone else.

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

2020-01-08 12:40 UTC #8 lore raw

Re: Exponential LED brightness Re: [PATCH v4 0/6] leds: lm3692x: Allow to set ovp and brigthness mode

Hi Pavel,

Quoted 23 lines · Pavel Machek

Hi!
Quoted 10 lines
Overvoltage protection and brightness mode are currently hardcoded as 29V and disabled in the driver. Make these configurable via DT.
This v4 moves the exponential brightness mode to the back of the series as per Pavel's request:
https://lore.kernel.org/linux-next/20200106103233.GA32426@amd/T/#m93270a9bf10b88e060f4e4cf5701c527476de985
The end result is identical and i've tested everything still works when dropping the last to patches and checked compiltion via
Thank you. Applied 1-4 (with some reformatting of changelog, and led->LED).
Exponential mode:
We should decide if LEDs should be linear or not. Most LEDs are linear now, and we may want to make it part of the API. Additional advantage is that linear is "well defined". It is actually quite important for RGB LEDs, because you get wrong colors otherwise.

Quoted 4 lines

(Non-linear can have advantages, too... like needing less bits.)
So, my suggestion is to document LEDs as linear, and leave exponential->linear conversion to someone else.

That would mean doing a conversion in the kernel that can be done by the chip. Would exposing non-linearity like in /sys/class/backlight/<backlight>/scale be an option?
Cheers,
-- Guido

Quoted 7 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