linux-kernel - Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eb4e7a42-bd5c-3ae2-ccb7-d1a73d5ae99c@ysoft.com>
Date:   Wed, 7 Nov 2018 13:32:10 +0000
From:   Vokáč Michal <Michal.Vokac@...ft.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     Thierry Reding <thierry.reding@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lukasz Majewski <l.majewski@...ess.pl>,
        Fabio Estevam <fabio.estevam@....com>,
        Lothar Waßmann <LW@...o-electronics.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state

Hi Uwe,

On 7.11.2018 10:33, Uwe Kleine-König wrote:
> 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.

> 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:

# 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

> 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 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.

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.

I agree it would be nice if such a simple change as you proposed would
solve the problem. Still, I do not see any other solution than pinctrl
to deal with all the problems I would like to address.

Anyway, thank you for the idea!

All the best,
Michal