| Message ID | 1511220443-26629-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] pwm: imx: Let PWM be active during suspend | expand |
Hi Thierry, On Mon, Nov 20, 2017 at 9:27 PM, Fabio Estevam <festevam@gmail.com> wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > On a imx6q-cubox-i board, which has an LED driven by PWM, when the system > goes into suspend the PWM block is disabled by default, then the PWM pin > goes to logic level zero and turn on the LED during suspend, which is not > really the behaviour we want to see. > > By keeping the PWM enabled during suspend via STOPEN bit, the pwm-leds > driver sets the brightness to zero in suspend and then the LED is > turned off as expected. > > So always set the STOPEN to fix the PWM behaviour in suspend. > > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > Changes since v2: > - Remove the optional dt property and set the STOPEN bit unconditionally (Rob) Any comments? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 20, 2017 at 09:27:23PM -0200, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > On a imx6q-cubox-i board, which has an LED driven by PWM, when the system > goes into suspend the PWM block is disabled by default, then the PWM pin > goes to logic level zero and turn on the LED during suspend, which is not > really the behaviour we want to see. > > By keeping the PWM enabled during suspend via STOPEN bit, the pwm-leds > driver sets the brightness to zero in suspend and then the LED is > turned off as expected. > > So always set the STOPEN to fix the PWM behaviour in suspend. It looks like this would also keep other PWMs enabled in suspend. If for example you hooked up a fan to this PWM this change would make it so that the fan would remain on during suspend. That doesn't sound desirable to me. On v2, I see this reply from you: Please note that on imx6qdl-cuboxi the pwm is active low. I also see that imx6q-cubox-i uses the "fsl,imx27-pwm" compatible version of this controller and that supports polarity inversion. I think the correct thing to do here is to mark the PWM as inverted (according to the DTS file it is actually the pwmleds node that has an active-low property). If you invert the PWM you could add extra code to the PWM driver to deal with this properly and set STOPEN only for inverted PWM signals. Thierry
Hi Thierry, On Tue, Dec 5, 2017 at 6:47 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > It looks like this would also keep other PWMs enabled in suspend. If for > example you hooked up a fan to this PWM this change would make it so > that the fan would remain on during suspend. That doesn't sound > desirable to me. It is expected that the PWM fan driver would disable the fan upon entering into suspend. The old vendor driver also sets the STOPEN bit unconditionally: http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_2.6.35_11.09.01#n97 > > On v2, I see this reply from you: > > Please note that on imx6qdl-cuboxi the pwm is active low. What I meant to say is that a logic level 0 turns on the LED. > I also see that imx6q-cubox-i uses the "fsl,imx27-pwm" compatible > version of this controller and that supports polarity inversion. I think > the correct thing to do here is to mark the PWM as inverted (according > to the DTS file it is actually the pwmleds node that has an active-low > property). If you invert the PWM you could add extra code to the PWM > driver to deal with this properly and set STOPEN only for inverted PWM > signals. Polarity is correct: echo 0 > brightness --> turns off the LED echo 248 > brightness ---> turns on the LED with the maximum brightness It is only the behaviour during suspend which is not correct (LCD turns on). Tried your suggestion of changing the polarity and it does not work and breaks the usage during normal mode (non low power). Another way to fix this would be to do like tegra pwm driver does in implementing suspend/resume: https://pastebin.com/sAFdKpDv where the pinctrl state can be switched. I tested it here and it works too. Would it be acceptable to do add suspend/resume hooks for pwm-imx? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thierry, On Tue, Dec 5, 2017 at 4:56 PM, Fabio Estevam <festevam@gmail.com> wrote: > Hi Thierry, > > On Tue, Dec 5, 2017 at 6:47 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > >> It looks like this would also keep other PWMs enabled in suspend. If for >> example you hooked up a fan to this PWM this change would make it so >> that the fan would remain on during suspend. That doesn't sound >> desirable to me. > > It is expected that the PWM fan driver would disable the fan upon > entering into suspend. and the PWM fan driver does disable the PWM as expected: pwm_fan_suspend() calls pwm_disable(). > The old vendor driver also sets the STOPEN bit unconditionally: > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_2.6.35_11.09.01#n97 Same on the 3.0.35 vendor kernel for MX6: http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_3.0.35_4.1.0#n106 >> >> On v2, I see this reply from you: >> >> Please note that on imx6qdl-cuboxi the pwm is active low. > > What I meant to say is that a logic level 0 turns on the LED. > >> I also see that imx6q-cubox-i uses the "fsl,imx27-pwm" compatible >> version of this controller and that supports polarity inversion. I think >> the correct thing to do here is to mark the PWM as inverted (according >> to the DTS file it is actually the pwmleds node that has an active-low >> property). If you invert the PWM you could add extra code to the PWM >> driver to deal with this properly and set STOPEN only for inverted PWM >> signals. > > Polarity is correct: > > echo 0 > brightness --> turns off the LED > echo 248 > brightness ---> turns on the LED with the maximum brightness > > It is only the behaviour during suspend which is not correct (LCD turns on). > > Tried your suggestion of changing the polarity and it does not work > and breaks the usage during normal mode (non low power). > > Another way to fix this would be to do like tegra pwm driver does in > implementing suspend/resume: > https://pastebin.com/sAFdKpDv > > where the pinctrl state can be switched. > > I tested it here and it works too. > > Would it be acceptable to do add suspend/resume hooks for pwm-imx? It works, but I am still happier with the current proposal in version 3, where STOPEN bit is set. Please let me know if you are willing to consider such proposal. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 05, 2017 at 04:56:00PM -0200, Fabio Estevam wrote: > Hi Thierry, > > On Tue, Dec 5, 2017 at 6:47 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > > It looks like this would also keep other PWMs enabled in suspend. If for > > example you hooked up a fan to this PWM this change would make it so > > that the fan would remain on during suspend. That doesn't sound > > desirable to me. > > It is expected that the PWM fan driver would disable the fan upon > entering into suspend. > > The old vendor driver also sets the STOPEN bit unconditionally: > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_2.6.35_11.09.01#n97 > > > > > On v2, I see this reply from you: > > > > Please note that on imx6qdl-cuboxi the pwm is active low. > > What I meant to say is that a logic level 0 turns on the LED. That's the same thing. > > I also see that imx6q-cubox-i uses the "fsl,imx27-pwm" compatible > > version of this controller and that supports polarity inversion. I think > > the correct thing to do here is to mark the PWM as inverted (according > > to the DTS file it is actually the pwmleds node that has an active-low > > property). If you invert the PWM you could add extra code to the PWM > > driver to deal with this properly and set STOPEN only for inverted PWM > > signals. > > Polarity is correct: > > echo 0 > brightness --> turns off the LED > echo 248 > brightness ---> turns on the LED with the maximum brightness > > It is only the behaviour during suspend which is not correct (LCD turns on). This does indicate all the more that you're trying to invert the polarity in the user driver. If you look at the driver code it will simply invert the duty cycle for active-low LEDs. That matches the symptoms that you describe: when you set zero brightness you will in fact get full duty cycle and hence the LED turns off. However, this does no longer work if the PWM signal is truly inverted, since the "off" state of the PWM signal will actually be "on". > Tried your suggestion of changing the polarity and it does not work > and breaks the usage during normal mode (non low power). How does it break? Note that you have to change the polarity of the PWM signal (in the pwms property) and then remove the "active-low" property from the leds device tree node to avoid double inversion. Thierry
On Mon, Dec 11, 2017 at 10:16:25AM +0100, Thierry Reding wrote: > On Tue, Dec 05, 2017 at 04:56:00PM -0200, Fabio Estevam wrote: > > Hi Thierry, > > > > On Tue, Dec 5, 2017 at 6:47 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > It looks like this would also keep other PWMs enabled in suspend. If for > > > example you hooked up a fan to this PWM this change would make it so > > > that the fan would remain on during suspend. That doesn't sound > > > desirable to me. > > > > It is expected that the PWM fan driver would disable the fan upon > > entering into suspend. > > > > The old vendor driver also sets the STOPEN bit unconditionally: > > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_2.6.35_11.09.01#n97 > > > > > > > > On v2, I see this reply from you: > > > > > > Please note that on imx6qdl-cuboxi the pwm is active low. > > > > What I meant to say is that a logic level 0 turns on the LED. > > That's the same thing. > > > > I also see that imx6q-cubox-i uses the "fsl,imx27-pwm" compatible > > > version of this controller and that supports polarity inversion. I think > > > the correct thing to do here is to mark the PWM as inverted (according > > > to the DTS file it is actually the pwmleds node that has an active-low > > > property). If you invert the PWM you could add extra code to the PWM > > > driver to deal with this properly and set STOPEN only for inverted PWM > > > signals. > > > > Polarity is correct: > > > > echo 0 > brightness --> turns off the LED > > echo 248 > brightness ---> turns on the LED with the maximum brightness > > > > It is only the behaviour during suspend which is not correct (LCD turns on). > > This does indicate all the more that you're trying to invert the > polarity in the user driver. If you look at the driver code it will > simply invert the duty cycle for active-low LEDs. That matches the > symptoms that you describe: when you set zero brightness you will > in fact get full duty cycle and hence the LED turns off. However, > this does no longer work if the PWM signal is truly inverted, since > the "off" state of the PWM signal will actually be "on". There was discussion back in 2014 about this. The problem is the iMX6 PWM implementation is rather fscked. The pwm specification in DT has support for inverting the PWM output: "Optionally, the pwm-specifier can encode a number of flags (defined in <dt-bindings/pwm/pwm.h>) in a third cell: - PWM_POLARITY_INVERTED: invert the PWM signal polarity" The problem, though, is that the imx6 PWM driver never took advantage of that, and set #pwm-cells to 2. So, for imx6, the PWM specification is a phandle and a specifier, without the flags. Adding the flags is not trivial (as was discussed back then) as DT has no knowledge apart from the #*-cells property about how many entries there are - the < > is just for our eyeballs and is mostly otherwise meaningless. Had this not been the case, then adding support for PWM_POLARITY_INVERTED in pwm-imx6 would have been trivial, and probably the correct solution, but alas, the discussion back then pointed to the current solution being the best given the already broken implementation.
Hi Thierry, On Mon, Dec 11, 2017 at 7:16 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > How does it break? Note that you have to change the polarity of the PWM > signal (in the pwms property) and then remove the "active-low" property > from the leds device tree node to avoid double inversion. This is what I tried as per your suggestion: --- a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi +++ b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi @@ -42,6 +42,7 @@ #include "imx6qdl-microsom-ar8035.dtsi" #include <dt-bindings/input/input.h> #include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/pwm/pwm.h> / { ir_recv: ir-receiver { @@ -57,10 +58,9 @@ pinctrl-0 = <&pinctrl_cubox_i_pwm1>; front { - active-low; label = "imx6:red:front"; max-brightness = <248>; - pwms = <&pwm1 0 50000>; + pwms = <&pwm1 0 50000 PWM_POLARITY_INVERTED>; }; }; @@ -231,6 +231,7 @@ }; &pwm1 { + #pwm-cells = <3>; status = "okay"; }; The problem I see is that it seems to be an off-by-one calculation in the pwm-imx driver for the inverted polarity case. For example: # cd /sys/class/leds/imx6\:red\:front/ # echo 0 > brightness (This keeps the LED on with its maximum brightness) # echo 1 > brightness (This keeps the LED off) # echo 248 > brightness (This keeps the LED on with its maximum brightness) When I go to suspend: # echo enabled > /sys/class/tty/ttymxc0/power/wakeup # echo mem > /sys/power/state Then LED goes on with its maximum brightness. So the issue now is that I need to tell the pwm-imx driver that '0' means to turn the LED off. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 11, 2017 at 09:36:00AM +0000, Russell King - ARM Linux wrote: > On Mon, Dec 11, 2017 at 10:16:25AM +0100, Thierry Reding wrote: > > On Tue, Dec 05, 2017 at 04:56:00PM -0200, Fabio Estevam wrote: > > > Hi Thierry, > > > > > > On Tue, Dec 5, 2017 at 6:47 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > It looks like this would also keep other PWMs enabled in suspend. If for > > > > example you hooked up a fan to this PWM this change would make it so > > > > that the fan would remain on during suspend. That doesn't sound > > > > desirable to me. > > > > > > It is expected that the PWM fan driver would disable the fan upon > > > entering into suspend. > > > > > > The old vendor driver also sets the STOPEN bit unconditionally: > > > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_2.6.35_11.09.01#n97 > > > > > > > > > > > On v2, I see this reply from you: > > > > > > > > Please note that on imx6qdl-cuboxi the pwm is active low. > > > > > > What I meant to say is that a logic level 0 turns on the LED. > > > > That's the same thing. > > > > > > I also see that imx6q-cubox-i uses the "fsl,imx27-pwm" compatible > > > > version of this controller and that supports polarity inversion. I think > > > > the correct thing to do here is to mark the PWM as inverted (according > > > > to the DTS file it is actually the pwmleds node that has an active-low > > > > property). If you invert the PWM you could add extra code to the PWM > > > > driver to deal with this properly and set STOPEN only for inverted PWM > > > > signals. > > > > > > Polarity is correct: > > > > > > echo 0 > brightness --> turns off the LED > > > echo 248 > brightness ---> turns on the LED with the maximum brightness > > > > > > It is only the behaviour during suspend which is not correct (LCD turns on). > > > > This does indicate all the more that you're trying to invert the > > polarity in the user driver. If you look at the driver code it will > > simply invert the duty cycle for active-low LEDs. That matches the > > symptoms that you describe: when you set zero brightness you will > > in fact get full duty cycle and hence the LED turns off. However, > > this does no longer work if the PWM signal is truly inverted, since > > the "off" state of the PWM signal will actually be "on". > > There was discussion back in 2014 about this. The problem is the > iMX6 PWM implementation is rather fscked. > > The pwm specification in DT has support for inverting the PWM output: > > "Optionally, the pwm-specifier can encode a number of flags (defined in > <dt-bindings/pwm/pwm.h>) in a third cell: > - PWM_POLARITY_INVERTED: invert the PWM signal polarity" > > The problem, though, is that the imx6 PWM driver never took advantage > of that, and set #pwm-cells to 2. So, for imx6, the PWM specification > is a phandle and a specifier, without the flags. > > Adding the flags is not trivial (as was discussed back then) as DT > has no knowledge apart from the #*-cells property about how many > entries there are - the < > is just for our eyeballs and is mostly > otherwise meaningless. > > Had this not been the case, then adding support for PWM_POLARITY_INVERTED > in pwm-imx6 would have been trivial, and probably the correct > solution, but alas, the discussion back then pointed to the current > solution being the best given the already broken implementation. We ended up merging a solution for this earlier this year that everybody was confident would allow us to do this in a backwards-compatible way: commit 42883cbc086b3f7aca9f1754f2d570af922825fc Author: Lothar Wassmann <LW@KARO-electronics.de> Date: Sun Jan 29 22:54:13 2017 +0100 pwm: Make the PWM_POLARITY flag in DTB optional Change the PWM chip driver registration so that a chip driver that supports polarity inversion can still be used with DTBs that don't provide the polarity flag as part of the specifier. This is done to provide polarity inversion support for the pwm-imx driver without having to modify all existing DTS files. Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> Suggested-by: Sascha Hauer <s.hauer@pengutronix.de> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> I think this should allow pwm-imx to fully take advantage of the polarity inversion feature. Although the DT would have to be changed and all PWM specifiers updated at the same time as the #pwm-cells property. Adding Lothar for visibility since he's obviously been using this. Thierry
On Mon, Dec 11, 2017 at 08:54:19AM -0200, Fabio Estevam wrote: > Hi Thierry, > > On Mon, Dec 11, 2017 at 7:16 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > > How does it break? Note that you have to change the polarity of the PWM > > signal (in the pwms property) and then remove the "active-low" property > > from the leds device tree node to avoid double inversion. > > This is what I tried as per your suggestion: > > --- a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi > @@ -42,6 +42,7 @@ > #include "imx6qdl-microsom-ar8035.dtsi" > #include <dt-bindings/input/input.h> > #include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/pwm/pwm.h> > > / { > ir_recv: ir-receiver { > @@ -57,10 +58,9 @@ > pinctrl-0 = <&pinctrl_cubox_i_pwm1>; > > front { > - active-low; > label = "imx6:red:front"; > max-brightness = <248>; > - pwms = <&pwm1 0 50000>; > + pwms = <&pwm1 0 50000 PWM_POLARITY_INVERTED>; > }; > }; > > @@ -231,6 +231,7 @@ > }; > > &pwm1 { > + #pwm-cells = <3>; > status = "okay"; > }; > > The problem I see is that it seems to be an off-by-one calculation in > the pwm-imx driver for the inverted polarity case. > > For example: > > # cd /sys/class/leds/imx6\:red\:front/ > # echo 0 > brightness (This keeps the LED on with its maximum brightness) > # echo 1 > brightness (This keeps the LED off) > # echo 248 > brightness (This keeps the LED on with its maximum brightness) > > When I go to suspend: > > # echo enabled > /sys/class/tty/ttymxc0/power/wakeup > # echo mem > /sys/power/state > > Then LED goes on with its maximum brightness. > > So the issue now is that I need to tell the pwm-imx driver that '0' > means to turn the LED off. Yeah, my suggestion was to make setting the STOPEN bit dependent on the polarity inversion setting of the PWM. That way you don't have to hard- code it, which might not be the correct setting for non-inverted PWMs. Thierry
Adding Lukasz and Lothar who seem to have used pwm-imx driver with polarity inverted. On Mon, Dec 11, 2017 at 10:13 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > Yeah, my suggestion was to make setting the STOPEN bit dependent on the > polarity inversion setting of the PWM. That way you don't have to hard- > code it, which might not be the correct setting for non-inverted PWMs. After using the polarity inversion the LED comes turned on after boot. Setting the STOPEN bit does not help in this case, as It keeps on during suspend. The problem is that when polarity inversion is used I can no longer turn the LED off: # echo 0 > brightness (This keeps the LED on with its maximum brightness, so there is a bug here) # echo 1 > brightness (This keeps the LED off - well, actually it is turned on with a minimum brightness - expected behaviour) # echo 248 > brightness (This keeps the LED on with its maximum brightness - expected behaviour) So I think that once I managed to make 'echo 0 > brightness' to really turn off the LED then the original issue (LED turned on during suspend) will be gone. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, 11 Dec 2017 10:24:20 -0200 Fabio Estevam wrote: > Adding Lukasz and Lothar who seem to have used pwm-imx driver with > polarity inverted. > > On Mon, Dec 11, 2017 at 10:13 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > > Yeah, my suggestion was to make setting the STOPEN bit dependent on the > > polarity inversion setting of the PWM. That way you don't have to hard- > > code it, which might not be the correct setting for non-inverted PWMs. > > After using the polarity inversion the LED comes turned on after boot. > > Setting the STOPEN bit does not help in this case, as It keeps on > during suspend. > > The problem is that when polarity inversion is used I can no longer > turn the LED off: > > # echo 0 > brightness (This keeps the LED on with its maximum > brightness, so there is a bug here) > # echo 1 > brightness (This keeps the LED off - well, actually it is > turned on with a minimum brightness - expected behaviour) > # echo 248 > brightness (This keeps the LED on with its maximum > brightness - expected behaviour) > > So I think that once I managed to make 'echo 0 > brightness' to really > turn off the LED then the original issue (LED turned on during > suspend) will be gone. > The problem is, that the PWM output goes low whenever the PWM is disabled, no matter what polarity is programmed. So, you would need to prevent the PWM from being disabled whenever the duty cycle is set to 0. I'm using the PWM for an LCD backlight that has a separate ENABLE pin. Thus by driving that pin via the backlight power supply regulator, the backlight actually switches off, though the PWM output is constantly active (LOW). Lothar Waßmann -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lothar, On Mon, Dec 11, 2017 at 11:20 AM, Lothar Waßmann <LW@karo-electronics.de> wrote: > The problem is, that the PWM output goes low whenever the PWM is > disabled, no matter what polarity is programmed. So, you would need to > prevent the PWM from being disabled whenever the duty cycle is set to 0. Yes, that's the key point: PWM output goes low when PWM is disabled. I don't think we can workaround this behaviour in the pwm-imx driver. So at the moment I see two ways to fix this: 1. Set the STOPEN bit which is what I do in this patch or 2. Add support for allowing changing pinctrl in suspend: https://pastebin.com/71UGrnkA This way we can configure the pwm output pin as Hi-Z during suspend. What do you think? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, 11 Dec 2017 13:07:46 -0200 Fabio Estevam wrote: > Hi Lothar, > > On Mon, Dec 11, 2017 at 11:20 AM, Lothar Waßmann <LW@karo-electronics.de> wrote: > > > The problem is, that the PWM output goes low whenever the PWM is > > disabled, no matter what polarity is programmed. So, you would need to > > prevent the PWM from being disabled whenever the duty cycle is set to 0. > > Yes, that's the key point: PWM output goes low when PWM is disabled. > > I don't think we can workaround this behaviour in the pwm-imx driver. > > So at the moment I see two ways to fix this: > > 1. Set the STOPEN bit which is what I do in this patch > This should probably work. But this won't fix the issue, that the LED or backlight is turned fully on when the brightness value is set to 0, since both the leds-pwm and the pwmbl driver explicitly disable the PWM, when the brightness value is set to 0. > 2. Add support for allowing changing pinctrl in suspend: > https://pastebin.com/71UGrnkA > > This way we can configure the pwm output pin as Hi-Z during suspend. > > What do you think? > It would be better to enable the 22K pullup on the pin. Otherwise the pin will be floating when there is no external pullup resistor. Lothar Waßmann -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lothar, On Mon, Dec 11, 2017 at 1:55 PM, Lothar Waßmann <LW@karo-electronics.de> wrote: >> 1. Set the STOPEN bit which is what I do in this patch >> > This should probably work. > But this won't fix the issue, that the LED or backlight is turned fully > on when the brightness value is set to 0, since both the leds-pwm and > the pwmbl driver explicitly disable the PWM, when the brightness > value is set to 0. Yes, but in the case of imx6qdl-cubox-i.dtsi the PWM is used in the normal polarity and the inversion is achieved via 'active-low' property of pwm-leds. On this board the current dts + current pwm-imx driver behaves incorrecly only during suspend (LED is on). During normal mode the PWM operation is correct. By setting the STOPEN bit the LED is off in suspend. >> 2. Add support for allowing changing pinctrl in suspend: >> https://pastebin.com/71UGrnkA >> >> This way we can configure the pwm output pin as Hi-Z during suspend. >> >> What do you think? >> > It would be better to enable the 22K pullup on the pin. Otherwise the > pin will be floating when there is no external pullup resistor. Yes, but first we need to come to a conclusion if we go with solution 1 (enable STOPEN) or with solution 2 (allow changinh pinctrl) :-) I am more inclined to go with solution 1 as this was also used in the old vendor 3.0.35 kernel: http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_3.0.35_4.1.0#n106 Please let me know what you prefer. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, 11 Dec 2017 14:06:31 -0200 Fabio Estevam wrote: > Hi Lothar, > > On Mon, Dec 11, 2017 at 1:55 PM, Lothar Waßmann <LW@karo-electronics.de> wrote: > > >> 1. Set the STOPEN bit which is what I do in this patch > >> > > This should probably work. > > But this won't fix the issue, that the LED or backlight is turned fully > > on when the brightness value is set to 0, since both the leds-pwm and > > the pwmbl driver explicitly disable the PWM, when the brightness > > value is set to 0. > > Yes, but in the case of imx6qdl-cubox-i.dtsi the PWM is used in the > normal polarity and the inversion is achieved via 'active-low' > property of pwm-leds. > > On this board the current dts + current pwm-imx driver behaves > incorrecly only during suspend (LED is on). During normal mode the PWM > operation is correct. > > By setting the STOPEN bit the LED is off in suspend. > > >> 2. Add support for allowing changing pinctrl in suspend: > >> https://pastebin.com/71UGrnkA > >> > >> This way we can configure the pwm output pin as Hi-Z during suspend. > >> > >> What do you think? > >> > > It would be better to enable the 22K pullup on the pin. Otherwise the > > pin will be floating when there is no external pullup resistor. > > Yes, but first we need to come to a conclusion if we go with solution > 1 (enable STOPEN) or with solution 2 (allow changinh pinctrl) :-) > > I am more inclined to go with solution 1 as this was also used in the > old vendor 3.0.35 kernel: > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/plat-mxc/pwm.c?h=imx_3.0.35_4.1.0#n106 > > Please let me know what you prefer. > I also prefer the patch that doesn't mess with the pin configs. Lothar Waßmann -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lothar,
On Mon, Dec 11, 2017 at 2:52 PM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> I also prefer the patch that doesn't mess with the pin configs.
Thanks for your feedback.
Would you mind testing this patch on your system?
https://patchwork.ozlabs.org/patch/839834/
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, 11 Dec 2017 16:48:39 -0200 Fabio Estevam wrote: > Hi Lothar, > > On Mon, Dec 11, 2017 at 2:52 PM, Lothar Waßmann <LW@karo-electronics.de> wrote: > > > I also prefer the patch that doesn't mess with the pin configs. > > Thanks for your feedback. > > Would you mind testing this patch on your system? > https://patchwork.ozlabs.org/patch/839834/ > Sorry for the late response. The patch does not affect the usage of the PWM with the pwm_bl driver, since the pwm_bl driver configures the duty cycle to '0' and disables the PWM upon suspend. Lothar Waßmann -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lothar, On Fri, Dec 15, 2017 at 10:02 AM, Lothar Waßmann <LW@karo-electronics.de> wrote: >> Would you mind testing this patch on your system? >> https://patchwork.ozlabs.org/patch/839834/ >> > Sorry for the late response. > The patch does not affect the usage of the PWM with the pwm_bl driver, > since the pwm_bl driver configures the duty cycle to '0' and disables > the PWM upon suspend. Ok,so I take this as a no-objection for this patch from you. Thierry, could you please consider applying this one? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c index 2ba5c3a..08cbe81 100644 --- a/drivers/pwm/pwm-imx.c +++ b/drivers/pwm/pwm-imx.c @@ -35,6 +35,7 @@ #define MX3_PWMSAR 0x0C /* PWM Sample Register */ #define MX3_PWMPR 0x10 /* PWM Period Register */ #define MX3_PWMCR_PRESCALER(x) ((((x) - 1) & 0xFFF) << 4) +#define MX3_PWMCR_STOPEN (1 << 25) #define MX3_PWMCR_DOZEEN (1 << 24) #define MX3_PWMCR_WAITEN (1 << 23) #define MX3_PWMCR_DBGEN (1 << 22) @@ -210,7 +211,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm, writel(period_cycles, imx->mmio_base + MX3_PWMPR); cr = MX3_PWMCR_PRESCALER(prescale) | - MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH | MX3_PWMCR_EN;