[RFC PATCH v4 0/2] pwm: imx: Configure output to GPIO in disabled state

[RFC PATCH v4 0/2] pwm: imx: Configure output to GPIO in disabled state

[Vokáč Michal]

This is an attempt to deal with i.MX SoC PWM HW limitation.
When a pad is configured as a PWM output the output level is always 0V
if the PWM block is disabled. This can cause problems when inverted PWM
signal is needed to drive the connected circuit. With inverted output
duty cycle = 0% corresponds to high output level and duty cycle = 100%
corresponds to low output level. This means that whenever the PWM block
is disabled the connected circuit is fed with 100% duty cycle.

This binding is totally optional and current users are not affected.
The idea is to use two new pinctrl states, "pwm" and "gpio", instead of
the "default" pinctrl state. The gpio state is selected  when PWM is
disabled and the pwm pinctrl state is selected when PWM is enabled. In the
gpio state level on the output is controlled by the internal pull-up.

Michal Vokáč (2):
  dt-bindings: pwm: imx: Allow switching PWM output between PWM and GPIO
  pwm: imx: Configure output to GPIO in disabled state

 Documentation/devicetree/bindings/pwm/imx-pwm.txt | 49 +++++++++++++++
 drivers/pwm/pwm-imx.c                             | 77 +++++++++++++++++++++++
 2 files changed, 126 insertions(+)

-- 
2.1.4

[RFC PATCH v4 1/2] dt-bindings: pwm: imx: Allow switching PWM output between PWM and GPIO

[Vokáč Michal]

Output of the PWM block on i.MX SoCs is always low when the block is
disabled. This can cause issues when inverted PWM polarity is needed.
With inverted polarity a duty cycle = 0% corresponds to high level on
the output. Now, when PWM is disabled its output instantly goes low
which corresponds to duty cycle = 100%.

To get a truly inverted PWM output two pinctrl states of the PWM pin
can be used. Configure the pin to GPIO function when PWM is disabled
and switch back to PWM function whenever non-zero duty cycle is needed.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes in v4:
 - Add R-by from Rob.

Changes in v3:
 - Slightly different description of the pinctrl and pwm-gpio.

Changes in v2:
 - Do not use the "default" pinctrl state for GPIO.
 - Use two new "pwm" and "gpio" pinctrl states.
 - Add a new pwm-gpios signal.

 Documentation/devicetree/bindings/pwm/imx-pwm.txt | 49 +++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
index c61bdf8..2a555b6 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
@@ -14,6 +14,16 @@ See the clock consumer binding,
 	Documentation/devicetree/bindings/clock/clock-bindings.txt
 - interrupts: The interrupt for the pwm controller
 
