leds: lm3692x: Allow to set ovp and brigthness mode

PATCH Superseded Guido Günther 4 participants 15 messages Dec 16, 2019 – Dec 27, 2019

Subsystems (2)

Device tree bindings LED

lore

Share & tools

lore

Guido Günther

author

2019-12-16 12:28 UTC #0 lore raw

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

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

Guido Günther (2):
dt: bindings: lm3692x: Document new properties
leds: lm3692x: Allow to set ovp and brigthness mode

 .../devicetree/bindings/leds/leds-lm3692x.txt |  4 ++
 drivers/leds/leds-lm3692x.c                   | 43 ++++++++++++++++---
 2 files changed, 41 insertions(+), 6 deletions(-)

Signature · 1 line

2.23.0

Guido Günther

author

2019-12-16 12:28 UTC #1 lore raw

PATCH 1/2 dt: bindings: lm3692x: Document new properties

We allow configuration of brightness mode and over voltage protection.

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

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

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

index 4c2d923f8758..f195e8b45d85 100644
@@ -18,6 +18,10 @@ 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,overvoltage-volts: Overvoltage protection in Volts, can
+	    be 0 (unprotected), 21, 25 or 29V
 
 Required child properties:
 	- reg : 0 - Will enable all LED sync paths

Signature · 1 line

2.23.0

Dan Murphy

2019-12-17 12:44 UTC #2 lore raw

Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties

Guido

Thanks for the patch

Quoted 13 lines · Guido Günther

We allow configuration of brightness mode and over voltage protection.
Signed-off-by Guido Günther <[email protected]>
---
  Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++++
  1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/leds/leds-lm3692x.txt +0 -0
index 4c2d923f8758..f195e8b45d85 100644

Documentation/devicetree/bindings/leds/leds-lm3692x.txt Required properties: · line 25

Quoted 8 lines

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

@@ -18,6 +18,10 @@ 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,overvoltage-volts: Overvoltage protection in Volts, can
+	    be 0 (unprotected), 21, 25 or 29V

Can you show examples of these in the DT binding?

Dan

Pavel Machek

2019-12-21 19:15 UTC #3 lore raw

Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties

In reply to Guido Günther

Quoted 6 lines · Guido Günther

We allow configuration of brightness mode and over voltage protection.
Signed-off-by Guido Günther <[email protected]>
+++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt

Documentation/devicetree/bindings/leds/leds-lm3692x.txt Required properties: · line 25

Quoted 8 lines

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

@@ -18,6 +18,10 @@ 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,overvoltage-volts: Overvoltage protection in Volts, can
+	    be 0 (unprotected), 21, 25 or 29V

We usually use microvolts in various device tree properties...

Exponential mapping s not really property of the hardware, is it? Does it belong here or somewhere in the backlight binding?

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-24 11:45 UTC #4 lore raw

Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties

Hi,

Quoted 4 lines · Pavel Machek

Quoted 3 lines · Guido Günther
We allow configuration of brightness mode and over voltage protection.

Quoted 13 lines

Documentation/devicetree/bindings/leds/leds-lm3692x.txt Required properties: · line 25
Quoted 11 lines
Signed-off-by Guido Günther <[email protected]>
+++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
Documentation/devicetree/bindings/leds/leds-lm3692x.txt +4 -0
@@ -18,6 +18,10 @@ 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,overvoltage-volts: Overvoltage protection in Volts, can
+	    be 0 (unprotected), 21, 25 or 29V
 
We usually use microvolts in various device tree properties...

Make sense.

Quoted 2 lines

Exponential mapping s not really property of the hardware, is it? Does it belong here or somewhere in the backlight binding?

I opted for having it with the hardware since the property can't be configured per backlight led strip individually.

Cheers,
-- Guido

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

Rob Herring

maintainer

2019-12-26 19:19 UTC #5 lore raw

Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties

In reply to Guido Günther

Quoted 13 lines · Guido Günther

We allow configuration of brightness mode and over voltage protection.
Signed-off-by Guido Günther <[email protected]>
---
 Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/leds/leds-lm3692x.txt +0 -0
index 4c2d923f8758..f195e8b45d85 100644

Documentation/devicetree/bindings/leds/leds-lm3692x.txt Required properties: · line 24

Quoted 7 lines

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

