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

Message ID 1539163920-9442-3-git-send-email-michal.vokac@ysoft.com
State New
Headers show
Series
  • pwm: imx: Configure output to GPIO in disabled state
Related show

Commit Message

Vokáč Michal Oct. 10, 2018, 9:33 a.m.
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@ysoft.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(+)

Comments

Uwe Kleine-König Oct. 12, 2018, 8:57 a.m. | #1
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@ysoft.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
Vokáč Michal Oct. 12, 2018, 3:04 p.m. | #2
On 12.10.2018 10:57, Uwe Kleine-König wrote:
> 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.

Thank you for taking time to comment on this one as well Uwe.

>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.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.

That is totally new information for me. I've never seen any usage of
"init" pinctrl state before. The total number of users is 6 (rockchip)
so that explains a lot.

I think that it would not work though. The basic idea behind my
solution is that the pinctrl driver does not know what is the correct
pin configuration at probe whereas the pwm-imx driver does.

With the "init" and "default" states the pinctrl driver will always
select the "default" if it find the pins in "init" state.

	https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/core.c#L1469

But we want the pins to stay in the "init" GPIO state unless a client
enables the 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.

OK, thank you for valuable info.
So it seems like the I2C folks are in troubles as well:

	https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L996

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

Sorry, I can't answer that now. I need to look deeper into that.

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

True. It is a copy-paste and I could not really come up with something
more sensible for our case. I am happy to change that.
  
>> +		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/

Ack.
  
>> +		 * 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?

To me, it does more than just setting duty-cycle = 0. I think that the
user expect to find the PWM chip in disabled state after this. So that
when you read the HW registers you see the chip is disabled.

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

Obviously not. It is working (not only) for me like that a long time.
But I agree, a comment in gpiolib I found recently is clear about that
you must call that function beforehand.

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

This could be the first time that we want to disable the PWM as you
might start with PWM enabled from probe. And hence the GPIO is still
configured as input.

>> +			pinctrl_select_state(imx->pinctrl,
>> +					imx->pinctrl_pins_gpio);
> 
> Usually align function arguments to the opening (.

Oops, I will fix those. There is more than this one..
  
>> +		}
>> +
>>   		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.)

When I tripped over that issue I also thought it is i.MX specific.
It never came to my mind that something like that should be done in
higher layer.

But I have to admit that my current understanding of the overall
architecture is at such level that I will just do it how the maintainer
wants me to do it. Given that the solution improves the situation and
solves my original problem. And the pinctrl solution was already
suggested by Fabio Estevam a year ago [1] so I went that way.

[1] https://patchwork.ozlabs.org/patch/839834/#1819865

Michal
Thierry Reding Oct. 12, 2018, 3:54 p.m. | #3
On Fri, Oct 12, 2018 at 03:04:48PM +0000, Vokáč Michal wrote:
> On 12.10.2018 10:57, Uwe Kleine-König wrote:
> > On Wed, Oct 10, 2018 at 09:33:26AM +0000, Vokáč Michal wrote:
[...]
> >> +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.
> 
> OK, thank you for valuable info.
> So it seems like the I2C folks are in troubles as well:
> 
> 	https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L996

There's nothing inherently wrong with doing it like the above, just
maybe slightly unusual. PTR_ERR() is really just casting from a pointer
to an integer, so if the pointer happens to contain the value
-EPROBE_DEFER, then the above will be true. If it contains a valid
pointer, the above will be false, so it does exactly what you want.

Perhaps a more idiomatic way to write this would be:

	if (IS_ERR(imx_chip->pwm_gpiod)) {
		if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER)
			return -EPROBE_DEFER;
	}

But that's not much clearer than what you have, so feel free to keep it
that way.

Thierry
Thierry Reding Oct. 12, 2018, 4 p.m. | #4
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@ysoft.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
Uwe Kleine-König Oct. 12, 2018, 4:08 p.m. | #5
Hello,

On Fri, Oct 12, 2018 at 03:04:48PM +0000, Vokáč Michal wrote:
> On 12.10.2018 10:57, Uwe Kleine-König wrote:
> > 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.
> 
> Thank you for taking time to comment on this one as well Uwe.
> 
> >> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.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.
> 
> That is totally new information for me. I've never seen any usage of
> "init" pinctrl state before. The total number of users is 6 (rockchip)
> so that explains a lot.
> 
> I think that it would not work though. The basic idea behind my
> solution is that the pinctrl driver does not know what is the correct
> pin configuration at probe whereas the pwm-imx driver does.
> 
> With the "init" and "default" states the pinctrl driver will always
> select the "default" if it find the pins in "init" state.
> 
> 	https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/core.c#L1469
> 
> But we want the pins to stay in the "init" GPIO state unless a client
> enables the PWM.

