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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230614083953.e4kkweddjz7wztby@pengutronix.de>
Date:   Wed, 14 Jun 2023 10:39:53 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Artur Weber <aweber.kernel@...il.com>
Cc:     Lee Jones <lee@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>, Pavel Machek <pavel@....cz>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Helge Deller <deller@....de>, dri-devel@...ts.freedesktop.org,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-fbdev@...r.kernel.org,
        linux-pwm@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 2/4] video: backlight: lp855x: get PWM for PWM mode
 during probe

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@...il.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/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)