Hello Michal,

On Wed, Nov 07, 2018 at 01:32:10PM +0000, Vokáč Michal wrote:

On 7.11.2018 10:33, Uwe Kleine-König wrote:

quoted

Hello Michal,

just to state it more explicitly, I think the following patch (not even
compile tested) is much preferable over your approach:

Interesting idea. I just wonder why nobody else did not come up with such
a simple solution before.

I think I mentioned it already in this thread, but it went unnoticed :-)

quoted

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 1d5242c9cde0..af88644b5efb 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c

@@ -216,7 +216,14 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
  			cr |= MX3_PWMCR_POUTC;
  
  		writel(cr, imx->mmio_base + MX3_PWMCR);
-	} else if (cstate.enabled) {
+	} else if (cstate.enabled && state->polarity == PWM_POLARITY_NORMAL) {
+		/*
+		 * When disabled in hardware the output pin goes to 0
+		 * independant of the polarity setting. The expectation of some
+		 * people however is that after disabling the pin goes to the
+		 * inactive level which isn't given for an inversed pwm, so
+		 * only disable for normal polarity.
+		 */
  		writel(0, imx->mmio_base + MX3_PWMCR);
  
  		clk_disable_unprepare(imx->clk_per);

I tested your patch. It does not work as you expected.

In v4.20-rc1 the pwm-backlight driver has been converted to atomic API.
So the pwm_apply_v2 function is called only once to set new period/duty
and state. With your patch that means that "echo 0 > brightness" has no
visible effect. It leaves the PWM chip enabled with period/duty set to
however it was. But the core thinks it was reconfigured:

Then the patch isn't correct yet. The idea is always keep the hardware
running and only disable it if it's uninverted.

# cat /sys/class/backlight/backight/brightness
0

# cat /sys/kernel/debug/pwm
platform/2080000.pwm, 1 PWM device
  pwm-0   (backlight           ): requested period: 500000 ns duty: 0 ns polarity: inverse

quoted

I think it solves most if not all problems you want to address with the
pinctrl stuff.

Unfortunately not. I also tested your patch on v4.19. It works as you
probably intended - it is possible to disable backlight without the PWM
chip being disabled. But it does not solve the time frame between
imx_pwm_probe() and imx_pwm_apply_v2().

In imx_pwm_probe it's not yet known what the polarity is supposed to be,
right? So the right thing to do there is to not touch the configuration
of the pwm. I think all states that are problematic then are also
problematic with the gpio/pinmux approach. (I might be wrong here, so
feel free to invest some brain cycles to prove me wrong. But I'd be
surprised if there are problems that are relevant.)
 

In probe you do not have any users yet. So you do not know the requested
output polarity. With "default" pinctrl the PWM output would be muxed to
the selected pin and since the PWM chip is most probably disabled
(unless you enabled it in bootloader) you would get low level on the pin.
That means your backlight is fully enabled until the first call to
imx_pwm_apply_v2(). On my system this is 2 seconds.

With the gpio/pinmux approach you don't know the intended polarity
either and maybe enable the display, too. For both the solution is to
let the bootloader enable the pwm with the right output level.
Am I missing something?

It might not be a big issue for backlight but for motor control it is
not the right thing to do.

The other thing is I would prefer to make the change optional. With your
approach you are changing the behavior for all current users of inverted
PWM. I do not think all imx6 users are aware of the problem so they might
not be OK with the sudden change in the behavior.

Isn't my change an improvement for all users? What state do you have in
mind that make things worse than they are now?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |