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 <aweber.ker...@gmail.com>
> ---
> 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