+Optional properties:
+- pinctrl: For i.MX27 and newer SoCs. Use "pwm" and "gpio" specific pinctrls
+  instead of the "default" to configure the PWM pin to GPIO and PWM function.
+  It allows control over the pin output level when the PWM block is disabled.
+  This is useful if you use the PWM for single purpose and you need inverted
+  polarity of the PWM signal. See "Inverted PWM output" section bellow.
+- pwm-gpios: Specify the GPIO pin that will act as the PWM output. This should
+  be the same pin as is used for normal PWM output. See "Inverted PWM output"
+  section bellow.
+
 Example:
 
 pwm1: pwm@53fb4000 {
@@ -25,3 +35,42 @@ pwm1: pwm@53fb4000 {
 	clock-names = "ipg", "per";
 	interrupts = <61>;
 };
+
+Inverted PWM output
+-------------------
+
+The i.MX SoC has such limitation that whenever a pin is configured as a PWM
+output, the output level on the pin is always low when the PWM block is
+disabled. The low output level is actively driven by the output stage of the
+PWM block and it does not matter what polarity a PWM client (e.g. backlight)
+requested.
+
+To gain control of the output level in PWM disabled state two pinctrl states
+can be used. A "pwm" state and a "gpio" state. In the pwm state the pin is
+configured as a normal PWM output. In the gpio state the pin is configured as
+a GPIO input. In the gpio state the output level is controlled by the pull-up
+setting. This setup assures that the PWM output is at the required level that
+corresponds to duty cycle = 0 when PWM is disabled.
+
+Example:
+
+&pwm1 {
+	pinctrl-names = "pwm", "gpio";
+	pinctrl-0 = <&pinctrl_backlight_pwm>;
+	pinctrl-1 = <&pinctrl_backlight_gpio>;
+	pwm-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>
+}
+
+pinctrl_backlight_gpio: pwm1grp-gpio {
+	fsl,pins = <
+		/* GPIO with 100kOhm pull-up */
+		MX6QDL_PAD_GPIO_9__GPIO1_IO09	0xb000
+	>;
+};
+
+pinctrl_backlight_pwm: pwm1grp-pwm {
+	fsl,pins = <
+		/* PWM output */
+		MX6QDL_PAD_GPIO_9__PWM1_OUT	0x8
+	>;
+};
-- 
2.1.4

[RFC PATCH v4 2/2] pwm: imx: Configure output to GPIO in disabled state

[Vokáč Michal]

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 "pwm" and a "gpio" pinctrl states. The pwm pinctrl
state is selected when PWM is enabled and the gpio pinctrl state is
selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
configured as input and the internal pull-up resistor is used to pull the
output level high.

If all the pinctrl states and the pwm-gpios GPIO are not correctly
specified in DT the PWM work as usual.

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.

Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:

+--------------+------------+---------------+----------- +-------------+
| After reset  | Bootloader |   PWM probe   |     PWM    |     PWM     |
| 100k pull-up |            |               | enable 30% |   disable   |
+--------------+------------+---------------+------------+-------------+
| pinctrl      |    none    |    default    |   default  |   default   |
| out H   __________________                   __   __                 |
| out L                     \_________________/  \_/  \_/\____________ |
|                                           ^    ^    ^                |
+--------------+------------+---------------+------------+-------------+
| pinctrl      |    none    |      gpio     |     pwm    |     gpio    |
| out H   __________________________________   __   __   _____________ |
| out L                                     \_/  \_/  \_/              |
|                                           ^    ^    ^                |
+----------------------------------------------------------------------+

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes in v4:
 - Get the pinctrl first, if OK only then get the GPIO. (Uwe)
 - Use the non-optional variant of devm_gpiod_get().
 - Align function arguments to the opening parentheses. (Uwe)

Changes in v3:
 - Commit message update.
 - Minor fix in code comment (Uwe)
 - Align function arguments to the opening parentheses. (Uwe)
 - Do not test devm_pinctrl_get for NULL. (Thierry)
 - Convert all messages to dev_dbg() (Thierry)
 - Do not actively drive the pin in gpio state. Configure it as input
   and rely solely on the internal pull-up. (Thierry)

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 | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 1d5242c..b8782fe 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -12,10 +12,12 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/io.h>
 #include <linux/pwm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 
 /* i.MX1 and i.MX21 share the same PWM function block: */
 
@@ -52,10 +54,52 @@ 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 (IS_ERR(imx_chip->pinctrl)) {
+		dev_dbg(&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");
+
+	if (IS_ERR(imx_chip->pinctrl_pins_pwm) ||
+	    IS_ERR(imx_chip->pinctrl_pins_gpio)) {
+		dev_dbg(&pdev->dev, "pinctrl information incomplete\n");
+		goto out;
+	}
+
+	imx_chip->pwm_gpiod = devm_gpiod_get(&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)) {
+		dev_dbg(&pdev->dev, "GPIO information incomplete\n");
+		goto out;
+	}
+
+	return 0;
+
+out:
+	devm_pinctrl_put(imx_chip->pinctrl);
+	imx_chip->pinctrl = NULL;
+
+	return 0;
+}
+
 static int imx_pwm_config_v1(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
@@ -216,7 +260,25 @@ 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);
+
+		/*
+		 * 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. Let's use
+		 * pinctrl to switch the output pin to GPIO functon and keep
+		 * the output at the same level as for duty-cycle = 0.
+		 */
+		if (imx->pinctrl)
+			pinctrl_select_state(imx->pinctrl,
+					     imx->pinctrl_pins_gpio);
+
 		writel(0, imx->mmio_base + MX3_PWMCR);
 
 		clk_disable_unprepare(imx->clk_per);
@@ -263,6 +325,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;
@@ -294,6 +357,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))
@@ -303,6 +370,24 @@ 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 {
+			pinctrl_select_state(imx->pinctrl,
+					     imx->pinctrl_pins_gpio);
+		}
+	}
+
 	platform_set_drvdata(pdev, imx);
 	return 0;
 }