@@ -18,6 +18,10 @@ 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,overvoltage-volts: Overvoltage protection in Volts, can
+	    be 0 (unprotected), 21, 25 or 29V

'-microvolt' is the standard unit suffix.

Quoted 6 lines

Documentation/devicetree/bindings/leds/leds-lm3692x.txt +0 -0
 Required child properties:
 	- reg : 0 - Will enable all LED sync paths
Signature · 1 line
2.23.0

Guido Günther

author

2019-12-27 10:16 UTC #6 lore raw

Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties

Hi,

Quoted 9 lines · Rob Herring

Quoted 8 lines · Guido Günther
We allow configuration of brightness mode and over voltage protection.
Signed-off-by Guido Günther <[email protected]>
---
 Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++++
 1 file changed, 4 insertions(+)

Quoted 14 lines

Documentation/devicetree/bindings/leds/leds-lm3692x.txt Required properties: · line 24
Quoted 12 lines
Documentation/devicetree/bindings/leds/leds-lm3692x.txt +4 -0
index 4c2d923f8758..f195e8b45d85 100644
@@ -18,6 +18,10 @@ 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,overvoltage-volts: Overvoltage protection in Volts, can
+	    be 0 (unprotected), 21, 25 or 29V
'-microvolt' is the standard unit suffix.

Fixed in v2:

https://lore.kernel.org/linux-leds/20191226100615.GA4033@amd/T/#u

Cheers,
-- Guido

Quoted 8 lines

Quoted 7 lines
Documentation/devicetree/bindings/leds/leds-lm3692x.txt +0 -0
 Required child properties:
 	- reg : 0 - Will enable all LED sync paths
Signature · 1 line
2.23.0

Guido Günther

author

2019-12-16 12:28 UTC #7 lore raw

PATCH 2/2 leds: lm3692x: Allow to set ovp and brigthness mode

In reply to Guido Günther

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

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

---
 drivers/leds/leds-lm3692x.c | 43 +++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

drivers/leds/leds-lm3692x.c +37 -6

index 8b408102e138..2c084b333628 100644
@@ -114,6 +114,7 @@ struct lm3692x_led {
 	struct regulator *regulator;
 	int led_enable;
 	int model_id;
+	u8 boost_ctrl, brightness_ctrl;
 };
 
 static const struct reg_default lm3692x_reg_defs[] = {

@@ -249,10 +250,7 @@ static int lm3692x_init(struct lm3692x_led *led)
 	if (ret)
 		goto out;
 
-	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,
-			LM3692X_BOOST_SW_1MHZ |
-			LM3692X_BOOST_SW_NO_SHIFT |
-			LM3692X_OCP_PROT_1_5A);
+	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL, led->boost_ctrl);
 	if (ret)
 		goto out;

@@ -268,8 +266,7 @@ static int lm3692x_init(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;

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

@@ -350,6 +349,38 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
 		led->regulator = NULL;
 	}
 
+	led->boost_ctrl = LM3692X_BOOST_SW_1MHZ |
+		LM3692X_BOOST_SW_NO_SHIFT |
+		LM3692X_OCP_PROT_1_5A;
+	ret = device_property_read_u32(&led->client->dev,
+				       "ti,overvoltage-volts", &ovp);
+	if (!ret) {
+		switch (ovp) {
+		case 0:
+			break;
+		case 22:
+			led->boost_ctrl |= LM3692X_OVP_21V;
+			break;
+		case 25:
+			led->boost_ctrl |= LM3692X_OVP_25V;
+			break;
+		case 29:
+			led->boost_ctrl |= LM3692X_OVP_29V;
+			break;
+		default:
+			dev_err(&led->client->dev, "Invalid OVP %d\n", ovp);
+			return -EINVAL;
+		}
+	}
+	dev_dbg(&led->client->dev, "OVP: %dV", ovp);
+
+	led->brightness_ctrl = LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN;
+	exp_mode = device_property_read_bool(&led->client->dev,
+				     "ti,brightness-mapping-exponential");
+	dev_dbg(&led->client->dev, "Exponential brightness: %d", exp_mode);
+	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

Guido Günther

author

2019-12-17 15:41 UTC #9 lore raw

Hi Dan,

Quoted 7 lines · Dan Murphy

