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] [day] [month] [year] [list]
Date:   Fri, 16 Jun 2023 18:41:10 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Artur Weber <aweber.kernel@...il.com>,
        Thierry Reding <thierry.reding@...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>,
        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

Hello,

On Fri, Jun 16, 2023 at 05:29:08PM +0200, Artur Weber wrote:
> On 14/06/2023 10:39, Uwe Kleine-König wrote:
> > 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
> >> ...
> >> @@ -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?
> 
> EINVAL is returned when the pwms property is not found in the DT node
> for the backlight. Not sure if there's a better way of separately
> detecting whether it's present (especially when taking into
> consideration non-DT platforms that might use the driver). Would be nice
> to have something like devm_regulator_get_optional but for PWMs...

Ah, that is because of_pwm_get() calls of_property_match_string(np,
"pwm-names", con_id) which returns -EINVAL if there is no pwm-names
property. This is different for clocks. I wonder if pwm should adapt
accordingly? Thierry?

> Still, someone who's setting up the driver could check the debug
> messages to see if the backlight was set up in PWM mode or register mode.
> 
> > ...
> >> +		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?
> 
> I didn't really consider the implication of this in this way, as on the
> device I was testing this on (Exynos4212-based tablet) the PWM state
> would get reset during PWM chip initialization in the kernel anyways,

Which chip driver is in use here? That's a patch opportunity.

> meaning that the state from the bootloader would be lost regardless of
> this change. Either way, there's no guarantee that this would be the
> same on other devices, though I'd assume that in most cases it's not
> noticeable anyways as brightness is usually set somewhere in userspace
> (or even earlier, in the driver, if the init-brt property is set).
> Nonetheless, that's an oversight on my part.
> 
> As for the reasoning for this change in behavior - the previous behavior
> was to silently fail if, while setting the brightness, the PWM could not

This sounds wrong.

> be set up. This seemed rather confusing to me (I encountered this while
> I was initially working on the tablet, I added a "pwm" property instead
> of "pwms" and was wondering why the backlight didn't work...)
> 
> Of course, that could be fixed by adding error detection in the
> brightness set function, but since I was already working on it, it made
> more sense to me for the PWM to be set up during the probing process,
> given that this way we could 1. warn about errors early, and 2. catch
> deferred probes and defer the backlight's probe if we're still waiting
> for the PWM. That's why it's done the way it is in this patch.
> 
> If this is undesired behavior, let me know and I'll submit another patch
> to revert it.

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)