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@xxxxxxxxxxxxxxx> > --- > > drivers/video/backlight/pwm_bl.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > index fb45f866b923..422f7903b382 100644 > > --- a/drivers/video/backlight/pwm_bl.c > > +++ b/drivers/video/backlight/pwm_bl.c > > @@ -16,6 +16,7 @@ > > #include <linux/module.h> > > #include <linux/kernel.h> > > #include <linux/init.h> > > +#include <linux/pinctrl/consumer.h> > > #include <linux/platform_device.h> > > #include <linux/fb.h> > > #include <linux/backlight.h> > > @@ -50,6 +51,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > struct pwm_state state; > > int err; > > + pinctrl_pm_select_default_state(pb->dev); > > + > > pwm_get_state(pb->pwm, &state); > > if (pb->enabled) > > return; > > @@ -90,6 +93,8 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) > > regulator_disable(pb->power_supply); > > pb->enabled = false; > > + > > + pinctrl_pm_select_sleep_state(pb->dev); > > } > > static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > > @@ -626,6 +631,10 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > backlight_update_status(bl); > > platform_set_drvdata(pdev, bl); > > + > > + if (bl->props.power == FB_BLANK_POWERDOWN) > > + pinctrl_pm_select_sleep_state(&pdev->dev); > > Didn't backlight_update_status(bl) already do this? > > > Daniel. > > > > + > > return 0; > > err_alloc: > > >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel