Messages in this thread | | | Date | Tue, 25 Oct 2022 11:02:00 +0100 | From | Paul Cercueil <> | Subject | Re: [PATCH 1/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 1 |
| |
Le mar. 25 oct. 2022 à 08:21:29 +0200, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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@crapouillou.net> >> Cc: stable@vger.kernel.org > > 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/ |
| |