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: <20181012160037.GH31561@ulmo>
Date:   Fri, 12 Oct 2018 18:00:37 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Vokáč Michal <Michal.Vokac@...ft.com>
Cc:     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>
Subject: Re: [RCF PATCH v2 2/2] pwm: imx:
 Configure output to GPIO in disabled state

On Wed, Oct 10, 2018 at 09:33:26AM +0000, Vokáč Michal wrote:
> Normally the PWM output is held LOW when PWM is disabled. This can cause
> problems when inverted PWM signal polarity is needed. With this behavior
> the connected circuit is fed by 100% duty cycle instead of being shut-off.
> 
> Allow users to define a "gpio" and a "pwm" pinctrl states. The pwm pinctrl
> state is then selected when PWM is enabled and the gpio pinctrl state is
> selected when PWM is disabled. Also add a new pwm-gpios GPIO that is used
> to drive the output in the gpio state.
> 
> If all the pinctrl states and the pwm-gpios are not correctly specified
> in DT the logic will work as before.
> 
> As an example, with this patch a PWM controlled backlight with inversed
> signal polarity can be used in full brightness range. Without this patch
> the backlight can not be turned off as brightness = 0 disables the PWM
> and that in turn set PWM output LOW, that is full brightness.
> 
> Output of the PWM with "default" pinctrl and with "pwm"+"gpio" pinctrl:
> 
> +--------------+------------+---------------+---------------------------+
> | After reset  | Bootloader | Linux pinctrl | User (sysfs, backlight..) |
> | 100k pull-up | (not used) |               |   enable    |   disable   |
> +--------------+------------+---------------+---------------------------+
>  ___________________________    default        _   _   _
>                             |_________________| |_| |_| |_|_____________
> 
>                                pwm + gpio
>  ___________________________________________   _   _   _   _____________
>                                             |_| |_| |_| |_|
> 
> Signed-off-by: Michal Vokáč <michal.vokac@...ft.com>
> ---
> Changes in v2:
>  - Utilize the "pwm" and "gpio" pinctrl states.
>  - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
>  - Select the right pinctrl state in probe. 
> 
>  drivers/pwm/pwm-imx.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 6cd3b72..3502123 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -10,11 +10,13 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
> @@ -92,10 +94,45 @@ struct imx_chip {
>  	void __iomem	*mmio_base;
>  
>  	struct pwm_chip	chip;
> +
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_pins_gpio;
> +	struct pinctrl_state *pinctrl_pins_pwm;
> +	struct gpio_desc *pwm_gpiod;
>  };
>  
> +
>  #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
>  
> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
> +		struct platform_device *pdev)
> +{
> +	imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!imx_chip->pinctrl || IS_ERR(imx_chip->pinctrl)) {

This is not correct. First, I don't think devm_pinctrl_get() will ever
return NULL, so the !imx_chip->pinctrl is useless. And if it did return
NULL and imx_chip->pinctrl could therefore be NULL, then...

> +		dev_info(&pdev->dev, "can not get pinctrl\n");
> +		return PTR_ERR(imx_chip->pinctrl);

... this is rubbish because it returns PTR_ERR(NULL) which is 0 and that
represents success.

While at it, dev_info() -> dev_err() might be more appropriate here. Or
if you want to make pinctrl support optional make this dev_dbg() like
the message further below.

> +	}
> +
> +	imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
> +			"pwm");
> +	imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
> +			"gpio");
> +	imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
> +			GPIOD_IN);
> +
> +	if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;
> +	} else if (IS_ERR(imx_chip->pwm_gpiod) ||
> +		   IS_ERR(imx_chip->pinctrl_pins_pwm) ||
> +		   IS_ERR(imx_chip->pinctrl_pins_gpio)) {
> +		dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");

Another option would be to make this (and the above) dev_warn() since we
do want people to update the DTB if at all possible. Without the DTB the
PWM could be susceptible to the issue that we're trying to fix.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)