Note I don't know this very well either. What should work then is to
have "init" as GPIO and "active" as PWM. Well, though this breaks if you
intend to probe a running pwm without stopping it.
 
> >>   };
> >>   
> >> +
> >>   #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.
> 
> OK, thank you for valuable info.
> So it seems like the I2C folks are in troubles as well:
> 
> 	https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L996

correct. I cannot find this documented though.

> >> +		 * 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?
> 
> To me, it does more than just setting duty-cycle = 0. I think that the
> user expect to find the PWM chip in disabled state after this. So that
> when you read the HW registers you see the chip is disabled.

The PWM API user doesn't (have to) care how the lowlevel driver
implements the .disable() callback.

I personally would prefer to keep the driver simple (without pinctrl and
gpio stuff which also simplifies matters for dts authors) and spend that
little energy keeping the HW on.
 
> > 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.
> 
> This could be the first time that we want to disable the PWM as you
> might start with PWM enabled from probe. And hence the GPIO is still
> configured as input.

I fail to follow your case. Do you mean the pwm is already running when
the driver probes? Then it is save to set the gpio to output as this has
no effect as long as the pin is muxed in its pwm function.

> >> +			pinctrl_select_state(imx->pinctrl,
> >> +					imx->pinctrl_pins_gpio);
> > 
> > Usually align function arguments to the opening (.
> 
> Oops, I will fix those. There is more than this one..
>   
> >> +		}
> >> +
> >>   		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.)
> 
> When I tripped over that issue I also thought it is i.MX specific.
> It never came to my mind that something like that should be done in
> higher layer.
> 
> But I have to admit that my current understanding of the overall
> architecture is at such level that I will just do it how the maintainer
> wants me to do it. Given that the solution improves the situation and
> solves my original problem. And the pinctrl solution was already
> suggested by Fabio Estevam a year ago [1] so I went that way.
> 
> [1] https://patchwork.ozlabs.org/patch/839834/#1819865

In message 15 both Lothar and Fabio agree that they would prefer a
solution without messing with pin configs. But also note that they
discussed about suspend, too, not only pwm_disable().

Best regards
Uwe
Uwe Kleine-König Oct. 14, 2018, 8:24 p.m. | #6
Hello,

On Fri, Oct 12, 2018 at 06:08:54PM +0200, Uwe Kleine-König wrote:
> > >> +	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.
> > 
> > OK, thank you for valuable info.
> > So it seems like the I2C folks are in troubles as well:
> > 
> > 	https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L996
> 
> correct. I cannot find this documented though.

I found in LDD3[1], chapter 11 on page 295:

	If you need the actual error code, it can be extracted with:

		long PTR_ERR(const void *ptr);

	You should use PTR_ERR only on a value for which IS_ERR returns a true
	value; any other value is a valid pointer.

That is probably where I have my claim from. There is no further
explanation though, so I'll post a patch adding a comment to the
definition of PTR_ERR to find out if there is a relevant reason.

Best regards
Uwe

[1] https://lwn.net/Kernel/LDD3/
Thierry Reding Oct. 15, 2018, 8:45 a.m. | #7
On Sun, Oct 14, 2018 at 10:24:57PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Oct 12, 2018 at 06:08:54PM +0200, Uwe Kleine-König wrote:
> > > >> +	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.
> > > 
> > > OK, thank you for valuable info.
> > > So it seems like the I2C folks are in troubles as well:
> > > 
> > > 	https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L996
> > 
> > correct. I cannot find this documented though.
> 
> I found in LDD3[1], chapter 11 on page 295:
> 
> 	If you need the actual error code, it can be extracted with:
> 
> 		long PTR_ERR(const void *ptr);
> 
> 	You should use PTR_ERR only on a value for which IS_ERR returns a true
> 	value; any other value is a valid pointer.
> 
> That is probably where I have my claim from. There is no further
> explanation though, so I'll post a patch adding a comment to the
> definition of PTR_ERR to find out if there is a relevant reason.

Michal's code above does an implicit IS_ERR() by comparing to an actual
error code. It's certainly true that PTR_ERR() on any pointer and then
using that value can be risky because it may not actually be an error.
So if you go and unconditionally print that error code even if it isn't
an error but a valid pointer, you've leaked a kernel address.

However, it's perfectly safe to use PTR_ERR(ptr) == -EPROBE_DEFER (or
for any other error code for that matter).

Thierry

Patch

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)) {
+		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) {
+		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");
+		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
+		 * pinctrl to switch the output pin to GPIO functon and keep
+		 * the output at the same level as for duty-cycle = 0.
+		 *
+		 * 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);
+			pinctrl_select_state(imx->pinctrl,
+					imx->pinctrl_pins_gpio);
+		}
+
 		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;
 }