Le lun. 24 juin 2019 à 13:28, Daniel Thompson
<daniel.thompson@xxxxxxxxxx> a écrit :
On Fri, Jun 21, 2019 at 03:56:08PM +0200, Thierry Reding wrote:
On Fri, Jun 21, 2019 at 01:41:45PM +0100, Daniel Thompson wrote:
> On 22/05/2019 17:34, Paul Cercueil wrote:
> > When the driver probes, the PWM pin is automatically configured
to its
> > default state, which should be the "pwm" function.
>
> At which point in the probe... and by who?
The driver core will select the "default" state of a device right
before
calling the driver's probe, see:
drivers/base/pinctrl.c: pinctrl_bind_pins()
which is called from:
drivers/base/dd.c: really_probe()
Thanks. I assumed it would be something like that... although given
pwm-backlight is essentially a wrapper driver round a PWM I wondered
why
the pinctrl was on the backlight node (rather than the PWM node).
Looking at the DTs in the upstream kernel it looks like ~20% of the
backlight drivers have pinctrl on the backlight node. Others
presumable
have none or have it on the PWM node (and it looks like support for
sleeping the pins is *very* rare amoung the PWM drivers).
If your PWM driver has more than one channel and has the pinctrl node,
you
cannot fine-tune the state of individual pins. They all share the same
state.
> > However, at this
> > point we don't know the actual level of the pin, which may be
active or
> > inactive. As a result, if the driver probes without enabling the
> > backlight, the PWM pin might be active, and the backlight would
be
> > lit way before being officially enabled.
> >
> > To work around this, if the probe function doesn't enable the
backlight,
> > the pin is set to its sleep state instead of the default one,
until the
> > backlight is enabled. Whenk the backlight is disabled, the pin
is reset
> > to its sleep state.
> Doesn't this workaround result in a backlight flash between
whatever enables
> it and the new code turning it off again?
Yeah, I think it would. I guess if you're very careful on how you
set up
the device tree you might be able to work around it. Besides the
default
and idle standard pinctrl states, there's also the "init" state. The
core will select that instead of the default state if available.
However
there's also pinctrl_init_done() which will try again to switch to
the
default state after probe has finished and the driver didn't switch
away
from the init state.
So you could presumably set up the device tree such that you have
three
states defined: "default" would be the one where the PWM pin is
active,
"idle" would be used when backlight is off (PWM pin inactive) and
then
another "init" state that would be the same as "idle" to be used
during
probe. During probe the driver could then switch to the "idle"
state so
that the pin shouldn't glitch.
I'm not sure this would actually work because I think the way that
pinctrl handles states both "init" and "idle" would be the same
pointer
values and therefore pinctrl_init_done() would think the driver
didn't
change away from the "init" state because it is the same pointer
value
as the "idle" state that the driver selected. One way to work around
that would be to duplicate the "idle" state definition and
associate one
instance of it with the "idle" state and the other with the "init"
state. At that point both states should be different (different
pointer
values) and we'd get the init state selected automatically before
probe,
select "idle" during probe and then the core will leave it alone.
That's
of course ugly because we duplicate the pinctrl state in DT, but
perhaps
it's the least ugly solution.
Adding Linus for visibility. Perhaps he can share some insight.
To be honest I'm happy to summarize in my head as "if it flashes then
it's not
a pwm_bl.c's problem" ;-).
It does not flash. But the backlight lits way too early, so we have a
1-2 seconds
of "white screen" before the panel driver starts.
-Paul
Daniel.
On that note, I'm wondering if perhaps it'd make sense for pinctrl
to
support some mode where a device would start out in idle mode. That
is,
where pinctrl_bind_pins() would select the "idle" mode as the
default
before probe. With something like that we could easily support this
use-case without glitching.
I suppose yet another variant would be for the PWM backlight to not
use
any of the standard pinctrl states at all. Instead it could just
define
custom states, say "active" and "inactive". Looking at the code that
would prevent pinctrl_bind_pins() from doing anything with pinctrl
states and given the driver exact control over when each of the
states
will be selected. That's somewhat suboptimal because we can't make
use
of the pinctrl PM helpers and it'd require more boilerplate.
Thierry
> > Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx> > ---
> > drivers/video/backlight/pwm_bl.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
> > index fb45f866b923..422f7903b382 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -16,6 +16,7 @@
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/init.h>
> > +#include <linux/pinctrl/consumer.h>
> > #include <linux/platform_device.h>
> > #include <linux/fb.h>
> > #include <linux/backlight.h>
> > @@ -50,6 +51,8 @@ static void pwm_backlight_power_on(struct
pwm_bl_data *pb)
> > struct pwm_state state;
> > int err;
> > + pinctrl_pm_select_default_state(pb->dev);
> > +
> > pwm_get_state(pb->pwm, &state);
> > if (pb->enabled)
> > return;
> > @@ -90,6 +93,8 @@ static void pwm_backlight_power_off(struct
pwm_bl_data *pb)
> > regulator_disable(pb->power_supply);
> > pb->enabled = false;
> > +
> > + pinctrl_pm_select_sleep_state(pb->dev);
> > }
> > static int compute_duty_cycle(struct pwm_bl_data *pb, int
brightness)
> > @@ -626,6 +631,10 @@ static int pwm_backlight_probe(struct
platform_device *pdev)
> > backlight_update_status(bl);
> > platform_set_drvdata(pdev, bl);
> > +
> > + if (bl->props.power == FB_BLANK_POWERDOWN)
> > + pinctrl_pm_select_sleep_state(&pdev->dev);
>
> Didn't backlight_update_status(bl) already do this?
>
>
> Daniel.
>
>
> > +
> > return 0;
> > err_alloc:
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
