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
