Re: [PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1

[Paul Cercueil]

Le mar. 25 oct. 2022 à 08:21:29 +0200, Uwe Kleine-König 
<u.kleine-koenig@xxxxxxxxxxxxxx> a écrit :
> Hello,
>
> On Mon, Oct 24, 2022 at 09:52:09PM +0100, Paul Cercueil wrote:
> >  The "duty > cycle" trick to force the pin level of a disabled TCU2
> >  channel would only work when the channel had been enabled 
> > previously.
> >
> >  Address this issue by enabling the PWM mode in jz4740_pwm_disable
> >  (I know, right) so that the "duty > cycle" trick works before 
> > disabling
> >  the PWM channel right after.
> >
> >  This issue went unnoticed, as the PWM pins on the majority of the 
> > boards
> >  tested would default to the inactive level once the corresponding 
> > TCU
> >  clock was enabled, so the first call to jz4740_pwm_disable() would 
> > not
> >  actually change the pin levels.
> >
> >  On the GCW Zero however, the PWM pin for the backlight (PWM1, which 
> > is
> >  a TCU2 channel) goes active as soon as the timer1 clock is enabled.
> >  Since the jz4740_pwm_disable() function did not work on channels not
> >  previously enabled, the backlight would shine at full brightness 
> > from
> >  the moment the backlight driver would probe, until the backlight 
> > driver
> >  tried to *enable* the PWM output.
> >
> >  With this fix, the PWM pins will be forced inactive as soon as
> >  jz4740_pwm_apply() is called (and might be reconfigured to active if
> >  dictated by the pwm_state). This means that there is still a tiny 
> > time
> >  frame between the .request() and .apply() callbacks where the PWM 
> > pin
> >  might be active. Sadly, there is no way to fix this issue: it is
> >  impossible to write a PWM channel's registers if the corresponding 
> > clock
> >  is not enabled, and enabling the clock is what causes the PWM pin 
> > to go
> >  active.
> >
> >  There is a workaround, though, which complements this fix: simply
> >  starting the backlight driver (or any PWM client driver) with a 
> > "init"
> >  pinctrl state that sets the pin as an inactive GPIO. Once the 
> > driver is
> >  probed and the pinctrl state switches to "default", the regular PWM 
> > pin
> >  configuration can be used as it will be properly driven.
> >
> >  Fixes: c2693514a0a1 ("pwm: jz4740: Obtain regmap from parent node")
> >  Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >  Cc: stable@xxxxxxxxxxxxxxx
>
> OK, understood the issue. I think there is another similar issue: The
> clk is get and enabled only in the .request() callback. The result is 
> (I
> think---depends on a few further conditions) that if you have the
> backlight driver as a module and the bootloader enables the backlight 
> to
> show a splash screen, the backlight goes off because of the
> clk_disable_unused initcall.

I will have to verify, but I'm pretty sure disabling the clock doesn't 
change the pin level back to inactive.

-Paul

> So the right thing to do is to get the clock in .probe(), and ensure 
> it
> is kept on if the PWM is running already. Then you can also enable the
> counter in .probe() and don't care for it in the enable and disable
> functions.
>
> The init pinctrl then has to be on the PWM then, but that's IMHO ok.
>
> Best regards
> Uwe
>
> PS: While looking into the driver I noticed that .request() uses
> dev_err_probe(). That's wrong, this function is only supposed to be 
> used
> in .probe().
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> https://www.pengutronix.de/ |