On Sat, Apr 29, 2023 at 12:45:32PM +0200, Artur Weber wrote: > Also deprecate the pwm-period DT property, as it is now redundant > (pwms property already contains period value). > > Signed-off-by: Artur Weber <[email protected]> > --- > drivers/video/backlight/lp855x_bl.c | 48 ++++++++++++++++------------- > 1 file changed, 26 insertions(+), 22 deletions(-) > > diff --git a/drivers/video/backlight/lp855x_bl.c > b/drivers/video/backlight/lp855x_bl.c > index 81012bf29baf..21eb4943ed56 100644 > --- a/drivers/video/backlight/lp855x_bl.c > +++ b/drivers/video/backlight/lp855x_bl.c > @@ -218,23 +218,10 @@ static int lp855x_configure(struct lp855x *lp) > > static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) > { > - struct pwm_device *pwm; > struct pwm_state state; > > - /* request pwm device with the consumer name */ > - if (!lp->pwm) { > - pwm = devm_pwm_get(lp->dev, lp->chipname); > - if (IS_ERR(pwm)) > - return; > - > - lp->pwm = pwm; > - > - pwm_init_state(lp->pwm, &state); > - } else { > - pwm_get_state(lp->pwm, &state); > - } > + pwm_get_state(lp->pwm, &state);
pwm_get_state returns an error code. Do you care if it fails? (You
probably should.)
>
> - state.period = lp->pdata->period_ns;
> state.duty_cycle = div_u64(br * state.period, max_br);
> state.enabled = state.duty_cycle;
>
> @@ -339,6 +326,7 @@ static int lp855x_parse_dt(struct lp855x *lp)
> of_property_read_string(node, "bl-name", &pdata->name);
> of_property_read_u8(node, "dev-ctrl", &pdata->device_control);
> of_property_read_u8(node, "init-brt", &pdata->initial_brightness);
> + /* Deprecated, specify period in pwms property instead */
> of_property_read_u32(node, "pwm-period", &pdata->period_ns);
>
> /* Fill ROM platform data if defined */
> @@ -399,6 +387,7 @@ static int lp855x_probe(struct i2c_client *cl)
> const struct i2c_device_id *id = i2c_client_get_device_id(cl);
> const struct acpi_device_id *acpi_id = NULL;
> struct device *dev = &cl->dev;
> + struct pwm_state pwmstate;
> struct lp855x *lp;
> int ret;
>
> @@ -457,11 +446,6 @@ static int lp855x_probe(struct i2c_client *cl)
> }
> }
>
> - if (lp->pdata->period_ns > 0)
> - lp->mode = PWM_BASED;
> - else
> - lp->mode = REGISTER_BASED;
> -
> lp->supply = devm_regulator_get(dev, "power");
> if (IS_ERR(lp->supply)) {
> if (PTR_ERR(lp->supply) == -EPROBE_DEFER)
> @@ -472,11 +456,31 @@ static int lp855x_probe(struct i2c_client *cl)
> lp->enable = devm_regulator_get_optional(dev, "enable");
> if (IS_ERR(lp->enable)) {
> ret = PTR_ERR(lp->enable);
> - if (ret == -ENODEV) {
> + if (ret == -ENODEV)
> lp->enable = NULL;
> - } else {
> + else
> return dev_err_probe(dev, ret, "getting enable
> regulator\n");
> - }
> + }
> +
> + lp->pwm = devm_pwm_get(lp->dev, lp->chipname);
> + if (IS_ERR(lp->pwm)) {
> + ret = PTR_ERR(lp->pwm);
> + if (ret == -ENODEV || ret == -EINVAL)
Why would you ignore EINVAL?
> + lp->pwm = NULL;
> + else
> + return dev_err_probe(dev, ret, "getting PWM\n");
> +
> + lp->mode = REGISTER_BASED;
> + dev_dbg(dev, "mode: register based\n");
> + } else {
pwmstate could be declared here.
> + pwm_init_state(lp->pwm, &pwmstate);
> + /* Legacy platform data compatibility */
> + if (lp->pdata->period_ns > 0)
> + pwmstate.period = lp->pdata->period_ns;
> + pwm_apply_state(lp->pwm, &pwmstate);
This is a change in behaviour. Before lp855x_probe() didn't modify the
state the bootloader left the backlight in. Now you're disabling it (I
think). Is this intended?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
