Re: [PATCH 2/4] video: backlight: lp855x: get PWM for PWM mode during probe

[Uwe Kleine-König]

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.kernel@xxxxxxxxx>
> ---
>  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/ |

Attachment: signature.asc
Description: PGP signature