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@xxxxxxxxxxx> > > --- > > 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 > >