Re: [PATCH v3] pwm: imx: Let PWM be active during suspend

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4913 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]