Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down

[Paul Cercueil]

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:
> >  > >
> >  >