* [PATCH 0/2] leds: lm3692x: Allow to set ovp and brigthness mode @ 2019-12-16 12:28 Guido Günther 2019-12-16 12:28 ` [PATCH 1/2] dt: bindings: lm3692x: Document new properties Guido Günther 2019-12-16 12:28 ` [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther 0 siblings, 2 replies; 15+ messages in thread From: Guido Günther @ 2019-12-16 12:28 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel 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(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] dt: bindings: lm3692x: Document new properties 2019-12-16 12:28 [PATCH 0/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther @ 2019-12-16 12:28 ` Guido Günther 2019-12-17 12:41 ` Dan Murphy ` (2 more replies) 2019-12-16 12:28 ` [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther 1 sibling, 3 replies; 15+ messages in thread From: Guido Günther @ 2019-12-16 12:28 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel We allow configuration of brightness mode and over voltage protection. Signed-off-by: Guido Günther <agx@sigxcpu.org> --- Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt index 4c2d923f8758..f195e8b45d85 100644 --- a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt @@ -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 -- 2.23.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties 2019-12-16 12:28 ` [PATCH 1/2] dt: bindings: lm3692x: Document new properties Guido Günther @ 2019-12-17 12:41 ` Dan Murphy 2019-12-21 19:15 ` Pavel Machek 2019-12-26 19:19 ` Rob Herring 2 siblings, 0 replies; 15+ messages in thread From: Dan Murphy @ 2019-12-17 12:41 UTC (permalink / raw) To: Guido Günther, Jacek Anaszewski, Pavel Machek, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel Guido Thanks for the patch On 12/16/19 6:28 AM, Guido Günther wrote: > We allow configuration of brightness mode and over voltage > protection. > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > --- > Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > index 4c2d923f8758..f195e8b45d85 100644 > --- a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > @@ -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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties 2019-12-16 12:28 ` [PATCH 1/2] dt: bindings: lm3692x: Document new properties Guido Günther 2019-12-17 12:41 ` Dan Murphy @ 2019-12-21 19:15 ` Pavel Machek 2019-12-24 11:45 ` Guido Günther 2019-12-26 19:19 ` Rob Herring 2 siblings, 1 reply; 15+ messages in thread From: Pavel Machek @ 2019-12-21 19:15 UTC (permalink / raw) To: Guido Günther Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 985 bytes --] On Mon 2019-12-16 13:28:05, Guido Günther wrote: > We allow configuration of brightness mode and over voltage > protection. > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > @@ -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 -- (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] 15+ messages in thread
* Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties 2019-12-21 19:15 ` Pavel Machek @ 2019-12-24 11:45 ` Guido Günther 0 siblings, 0 replies; 15+ messages in thread From: Guido Günther @ 2019-12-24 11:45 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel Hi, On Sat, Dec 21, 2019 at 08:15:15PM +0100, Pavel Machek wrote: > On Mon 2019-12-16 13:28:05, Guido Günther wrote: > > We allow configuration of brightness mode and over voltage > > protection. > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > > @@ -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. > 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 > Best regards, > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties 2019-12-16 12:28 ` [PATCH 1/2] dt: bindings: lm3692x: Document new properties Guido Günther 2019-12-17 12:41 ` Dan Murphy 2019-12-21 19:15 ` Pavel Machek @ 2019-12-26 19:19 ` Rob Herring 2019-12-27 10:16 ` Guido Günther 2 siblings, 1 reply; 15+ messages in thread From: Rob Herring @ 2019-12-26 19:19 UTC (permalink / raw) To: Guido Günther Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Mark Rutland, linux-leds, devicetree, linux-kernel On Mon, Dec 16, 2019 at 01:28:05PM +0100, Guido Günther wrote: > We allow configuration of brightness mode and over voltage > protection. > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > --- > Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > index 4c2d923f8758..f195e8b45d85 100644 > --- a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > @@ -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. > > Required child properties: > - reg : 0 - Will enable all LED sync paths > -- > 2.23.0 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt: bindings: lm3692x: Document new properties 2019-12-26 19:19 ` Rob Herring @ 2019-12-27 10:16 ` Guido Günther 0 siblings, 0 replies; 15+ messages in thread From: Guido Günther @ 2019-12-27 10:16 UTC (permalink / raw) To: Rob Herring Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Mark Rutland, linux-leds, devicetree, linux-kernel Hi, On Thu, Dec 26, 2019 at 12:19:06PM -0700, Rob Herring wrote: > On Mon, Dec 16, 2019 at 01:28:05PM +0100, Guido Günther wrote: > > We allow configuration of brightness mode and over voltage > > protection. > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > --- > > Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > > index 4c2d923f8758..f195e8b45d85 100644 > > --- a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > > +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt > > @@ -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 > > > > > Required child properties: > > - reg : 0 - Will enable all LED sync paths > > -- > > 2.23.0 > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-16 12:28 [PATCH 0/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther 2019-12-16 12:28 ` [PATCH 1/2] dt: bindings: lm3692x: Document new properties Guido Günther @ 2019-12-16 12:28 ` Guido Günther 2019-12-17 12:53 ` Dan Murphy 2019-12-21 19:18 ` Pavel Machek 1 sibling, 2 replies; 15+ messages in thread From: Guido Günther @ 2019-12-16 12:28 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel Overvoltage protection and brightness mode are currently hardcoded as disabled in the driver. Make these configurable via DT. Signed-off-by: Guido Günther <agx@sigxcpu.org> --- drivers/leds/leds-lm3692x.c | 43 +++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c index 8b408102e138..2c084b333628 100644 --- a/drivers/leds/leds-lm3692x.c +++ b/drivers/leds/leds-lm3692x.c @@ -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"); -- 2.23.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-16 12:28 ` [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther @ 2019-12-17 12:53 ` Dan Murphy 2019-12-17 15:40 ` Guido Günther ` (2 more replies) 2019-12-21 19:18 ` Pavel Machek 1 sibling, 3 replies; 15+ messages in thread From: Dan Murphy @ 2019-12-17 12:53 UTC (permalink / raw) To: Guido Günther, Jacek Anaszewski, Pavel Machek, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel Guido On 12/16/19 6:28 AM, Guido Günther wrote: > 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. > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > --- > drivers/leds/leds-lm3692x.c | 43 +++++++++++++++++++++++++++++++------ > 1 file changed, 37 insertions(+), 6 deletions(-) > > diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c > index 8b408102e138..2c084b333628 100644 > --- a/drivers/leds/leds-lm3692x.c > +++ b/drivers/leds/leds-lm3692x.c > @@ -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; regmap_update_bits > > @@ -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 > > @@ -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; Make this a #define and then it can be reused as a mask for regmap_update_bits > + 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 > + 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 > + 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); > + extra debug statement > + led->brightness_ctrl = LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN; Same comment as before on the #define > + 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 Dan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-17 12:53 ` Dan Murphy @ 2019-12-17 15:40 ` Guido Günther 2019-12-17 17:01 ` Dan Murphy 2019-12-21 19:17 ` Pavel Machek 2019-12-24 11:30 ` Guido Günther 2 siblings, 1 reply; 15+ messages in thread From: Guido Günther @ 2019-12-17 15:40 UTC (permalink / raw) To: Dan Murphy Cc: Jacek Anaszewski, Pavel Machek, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel Hi Dan, On Tue, Dec 17, 2019 at 06:53:45AM -0600, Dan Murphy wrote: > Guido > > On 12/16/19 6:28 AM, Guido Günther wrote: > > 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? > We are adding 2 separate features and if something is incorrect with one of > the changes it is a bit hard to debug. > > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > --- > > drivers/leds/leds-lm3692x.c | 43 +++++++++++++++++++++++++++++++------ > > 1 file changed, 37 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c > > index 8b408102e138..2c084b333628 100644 > > --- a/drivers/leds/leds-lm3692x.c > > +++ b/drivers/leds/leds-lm3692x.c > > @@ -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; > > regmap_update_bits > > > > @@ -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 > > @@ -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; > Make this a #define and then it can be reused as a mask for > regmap_update_bits > > + 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 > > > + 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 > > + 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); > > + > extra debug statement > > + led->brightness_ctrl = LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN; > Same comment as before on the #define > > + 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 > > Dan > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-17 15:40 ` Guido Günther @ 2019-12-17 17:01 ` Dan Murphy 0 siblings, 0 replies; 15+ messages in thread From: Dan Murphy @ 2019-12-17 17:01 UTC (permalink / raw) To: Guido Günther Cc: Jacek Anaszewski, Pavel Machek, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel Guido On 12/17/19 9:40 AM, Guido Günther wrote: > Hi Dan, > On Tue, Dec 17, 2019 at 06:53:45AM -0600, Dan Murphy wrote: >> Guido >> >> On 12/16/19 6:28 AM, Guido Günther wrote: >>> 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> >> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-17 12:53 ` Dan Murphy 2019-12-17 15:40 ` Guido Günther @ 2019-12-21 19:17 ` Pavel Machek 2019-12-24 11:30 ` Guido Günther 2 siblings, 0 replies; 15+ messages in thread From: Pavel Machek @ 2019-12-21 19:17 UTC (permalink / raw) To: Dan Murphy Cc: Guido Günther, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 566 bytes --] On Tue 2019-12-17 06:53:45, Dan Murphy wrote: > Guido > > On 12/16/19 6:28 AM, Guido Günther wrote: > >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 -- (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] 15+ messages in thread
* Re: [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-17 12:53 ` Dan Murphy 2019-12-17 15:40 ` Guido Günther 2019-12-21 19:17 ` Pavel Machek @ 2019-12-24 11:30 ` Guido Günther 2 siblings, 0 replies; 15+ messages in thread From: Guido Günther @ 2019-12-24 11:30 UTC (permalink / raw) To: Dan Murphy Cc: Jacek Anaszewski, Pavel Machek, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel 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? On Tue, Dec 17, 2019 at 06:53:45AM -0600, Dan Murphy wrote: > Guido > > On 12/16/19 6:28 AM, Guido Günther wrote: > > 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. > > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > --- > > drivers/leds/leds-lm3692x.c | 43 +++++++++++++++++++++++++++++++------ > > 1 file changed, 37 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c > > index 8b408102e138..2c084b333628 100644 > > --- a/drivers/leds/leds-lm3692x.c > > +++ b/drivers/leds/leds-lm3692x.c > > @@ -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; > > 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. > > @@ -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. > > @@ -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; > Make this a #define and then it can be reused as a mask for > regmap_update_bits > > + 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 > Fixed. > > + 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. > > + 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); > > + > extra debug statement dropped. > > + led->brightness_ctrl = LM3692X_BL_ADJ_POL | LM3692X_RAMP_EN; > Same comment as before on the #define > > + 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 > > Dan > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-16 12:28 ` [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther 2019-12-17 12:53 ` Dan Murphy @ 2019-12-21 19:18 ` Pavel Machek 2019-12-24 11:26 ` Guido Günther 1 sibling, 1 reply; 15+ messages in thread From: Pavel Machek @ 2019-12-21 19:18 UTC (permalink / raw) To: Guido Günther Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 719 bytes --] Hi! > 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? > Signed-off-by: Guido Günther <agx@sigxcpu.org> > + 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 -- (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] 15+ messages in thread
* Re: [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode 2019-12-21 19:18 ` Pavel Machek @ 2019-12-24 11:26 ` Guido Günther 0 siblings, 0 replies; 15+ messages in thread From: Guido Günther @ 2019-12-24 11:26 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Mark Rutland, linux-leds, devicetree, linux-kernel Hi, On Sat, Dec 21, 2019 at 08:18:44PM +0100, Pavel Machek wrote: > Hi! > > > 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 > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > > + 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 > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-12-27 10:16 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-16 12:28 [PATCH 0/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther 2019-12-16 12:28 ` [PATCH 1/2] dt: bindings: lm3692x: Document new properties Guido Günther 2019-12-17 12:41 ` Dan Murphy 2019-12-21 19:15 ` Pavel Machek 2019-12-24 11:45 ` Guido Günther 2019-12-26 19:19 ` Rob Herring 2019-12-27 10:16 ` Guido Günther 2019-12-16 12:28 ` [PATCH 2/2] leds: lm3692x: Allow to set ovp and brigthness mode Guido Günther 2019-12-17 12:53 ` Dan Murphy 2019-12-17 15:40 ` Guido Günther 2019-12-17 17:01 ` Dan Murphy 2019-12-21 19:17 ` Pavel Machek 2019-12-24 11:30 ` Guido Günther 2019-12-21 19:18 ` Pavel Machek 2019-12-24 11:26 ` Guido Günther
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).