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

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

[Paul Cercueil]

When the driver probes, the PWM pin is automatically configured to its
default state, which should be the "pwm" function. 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. When the backlight is disabled, the pin is reset
to its sleep state.

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);
+
 	return 0;
 
 err_alloc:
-- 
2.21.0.593.g511ec345e18

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

[Daniel Thompson]

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?

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

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5390 bytes --]

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.

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Thompson]

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

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

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

[Daniel Thompson]

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.

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

[Paul Cercueil]

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

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

[Thierry Reding]

[-- 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 --]

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

[Paul Cercueil]

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

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

[Thierry Reding]

[-- 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 --]

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

[Uwe Kleine-König]

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

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

[Thierry Reding]

[-- 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 --]

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

[Uwe Kleine-König]

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

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

[Paul Cercueil]

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

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

[Linus Walleij]

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

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

[Uwe Kleine-König]

On Wed, May 22, 2019 at 06:34:28PM +0200, Paul Cercueil wrote:
> When the driver probes, the PWM pin is automatically configured to its
> default state, which should be the "pwm" function. 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.

I'm not sure I understand the problem completely here. Let me try to
summarize the problem you solve here:

The backlight device's default pinctrl contains the PWM function of the
PWM pin. As the PWM is (or at least might be) in an undefined state the
default pinctrl should only be switched to when it's clear if the
backlight should be on or off.

So you use the "init"-pinctrl to keep the PWM pin in some (undriven?)
state and by switching to "sleep" you prevent "default" getting active.

Did I get this right? If not, please correct me.

What is the PWM pin configured to in "init" in your case? Is the pinctrl
just empty? Or is it a gpio-mode (together with a gpio-hog)?

My thoughts to this is are:

 a) This is a general problem that applies (I think) to most if not all
    PWM consumers. If the PWM drives a motor, or makes your mobile
    vibrate, or drives an LED, or a clk, the PWM shouldn't start
    to do something before its consumer is ready.

 b) Thierry made it quite clear[1] that the PWM pin should be configured
    in a pinctrl of the pwm device, not the backlight (or more general:
    the consumer) device.

While I don't entirely agree with b) I think that even a) alone
justifies to think a bit more about the problem and preferably come up
with a solution that helps other consumers, too. Ideally if the
bootloader sets up the PWM to do something sensible, probing the
lowlevel PWM driver and the consumer driver should not interfere with
the bootloader's intention until the situation reaches a controlled
state. (I.e. if the backlight was left on by the bootloader to show a
nice logo, it should not flicker until a userspace program takes over
the display device.)

A PWM is special in contrast to other devices as its intended behaviour
is only fixed once a consumer is present. Without a consumer it is
unknown if the PWM is inverted or not. And so the common approach that
pinctrl is setup by the device core only doesn't work without drawbacks
for PWMs.

So if a PWM driver is probing and the PWM hardware already runs at say
constant one, some instance must define if the pin is supposed to be
configured in its "default" or "sleep" pinctrl. IMHO this isn't possible
in general without knowing the polarity of the PWM. (And even if it were
known that the polarity is inversed, it might be hard to say if your
PWM's hardware doesn't implement a disabled state and has to simulate
that using a 0% duty cycle.)

Another thing that complicates the matter is that at least pwm-imx27 has
the annoying property that disabling it (in hardware) drives the pin low
irrespective of the configured polarity. So if you want this type of
device to behave properly on disable, it must first drive a 0% duty
cycle, then switch the pinctrl state and only then disable the hardware.
This rules out that the lowlevel driver is unaware of the pinctrl stuff
which would be nice (or an inverted PWM won't be disabled in hardware or
you need an ugly sequence of callbacks to disable glitch-free). Also if
there is no sleep state, you better don't disable an inversed pwm-imx27
at all (in hardware)? (Alternatively we could drop the (undocumented)
guarantee that a disabled PWM results in the pin staying in its idle
level.)

What are the ways out? I think that if we go and apply your patch, we
should at least write some documentation with the details to provide some
"standard" way to solve similar problems.

Also it might be a good idea to let a PWM know if it is inverted or not
such that even without the presence of a consumer it can determine if
the hardware is active or not at probe time (in most cases at least).

Best regards
Uwe

[1] https://www.spinics.net/lists/linux-pwm/msg08246.html

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6114 bytes --]

