Date:   Fri, 12 Oct 2018 10:57:24 +0200
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Vokáč Michal <Michal.Vokac@...ft.com>
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
Subject: Re: [RCF PATCH,v2,2/2] pwm:
 imx: Configure output to GPIO in disabled state

Hello,

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
>  ___________________________________________   _   _   _   _____________
>                                             |_| |_| |_| |_|

I was made aware of this patch by Thierry while discussion about a patch
opportunity. I already pointed out some stuff I don't like about this
patch in the repective thread, but I repeat it here to have it at the
right place.

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

The pinctrl framework already knows about "init" and "default". These
should be enough. i.e. "init" configures as gpio and "default" als pwm.

>  };
>  
> +
>  #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)) {
> +		dev_info(&pdev->dev, "can not get pinctrl\n");
> +		return PTR_ERR(imx_chip->pinctrl);
> +	}
> +
> +	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) {

You must not use PTR_ERR on a value that might not contain an error
pointer.

> +		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)) {

Would it be more correct to handle imx_chip->pinctrl_pins_pwm ==
ERR_PTR(-EPROBE_DEFER) similar to imx_chip->pwm_gpiod ==
ERR_PTR(-EPROBE_DEFER)?

> +		dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");

I wouldn't call that "incomplete". It's incomplete for the gpio
switching trick, but enough in general.

> +		devm_pinctrl_put(imx_chip->pinctrl);
> +		imx_chip->pinctrl = NULL;
> +	}
> +
> +	return 0;
> +}
> +
>  static void imx_pwm_get_state(struct pwm_chip *chip,
>  		struct pwm_device *pwm, struct pwm_state *state)
>  {
> @@ -306,7 +343,31 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
>  					MX3_PWMCR_POUTC_INVERTED);
>  
>  		writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> +		/*
> +		 * If we are in charge of pinctrl then switch output to
> +		 * the PWM signal.
> +		 */
> +		if (imx->pinctrl)
> +			pinctrl_select_state(imx->pinctrl,
> +					imx->pinctrl_pins_pwm);
>  	} else if (cstate.enabled) {
> +		/*
> +		 * PWM block will be disabled. Normally its output will be set
> +		 * low no matter what output polarity is configured. Lets use

s/Lets/Let's/

> +		 * pinctrl to switch the output pin to GPIO functon and keep
> +		 * the output at the same level as for duty-cycle = 0.

Is it obvious that using a GPIO is more efficient/better/worth the
complexity than just enabling the PWM with duty-cycle 0 and the right
polarity?

> +		 * First set the GPIO to the desired level, then switch the
> +		 * muxing and at last disable PWM. In that order we do not get
> +		 * unwanted logic level changes on the output.
> +		 */
> +		if (imx->pinctrl) {
> +			gpiod_set_value_cansleep(imx->pwm_gpiod, 0);

You must call gpiod_direction_output for this to have any effect.
There might be mechanisms in pincontrol that automatically mux the pin
if it's configured as gpio, I didn't follow the details though.

Also it should be possible to configure the GPIO as output immediatly.
If the pinmuxing is set to the PWM function this doesn't have a visible
side effect.

> +			pinctrl_select_state(imx->pinctrl,
> +					imx->pinctrl_pins_gpio);

Usually align function arguments to the opening (.

> +		}
> +
>  		writel(0, imx->mmio_base + MX3_PWMCR);
>  
>  		clk_disable_unprepare(imx->clk_per);
> @@ -354,6 +415,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  	const struct of_device_id *of_id =
>  			of_match_device(imx_pwm_dt_ids, &pdev->dev);
>  	const struct imx_pwm_data *data;
> +	struct pwm_state cstate;
>  	struct imx_chip *imx;
>  	struct resource *r;
>  	int ret = 0;
> @@ -385,6 +447,10 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  		imx->chip.of_pwm_n_cells = 3;
>  	}
>  
> +	ret = imx_pwm_init_pinctrl_info(imx, pdev);
> +	if (ret)
> +		return ret;
> +
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
>  	if (IS_ERR(imx->mmio_base))
> @@ -394,6 +460,26 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (imx->pinctrl) {
> +		/*
> +		 * Update cstate after pwmchip_add() call as the core might
> +		 * call the get_state() function to read the PWM registers
> +		 * to get the actual HW state.
> +		 */
> +		pwm_get_state(imx->chip.pwms, &cstate);
> +		if (cstate.enabled) {
> +			dev_dbg(&pdev->dev,
> +				"PWM entered probe in enabled state\n");
> +			pinctrl_select_state(imx->pinctrl,
> +					imx->pinctrl_pins_pwm);
> +		} else {
> +			gpiod_set_value_cansleep(imx->pwm_gpiod, 0);
> +			pinctrl_select_state(imx->pinctrl,
> +					imx->pinctrl_pins_gpio);
> +
> +		}
> +	}
> +
>  	platform_set_drvdata(pdev, imx);
>  	return 0;
>  }

There is nothing in this patch that would prevent this code to live in a
place where other drivers could reuse this. (But attention, there are
dragons: Thierry already replied on my topic that his view is different
in this aspect compared to other maintainers though. His POV is that as
long as there is only a single driver known that has a problem this
should be handled in driver specific code.)

Best regards
Uwe

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

Powered by blists - more mailing lists