| Message ID | cover.1576499103.git.agx@sigxcpu.org |
|---|---|
| Headers | show |
| Series | leds: lm3692x: Allow to set ovp and brigthness mode | expand |
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
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 > >
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
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
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
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
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 > >