On Tue, Jun 25, 2019 at 09:42:20AM +0200, Uwe Kleine-König wrote:
> On Wed, May 22, 2019 at 06:34:28PM +0200, Paul Cercueil wrote:
> > When the driver probes, the PWM pin is automatically configured to its
> > default state, which should be the "pwm" function. 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.
> 
> I'm not sure I understand the problem completely here. Let me try to
> summarize the problem you solve here:
> 
> The backlight device's default pinctrl contains the PWM function of the
> PWM pin. As the PWM is (or at least might be) in an undefined state the
> default pinctrl should only be switched to when it's clear if the
> backlight should be on or off.
> 
> So you use the "init"-pinctrl to keep the PWM pin in some (undriven?)
> state and by switching to "sleep" you prevent "default" getting active.
> 
> Did I get this right? If not, please correct me.
> 
> What is the PWM pin configured to in "init" in your case? Is the pinctrl
> just empty? Or is it a gpio-mode (together with a gpio-hog)?
> 
> My thoughts to this is are:
> 
>  a) This is a general problem that applies (I think) to most if not all
>     PWM consumers. If the PWM drives a motor, or makes your mobile
>     vibrate, or drives an LED, or a clk, the PWM shouldn't start
>     to do something before its consumer is ready.

Yes, it shouldn't start before it is explicitly told to do so by the
consumer. One exception is if the PWM was already set up by firmware
to run. So I think in general terms we always want the PWM to remain
in the current state upon probe.

The atomic PWM API was designed with that in mind. The original use-
case was to allow seamlessly taking over from a PWM regulator. In order
to do so, the driver needs to be able to read back the hardware state
and *not* initialize the PWM to some default state.

I think that same approach can be extended to backlights. The driver's
probe needs to determine what the current state of the backlight is and
preferable not touch it. And that basically propagates all the way to
the display driver, which ultimately needs to determine whether or not
the display configuration (including the backlight) is enabled.

>  b) Thierry made it quite clear[1] that the PWM pin should be configured
>     in a pinctrl of the pwm device, not the backlight (or more general:
>     the consumer) device.
> 
> While I don't entirely agree with b) I think that even a) alone
> justifies to think a bit more about the problem and preferably come up
> with a solution that helps other consumers, too. Ideally if the
> bootloader sets up the PWM to do something sensible, probing the
> lowlevel PWM driver and the consumer driver should not interfere with
> the bootloader's intention until the situation reaches a controlled
> state. (I.e. if the backlight was left on by the bootloader to show a
> nice logo, it should not flicker until a userspace program takes over
> the display device.)

Yes, exactly that.

> A PWM is special in contrast to other devices as its intended behaviour
> is only fixed once a consumer is present. Without a consumer it is
> unknown if the PWM is inverted or not. And so the common approach that
> pinctrl is setup by the device core only doesn't work without drawbacks
> for PWMs.

Actually I don't think PWMs are special in this regard. A GPIO, for
example, can also be active-low or active-high, and without a consumer
there's not enough context to determine which one it should be.

So this is really a more general problem. Whenever you want to take over
some bootloader configuration, basically all of your OS drivers have to
be taught to handle it properly, which usually means not touching any
hardware at probe time, preferably reading back the current hardware
state and "apply the delta" once the consumer says so.

Thierry

> So if a PWM driver is probing and the PWM hardware already runs at say
> constant one, some instance must define if the pin is supposed to be
> configured in its "default" or "sleep" pinctrl. IMHO this isn't possible
> in general without knowing the polarity of the PWM. (And even if it were
> known that the polarity is inversed, it might be hard to say if your
> PWM's hardware doesn't implement a disabled state and has to simulate
> that using a 0% duty cycle.)
> 
> Another thing that complicates the matter is that at least pwm-imx27 has
> the annoying property that disabling it (in hardware) drives the pin low
> irrespective of the configured polarity. So if you want this type of
> device to behave properly on disable, it must first drive a 0% duty
> cycle, then switch the pinctrl state and only then disable the hardware.
> This rules out that the lowlevel driver is unaware of the pinctrl stuff
> which would be nice (or an inverted PWM won't be disabled in hardware or
> you need an ugly sequence of callbacks to disable glitch-free). Also if
> there is no sleep state, you better don't disable an inversed pwm-imx27
> at all (in hardware)? (Alternatively we could drop the (undocumented)
> guarantee that a disabled PWM results in the pin staying in its idle
> level.)
> 
> What are the ways out? I think that if we go and apply your patch, we
> should at least write some documentation with the details to provide some
> "standard" way to solve similar problems.
> 
> Also it might be a good idea to let a PWM know if it is inverted or not
> such that even without the presence of a consumer it can determine if
> the hardware is active or not at probe time (in most cases at least).
> 
> Best regards
> Uwe
> 
> [1] https://www.spinics.net/lists/linux-pwm/msg08246.html
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Uwe Kleine-König]

