lkml.org 
[lkml]   [2019]   [Jun]   [21]   [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
    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:
    > >
    >
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2019-06-21 15:56    [W:2.249 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site