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

[Uwe Kleine-König]

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

Attachment: signature.asc
Description: PGP signature