On Tue, Jun 25, 2019 at 11:58:21AM +0200, Thierry Reding wrote:
> On Tue, Jun 25, 2019 at 09:42:20AM +0200, Uwe Kleine-König wrote:
> > On Wed, May 22, 2019 at 06:34:28PM +0200, Paul Cercueil wrote:
> > > When the driver probes, the PWM pin is automatically configured to its
> > > default state, which should be the "pwm" function. 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.
> > 
> > I'm not sure I understand the problem completely here. Let me try to
> > summarize the problem you solve here:
> > 
> > The backlight device's default pinctrl contains the PWM function of the
> > PWM pin. As the PWM is (or at least might be) in an undefined state the
> > default pinctrl should only be switched to when it's clear if the
> > backlight should be on or off.
> > 
> > So you use the "init"-pinctrl to keep the PWM pin in some (undriven?)
> > state and by switching to "sleep" you prevent "default" getting active.
> > 
> > Did I get this right? If not, please correct me.
> > 
> > What is the PWM pin configured to in "init" in your case? Is the pinctrl
> > just empty? Or is it a gpio-mode (together with a gpio-hog)?
> > 
> > My thoughts to this is are:
> > 
> >  a) This is a general problem that applies (I think) to most if not all
> >     PWM consumers. If the PWM drives a motor, or makes your mobile
> >     vibrate, or drives an LED, or a clk, the PWM shouldn't start
> >     to do something before its consumer is ready.
> 
> Yes, it shouldn't start before it is explicitly told to do so by the
> consumer. One exception is if the PWM was already set up by firmware
> to run. So I think in general terms we always want the PWM to remain
> in the current state upon probe.

In the end this means that also pinmuxing should not be touched when the
PWM device probes.

> The atomic PWM API was designed with that in mind. The original use-
> case was to allow seamlessly taking over from a PWM regulator. In order
> to do so, the driver needs to be able to read back the hardware state
> and *not* initialize the PWM to some default state.
> 
> I think that same approach can be extended to backlights. The driver's
> probe needs to determine what the current state of the backlight is and
> preferable not touch it. And that basically propagates all the way to
> the display driver, which ultimately needs to determine whether or not
> the display configuration (including the backlight) is enabled.

Are you ambitious enough to handle cases like: PWM is running (maybe
because it cannot be disabled), but the pin is muxed to an "unconnected"
configuration such that the pin doesn't oscillate? In this case you'd
need an inspection function for pinmuxing.

> >  b) Thierry made it quite clear[1] that the PWM pin should be configured
> >     in a pinctrl of the pwm device, not the backlight (or more general:
> >     the consumer) device.
> > 
> > While I don't entirely agree with b) I think that even a) alone
> > justifies to think a bit more about the problem and preferably come up
> > with a solution that helps other consumers, too. Ideally if the
> > bootloader sets up the PWM to do something sensible, probing the
> > lowlevel PWM driver and the consumer driver should not interfere with
> > the bootloader's intention until the situation reaches a controlled
> > state. (I.e. if the backlight was left on by the bootloader to show a
> > nice logo, it should not flicker until a userspace program takes over
> > the display device.)
> 
> Yes, exactly that.
> 
> > A PWM is special in contrast to other devices as its intended behaviour
> > is only fixed once a consumer is present. Without a consumer it is
> > unknown if the PWM is inverted or not. And so the common approach that
> > pinctrl is setup by the device core only doesn't work without drawbacks
> > for PWMs.
> 
> Actually I don't think PWMs are special in this regard. A GPIO, for
> example, can also be active-low or active-high, and without a consumer
> there's not enough context to determine which one it should be.

Right, PWMs are more similar to GPIOs than to (say) backlight devices.
With your request to configure the pinmux for a PWM pin with the PWM
device instead of its consumer you're making some things more difficult.
For GPIOs it's quite common that they are muxed from their consumer
because there the same problems are present.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |