* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-21 13:56 ` Thierry Reding
@ 2019-06-24 11:28 ` Daniel Thompson
2019-06-24 14:31 ` Paul Cercueil
2019-06-25 9:38 ` Thierry Reding
2019-06-24 14:39 ` Paul Cercueil
2019-06-24 22:02 ` Linus Walleij
2 siblings, 2 replies; 18+ messages in thread
From: Daniel Thompson @ 2019-06-24 11:28 UTC (permalink / raw)
To: Thierry Reding
Cc: Paul Cercueil, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
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).
> > > 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" ;-).
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@crapouillou.net> > ---
> > > 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:
> > >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-24 11:28 ` Daniel Thompson
@ 2019-06-24 14:31 ` Paul Cercueil
2019-06-24 15:46 ` Daniel Thompson
2019-06-25 9:47 ` Thierry Reding
2019-06-25 9:38 ` Thierry Reding
1 sibling, 2 replies; 18+ messages in thread
From: Paul Cercueil @ 2019-06-24 14:31 UTC (permalink / raw)
To: Daniel Thompson
Cc: Thierry Reding, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
Le lun. 24 juin 2019 à 13:28, Daniel Thompson
<daniel.thompson@linaro.org> 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@crapouillou.net> > ---
>> > > 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:
>> > >
>> >
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-24 14:31 ` Paul Cercueil
@ 2019-06-24 15:46 ` Daniel Thompson
2019-06-24 16:06 ` Paul Cercueil
2019-06-25 9:47 ` Thierry Reding
1 sibling, 1 reply; 18+ messages in thread
From: Daniel Thompson @ 2019-06-24 15:46 UTC (permalink / raw)
To: Paul Cercueil
Cc: Thierry Reding, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
On Mon, Jun 24, 2019 at 04:31:57PM +0200, Paul Cercueil wrote:
>
>
> Le lun. 24 juin 2019 à 13:28, Daniel Thompson <daniel.thompson@linaro.org> 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.
Good point. Thanks.
> > > > > 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.
That's the current behaviour.
What I original asked about is whether a panel that was dark before the
driver probes could end up flashing after the patch because it is
activated pre-probe and only goes to sleep afterwards.
Anyhow I got an answer; if it flashes after the patch then the problem
does not originate in pwm_bl.c and is likely a problem with the handling
of the pinctrl idel state (i.e. probably DT misconfiguration)
So I think that just leaves my comment about the spurious sleep in the
probe function.
Daniel.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-24 15:46 ` Daniel Thompson
@ 2019-06-24 16:06 ` Paul Cercueil
0 siblings, 0 replies; 18+ messages in thread
From: Paul Cercueil @ 2019-06-24 16:06 UTC (permalink / raw)
To: Daniel Thompson
Cc: Thierry Reding, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
Le lun. 24 juin 2019 à 17:46, Daniel Thompson
<daniel.thompson@linaro.org> a écrit :
> On Mon, Jun 24, 2019 at 04:31:57PM +0200, Paul Cercueil wrote:
>>
>>
>> Le lun. 24 juin 2019 à 13:28, Daniel Thompson
>> <daniel.thompson@linaro.org> 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.
>
> Good point. Thanks.
>
>
>> > > > > 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.
>
> That's the current behaviour.
>
> What I original asked about is whether a panel that was dark before
> the
> driver probes could end up flashing after the patch because it is
> activated pre-probe and only goes to sleep afterwards.
>
> Anyhow I got an answer; if it flashes after the patch then the problem
> does not originate in pwm_bl.c and is likely a problem with the
> handling
> of the pinctrl idel state (i.e. probably DT misconfiguration)
>
> So I think that just leaves my comment about the spurious sleep in the
> probe function.
The probe function calls backlight_update_status(), which then calls
pwm_backlight_power_off(), which returns early as pb->enabled is false.
So the pins are still in "default" (or "init") state after the call
to backlight_update_status().
-Paul
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-24 14:31 ` Paul Cercueil
2019-06-24 15:46 ` Daniel Thompson
@ 2019-06-25 9:47 ` Thierry Reding
2019-07-07 2:13 ` Paul Cercueil
1 sibling, 1 reply; 18+ messages in thread
From: Thierry Reding @ 2019-06-25 9:47 UTC (permalink / raw)
To: Paul Cercueil
Cc: Daniel Thompson, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6221 bytes --]
On Mon, Jun 24, 2019 at 04:31:57PM +0200, Paul Cercueil wrote:
>
>
> Le lun. 24 juin 2019 à 13:28, Daniel Thompson <daniel.thompson@linaro.org> 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.
But that's something that could be changed, right? We could for example
extend the PWM bindings to allow describing each PWM instance via a sub-
node of the controller node. Pin control states could be described on a
per-channel basis that way.
> > > > > 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.
I think this always goes both ways. If you set the sleep state for the
PWM on backlight probe with this patch, you may be able to work around
the problem of the backlight lighting up too early. But what if your
bootloader had already enabled the backlight and is showing a splash
screen during boot? Your patch would turn off the backlight and then it
would turn on again after everything else was initialized. That's one
type of flashing.
What we need in this case are explicit pin control states that will
enable fine-grained control over what happens. Anything implicit is
bound to fail because it bakes in an assumption (either that the
backlight is off during boot, or that it has been turned on already).
Ideally we'd need to detect that the backlight is on and if it is we
just don't do anything with it. Actually, I think that's what we want
even if the backlight is off. During probe the backlight state should
not be modified. You only want to modify it when you know that some
display driver is going to take over. If you can't seamlessly transition
to the kernel display driver, flashing may be okay. If your display
driver can take over seamlessly, then the backlight is likely already in
the desired state anyway.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-25 9:47 ` Thierry Reding
@ 2019-07-07 2:13 ` Paul Cercueil
0 siblings, 0 replies; 18+ messages in thread
From: Paul Cercueil @ 2019-07-07 2:13 UTC (permalink / raw)
To: Thierry Reding
Cc: Daniel Thompson, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
Le mar. 25 juin 2019 à 5:47, Thierry Reding <thierry.reding@gmail.com>
a écrit :
> On Mon, Jun 24, 2019 at 04:31:57PM +0200, Paul Cercueil wrote:
>>
>>
>> Le lun. 24 juin 2019 à 13:28, Daniel Thompson
>> <daniel.thompson@linaro.org> 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.
>
> But that's something that could be changed, right? We could for
> example
> extend the PWM bindings to allow describing each PWM instance via a
> sub-
> node of the controller node. Pin control states could be described on
> a
> per-channel basis that way.
There could be an API to dynamically add/remove pin groups to a given
pinctrl state. The PWM driver would start with an empty (no groups)
"default" state, then when enabling e.g. PWM1, the driver would call
a function to add the "pwm1" pin group to the "default" state.
Does that sound like a good idea?
Thanks,
-Paul
>> > > > > 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.
>
> I think this always goes both ways. If you set the sleep state for the
> PWM on backlight probe with this patch, you may be able to work around
> the problem of the backlight lighting up too early. But what if your
> bootloader had already enabled the backlight and is showing a splash
> screen during boot? Your patch would turn off the backlight and then
> it
> would turn on again after everything else was initialized. That's one
> type of flashing.
>
> What we need in this case are explicit pin control states that will
> enable fine-grained control over what happens. Anything implicit is
> bound to fail because it bakes in an assumption (either that the
> backlight is off during boot, or that it has been turned on already).
>
> Ideally we'd need to detect that the backlight is on and if it is we
> just don't do anything with it. Actually, I think that's what we want
> even if the backlight is off. During probe the backlight state should
> not be modified. You only want to modify it when you know that some
> display driver is going to take over. If you can't seamlessly
> transition
> to the kernel display driver, flashing may be okay. If your display
> driver can take over seamlessly, then the backlight is likely already
> in
> the desired state anyway.
>
> Thierry
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-24 11:28 ` Daniel Thompson
2019-06-24 14:31 ` Paul Cercueil
@ 2019-06-25 9:38 ` Thierry Reding
2019-06-26 8:58 ` Uwe Kleine-König
1 sibling, 1 reply; 18+ messages in thread
From: Thierry Reding @ 2019-06-25 9:38 UTC (permalink / raw)
To: Daniel Thompson
Cc: Paul Cercueil, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]
On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> 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).
I agree with this. We're defining the pin control state for the PWM pin,
so in my opinion it should be the PWM driver that controls it.
One reason why I think this is important is if we ever end up with a
device that requires pins from two different controllers to be
configured at runtime, then how would we model that? Since pin control
states cannot be aggregated, so you'd have to have multiple "default"
states, each for the pins that they control.
On the other hand if we associate the pin control states with each of
the resources that need those states, then when those resources are
controlled, they will automatically know how to deal with the states.
The top-level device (i.e. backlight) doesn't need to concern itself
with those details.
> 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).
I suspect that that's mostly a sign of our device trees and kernel
subsystems still maturing. For example, I think it's fairly rare for a
device to seamlessly take over the display configuration from the
bootloader. Most of the time you'll just see things go black (that's
actually one of the better cases) when the kernel takes over and then
the backlight will come up again at some point.
Taking over the bootloader's display configuration is pretty hard and
there are numerous pieces to the puzzle (need to make sure clocks and
power supplies are not automatically disabled after the initcalls,
display drivers need to know how to read out hardware, claim whatever
memory region the bootloader was using for a bootsplash, backlight is
supposed to remain enabled if the bootloader turned it on, ...).
I don't think the fact that PWM drivers don't support this implies that
hardware doesn't support it. I think we've just never needed it before
because we get away with it.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-25 9:38 ` Thierry Reding
@ 2019-06-26 8:58 ` Uwe Kleine-König
2019-06-26 9:58 ` Thierry Reding
0 siblings, 1 reply; 18+ messages in thread
From: Uwe Kleine-König @ 2019-06-26 8:58 UTC (permalink / raw)
To: Thierry Reding
Cc: Daniel Thompson, Paul Cercueil, Lee Jones, Jingoo Han,
Linus Walleij, Bartlomiej Zolnierkiewicz, od, linux-pwm,
dri-devel, linux-fbdev, linux-kernel, kernel
On Tue, Jun 25, 2019 at 11:38:39AM +0200, Thierry Reding wrote:
> On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> > [...] 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).
>
> I agree with this. We're defining the pin control state for the PWM pin,
> so in my opinion it should be the PWM driver that controls it.
>
> One reason why I think this is important is if we ever end up with a
> device that requires pins from two different controllers to be
> configured at runtime, then how would we model that? Since pin control
> states cannot be aggregated, so you'd have to have multiple "default"
> states, each for the pins that they control.
I thought you can do:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_in_first_pincontroller>, <&pinctrl_in_another_controller>;
if two (or more) controllers are involved.
> On the other hand if we associate the pin control states with each of
> the resources that need those states, then when those resources are
> controlled, they will automatically know how to deal with the states.
> The top-level device (i.e. backlight) doesn't need to concern itself
> with those details.
So the options are:
a) put "active" and "inactive" pinctrls into the pwm-node, and nothing
related to the involved PWM pins in the consumer
b) put the PWM pin config in the consumer's "default" pinctrl (and
maybe leave it out int "init" if you want smooth taking over).
(Or maybe use "enabled" and "disabled" in a) to match the pwm_states
.enabled?)
The advantages I see in b) over a) are:
- "default" and "init" are a known pinctrl concept that most people
should have understood.
- You have all pinctrl config for the backlight in a single place.
- none of the involved driver must explicitly handle pinctrl stuff
You presume that b) being commonly done is a sign of "our device trees
and kernel subsystems still maturing". But maybe it's only that the
capabilities provided by pinctrl subsystem without extra effort is good
enough?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-26 8:58 ` Uwe Kleine-König
@ 2019-06-26 9:58 ` Thierry Reding
2019-06-26 10:16 ` Uwe Kleine-König
0 siblings, 1 reply; 18+ messages in thread
From: Thierry Reding @ 2019-06-26 9:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Daniel Thompson, Paul Cercueil, Lee Jones, Jingoo Han,
Linus Walleij, Bartlomiej Zolnierkiewicz, od, linux-pwm,
dri-devel, linux-fbdev, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 4401 bytes --]
On Wed, Jun 26, 2019 at 10:58:27AM +0200, Uwe Kleine-König wrote:
> On Tue, Jun 25, 2019 at 11:38:39AM +0200, Thierry Reding wrote:
> > On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> > > [...] 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).
> >
> > I agree with this. We're defining the pin control state for the PWM pin,
> > so in my opinion it should be the PWM driver that controls it.
> >
> > One reason why I think this is important is if we ever end up with a
> > device that requires pins from two different controllers to be
> > configured at runtime, then how would we model that? Since pin control
> > states cannot be aggregated, so you'd have to have multiple "default"
> > states, each for the pins that they control.
>
> I thought you can do:
>
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_in_first_pincontroller>, <&pinctrl_in_another_controller>;
>
> if two (or more) controllers are involved.
You're right. Both the bindings say that this can be done and the code
is also there to parse multiple states per pinctrl-* entry.
> > On the other hand if we associate the pin control states with each of
> > the resources that need those states, then when those resources are
> > controlled, they will automatically know how to deal with the states.
> > The top-level device (i.e. backlight) doesn't need to concern itself
> > with those details.
>
> So the options are:
>
> a) put "active" and "inactive" pinctrls into the pwm-node, and nothing
> related to the involved PWM pins in the consumer
>
> b) put the PWM pin config in the consumer's "default" pinctrl (and
> maybe leave it out int "init" if you want smooth taking over).
You can't put it into the "default" state because that state is applied
before the consumer driver's ->probe().
>
> (Or maybe use "enabled" and "disabled" in a) to match the pwm_states
> .enabled?)
Yeah, I think this is what we'll need to do in order to implement the
explicit behaviour that we need here.
> The advantages I see in b) over a) are:
>
> - "default" and "init" are a known pinctrl concept that most people
> should have understood.
The problem is that they won't work in this case. The "init" state will
be applied before the consumer driver's ->probe() if it exists. If it
doesn't then "default" will be applied instead. Both cases are not
something that we want if we want to take over the existing
configuration.
> - You have all pinctrl config for the backlight in a single place.
Depending on your point of view this could be considered a disadvantage.
> - none of the involved driver must explicitly handle pinctrl stuff
Like I said, none of the automatic state handling is flexible enough for
this situation. Also, my understanding is that even if you use the
standard pinctrl state names ("default" and "idle") you still need to
explicitly select them at the right time. "default" will always be
applied before the consumer driver's ->probe(), but if you want to go to
the "idle" state you have to make that explicit. Now, there are helpers
to simplify this a bit, but you still need to implement suspend/resume
callbacks (or however you want to deal with it) that call these helpers.
In the case of PWM I think what we want is to select an "active" and
"idle" state on enable and disable, respectively. I suppose we could add
some infrastructure to help with this, such as perhaps scanning the
device tree for per-PWM pin control states at PWM chip registration time
and then adding helpers to select these states at the driver's
discretion. I don't think we can add generic code to do this because the
exact time when the pin control state needs to be applied may vary from
one PWM controller to another.
> You presume that b) being commonly done is a sign of "our device trees
> and kernel subsystems still maturing". But maybe it's only that the
> capabilities provided by pinctrl subsystem without extra effort is good
> enough?
Like I pointed out above, I don't think that's the case. But I don't
want to overcomplicate things, so if you can prove that it can be done
with the existing pinctrl helpers, I'd be happy to be proven wrong.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-26 9:58 ` Thierry Reding
@ 2019-06-26 10:16 ` Uwe Kleine-König
0 siblings, 0 replies; 18+ messages in thread
From: Uwe Kleine-König @ 2019-06-26 10:16 UTC (permalink / raw)
To: Thierry Reding
Cc: Daniel Thompson, Paul Cercueil, Lee Jones, Jingoo Han,
Linus Walleij, Bartlomiej Zolnierkiewicz, od, linux-pwm,
dri-devel, linux-fbdev, linux-kernel, kernel
On Wed, Jun 26, 2019 at 11:58:44AM +0200, Thierry Reding wrote:
> On Wed, Jun 26, 2019 at 10:58:27AM +0200, Uwe Kleine-König wrote:
> > On Tue, Jun 25, 2019 at 11:38:39AM +0200, Thierry Reding wrote:
> > > On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> > > > [...] 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).
> > >
> > > I agree with this. We're defining the pin control state for the PWM pin,
> > > so in my opinion it should be the PWM driver that controls it.
> > >
> > > One reason why I think this is important is if we ever end up with a
> > > device that requires pins from two different controllers to be
> > > configured at runtime, then how would we model that? Since pin control
> > > states cannot be aggregated, so you'd have to have multiple "default"
> > > states, each for the pins that they control.
> >
> > I thought you can do:
> >
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_in_first_pincontroller>, <&pinctrl_in_another_controller>;
> >
> > if two (or more) controllers are involved.
>
> You're right. Both the bindings say that this can be done and the code
> is also there to parse multiple states per pinctrl-* entry.
>
> > > On the other hand if we associate the pin control states with each of
> > > the resources that need those states, then when those resources are
> > > controlled, they will automatically know how to deal with the states.
> > > The top-level device (i.e. backlight) doesn't need to concern itself
> > > with those details.
> >
> > So the options are:
> >
> > a) put "active" and "inactive" pinctrls into the pwm-node, and nothing
> > related to the involved PWM pins in the consumer
> >
> > b) put the PWM pin config in the consumer's "default" pinctrl (and
> > maybe leave it out int "init" if you want smooth taking over).
>
> You can't put it into the "default" state because that state is applied
> before the consumer driver's ->probe().
If you do:
mybacklight {
pinctrl-names = "init", "default";
pinctrl-0 = <&pinctrl_without_pwm>
pinctrl-1 = <&pinctrl_with_pwm>;
...
};
Then nothing is done before probing of the backlight and only when the
probing is done and the pwm is taken over, the PWM-pinctrl is applied.
The only ugly thing here I can identify is that probe() might exit with
the PWM running and then enabling the pinmux for the PWM pin results in
an incomplete period at the beginning. But this happens only in some
corner cases that might not matter. (i.e. if the bootloader enabled the
PWM but didn't setup the pinmux; and if .probe enabled the PWM which we
agreed it probably shouldn't on it's own.)
> > (Or maybe use "enabled" and "disabled" in a) to match the pwm_states
> > .enabled?)
>
> Yeah, I think this is what we'll need to do in order to implement the
> explicit behaviour that we need here.
>
> > The advantages I see in b) over a) are:
> >
> > - "default" and "init" are a known pinctrl concept that most people
> > should have understood.
>
> The problem is that they won't work in this case. The "init" state will
> be applied before the consumer driver's ->probe() if it exists. If it
> doesn't then "default" will be applied instead. Both cases are not
> something that we want if we want to take over the existing
> configuration.
>
> > - You have all pinctrl config for the backlight in a single place.
>
> Depending on your point of view this could be considered a disadvantage.
Yeah, right, this is subjective.
> [...]
> Like I pointed out above, I don't think that's the case. But I don't
> want to overcomplicate things, so if you can prove that it can be done
> with the existing pinctrl helpers, I'd be happy to be proven wrong.
I tried, see above :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-21 13:56 ` Thierry Reding
2019-06-24 11:28 ` Daniel Thompson
@ 2019-06-24 14:39 ` Paul Cercueil
2019-06-24 22:02 ` Linus Walleij
2 siblings, 0 replies; 18+ messages in thread
From: Paul Cercueil @ 2019-06-24 14:39 UTC (permalink / raw)
To: Thierry Reding
Cc: Daniel Thompson, Lee Jones, Jingoo Han, Linus Walleij,
Bartlomiej Zolnierkiewicz, od, linux-pwm, dri-devel, linux-fbdev,
linux-kernel
Le ven. 21 juin 2019 à 15:56, Thierry Reding
<thierry.reding@gmail.com> a écrit :
> 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()
>
>> > 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.
That's exactly what I'm doing, yes (with the minor difference that your
"idle" state is my "sleep" state).
> 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.
That works perfectly on my side. I didn't have to duplicate the states
in DT.
> Adding Linus for visibility. Perhaps he can share some insight.
>
> 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.
You'd still need the driver to switch back between "default" and "idle"
states, and switching to the "idle" state in the probe is a one-liner,
so probably not worth the trouble, unless I don't understand the whole
picture.
Thanks,
-Paul
> 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@crapouillou.net> > ---
>> > 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:
>> >
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down
2019-06-21 13:56 ` Thierry Reding
2019-06-24 11:28 ` Daniel Thompson
2019-06-24 14:39 ` Paul Cercueil
@ 2019-06-24 22:02 ` Linus Walleij
2 siblings, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2019-06-24 22:02 UTC (permalink / raw)
To: Thierry Reding
Cc: Daniel Thompson, Paul Cercueil, Lee Jones, Jingoo Han,
Bartlomiej Zolnierkiewicz, od, linux-pwm,
open list:DRM PANEL DRIVERS, linux-fbdev, linux-kernel
On Fri, Jun 21, 2019 at 3:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> 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.
Right.
> 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.
If something needs special mockery and is not generic, I'd just
come up with whatever string PWM needs, like
"pwm-idle", "pwm-sleep", "pwm-init" etc instead of
complicating the stuff done before probe(). These states are
only handled there to make probe() simple in simple cases.
> Adding Linus for visibility. Perhaps he can share some insight.
I think Paul hashed it out. Or will.
> 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 would say the driver can come up with whatever state it need for
that and handle it explicitly. When there are so many of them that
it warrants growing the device core, we can move it into
drivers/base/pinctrl.c. But no upfront design.
> 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".
I would suggest doing that.
> 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.
The helpers are just for the dirt-simple cases anyway.
At one point one developer thought that the "default" set up
before probe would be the only thing any system would ever
want to do with pin control. It seems like not.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 18+ messages in thread