-- 
2.1.4

Re: [RFC PATCH v4 2/2] pwm: imx: Configure output to GPIO in disabled state

[Uwe Kleine-König]

Hello,

On Wed, Dec 12, 2018 at 12:04:51PM +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 "pwm" and a "gpio" pinctrl states. The pwm pinctrl
> state is selected when PWM is enabled and the gpio pinctrl state is
> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
> configured as input and the internal pull-up resistor is used to pull the
> output level high.
> 
> If all the pinctrl states and the pwm-gpios GPIO are not correctly
> specified in DT the PWM work as usual.
> 
> 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.
> 
> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
> 
> +--------------+------------+---------------+----------- +-------------+
> | After reset  | Bootloader |   PWM probe   |     PWM    |     PWM     |
> | 100k pull-up |            |               | enable 30% |   disable   |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl      |    none    |    default    |   default  |   default   |
> | out H   __________________                   __   __                 |
> | out L                     \_________________/  \_/  \_/\____________ |
> |                                           ^    ^    ^                |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl      |    none    |      gpio     |     pwm    |     gpio    |
> | out H   __________________________________   __   __   _____________ |
> | out L                                     \_/  \_/  \_/              |
> |                                           ^    ^    ^                |
> +----------------------------------------------------------------------+

Just for the record: My last concern against this patch set (that I sent
for v3) and v4 of the series criss-crossed. So the problem with the
peaks that could happen is still unaddressed.

Best regards
Uwe

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

Re: [RFC PATCH v4 2/2] pwm: imx: Configure output to GPIO in disabled state

[Michal Vokáč]

On 13.12.2018 09:56, Uwe Kleine-König wrote:
> On Wed, Dec 12, 2018 at 12:04:51PM +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 "pwm" and a "gpio" pinctrl states. The pwm pinctrl
>> state is selected when PWM is enabled and the gpio pinctrl state is
>> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
>> configured as input and the internal pull-up resistor is used to pull the
>> output level high.
>>
>> If all the pinctrl states and the pwm-gpios GPIO are not correctly
>> specified in DT the PWM work as usual.
>>
>> 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.
>>
>> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
>>
>> +--------------+------------+---------------+----------- +-------------+
>> | After reset  | Bootloader |   PWM probe   |     PWM    |     PWM     |
>> | 100k pull-up |            |               | enable 30% |   disable   |
>> +--------------+------------+---------------+------------+-------------+
>> | pinctrl      |    none    |    default    |   default  |   default   |
>> | out H   __________________                   __   __                 |
>> | out L                     \_________________/  \_/  \_/\____________ |
>> |                                           ^    ^    ^                |
>> +--------------+------------+---------------+------------+-------------+
>> | pinctrl      |    none    |      gpio     |     pwm    |     gpio    |
>> | out H   __________________________________   __   __   _____________ |
>> | out L                                     \_/  \_/  \_/              |
>> |                                           ^    ^    ^                |
>> +----------------------------------------------------------------------+
> 
> Just for the record: My last concern against this patch set (that I sent
> for v3) and v4 of the series criss-crossed. So the problem with the
> peaks that could happen is still unaddressed.

Hi Uwe et al.

Sorry for the huge delay. I hope we will be able to refresh our
memories and continue on the discussion. I will react to your
comments in the appropriate v3 thread. And sorry for this v4.
I was too fast on the trigger back then..

Michal