lkml.org 
[lkml]   [2019]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down


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


\
 
 \ /
  Last update: 2019-06-24 16:41    [W:0.922 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site