| Message ID | 20190522163428.7078-1-paul@crapouillou.net |
|---|---|
| State | Deferred |
| Headers | show |
| Series | backlight: pwm_bl: Set pin to sleep state when powered down | expand |
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: >
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: > > >
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: > > > > >
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: >> > > >> > > >
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: >> > >>
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.
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
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
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
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
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
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/ |
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
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
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
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
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
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:
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(+)