Guido
Quoted 2 lines · Guido Günther
Overvoltage protection and brightness mode are currently hardcoded as disabled in the driver. Make these configurable via DT.
Can we split these up to two separate patch series?

Sure, should the binding doc updates be split as well?

Quoted 95 lines

We are adding 2 separate features and if something is incorrect with one of the changes it is a bit hard to debug.
Quoted 18 lines
Signed-off-by Guido Günther <[email protected]>
---
  drivers/leds/leds-lm3692x.c | 43 +++++++++++++++++++++++++++++++------
  1 file changed, 37 insertions(+), 6 deletions(-)
drivers/leds/leds-lm3692x.c +1 -0
index 8b408102e138..2c084b333628 100644
@@ -114,6 +114,7 @@ struct lm3692x_led {
  	struct regulator *regulator;
  	int led_enable;
  	int model_id;
+	u8 boost_ctrl, brightness_ctrl;
  };
  static const struct reg_default lm3692x_reg_defs[] = {
drivers/leds/leds-lm3692x.c static int lm3692x_init(struct lm3692x_led *led) · line 254
Quoted 9 lines
drivers/leds/leds-lm3692x.c +1 -4
@@ -249,10 +250,7 @@ static int lm3692x_init(struct lm3692x_led *led)
  	if (ret)
  		goto out;
-	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,
-			LM3692X_BOOST_SW_1MHZ |
-			LM3692X_BOOST_SW_NO_SHIFT |
-			LM3692X_OCP_PROT_1_5A);
+	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL, led->boost_ctrl);
  	if (ret)
  		goto out;
regmap_update_bits
drivers/leds/leds-lm3692x.c static int lm3692x_init(struct lm3692x_led *led) · line 270
Quoted 8 lines
drivers/leds/leds-lm3692x.c +1 -2
@@ -268,8 +266,7 @@ static int lm3692x_init(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;
regmap_update_bits
Quoted 9 lines
drivers/leds/leds-lm3692x.c +2 -0
@@ -326,6 +323,8 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
  {
  	struct fwnode_handle *child = NULL;
  	struct led_init_data init_data = {};
+	u32 ovp = 0;
+	bool exp_mode;
  	int ret;
  	led->enable_gpio = devm_gpiod_get_optional(&led->client->dev,
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 353
Quoted 5 lines
drivers/leds/leds-lm3692x.c +3 -0
@@ -350,6 +349,38 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
  		led->regulator = NULL;
  	}
+	led->boost_ctrl = LM3692X_BOOST_SW_1MHZ |
+		LM3692X_BOOST_SW_NO_SHIFT |
+		LM3692X_OCP_PROT_1_5A;
Make this a #define and then it can be reused as a mask for regmap_update_bits
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 357
Quoted 3 lines
drivers/leds/leds-lm3692x.c +3 -0
+	ret = device_property_read_u32(&led->client->dev,
+				       "ti,overvoltage-volts", &ovp);
+	if (!ret) {
if (ret)
set boost_ctrl to default value since the default is not 0
led->boost_ctrl |= LM3692X_OVP_29V;
else
do case
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 361
Quoted 4 lines
drivers/leds/leds-lm3692x.c +4 -0
+		switch (ovp) {
+		case 0:
+			break;
+		case 22:
If the value is 21v why is this case 22? DT binding says 21 is the first value
Quoted 5 lines
drivers/leds/leds-lm3692x.c +5 -0
+			led->boost_ctrl |= LM3692X_OVP_21V;
+			break;
+		case 25:
+			led->boost_ctrl |= LM3692X_OVP_25V;
+			break;
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 376
Quoted 10 lines
drivers/leds/leds-lm3692x.c +10 -0
+		case 29:
+			led->boost_ctrl |= LM3692X_OVP_29V;
+			break;
+		default:
+			dev_err(&led->client->dev, "Invalid OVP %d\n", ovp);
+			return -EINVAL;
+		}
+	}
+	dev_dbg(&led->client->dev, "OVP: %dV", ovp);
+
extra debug statement

Quoted 7 lines

drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 377
Quoted 1 line
drivers/leds/leds-lm3692x.c +1 -0
+	led->brightness_ctrl = LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN;
Same comment as before on the #define
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 380
Quoted 3 lines
drivers/leds/leds-lm3692x.c +3 -0
+	exp_mode = device_property_read_bool(&led->client->dev,
+				     "ti,brightness-mapping-exponential");
+	dev_dbg(&led->client->dev, "Exponential brightness: %d", exp_mode);
extra debug statement

They're not extra but meant to ease debugging the driver long therm but i can drop these if that's not wanted. The rest makes a lot of sense. Thanks a lot for having a look so promptly!

Cheers,
-- Guido

Quoted 4 lines

Dan

Dan Murphy

2019-12-17 17:04 UTC #10 lore raw

Guido

Quoted 9 lines · Guido Günther

Hi Dan,
Quoted 6 lines · Dan Murphy
Guido
Quoted 2 lines · Guido Günther
Overvoltage protection and brightness mode are currently hardcoded as disabled in the driver. Make these configurable via DT.
Can we split these up to two separate patch series?
Sure, should the binding doc updates be split as well?

Yes.

<snip>

Quoted 4 lines

Quoted 1 line
extra debug statement
They're not extra but meant to ease debugging the driver long therm but i can drop these if that's not wanted. The rest makes a lot of sense. Thanks a lot for having a look so promptly!

Yes please remove those we don't need extra noise in the log.

If someone wants to debug this then they can add the statements themselves

Dan

Pavel Machek

2019-12-21 19:17 UTC #11 lore raw

In reply to Dan Murphy

Quoted 7 lines · Dan Murphy

Guido
Quoted 2 lines · Guido Günther
Overvoltage protection and brightness mode are currently hardcoded as disabled in the driver. Make these configurable via DT.
Can we split these up to two separate patch series?

No need to split it up to separate _patch series_. I actually believe single patch is okay here.

Thanks,
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-24 11:30 UTC #12 lore raw

In reply to Dan Murphy

Hi Dan,
I'm a bit confused about the regmap_write -> regmap_update_bits switch (see below), maybe you can shed some light on it?

Quoted 32 lines · Dan Murphy

Guido
Quoted 2 lines · Guido Günther
Overvoltage protection and brightness mode are currently hardcoded as disabled in the driver. Make these configurable via DT.
Can we split these up to two separate patch series?
We are adding 2 separate features and if something is incorrect with one of the changes it is a bit hard to debug.
Quoted 21 lines
Signed-off-by Guido Günther <[email protected]>
---
  drivers/leds/leds-lm3692x.c | 43 +++++++++++++++++++++++++++++++------
  1 file changed, 37 insertions(+), 6 deletions(-)
drivers/leds/leds-lm3692x.c +1 -1
index 8b408102e138..2c084b333628 100644
@@ -114,6 +114,7 @@ struct lm3692x_led {
  	struct regulator *regulator;
  	int led_enable;
  	int model_id;
+	u8 boost_ctrl, brightness_ctrl;
  };
  static const struct reg_default lm3692x_reg_defs[] = {
@@ -249,10 +250,7 @@ static int lm3692x_init(struct lm3692x_led *led)
  	if (ret)
  		goto out;
-	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,

Quoted 8 lines

drivers/leds/leds-lm3692x.c static int lm3692x_init(struct lm3692x_led *led) · line 255
Quoted 6 lines
drivers/leds/leds-lm3692x.c +1 -3
-			LM3692X_BOOST_SW_1MHZ |
-			LM3692X_BOOST_SW_NO_SHIFT |
-			LM3692X_OCP_PROT_1_5A);
+	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL, led->boost_ctrl);
  	if (ret)
  		goto out;
regmap_update_bits

The driver is writing full register values (regmap_write) here as before, do you want that to change? Likely i'm overlooking something.

Quoted 9 lines

drivers/leds/leds-lm3692x.c static int lm3692x_init(struct lm3692x_led *led) · line 270

Quoted 8 lines

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

@@ -268,8 +266,7 @@ static int lm3692x_init(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;

regmap_update_bits

Same here.

Quoted 22 lines

Quoted 9 lines
drivers/leds/leds-lm3692x.c +2 -0
@@ -326,6 +323,8 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
  {
  	struct fwnode_handle *child = NULL;
  	struct led_init_data init_data = {};
+	u32 ovp = 0;
+	bool exp_mode;
  	int ret;
  	led->enable_gpio = devm_gpiod_get_optional(&led->client->dev,
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 353
Quoted 5 lines
drivers/leds/leds-lm3692x.c +3 -0
@@ -350,6 +349,38 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
  		led->regulator = NULL;
  	}
+	led->boost_ctrl = LM3692X_BOOST_SW_1MHZ |
+		LM3692X_BOOST_SW_NO_SHIFT |
+		LM3692X_OCP_PROT_1_5A;
Make this a #define and then it can be reused as a mask for regmap_update_bits
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 357
Quoted 3 lines
drivers/leds/leds-lm3692x.c +3 -0
+	ret = device_property_read_u32(&led->client->dev,
+				       "ti,overvoltage-volts", &ovp);
+	if (!ret) {
if (ret)

Quoted 8 lines

set boost_ctrl to default value since the default is not 0
led->boost_ctrl |= LM3692X_OVP_29V;
else
do case

Fixed.

Quoted 6 lines

drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 361
Quoted 4 lines
drivers/leds/leds-lm3692x.c +4 -0
+		switch (ovp) {
+		case 0:
+			break;
+		case 22:
If the value is 21v why is this case 22? DT binding says 21 is the first value

Fixed, also added the 17V for the case where both bits a are 0.

Quoted 8 lines

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

+			led->boost_ctrl |= LM3692X_OVP_21V;
+			break;
+		case 25:
+			led->boost_ctrl |= LM3692X_OVP_25V;
+			break;
+		case 29:
+			led->boost_ctrl |= LM3692X_OVP_29V;
+			break;

Quoted 8 lines

drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 376
Quoted 7 lines
drivers/leds/leds-lm3692x.c +7 -0
+		default:
+			dev_err(&led->client->dev, "Invalid OVP %d\n", ovp);
+			return -EINVAL;
+		}
+	}
+	dev_dbg(&led->client->dev, "OVP: %dV", ovp);
+
extra debug statement

dropped.

Quoted 7 lines

drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 377
Quoted 1 line
drivers/leds/leds-lm3692x.c +1 -0
+	led->brightness_ctrl = LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN;
Same comment as before on the #define
drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 380
Quoted 3 lines
drivers/leds/leds-lm3692x.c +3 -0
+	exp_mode = device_property_read_bool(&led->client->dev,
+				     "ti,brightness-mapping-exponential");
+	dev_dbg(&led->client->dev, "Exponential brightness: %d", exp_mode);
extra debug statement

dropped.

Cheers and thanks for the comments,
-- Guido

Quoted 4 lines

Dan

Pavel Machek

2019-12-21 19:18 UTC #13 lore raw

In reply to Guido Günther

Hi!

Quoted 2 lines

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

What exactly is overvoltage protection good for? Should we default to 29V if we have no other information?

drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 363

Quoted 11 lines

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

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

+	ret = device_property_read_u32(&led->client->dev,
+				       "ti,overvoltage-volts", &ovp);
+	if (!ret) {
+		switch (ovp) {
+		case 0:
+			break;
+		case 22:
+			led->boost_ctrl |= LM3692X_OVP_21V;
+			break;

Should be case 21.
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-24 11:26 UTC #14 lore raw

Hi,

Quoted 7 lines · Pavel Machek

Hi!
Quoted 2 lines
Overvoltage protection and brightness mode are currently hardcoded as disabled in the driver. Make these configurable via DT.
What exactly is overvoltage protection good for? Should we default to 29V if we have no other information?

The OVP protects the IC from overvoltage conditions on the output side. While looking at the manual again I noticed that i misremembered the '00' value which means 17V - not unprotected. Also the chip defaults to 29V OVP so i've adjusted that too.

Cheers,
-- Guido

Quoted 18 lines

drivers/leds/leds-lm3692x.c static int lm3692x_probe_dt(struct lm3692x_led *led) · line 363
Quoted 12 lines
Signed-off-by Guido Günther <[email protected]>
drivers/leds/leds-lm3692x.c +9 -0
+	ret = device_property_read_u32(&led->client->dev,
+				       "ti,overvoltage-volts", &ovp);
+	if (!ret) {
+		switch (ovp) {
+		case 0:
+			break;
+		case 22:
+			led->boost_ctrl |= LM3692X_OVP_21V;
+			break;
Should be case 21.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html