[PATCH v3 0/6] Support PWM polarity control

[PATCH v3 0/6] Support PWM polarity control

[Bhuvanchandra DV]

Changes since v3:

- Set pwm->args.polarity, before using pwm_set_polarity() function as suggested
  by Lukasz.
- Squash Lukasz patch( https://lkml.org/lkml/2016/10/6/32 )

Tested on Toradex Colibri iMX6S module.

Changes since v2:

- Picked the stalled patchset[1] from Lothar Wassmann which adds the basic
  support for polarity control on imx-pwm driver and adds backward compatibility
  support for devices which does not have polarity control feature.

Changes since Lothars v6:

- Squash Lukasz patch[2].

[1] http://thread.gmane.org/gmane.linux.pwm/1621
[2] https://www.spinics.net/lists/arm-kernel/msg530818.html

Bhuvanchandra DV (3):
  arm: dts: imx7: Update #pwm-cells for PWM polarity control
  arm: dts: imx7-colibri: Use pwm polarity control
  arm: dts: imx7-colibri: Use enable-gpios for BL_ON

Lothar Wassmann (3):
  pwm: print error messages with pr_err() instead of pr_debug()
  pwm: core: make the PWM_POLARITY flag in DTB optional
  pwm: imx: support output polarity inversion

 Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
 arch/arm/boot/dts/imx7-colibri.dtsi               | 12 +++++-
 arch/arm/boot/dts/imx7s.dtsi                      |  8 ++--
 drivers/pwm/core.c                                | 26 ++++++------
 drivers/pwm/pwm-imx.c                             | 51 +++++++++++++++++++++--
 5 files changed, 79 insertions(+), 24 deletions(-)

-- 
2.10.0

[PATCH v3 1/6] pwm: print error messages with pr_err() instead of pr_debug()

[Bhuvanchandra DV]

From: Lothar Wassmann <LW@KARO-electronics.de>

Make the messages that are printed in case of fatal errors actually
visible to the user without having to recompile the driver with
debugging enabled.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
---
 drivers/pwm/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 0dbd29e..195373e 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -661,13 +661,13 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 	err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", index,
 					 &args);
 	if (err) {
-		pr_debug("%s(): can't parse \"pwms\" property\n", __func__);
+		pr_err("%s(): can't parse \"pwms\" property\n", __func__);
 		return ERR_PTR(err);
 	}
 
 	pc = of_node_to_pwmchip(args.np);
 	if (IS_ERR(pc)) {
-		pr_debug("%s(): PWM chip not found\n", __func__);
+		pr_err("%s(): PWM chip not found\n", __func__);
 		pwm = ERR_CAST(pc);
 		goto put;
 	}
-- 
2.10.0

[PATCH v3 2/6] pwm: core: make the PWM_POLARITY flag in DTB optional

[Bhuvanchandra DV]

From: Lothar Wassmann <LW@KARO-electronics.de>

Change the pwm chip driver registration, so that a chip driver that
supports polarity inversion can still be used with DTBs that don't
provide the 'PWM_POLARITY' flag.

This is done to provide polarity inversion support for the pwm-imx
driver without having to modify all existing DTS files.

Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de>
Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Suggested-by: Sascha Hauer <s.hauer at pengutronix.de>
---
 drivers/pwm/core.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 195373e..85cdda6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -137,9 +137,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
+	/* check, whether the driver supports a third cell for flags */
 	if (pc->of_pwm_n_cells < 3)
 		return ERR_PTR(-EINVAL);
 
+	/* flags in the third cell are optional */
+	if (args->args_count < 2)
+		return ERR_PTR(-EINVAL);
+
 	if (args->args[0] >= pc->npwm)
 		return ERR_PTR(-EINVAL);
 
@@ -148,11 +153,10 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 		return pwm;
 
 	pwm->args.period = args->args[1];
+	pwm->args.polarity = PWM_POLARITY_NORMAL;
 
-	if (args->args[2] & PWM_POLARITY_INVERTED)
+	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
-	else
-		pwm->args.polarity = PWM_POLARITY_NORMAL;
 
 	return pwm;
 }
@@ -163,9 +167,14 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
+	/* sanity check driver support */
 	if (pc->of_pwm_n_cells < 2)
 		return ERR_PTR(-EINVAL);
 
+	/* all cells are required */
+	if (args->args_count != pc->of_pwm_n_cells)
+		return ERR_PTR(-EINVAL);
+
 	if (args->args[0] >= pc->npwm)
 		return ERR_PTR(-EINVAL);
 
@@ -672,13 +681,6 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 		goto put;
 	}
 
-	if (args.args_count != pc->of_pwm_n_cells) {
-		pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
-			 args.np->full_name);
-		pwm = ERR_PTR(-EINVAL);
-		goto put;
-	}
-
 	pwm = pc->of_xlate(pc, &args);
 	if (IS_ERR(pwm))
 		goto put;
-- 
2.10.0

[PATCH v3 3/6] pwm: imx: support output polarity inversion

[Bhuvanchandra DV]

From: Lothar Wassmann <LW@KARO-electronics.de>

The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output
polarity. This patch adds support to utilize this feature where available.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
 drivers/pwm/pwm-imx.c                             | 51 +++++++++++++++++++++--
 2 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
index e00c2e9..c61bdf8 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
@@ -6,8 +6,8 @@ Required properties:
   - "fsl,imx1-pwm" for PWM compatible with the one integrated on i.MX1
   - "fsl,imx27-pwm" for PWM compatible with the one integrated on i.MX27
 - reg: physical base address and length of the controller's registers
-- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
-  the cells format.
+- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See pwm.txt
+  in this directory for a description of the cells format.
 - clocks : Clock specifiers for both ipg and per clocks.
 - clock-names : Clock names should include both "ipg" and "per"
 See the clock consumer binding,
@@ -17,7 +17,7 @@ See the clock consumer binding,
 Example:
 
 pwm1: pwm@53fb4000 {
-	#pwm-cells = <2>;
+	#pwm-cells = <3>;
 	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
 	reg = <0x53fb4000 0x4000>;
 	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5..c37d223 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -38,6 +38,7 @@
 #define MX3_PWMCR_DOZEEN		(1 << 24)
 #define MX3_PWMCR_WAITEN		(1 << 23)
 #define MX3_PWMCR_DBGEN			(1 << 22)
+#define MX3_PWMCR_POUTC			(1 << 18)
 #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
 #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
 #define MX3_PWMCR_SWR			(1 << 3)
@@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	if (enable)
 		cr |= MX3_PWMCR_EN;
 
+	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
+		cr |= MX3_PWMCR_POUTC;
+
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
 	return 0;
@@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	clk_disable_unprepare(imx->clk_per);
 }
 
-static struct pwm_ops imx_pwm_ops = {
+static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				enum pwm_polarity polarity)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	u32 val;
+
+	if (polarity == pwm->args.polarity)
+		return 0;
+
+	val = readl(imx->mmio_base + MX3_PWMCR);
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		val |= MX3_PWMCR_POUTC;
+	else
+		val &= ~MX3_PWMCR_POUTC;
+
+	writel(val, imx->mmio_base + MX3_PWMCR);
+
+	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
+		polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
+
+	return 0;
+}
+
+static struct pwm_ops imx_pwm_ops_v1 = {
 	.enable = imx_pwm_enable,
 	.disable = imx_pwm_disable,
 	.config = imx_pwm_config,
 	.owner = THIS_MODULE,
 };
 
+static struct pwm_ops imx_pwm_ops_v2 = {
+	.enable = imx_pwm_enable,
+	.disable = imx_pwm_disable,
+	.set_polarity = imx_pwm_set_polarity,
+	.config = imx_pwm_config,
+	.owner = THIS_MODULE,
+};
+
 struct imx_pwm_data {
 	int (*config)(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns);
 	void (*set_enable)(struct pwm_chip *chip, bool enable);
+	struct pwm_ops *pwm_ops;
 };
 
 static struct imx_pwm_data imx_pwm_data_v1 = {
 	.config = imx_pwm_config_v1,
 	.set_enable = imx_pwm_set_enable_v1,
+	.pwm_ops = &imx_pwm_ops_v1,
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
 	.config = imx_pwm_config_v2,
 	.set_enable = imx_pwm_set_enable_v2,
+	.pwm_ops = &imx_pwm_ops_v2,
 };
 
 static const struct of_device_id imx_pwm_dt_ids[] = {
@@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	if (!of_id)
 		return -ENODEV;
 
+	data = of_id->data;
+
 	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
 	if (imx == NULL)
 		return -ENOMEM;
@@ -300,18 +341,22 @@ static int imx_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(imx->clk_ipg);
 	}
 
-	imx->chip.ops = &imx_pwm_ops;
+	imx->chip.ops = data->pwm_ops;
 	imx->chip.dev = &pdev->dev;
 	imx->chip.base = -1;
 	imx->chip.npwm = 1;
 	imx->chip.can_sleep = true;
+	if (data->pwm_ops->set_polarity) {
+		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
+		imx->chip.of_xlate = of_pwm_xlate_with_flags;
+		imx->chip.of_pwm_n_cells = 3;
+	}
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
 	if (IS_ERR(imx->mmio_base))
 		return PTR_ERR(imx->mmio_base);
 
-	data = of_id->data;
 	imx->config = data->config;
 	imx->set_enable = data->set_enable;
 
-- 
2.10.0

[PATCH v3 4/6] arm: dts: imx7: Update #pwm-cells for PWM polarity control

[Bhuvanchandra DV]

Update #pwm-cells to 3 in order to support PWM signal polarity control.

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 arch/arm/boot/dts/imx7s.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 0d7d5ac..8d1d471 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -601,7 +601,7 @@
 				clocks = <&clks IMX7D_PWM1_ROOT_CLK>,
 					 <&clks IMX7D_PWM1_ROOT_CLK>;
 				clock-names = "ipg", "per";
-				#pwm-cells = <2>;
+				#pwm-cells = <3>;
 				status = "disabled";
 			};
 
@@ -612,7 +612,7 @@
 				clocks = <&clks IMX7D_PWM2_ROOT_CLK>,
 					 <&clks IMX7D_PWM2_ROOT_CLK>;
 				clock-names = "ipg", "per";
-				#pwm-cells = <2>;
+				#pwm-cells = <3>;
 				status = "disabled";
 			};
 
@@ -623,7 +623,7 @@
 				clocks = <&clks IMX7D_PWM3_ROOT_CLK>,
 					 <&clks IMX7D_PWM3_ROOT_CLK>;
 				clock-names = "ipg", "per";
-				#pwm-cells = <2>;
+				#pwm-cells = <3>;
 				status = "disabled";
 			};
 
@@ -634,7 +634,7 @@
 				clocks = <&clks IMX7D_PWM4_ROOT_CLK>,
 					 <&clks IMX7D_PWM4_ROOT_CLK>;
 				clock-names = "ipg", "per";
-				#pwm-cells = <2>;
+				#pwm-cells = <3>;
 				status = "disabled";
 			};
 
-- 
2.10.0

[PATCH v3 5/6] arm: dts: imx7-colibri: Use pwm polarity control

[Bhuvanchandra DV]

Configure PWM polarity control.

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
---
 arch/arm/boot/dts/imx7-colibri.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi b/arch/arm/boot/dts/imx7-colibri.dtsi
index a9cc657..2af5e3e 100644
--- a/arch/arm/boot/dts/imx7-colibri.dtsi
+++ b/arch/arm/boot/dts/imx7-colibri.dtsi
@@ -43,7 +43,7 @@
 / {
 	bl: backlight {
 		compatible = "pwm-backlight";
-		pwms = <&pwm1 0 5000000>;
+		pwms = <&pwm1 0 5000000 0>;
 	};
 
 	reg_module_3v3: regulator-module-3v3 {
-- 
2.10.0

[PATCH v3 6/6] arm: dts: imx7-colibri: Use enable-gpios for BL_ON

[Bhuvanchandra DV]

Use pwm-backlight driver 'enable-gpios' property for backlight on/off control.

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
---
 arch/arm/boot/dts/imx7-colibri.dtsi | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi b/arch/arm/boot/dts/imx7-colibri.dtsi
index 2af5e3e..ce5edb5 100644
--- a/arch/arm/boot/dts/imx7-colibri.dtsi
+++ b/arch/arm/boot/dts/imx7-colibri.dtsi
@@ -43,7 +43,10 @@
 / {
 	bl: backlight {
 		compatible = "pwm-backlight";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_gpio_bl_on>;
 		pwms = <&pwm1 0 5000000 0>;
+		enable-gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
 	};
 
 	reg_module_3v3: regulator-module-3v3 {
@@ -356,7 +359,6 @@
 		fsl,pins = <
 			MX7D_PAD_ECSPI2_SS0__GPIO4_IO23		0x14 /* SODIMM 65 */
 			MX7D_PAD_SD1_CD_B__GPIO5_IO0		0x14 /* SODIMM 69 */
-			MX7D_PAD_SD1_WP__GPIO5_IO1		0x14 /* SODIMM 71 */
 			MX7D_PAD_I2C4_SDA__GPIO4_IO15		0x14 /* SODIMM 75 */
 			MX7D_PAD_ECSPI1_MISO__GPIO4_IO18	0x14 /* SODIMM 79 */
 			MX7D_PAD_I2C3_SCL__GPIO4_IO12		0x14 /* SODIMM 81 */
@@ -388,6 +390,12 @@
 		>;
 	};
 
+		pinctrl_gpio_bl_on: gpio-bl-on {
+			fsl,pins = <
+				MX7D_PAD_SD1_WP__GPIO5_IO1		0x14
+			>;
+		};
+
 	pinctrl_i2c1_int: i2c1-int-grp { /* PMIC / TOUCH */
 		fsl,pins = <
 			MX7D_PAD_GPIO1_IO13__GPIO1_IO13	0x79
-- 
2.10.0

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 5813 bytes --]

Hi Bhuvanchandra,

> From: Lothar Wassmann <LW@KARO-electronics.de>
> 
> The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable
> output polarity. This patch adds support to utilize this feature
> where available.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

I've tested those patches on my iMX6q board on top of v4.7 linux kernel.

Tested-by: Lukasz Majewski <l.majewski@majess.pl>

Best regards,
Łukasz Majewski

> ---
>  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
>  drivers/pwm/pwm-imx.c                             | 51
> +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> e00c2e9..c61bdf8 100644 ---
> a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8 +6,8 @@
> Required properties:
>    - "fsl,imx1-pwm" for PWM compatible with the one integrated on
> i.MX1
>    - "fsl,imx27-pwm" for PWM compatible with the one integrated on
> i.MX27
>  - reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2. See pwm.txt in this directory for a
> description of
> -  the cells format.
> +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> pwm.txt
> +  in this directory for a description of the cells format.
>  - clocks : Clock specifiers for both ipg and per clocks.
>  - clock-names : Clock names should include both "ipg" and "per"
>  See the clock consumer binding,
> @@ -17,7 +17,7 @@ See the clock consumer binding,
>  Example:
>  
>  pwm1: pwm@53fb4000 {
> -	#pwm-cells = <2>;
> +	#pwm-cells = <3>;
>  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
>  	reg = <0x53fb4000 0x4000>;
>  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..c37d223 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -38,6 +38,7 @@
>  #define MX3_PWMCR_DOZEEN		(1 << 24)
>  #define MX3_PWMCR_WAITEN		(1 << 23)
>  #define MX3_PWMCR_DBGEN			(1 << 22)
> +#define MX3_PWMCR_POUTC			(1 << 18)
>  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
>  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
>  #define MX3_PWMCR_SWR			(1 << 3)
> @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
> *chip, if (enable)
>  		cr |= MX3_PWMCR_EN;
>  
> +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> +		cr |= MX3_PWMCR_POUTC;
> +
>  	writel(cr, imx->mmio_base + MX3_PWMCR);
>  
>  	return 0;
> @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> *chip, struct pwm_device *pwm) clk_disable_unprepare(imx->clk_per);
>  }
>  
> -static struct pwm_ops imx_pwm_ops = {
> +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> pwm_device *pwm,
> +				enum pwm_polarity polarity)
> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	u32 val;
> +
> +	if (polarity == pwm->args.polarity)
> +		return 0;
> +
> +	val = readl(imx->mmio_base + MX3_PWMCR);
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		val |= MX3_PWMCR_POUTC;
> +	else
> +		val &= ~MX3_PWMCR_POUTC;
> +
> +	writel(val, imx->mmio_base + MX3_PWMCR);
> +
> +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> +		polarity == PWM_POLARITY_INVERSED ? "inverted" :
> "normal"); +
> +	return 0;
> +}
> +
> +static struct pwm_ops imx_pwm_ops_v1 = {
>  	.enable = imx_pwm_enable,
>  	.disable = imx_pwm_disable,
>  	.config = imx_pwm_config,
>  	.owner = THIS_MODULE,
>  };
>  
> +static struct pwm_ops imx_pwm_ops_v2 = {
> +	.enable = imx_pwm_enable,
> +	.disable = imx_pwm_disable,
> +	.set_polarity = imx_pwm_set_polarity,
> +	.config = imx_pwm_config,
> +	.owner = THIS_MODULE,
> +};
> +
>  struct imx_pwm_data {
>  	int (*config)(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns);
>  	void (*set_enable)(struct pwm_chip *chip, bool enable);
> +	struct pwm_ops *pwm_ops;
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v1 = {
>  	.config = imx_pwm_config_v1,
>  	.set_enable = imx_pwm_set_enable_v1,
> +	.pwm_ops = &imx_pwm_ops_v1,
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v2 = {
>  	.config = imx_pwm_config_v2,
>  	.set_enable = imx_pwm_set_enable_v2,
> +	.pwm_ops = &imx_pwm_ops_v2,
>  };
>  
>  static const struct of_device_id imx_pwm_dt_ids[] = {
> @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device
> *pdev) if (!of_id)
>  		return -ENODEV;
>  
> +	data = of_id->data;
> +
>  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
>  	if (imx == NULL)
>  		return -ENOMEM;
> @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct platform_device
> *pdev) return PTR_ERR(imx->clk_ipg);
>  	}
>  
> -	imx->chip.ops = &imx_pwm_ops;
> +	imx->chip.ops = data->pwm_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.base = -1;
>  	imx->chip.npwm = 1;
>  	imx->chip.can_sleep = true;
> +	if (data->pwm_ops->set_polarity) {
> +		dev_dbg(&pdev->dev, "PWM supports output
> inversion\n");
> +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
> +		imx->chip.of_pwm_n_cells = 3;
> +	}
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
>  	if (IS_ERR(imx->mmio_base))
>  		return PTR_ERR(imx->mmio_base);
>  
> -	data = of_id->data;
>  	imx->config = data->config;
>  	imx->set_enable = data->set_enable;
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Rob Herring]

On Fri, Oct 07, 2016 at 08:41:26PM +0530, Bhuvanchandra DV wrote:
> From: Lothar Wassmann <LW@KARO-electronics.de>
> 
> The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output
> polarity. This patch adds support to utilize this feature where available.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pwm/pwm-imx.c                             | 51 +++++++++++++++++++++--
>  2 files changed, 51 insertions(+), 6 deletions(-)

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Stefan Agner]

On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> From: Lothar Wassmann <LW@KARO-electronics.de>
> 
> The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output
> polarity. This patch adds support to utilize this feature where available.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
>  drivers/pwm/pwm-imx.c                             | 51 +++++++++++++++++++++--
>  2 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> index e00c2e9..c61bdf8 100644
> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> @@ -6,8 +6,8 @@ Required properties:
>    - "fsl,imx1-pwm" for PWM compatible with the one integrated on i.MX1
>    - "fsl,imx27-pwm" for PWM compatible with the one integrated on i.MX27
>  - reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
> -  the cells format.
> +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See pwm.txt
> +  in this directory for a description of the cells format.
>  - clocks : Clock specifiers for both ipg and per clocks.
>  - clock-names : Clock names should include both "ipg" and "per"
>  See the clock consumer binding,
> @@ -17,7 +17,7 @@ See the clock consumer binding,
>  Example:
>  
>  pwm1: pwm@53fb4000 {
> -	#pwm-cells = <2>;
> +	#pwm-cells = <3>;
>  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
>  	reg = <0x53fb4000 0x4000>;
>  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..c37d223 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -38,6 +38,7 @@
>  #define MX3_PWMCR_DOZEEN		(1 << 24)
>  #define MX3_PWMCR_WAITEN		(1 << 23)
>  #define MX3_PWMCR_DBGEN			(1 << 22)
> +#define MX3_PWMCR_POUTC			(1 << 18)
>  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
>  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
>  #define MX3_PWMCR_SWR			(1 << 3)
> @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  	if (enable)
>  		cr |= MX3_PWMCR_EN;
>  
> +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> +		cr |= MX3_PWMCR_POUTC;
> +

This seems wrong to me, the config callback is meant for period/duty
cycle only. The set_polarity callback should get called in case a
different polarity is requested.


>  	writel(cr, imx->mmio_base + MX3_PWMCR);
>  
>  	return 0;
> @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> *chip, struct pwm_device *pwm)
>  	clk_disable_unprepare(imx->clk_per);
>  }
>  
> -static struct pwm_ops imx_pwm_ops = {
> +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				enum pwm_polarity polarity)
> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	u32 val;
> +
> +	if (polarity == pwm->args.polarity)
> +		return 0;

I don't think that this is right. Today, pwm_apply_args (in
include/linux/pwm.h) copies the polarity from args to state.polarity,
which is then passed as polarity argument to this function. So this will
always return 0 afaict.

I would just drop that.

There is probably one little problem in the current state of affairs: If
the bootloader makes use of a PWM channel with inverted state, then the
kernel would not know about that and currently assume a wrong initial
state... I guess at one point in time we should implement the state
retrieval callback and move to the new atomic PWM API, which would mean
to implement apply callback.

--
Stefan


> +
> +	val = readl(imx->mmio_base + MX3_PWMCR);
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		val |= MX3_PWMCR_POUTC;
> +	else
> +		val &= ~MX3_PWMCR_POUTC;
> +
> +	writel(val, imx->mmio_base + MX3_PWMCR);
> +
> +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> +		polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
> +
> +	return 0;
> +}
> +
> +static struct pwm_ops imx_pwm_ops_v1 = {
>  	.enable = imx_pwm_enable,
>  	.disable = imx_pwm_disable,
>  	.config = imx_pwm_config,
>  	.owner = THIS_MODULE,
>  };
>  
> +static struct pwm_ops imx_pwm_ops_v2 = {
> +	.enable = imx_pwm_enable,
> +	.disable = imx_pwm_disable,
> +	.set_polarity = imx_pwm_set_polarity,
> +	.config = imx_pwm_config,
> +	.owner = THIS_MODULE,
> +};
> +
>  struct imx_pwm_data {
>  	int (*config)(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns);
>  	void (*set_enable)(struct pwm_chip *chip, bool enable);
> +	struct pwm_ops *pwm_ops;
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v1 = {
>  	.config = imx_pwm_config_v1,
>  	.set_enable = imx_pwm_set_enable_v1,
> +	.pwm_ops = &imx_pwm_ops_v1,
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v2 = {
>  	.config = imx_pwm_config_v2,
>  	.set_enable = imx_pwm_set_enable_v2,
> +	.pwm_ops = &imx_pwm_ops_v2,
>  };
>  
>  static const struct of_device_id imx_pwm_dt_ids[] = {
> @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  	if (!of_id)
>  		return -ENODEV;
>  
> +	data = of_id->data;
> +
>  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
>  	if (imx == NULL)
>  		return -ENOMEM;
> @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  		return PTR_ERR(imx->clk_ipg);
>  	}
>  
> -	imx->chip.ops = &imx_pwm_ops;
> +	imx->chip.ops = data->pwm_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.base = -1;
>  	imx->chip.npwm = 1;
>  	imx->chip.can_sleep = true;
> +	if (data->pwm_ops->set_polarity) {
> +		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
> +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
> +		imx->chip.of_pwm_n_cells = 3;
> +	}
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
>  	if (IS_ERR(imx->mmio_base))
>  		return PTR_ERR(imx->mmio_base);
>  
> -	data = of_id->data;
>  	imx->config = data->config;
>  	imx->set_enable = data->set_enable;

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 7622 bytes --]

Hi Stefan,

> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> > From: Lothar Wassmann <LW@KARO-electronics.de>
> > 
> > The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable
> > output polarity. This patch adds support to utilize this feature
> > where available.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> > Acked-by: Shawn Guo <shawn.guo@linaro.org>
> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
> >  drivers/pwm/pwm-imx.c                             | 51
> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> > deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > index e00c2e9..c61bdf8 100644
> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > @@ -6,8 +6,8 @@ Required properties:
> >    - "fsl,imx1-pwm" for PWM compatible with the one integrated on
> > i.MX1
> >    - "fsl,imx27-pwm" for PWM compatible with the one integrated on
> > i.MX27
> >  - reg: physical base address and length of the controller's
> > registers -- #pwm-cells: should be 2. See pwm.txt in this directory
> > for a description of
> > -  the cells format.
> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> > pwm.txt
> > +  in this directory for a description of the cells format.
> >  - clocks : Clock specifiers for both ipg and per clocks.
> >  - clock-names : Clock names should include both "ipg" and "per"
> >  See the clock consumer binding,
> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> >  Example:
> >  
> >  pwm1: pwm@53fb4000 {
> > -	#pwm-cells = <2>;
> > +	#pwm-cells = <3>;
> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> >  	reg = <0x53fb4000 0x4000>;
> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..c37d223 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -38,6 +38,7 @@
> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
> >  #define MX3_PWMCR_WAITEN		(1 << 23)
> >  #define MX3_PWMCR_DBGEN			(1 << 22)
> > +#define MX3_PWMCR_POUTC			(1 << 18)
> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> >  #define MX3_PWMCR_SWR			(1 << 3)
> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, if (enable)
> >  		cr |= MX3_PWMCR_EN;
> >  
> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > +		cr |= MX3_PWMCR_POUTC;
> > +
> 
> This seems wrong to me, the config callback is meant for period/duty
> cycle only.

If it is meant only for that, then the polarity should be removed from
it.

However after very quick testing, at least on my setup, it turns out
that removing this lines causes polarity to _not_ being set (and the
polarity is not inverted).

I will investigate this further on my setup and hopefully sent proper
patch.

> The set_polarity callback should get called in case a
> different polarity is requested.

On my setup the pwm2 is set from DT and pwm_backlight_probe() calls
pwm_apply_args(), so everything should work. However, as I mentioned
above there still is some problem with inversion setting.

> 
> 
> >  	writel(cr, imx->mmio_base + MX3_PWMCR);
> >  
> >  	return 0;
> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> > *chip, struct pwm_device *pwm)
> >  	clk_disable_unprepare(imx->clk_per);
> >  }
> >  
> > -static struct pwm_ops imx_pwm_ops = {
> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> > pwm_device *pwm,
> > +				enum pwm_polarity polarity)
> > +{
> > +	struct imx_chip *imx = to_imx_chip(chip);
> > +	u32 val;
> > +
> > +	if (polarity == pwm->args.polarity)
> > +		return 0;
> 
> I don't think that this is right. Today, pwm_apply_args (in
> include/linux/pwm.h) copies the polarity from args to state.polarity,
> which is then passed as polarity argument to this function. So this
> will always return 0 afaict.

Yes, I've overlooked it (that the state is copied).

It can be dropped.

> 
> I would just drop that.
> 
> There is probably one little problem in the current state of affairs:
> If the bootloader makes use of a PWM channel with inverted state,
> then the kernel would not know about that and currently assume a
> wrong initial state... I guess at one point in time we should
> implement the state retrieval callback and move to the new atomic PWM
> API, which would mean to implement apply callback.

Are there any patches on the horizon?

> 
> --
> Stefan
> 
> 
> > +
> > +	val = readl(imx->mmio_base + MX3_PWMCR);
> > +
> > +	if (polarity == PWM_POLARITY_INVERSED)
> > +		val |= MX3_PWMCR_POUTC;
> > +	else
> > +		val &= ~MX3_PWMCR_POUTC;
> > +
> > +	writel(val, imx->mmio_base + MX3_PWMCR);
> > +
> > +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n",
> > __func__,
> > +		polarity == PWM_POLARITY_INVERSED ? "inverted" :
> > "normal"); +
> > +	return 0;
> > +}
> > +
> > +static struct pwm_ops imx_pwm_ops_v1 = {
> >  	.enable = imx_pwm_enable,
> >  	.disable = imx_pwm_disable,
> >  	.config = imx_pwm_config,
> >  	.owner = THIS_MODULE,
> >  };
> >  
> > +static struct pwm_ops imx_pwm_ops_v2 = {
> > +	.enable = imx_pwm_enable,
> > +	.disable = imx_pwm_disable,
> > +	.set_polarity = imx_pwm_set_polarity,
> > +	.config = imx_pwm_config,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> >  struct imx_pwm_data {
> >  	int (*config)(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int
> > period_ns); void (*set_enable)(struct pwm_chip *chip, bool enable);
> > +	struct pwm_ops *pwm_ops;
> >  };
> >  
> >  static struct imx_pwm_data imx_pwm_data_v1 = {
> >  	.config = imx_pwm_config_v1,
> >  	.set_enable = imx_pwm_set_enable_v1,
> > +	.pwm_ops = &imx_pwm_ops_v1,
> >  };
> >  
> >  static struct imx_pwm_data imx_pwm_data_v2 = {
> >  	.config = imx_pwm_config_v2,
> >  	.set_enable = imx_pwm_set_enable_v2,
> > +	.pwm_ops = &imx_pwm_ops_v2,
> >  };
> >  
> >  static const struct of_device_id imx_pwm_dt_ids[] = {
> > @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device
> > *pdev) if (!of_id)
> >  		return -ENODEV;
> >  
> > +	data = of_id->data;
> > +
> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> >  	if (imx == NULL)
> >  		return -ENOMEM;
> > @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct
> > platform_device *pdev) return PTR_ERR(imx->clk_ipg);
> >  	}
> >  
> > -	imx->chip.ops = &imx_pwm_ops;
> > +	imx->chip.ops = data->pwm_ops;
> >  	imx->chip.dev = &pdev->dev;
> >  	imx->chip.base = -1;
> >  	imx->chip.npwm = 1;
> >  	imx->chip.can_sleep = true;
> > +	if (data->pwm_ops->set_polarity) {
> > +		dev_dbg(&pdev->dev, "PWM supports output
> > inversion\n");
> > +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
> > +		imx->chip.of_pwm_n_cells = 3;
> > +	}
> >  
> >  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> >  	if (IS_ERR(imx->mmio_base))
> >  		return PTR_ERR(imx->mmio_base);
> >  
> > -	data = of_id->data;
> >  	imx->config = data->config;
> >  	imx->set_enable = data->set_enable;
> 

Best regards,

Łukasz Majewski

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Stefan Agner]

On 2016-10-12 15:15, Lukasz Majewski wrote:
> Hi Stefan,
> 
>> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
>> > From: Lothar Wassmann <LW@KARO-electronics.de>
>> >
>> > The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable
>> > output polarity. This patch adds support to utilize this feature
>> > where available.
>> >
>> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
>> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
>> > Acked-by: Shawn Guo <shawn.guo@linaro.org>
>> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
>> > ---
>> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
>> >  drivers/pwm/pwm-imx.c                             | 51
>> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
>> > deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > index e00c2e9..c61bdf8 100644
>> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > @@ -6,8 +6,8 @@ Required properties:
>> >    - "fsl,imx1-pwm" for PWM compatible with the one integrated on
>> > i.MX1
>> >    - "fsl,imx27-pwm" for PWM compatible with the one integrated on
>> > i.MX27
>> >  - reg: physical base address and length of the controller's
>> > registers -- #pwm-cells: should be 2. See pwm.txt in this directory
>> > for a description of
>> > -  the cells format.
>> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
>> > pwm.txt
>> > +  in this directory for a description of the cells format.
>> >  - clocks : Clock specifiers for both ipg and per clocks.
>> >  - clock-names : Clock names should include both "ipg" and "per"
>> >  See the clock consumer binding,
>> > @@ -17,7 +17,7 @@ See the clock consumer binding,
>> >  Example:
>> >
>> >  pwm1: pwm@53fb4000 {
>> > -	#pwm-cells = <2>;
>> > +	#pwm-cells = <3>;
>> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
>> >  	reg = <0x53fb4000 0x4000>;
>> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
>> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > index d600fd5..c37d223 100644
>> > --- a/drivers/pwm/pwm-imx.c
>> > +++ b/drivers/pwm/pwm-imx.c
>> > @@ -38,6 +38,7 @@
>> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
>> >  #define MX3_PWMCR_WAITEN		(1 << 23)
>> >  #define MX3_PWMCR_DBGEN			(1 << 22)
>> > +#define MX3_PWMCR_POUTC			(1 << 18)
>> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
>> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
>> >  #define MX3_PWMCR_SWR			(1 << 3)
>> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
>> > *chip, if (enable)
>> >  		cr |= MX3_PWMCR_EN;
>> >
>> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
>> > +		cr |= MX3_PWMCR_POUTC;
>> > +
>>
>> This seems wrong to me, the config callback is meant for period/duty
>> cycle only.
> 
> If it is meant only for that, then the polarity should be removed from
> it.
> 
> However after very quick testing, at least on my setup, it turns out
> that removing this lines causes polarity to _not_ being set (and the
> polarity is not inverted).
> 
> I will investigate this further on my setup and hopefully sent proper
> patch.
> 
>> The set_polarity callback should get called in case a
>> different polarity is requested.
> 
> On my setup the pwm2 is set from DT and pwm_backlight_probe() calls
> pwm_apply_args(), so everything should work. However, as I mentioned
> above there still is some problem with inversion setting.
> 
>>
>>
>> >  	writel(cr, imx->mmio_base + MX3_PWMCR);
>> >
>> >  	return 0;
>> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
>> > *chip, struct pwm_device *pwm)
>> >  	clk_disable_unprepare(imx->clk_per);
>> >  }
>> >
>> > -static struct pwm_ops imx_pwm_ops = {
>> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
>> > pwm_device *pwm,
>> > +				enum pwm_polarity polarity)
>> > +{
>> > +	struct imx_chip *imx = to_imx_chip(chip);
>> > +	u32 val;
>> > +
>> > +	if (polarity == pwm->args.polarity)
>> > +		return 0;
>>
>> I don't think that this is right. Today, pwm_apply_args (in
>> include/linux/pwm.h) copies the polarity from args to state.polarity,
>> which is then passed as polarity argument to this function. So this
>> will always return 0 afaict.
> 
> Yes, I've overlooked it (that the state is copied).
> 
> It can be dropped.

Did you do the above test with that line dropped?

> 
>>
>> I would just drop that.
>>
>> There is probably one little problem in the current state of affairs:
>> If the bootloader makes use of a PWM channel with inverted state,
>> then the kernel would not know about that and currently assume a
>> wrong initial state... I guess at one point in time we should
>> implement the state retrieval callback and move to the new atomic PWM
>> API, which would mean to implement apply callback.
> 
> Are there any patches on the horizon?
> 

Not that I know of...

--
Stefan

>>
>> --
>> Stefan
>>
>>
>> > +
>> > +	val = readl(imx->mmio_base + MX3_PWMCR);
>> > +
>> > +	if (polarity == PWM_POLARITY_INVERSED)
>> > +		val |= MX3_PWMCR_POUTC;
>> > +	else
>> > +		val &= ~MX3_PWMCR_POUTC;
>> > +
>> > +	writel(val, imx->mmio_base + MX3_PWMCR);
>> > +
>> > +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n",
>> > __func__,
>> > +		polarity == PWM_POLARITY_INVERSED ? "inverted" :
>> > "normal"); +
>> > +	return 0;
>> > +}
>> > +
>> > +static struct pwm_ops imx_pwm_ops_v1 = {
>> >  	.enable = imx_pwm_enable,
>> >  	.disable = imx_pwm_disable,
>> >  	.config = imx_pwm_config,
>> >  	.owner = THIS_MODULE,
>> >  };
>> >
>> > +static struct pwm_ops imx_pwm_ops_v2 = {
>> > +	.enable = imx_pwm_enable,
>> > +	.disable = imx_pwm_disable,
>> > +	.set_polarity = imx_pwm_set_polarity,
>> > +	.config = imx_pwm_config,
>> > +	.owner = THIS_MODULE,
>> > +};
>> > +
>> >  struct imx_pwm_data {
>> >  	int (*config)(struct pwm_chip *chip,
>> >  		struct pwm_device *pwm, int duty_ns, int
>> > period_ns); void (*set_enable)(struct pwm_chip *chip, bool enable);
>> > +	struct pwm_ops *pwm_ops;
>> >  };
>> >
>> >  static struct imx_pwm_data imx_pwm_data_v1 = {
>> >  	.config = imx_pwm_config_v1,
>> >  	.set_enable = imx_pwm_set_enable_v1,
>> > +	.pwm_ops = &imx_pwm_ops_v1,
>> >  };
>> >
>> >  static struct imx_pwm_data imx_pwm_data_v2 = {
>> >  	.config = imx_pwm_config_v2,
>> >  	.set_enable = imx_pwm_set_enable_v2,
>> > +	.pwm_ops = &imx_pwm_ops_v2,
>> >  };
>> >
>> >  static const struct of_device_id imx_pwm_dt_ids[] = {
>> > @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device
>> > *pdev) if (!of_id)
>> >  		return -ENODEV;
>> >
>> > +	data = of_id->data;
>> > +
>> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
>> >  	if (imx == NULL)
>> >  		return -ENOMEM;
>> > @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct
>> > platform_device *pdev) return PTR_ERR(imx->clk_ipg);
>> >  	}
>> >
>> > -	imx->chip.ops = &imx_pwm_ops;
>> > +	imx->chip.ops = data->pwm_ops;
>> >  	imx->chip.dev = &pdev->dev;
>> >  	imx->chip.base = -1;
>> >  	imx->chip.npwm = 1;
>> >  	imx->chip.can_sleep = true;
>> > +	if (data->pwm_ops->set_polarity) {
>> > +		dev_dbg(&pdev->dev, "PWM supports output
>> > inversion\n");
>> > +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
>> > +		imx->chip.of_pwm_n_cells = 3;
>> > +	}
>> >
>> >  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
>> >  	if (IS_ERR(imx->mmio_base))
>> >  		return PTR_ERR(imx->mmio_base);
>> >
>> > -	data = of_id->data;
>> >  	imx->config = data->config;
>> >  	imx->set_enable = data->set_enable;
>>
> 
> Best regards,
> 
> Łukasz Majewski

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 8568 bytes --]

Hi Stefan,

> On 2016-10-12 15:15, Lukasz Majewski wrote:
> > Hi Stefan,
> > 
> >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> >> > From: Lothar Wassmann <LW@KARO-electronics.de>
> >> >
> >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> >> > configurable output polarity. This patch adds support to utilize
> >> > this feature where available.
> >> >
> >> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> >> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> >> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> >> > Acked-by: Shawn Guo <shawn.guo@linaro.org>
> >> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> >> > ---
> >> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
> >> >  drivers/pwm/pwm-imx.c                             | 51
> >> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> >> > deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > index e00c2e9..c61bdf8 100644
> >> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > @@ -6,8 +6,8 @@ Required properties:
> >> >    - "fsl,imx1-pwm" for PWM compatible with the one integrated on
> >> > i.MX1
> >> >    - "fsl,imx27-pwm" for PWM compatible with the one integrated
> >> > on i.MX27
> >> >  - reg: physical base address and length of the controller's
> >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> >> > directory for a description of
> >> > -  the cells format.
> >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> >> > pwm.txt
> >> > +  in this directory for a description of the cells format.
> >> >  - clocks : Clock specifiers for both ipg and per clocks.
> >> >  - clock-names : Clock names should include both "ipg" and "per"
> >> >  See the clock consumer binding,
> >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> >> >  Example:
> >> >
> >> >  pwm1: pwm@53fb4000 {
> >> > -	#pwm-cells = <2>;
> >> > +	#pwm-cells = <3>;
> >> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> >> >  	reg = <0x53fb4000 0x4000>;
> >> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> >> > index d600fd5..c37d223 100644
> >> > --- a/drivers/pwm/pwm-imx.c
> >> > +++ b/drivers/pwm/pwm-imx.c
> >> > @@ -38,6 +38,7 @@
> >> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
> >> >  #define MX3_PWMCR_WAITEN		(1 << 23)
> >> >  #define MX3_PWMCR_DBGEN			(1 << 22)
> >> > +#define MX3_PWMCR_POUTC			(1 << 18)
> >> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> >> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> >> >  #define MX3_PWMCR_SWR			(1 << 3)
> >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
> >> > *chip, if (enable)
> >> >  		cr |= MX3_PWMCR_EN;
> >> >
> >> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> >> > +		cr |= MX3_PWMCR_POUTC;
> >> > +
> >>
> >> This seems wrong to me, the config callback is meant for
> >> period/duty cycle only.
> > 
> > If it is meant only for that, then the polarity should be removed
> > from it.
> > 
> > However after very quick testing, at least on my setup, it turns out
> > that removing this lines causes polarity to _not_ being set (and the
> > polarity is not inverted).
> > 
> > I will investigate this further on my setup and hopefully sent
> > proper patch.
> > 
> >> The set_polarity callback should get called in case a
> >> different polarity is requested.
> > 
> > On my setup the pwm2 is set from DT and pwm_backlight_probe() calls
> > pwm_apply_args(), so everything should work. However, as I mentioned
> > above there still is some problem with inversion setting.
> > 
> >>
> >>
> >> >  	writel(cr, imx->mmio_base + MX3_PWMCR);
> >> >
> >> >  	return 0;
> >> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> >> > *chip, struct pwm_device *pwm)
> >> >  	clk_disable_unprepare(imx->clk_per);
> >> >  }
> >> >
> >> > -static struct pwm_ops imx_pwm_ops = {
> >> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> >> > pwm_device *pwm,
> >> > +				enum pwm_polarity polarity)
> >> > +{
> >> > +	struct imx_chip *imx = to_imx_chip(chip);
> >> > +	u32 val;
> >> > +
> >> > +	if (polarity == pwm->args.polarity)
> >> > +		return 0;
> >>
> >> I don't think that this is right. Today, pwm_apply_args (in
> >> include/linux/pwm.h) copies the polarity from args to
> >> state.polarity, which is then passed as polarity argument to this
> >> function. So this will always return 0 afaict.
> > 
> > Yes, I've overlooked it (that the state is copied).
> > 
> > It can be dropped.
> 
> Did you do the above test with that line dropped?

Yes. The above code has been also removed.

Best regards,
Łukasz Majewski

> 
> > 
> >>
> >> I would just drop that.
> >>
> >> There is probably one little problem in the current state of
> >> affairs: If the bootloader makes use of a PWM channel with
> >> inverted state, then the kernel would not know about that and
> >> currently assume a wrong initial state... I guess at one point in
> >> time we should implement the state retrieval callback and move to
> >> the new atomic PWM API, which would mean to implement apply
> >> callback.
> > 
> > Are there any patches on the horizon?
> > 
> 
> Not that I know of...
> 
> --
> Stefan
> 
> >>
> >> --
> >> Stefan
> >>
> >>
> >> > +
> >> > +	val = readl(imx->mmio_base + MX3_PWMCR);
> >> > +
> >> > +	if (polarity == PWM_POLARITY_INVERSED)
> >> > +		val |= MX3_PWMCR_POUTC;
> >> > +	else
> >> > +		val &= ~MX3_PWMCR_POUTC;
> >> > +
> >> > +	writel(val, imx->mmio_base + MX3_PWMCR);
> >> > +
> >> > +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n",
> >> > __func__,
> >> > +		polarity == PWM_POLARITY_INVERSED ? "inverted" :
> >> > "normal"); +
> >> > +	return 0;
> >> > +}
> >> > +
> >> > +static struct pwm_ops imx_pwm_ops_v1 = {
> >> >  	.enable = imx_pwm_enable,
> >> >  	.disable = imx_pwm_disable,
> >> >  	.config = imx_pwm_config,
> >> >  	.owner = THIS_MODULE,
> >> >  };
> >> >
> >> > +static struct pwm_ops imx_pwm_ops_v2 = {
> >> > +	.enable = imx_pwm_enable,
> >> > +	.disable = imx_pwm_disable,
> >> > +	.set_polarity = imx_pwm_set_polarity,
> >> > +	.config = imx_pwm_config,
> >> > +	.owner = THIS_MODULE,
> >> > +};
> >> > +
> >> >  struct imx_pwm_data {
> >> >  	int (*config)(struct pwm_chip *chip,
> >> >  		struct pwm_device *pwm, int duty_ns, int
> >> > period_ns); void (*set_enable)(struct pwm_chip *chip, bool
> >> > enable);
> >> > +	struct pwm_ops *pwm_ops;
> >> >  };
> >> >
> >> >  static struct imx_pwm_data imx_pwm_data_v1 = {
> >> >  	.config = imx_pwm_config_v1,
> >> >  	.set_enable = imx_pwm_set_enable_v1,
> >> > +	.pwm_ops = &imx_pwm_ops_v1,
> >> >  };
> >> >
> >> >  static struct imx_pwm_data imx_pwm_data_v2 = {
> >> >  	.config = imx_pwm_config_v2,
> >> >  	.set_enable = imx_pwm_set_enable_v2,
> >> > +	.pwm_ops = &imx_pwm_ops_v2,
> >> >  };
> >> >
> >> >  static const struct of_device_id imx_pwm_dt_ids[] = {
> >> > @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct
> >> > platform_device *pdev) if (!of_id)
> >> >  		return -ENODEV;
> >> >
> >> > +	data = of_id->data;
> >> > +
> >> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx),
> >> > GFP_KERNEL); if (imx == NULL)
> >> >  		return -ENOMEM;
> >> > @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct
> >> > platform_device *pdev) return PTR_ERR(imx->clk_ipg);
> >> >  	}
> >> >
> >> > -	imx->chip.ops = &imx_pwm_ops;
> >> > +	imx->chip.ops = data->pwm_ops;
> >> >  	imx->chip.dev = &pdev->dev;
> >> >  	imx->chip.base = -1;
> >> >  	imx->chip.npwm = 1;
> >> >  	imx->chip.can_sleep = true;
> >> > +	if (data->pwm_ops->set_polarity) {
> >> > +		dev_dbg(&pdev->dev, "PWM supports output
> >> > inversion\n");
> >> > +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
> >> > +		imx->chip.of_pwm_n_cells = 3;
> >> > +	}
> >> >
> >> >  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> >  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> >> >  	if (IS_ERR(imx->mmio_base))
> >> >  		return PTR_ERR(imx->mmio_base);
> >> >
> >> > -	data = of_id->data;
> >> >  	imx->config = data->config;
> >> >  	imx->set_enable = data->set_enable;
> >>
> > 
> > Best regards,
> > 
> > Łukasz Majewski


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 10863 bytes --]

Hi Stefan,

> Hi Stefan,
> 
> > On 2016-10-12 15:15, Lukasz Majewski wrote:
> > > Hi Stefan,
> > > 
> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> > >> > From: Lothar Wassmann <LW@KARO-electronics.de>
> > >> >
> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> > >> > configurable output polarity. This patch adds support to
> > >> > utilize this feature where available.
> > >> >
> > >> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > >> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > >> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> > >> > Acked-by: Shawn Guo <shawn.guo@linaro.org>
> > >> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> > >> > ---
> > >> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
> > >> >  drivers/pwm/pwm-imx.c                             | 51
> > >> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> > >> > deletions(-)
> > >> >
> > >> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > index e00c2e9..c61bdf8 100644
> > >> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > @@ -6,8 +6,8 @@ Required properties:
> > >> >    - "fsl,imx1-pwm" for PWM compatible with the one integrated
> > >> > on i.MX1
> > >> >    - "fsl,imx27-pwm" for PWM compatible with the one integrated
> > >> > on i.MX27
> > >> >  - reg: physical base address and length of the controller's
> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> > >> > directory for a description of
> > >> > -  the cells format.
> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> > >> > pwm.txt
> > >> > +  in this directory for a description of the cells format.
> > >> >  - clocks : Clock specifiers for both ipg and per clocks.
> > >> >  - clock-names : Clock names should include both "ipg" and
> > >> > "per" See the clock consumer binding,
> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > >> >  Example:
> > >> >
> > >> >  pwm1: pwm@53fb4000 {
> > >> > -	#pwm-cells = <2>;
> > >> > +	#pwm-cells = <3>;
> > >> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > >> >  	reg = <0x53fb4000 0x4000>;
> > >> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > >> > index d600fd5..c37d223 100644
> > >> > --- a/drivers/pwm/pwm-imx.c
> > >> > +++ b/drivers/pwm/pwm-imx.c
> > >> > @@ -38,6 +38,7 @@
> > >> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
> > >> >  #define MX3_PWMCR_WAITEN		(1 << 23)
> > >> >  #define MX3_PWMCR_DBGEN			(1 << 22)
> > >> > +#define MX3_PWMCR_POUTC			(1 << 18)
> > >> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> > >> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> > >> >  #define MX3_PWMCR_SWR			(1 << 3)
> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> > >> > pwm_chip *chip, if (enable)
> > >> >  		cr |= MX3_PWMCR_EN;
> > >> >
> > >> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > >> > +		cr |= MX3_PWMCR_POUTC;
> > >> > +
> > >>
> > >> This seems wrong to me, the config callback is meant for
> > >> period/duty cycle only.

Unfortunately, it also resets the PWM IP block and setups it again (by
writing to PWMCR register). In that function we setup for example MX3_PWMCR_DOZEEN
and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?


I've double checked the backlight and pwm code flow.

Please find following snippet:

[    0.135545] ######### imx_pwm_probe
[    0.135581] PWM supports output inversion
[    0.136864] ######### pwm_backlight_probe
[    0.136913] backlight supply power not found, using dummy regulator
[    0.136984] ######### imx_pwm_set_polarity 1
[    0.136995] imx_pwm_set_polarity: polarity set to inverted cr: 0x40000 0xf08f8000 
[    0.137005] #########0 imx_pwm_config_v2 cr: 0x40000 
[    0.137683] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
[    0.137693] #########2 imx_pwm_config_v2 cr: 0x1c20050
[    0.137702] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
[    0.137711] @@@@@@@@@@ pwm_apply_state

Here the pwm_backlight_probe calls set_polarity callback available in
pwm - the polarity is set (the 0x40000 value).

The above operation is performed in pwm_apply_state (@ drivers/pwm/core.c). 
In the same function, latter we call the pwm->chip->ops->config(), which is the
pointer to config_v2.
Since the PWM is not yet enabled, this function performs SW reset and
PWM inversion setting is cleared.

Possible solutions:

1. Leave the original patch from Bhuvanchandra as it was (I'm for this 
option)

2. Enable early PWM (in core, or in bl driver) so the config_v2 is not
calling SW reset on the PWM. (but this solutions seems _really_ bad to me)

3. Perform defer probe of pwm backlight driver (pwm_bl.c) until the pwm
is fully configured (it might be a bit tricky).


Best regards,
Łukasz Majewski

> > > 
> > > If it is meant only for that, then the polarity should be removed
> > > from it.
> > > 
> > > However after very quick testing, at least on my setup, it turns
> > > out that removing this lines causes polarity to _not_ being set
> > > (and the polarity is not inverted).
> > > 
> > > I will investigate this further on my setup and hopefully sent
> > > proper patch.
> > > 
> > >> The set_polarity callback should get called in case a
> > >> different polarity is requested.
> > > 
> > > On my setup the pwm2 is set from DT and pwm_backlight_probe()
> > > calls pwm_apply_args(), so everything should work. However, as I
> > > mentioned above there still is some problem with inversion
> > > setting.
> > > 
> > >>
> > >>
> > >> >  	writel(cr, imx->mmio_base + MX3_PWMCR);
> > >> >
> > >> >  	return 0;
> > >> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct
> > >> > pwm_chip *chip, struct pwm_device *pwm)
> > >> >  	clk_disable_unprepare(imx->clk_per);
> > >> >  }
> > >> >
> > >> > -static struct pwm_ops imx_pwm_ops = {
> > >> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> > >> > pwm_device *pwm,
> > >> > +				enum pwm_polarity polarity)
> > >> > +{
> > >> > +	struct imx_chip *imx = to_imx_chip(chip);
> > >> > +	u32 val;
> > >> > +
> > >> > +	if (polarity == pwm->args.polarity)
> > >> > +		return 0;
> > >>
> > >> I don't think that this is right. Today, pwm_apply_args (in
> > >> include/linux/pwm.h) copies the polarity from args to
> > >> state.polarity, which is then passed as polarity argument to this
> > >> function. So this will always return 0 afaict.
> > > 
> > > Yes, I've overlooked it (that the state is copied).
> > > 
> > > It can be dropped.
> > 
> > Did you do the above test with that line dropped?
> 
> Yes. The above code has been also removed.
> 
> Best regards,
> Łukasz Majewski
> 
> > 
> > > 
> > >>
> > >> I would just drop that.
> > >>
> > >> There is probably one little problem in the current state of
> > >> affairs: If the bootloader makes use of a PWM channel with
> > >> inverted state, then the kernel would not know about that and
> > >> currently assume a wrong initial state... I guess at one point in
> > >> time we should implement the state retrieval callback and move to
> > >> the new atomic PWM API, which would mean to implement apply
> > >> callback.
> > > 
> > > Are there any patches on the horizon?
> > > 
> > 
> > Not that I know of...
> > 
> > --
> > Stefan
> > 
> > >>
> > >> --
> > >> Stefan
> > >>
> > >>
> > >> > +
> > >> > +	val = readl(imx->mmio_base + MX3_PWMCR);
> > >> > +
> > >> > +	if (polarity == PWM_POLARITY_INVERSED)
> > >> > +		val |= MX3_PWMCR_POUTC;
> > >> > +	else
> > >> > +		val &= ~MX3_PWMCR_POUTC;
> > >> > +
> > >> > +	writel(val, imx->mmio_base + MX3_PWMCR);
> > >> > +
> > >> > +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n",
> > >> > __func__,
> > >> > +		polarity == PWM_POLARITY_INVERSED ?
> > >> > "inverted" : "normal"); +
> > >> > +	return 0;
> > >> > +}
> > >> > +
> > >> > +static struct pwm_ops imx_pwm_ops_v1 = {
> > >> >  	.enable = imx_pwm_enable,
> > >> >  	.disable = imx_pwm_disable,
> > >> >  	.config = imx_pwm_config,
> > >> >  	.owner = THIS_MODULE,
> > >> >  };
> > >> >
> > >> > +static struct pwm_ops imx_pwm_ops_v2 = {
> > >> > +	.enable = imx_pwm_enable,
> > >> > +	.disable = imx_pwm_disable,
> > >> > +	.set_polarity = imx_pwm_set_polarity,
> > >> > +	.config = imx_pwm_config,
> > >> > +	.owner = THIS_MODULE,
> > >> > +};
> > >> > +
> > >> >  struct imx_pwm_data {
> > >> >  	int (*config)(struct pwm_chip *chip,
> > >> >  		struct pwm_device *pwm, int duty_ns, int
> > >> > period_ns); void (*set_enable)(struct pwm_chip *chip, bool
> > >> > enable);
> > >> > +	struct pwm_ops *pwm_ops;
> > >> >  };
> > >> >
> > >> >  static struct imx_pwm_data imx_pwm_data_v1 = {
> > >> >  	.config = imx_pwm_config_v1,
> > >> >  	.set_enable = imx_pwm_set_enable_v1,
> > >> > +	.pwm_ops = &imx_pwm_ops_v1,
> > >> >  };
> > >> >
> > >> >  static struct imx_pwm_data imx_pwm_data_v2 = {
> > >> >  	.config = imx_pwm_config_v2,
> > >> >  	.set_enable = imx_pwm_set_enable_v2,
> > >> > +	.pwm_ops = &imx_pwm_ops_v2,
> > >> >  };
> > >> >
> > >> >  static const struct of_device_id imx_pwm_dt_ids[] = {
> > >> > @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct
> > >> > platform_device *pdev) if (!of_id)
> > >> >  		return -ENODEV;
> > >> >
> > >> > +	data = of_id->data;
> > >> > +
> > >> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx),
> > >> > GFP_KERNEL); if (imx == NULL)
> > >> >  		return -ENOMEM;
> > >> > @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct
> > >> > platform_device *pdev) return PTR_ERR(imx->clk_ipg);
> > >> >  	}
> > >> >
> > >> > -	imx->chip.ops = &imx_pwm_ops;
> > >> > +	imx->chip.ops = data->pwm_ops;
> > >> >  	imx->chip.dev = &pdev->dev;
> > >> >  	imx->chip.base = -1;
> > >> >  	imx->chip.npwm = 1;
> > >> >  	imx->chip.can_sleep = true;
> > >> > +	if (data->pwm_ops->set_polarity) {
> > >> > +		dev_dbg(&pdev->dev, "PWM supports output
> > >> > inversion\n");
> > >> > +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
> > >> > +		imx->chip.of_pwm_n_cells = 3;
> > >> > +	}
> > >> >
> > >> >  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >> >  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> > >> >  	if (IS_ERR(imx->mmio_base))
> > >> >  		return PTR_ERR(imx->mmio_base);
> > >> >
> > >> > -	data = of_id->data;
> > >> >  	imx->config = data->config;
> > >> >  	imx->set_enable = data->set_enable;
> > >>
> > > 
> > > Best regards,
> > > 
> > > Łukasz Majewski
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Stefan Agner]

On 2016-10-20 01:30, Lukasz Majewski wrote:
> Hi Stefan,
> 
>> Hi Stefan,
>>
>> > On 2016-10-12 15:15, Lukasz Majewski wrote:
>> > > Hi Stefan,
>> > >
>> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
>> > >> > From: Lothar Wassmann <LW@KARO-electronics.de>
>> > >> >
>> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
>> > >> > configurable output polarity. This patch adds support to
>> > >> > utilize this feature where available.
>> > >> >
>> > >> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
>> > >> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>> > >> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
>> > >> > Acked-by: Shawn Guo <shawn.guo@linaro.org>
>> > >> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
>> > >> > ---
>> > >> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
>> > >> >  drivers/pwm/pwm-imx.c                             | 51
>> > >> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
>> > >> > deletions(-)
>> > >> >
>> > >> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > index e00c2e9..c61bdf8 100644
>> > >> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > @@ -6,8 +6,8 @@ Required properties:
>> > >> >    - "fsl,imx1-pwm" for PWM compatible with the one integrated
>> > >> > on i.MX1
>> > >> >    - "fsl,imx27-pwm" for PWM compatible with the one integrated
>> > >> > on i.MX27
>> > >> >  - reg: physical base address and length of the controller's
>> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
>> > >> > directory for a description of
>> > >> > -  the cells format.
>> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
>> > >> > pwm.txt
>> > >> > +  in this directory for a description of the cells format.
>> > >> >  - clocks : Clock specifiers for both ipg and per clocks.
>> > >> >  - clock-names : Clock names should include both "ipg" and
>> > >> > "per" See the clock consumer binding,
>> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
>> > >> >  Example:
>> > >> >
>> > >> >  pwm1: pwm@53fb4000 {
>> > >> > -	#pwm-cells = <2>;
>> > >> > +	#pwm-cells = <3>;
>> > >> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
>> > >> >  	reg = <0x53fb4000 0x4000>;
>> > >> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
>> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > >> > index d600fd5..c37d223 100644
>> > >> > --- a/drivers/pwm/pwm-imx.c
>> > >> > +++ b/drivers/pwm/pwm-imx.c
>> > >> > @@ -38,6 +38,7 @@
>> > >> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
>> > >> >  #define MX3_PWMCR_WAITEN		(1 << 23)
>> > >> >  #define MX3_PWMCR_DBGEN			(1 << 22)
>> > >> > +#define MX3_PWMCR_POUTC			(1 << 18)
>> > >> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
>> > >> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
>> > >> >  #define MX3_PWMCR_SWR			(1 << 3)
>> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
>> > >> > pwm_chip *chip, if (enable)
>> > >> >  		cr |= MX3_PWMCR_EN;
>> > >> >
>> > >> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
>> > >> > +		cr |= MX3_PWMCR_POUTC;
>> > >> > +
>> > >>
>> > >> This seems wrong to me, the config callback is meant for
>> > >> period/duty cycle only.
> 
> Unfortunately, it also resets the PWM IP block and setups it again (by
> writing to PWMCR register). In that function we setup for example
> MX3_PWMCR_DOZEEN
> and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
> 
> 
> I've double checked the backlight and pwm code flow.
> 
> Please find following snippet:
> 
> [    0.135545] ######### imx_pwm_probe
> [    0.135581] PWM supports output inversion
> [    0.136864] ######### pwm_backlight_probe
> [    0.136913] backlight supply power not found, using dummy regulator
> [    0.136984] ######### imx_pwm_set_polarity 1
> [    0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> 0x40000 0xf08f8000
> [    0.137005] #########0 imx_pwm_config_v2 cr: 0x40000 
> [    0.137683] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [    0.137693] #########2 imx_pwm_config_v2 cr: 0x1c20050
> [    0.137702] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [    0.137711] @@@@@@@@@@ pwm_apply_state
> 
> Here the pwm_backlight_probe calls set_polarity callback available in
> pwm - the polarity is set (the 0x40000 value).
> 
> The above operation is performed in pwm_apply_state (@ drivers/pwm/core.c). 
> In the same function, latter we call the pwm->chip->ops->config(), which is the
> pointer to config_v2.
> Since the PWM is not yet enabled, this function performs SW reset and
> PWM inversion setting is cleared.

That function should not do that. It was probably already problematic in
the old times, it is definitely now with the atomic PWM stuff.

> 
> Possible solutions:
> 
> 1. Leave the original patch from Bhuvanchandra as it was (I'm for this 
> option)

That really seems like a hack to me, and makes transition to the atomic
PWM API more complex.

If we can't make it happen properly in the current state of affairs, we
probably should first move to the atomic API.

It really should not be that hard, since we already do almost everything
in one function (imx_pwm_config_v2). We probably can almost just assign
that function to the new apply function pointer and read information
from the new state struct.

There are examples for instance here:
https://patchwork.kernel.org/patch/7228221/

--
Stefan

> 
> 2. Enable early PWM (in core, or in bl driver) so the config_v2 is not
> calling SW reset on the PWM. (but this solutions seems _really_ bad to me)
> 
> 3. Perform defer probe of pwm backlight driver (pwm_bl.c) until the pwm
> is fully configured (it might be a bit tricky).
> 
> 
> Best regards,
> Łukasz Majewski
> 
>> > >
>> > > If it is meant only for that, then the polarity should be removed
>> > > from it.
>> > >
>> > > However after very quick testing, at least on my setup, it turns
>> > > out that removing this lines causes polarity to _not_ being set
>> > > (and the polarity is not inverted).
>> > >
>> > > I will investigate this further on my setup and hopefully sent
>> > > proper patch.
>> > >
>> > >> The set_polarity callback should get called in case a
>> > >> different polarity is requested.
>> > >
>> > > On my setup the pwm2 is set from DT and pwm_backlight_probe()
>> > > calls pwm_apply_args(), so everything should work. However, as I
>> > > mentioned above there still is some problem with inversion
>> > > setting.
>> > >
>> > >>
>> > >>
>> > >> >  	writel(cr, imx->mmio_base + MX3_PWMCR);
>> > >> >
>> > >> >  	return 0;
>> > >> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct
>> > >> > pwm_chip *chip, struct pwm_device *pwm)
>> > >> >  	clk_disable_unprepare(imx->clk_per);
>> > >> >  }
>> > >> >
>> > >> > -static struct pwm_ops imx_pwm_ops = {
>> > >> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
>> > >> > pwm_device *pwm,
>> > >> > +				enum pwm_polarity polarity)
>> > >> > +{
>> > >> > +	struct imx_chip *imx = to_imx_chip(chip);
>> > >> > +	u32 val;
>> > >> > +
>> > >> > +	if (polarity == pwm->args.polarity)
>> > >> > +		return 0;
>> > >>
>> > >> I don't think that this is right. Today, pwm_apply_args (in
>> > >> include/linux/pwm.h) copies the polarity from args to
>> > >> state.polarity, which is then passed as polarity argument to this
>> > >> function. So this will always return 0 afaict.
>> > >
>> > > Yes, I've overlooked it (that the state is copied).
>> > >
>> > > It can be dropped.
>> >
>> > Did you do the above test with that line dropped?
>>
>> Yes. The above code has been also removed.
>>
>> Best regards,
>> Łukasz Majewski
>>
>> >
>> > >
>> > >>
>> > >> I would just drop that.
>> > >>
>> > >> There is probably one little problem in the current state of
>> > >> affairs: If the bootloader makes use of a PWM channel with
>> > >> inverted state, then the kernel would not know about that and
>> > >> currently assume a wrong initial state... I guess at one point in
>> > >> time we should implement the state retrieval callback and move to
>> > >> the new atomic PWM API, which would mean to implement apply
>> > >> callback.
>> > >
>> > > Are there any patches on the horizon?
>> > >
>> >
>> > Not that I know of...
>> >
>> > --
>> > Stefan
>> >
>> > >>
>> > >> --
>> > >> Stefan
>> > >>
>> > >>
>> > >> > +
>> > >> > +	val = readl(imx->mmio_base + MX3_PWMCR);
>> > >> > +
>> > >> > +	if (polarity == PWM_POLARITY_INVERSED)
>> > >> > +		val |= MX3_PWMCR_POUTC;
>> > >> > +	else
>> > >> > +		val &= ~MX3_PWMCR_POUTC;
>> > >> > +
>> > >> > +	writel(val, imx->mmio_base + MX3_PWMCR);
>> > >> > +
>> > >> > +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n",
>> > >> > __func__,
>> > >> > +		polarity == PWM_POLARITY_INVERSED ?
>> > >> > "inverted" : "normal"); +
>> > >> > +	return 0;
>> > >> > +}
>> > >> > +
>> > >> > +static struct pwm_ops imx_pwm_ops_v1 = {
>> > >> >  	.enable = imx_pwm_enable,
>> > >> >  	.disable = imx_pwm_disable,
>> > >> >  	.config = imx_pwm_config,
>> > >> >  	.owner = THIS_MODULE,
>> > >> >  };
>> > >> >
>> > >> > +static struct pwm_ops imx_pwm_ops_v2 = {
>> > >> > +	.enable = imx_pwm_enable,
>> > >> > +	.disable = imx_pwm_disable,
>> > >> > +	.set_polarity = imx_pwm_set_polarity,
>> > >> > +	.config = imx_pwm_config,
>> > >> > +	.owner = THIS_MODULE,
>> > >> > +};
>> > >> > +
>> > >> >  struct imx_pwm_data {
>> > >> >  	int (*config)(struct pwm_chip *chip,
>> > >> >  		struct pwm_device *pwm, int duty_ns, int
>> > >> > period_ns); void (*set_enable)(struct pwm_chip *chip, bool
>> > >> > enable);
>> > >> > +	struct pwm_ops *pwm_ops;
>> > >> >  };
>> > >> >
>> > >> >  static struct imx_pwm_data imx_pwm_data_v1 = {
>> > >> >  	.config = imx_pwm_config_v1,
>> > >> >  	.set_enable = imx_pwm_set_enable_v1,
>> > >> > +	.pwm_ops = &imx_pwm_ops_v1,
>> > >> >  };
>> > >> >
>> > >> >  static struct imx_pwm_data imx_pwm_data_v2 = {
>> > >> >  	.config = imx_pwm_config_v2,
>> > >> >  	.set_enable = imx_pwm_set_enable_v2,
>> > >> > +	.pwm_ops = &imx_pwm_ops_v2,
>> > >> >  };
>> > >> >
>> > >> >  static const struct of_device_id imx_pwm_dt_ids[] = {
>> > >> > @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct
>> > >> > platform_device *pdev) if (!of_id)
>> > >> >  		return -ENODEV;
>> > >> >
>> > >> > +	data = of_id->data;
>> > >> > +
>> > >> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx),
>> > >> > GFP_KERNEL); if (imx == NULL)
>> > >> >  		return -ENOMEM;
>> > >> > @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct
>> > >> > platform_device *pdev) return PTR_ERR(imx->clk_ipg);
>> > >> >  	}
>> > >> >
>> > >> > -	imx->chip.ops = &imx_pwm_ops;
>> > >> > +	imx->chip.ops = data->pwm_ops;
>> > >> >  	imx->chip.dev = &pdev->dev;
>> > >> >  	imx->chip.base = -1;
>> > >> >  	imx->chip.npwm = 1;
>> > >> >  	imx->chip.can_sleep = true;
>> > >> > +	if (data->pwm_ops->set_polarity) {
>> > >> > +		dev_dbg(&pdev->dev, "PWM supports output
>> > >> > inversion\n");
>> > >> > +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
>> > >> > +		imx->chip.of_pwm_n_cells = 3;
>> > >> > +	}
>> > >> >
>> > >> >  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > >> >  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
>> > >> >  	if (IS_ERR(imx->mmio_base))
>> > >> >  		return PTR_ERR(imx->mmio_base);
>> > >> >
>> > >> > -	data = of_id->data;
>> > >> >  	imx->config = data->config;
>> > >> >  	imx->set_enable = data->set_enable;
>> > >>
>> > >
>> > > Best regards,
>> > >
>> > > Łukasz Majewski
>>

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 15770 bytes --]

Hi Stefan,

> On 2016-10-20 01:30, Lukasz Majewski wrote:
> > Hi Stefan,
> > 
> >> Hi Stefan,
> >>
> >> > On 2016-10-12 15:15, Lukasz Majewski wrote:
> >> > > Hi Stefan,
> >> > >
> >> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> >> > >> > From: Lothar Wassmann <LW@KARO-electronics.de>
> >> > >> >
> >> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> >> > >> > configurable output polarity. This patch adds support to
> >> > >> > utilize this feature where available.
> >> > >> >
> >> > >> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> >> > >> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> >> > >> > Signed-off-by: Bhuvanchandra DV
> >> > >> > <bhuvanchandra.dv@toradex.com> Acked-by: Shawn Guo
> >> > >> > <shawn.guo@linaro.org> Reviewed-by: Sascha Hauer
> >> > >> > <s.hauer@pengutronix.de> ---
> >> > >> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
> >> > >> >  drivers/pwm/pwm-imx.c                             | 51
> >> > >> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> >> > >> > deletions(-)
> >> > >> >
> >> > >> > diff --git
> >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> >> > >> > e00c2e9..c61bdf8 100644 ---
> >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8
> >> > >> > +6,8 @@ Required properties:
> >> > >> >    - "fsl,imx1-pwm" for PWM compatible with the one
> >> > >> > integrated on i.MX1
> >> > >> >    - "fsl,imx27-pwm" for PWM compatible with the one
> >> > >> > integrated on i.MX27
> >> > >> >  - reg: physical base address and length of the controller's
> >> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> >> > >> > directory for a description of
> >> > >> > -  the cells format.
> >> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs.
> >> > >> > See pwm.txt
> >> > >> > +  in this directory for a description of the cells format.
> >> > >> >  - clocks : Clock specifiers for both ipg and per clocks.
> >> > >> >  - clock-names : Clock names should include both "ipg" and
> >> > >> > "per" See the clock consumer binding,
> >> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> >> > >> >  Example:
> >> > >> >
> >> > >> >  pwm1: pwm@53fb4000 {
> >> > >> > -	#pwm-cells = <2>;
> >> > >> > +	#pwm-cells = <3>;
> >> > >> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> >> > >> >  	reg = <0x53fb4000 0x4000>;
> >> > >> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> >> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> >> > >> > index d600fd5..c37d223 100644
> >> > >> > --- a/drivers/pwm/pwm-imx.c
> >> > >> > +++ b/drivers/pwm/pwm-imx.c
> >> > >> > @@ -38,6 +38,7 @@
> >> > >> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
> >> > >> >  #define MX3_PWMCR_WAITEN		(1 << 23)
> >> > >> >  #define MX3_PWMCR_DBGEN			(1 << 22)
> >> > >> > +#define MX3_PWMCR_POUTC			(1 << 18)
> >> > >> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> >> > >> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> >> > >> >  #define MX3_PWMCR_SWR			(1 << 3)
> >> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> >> > >> > pwm_chip *chip, if (enable)
> >> > >> >  		cr |= MX3_PWMCR_EN;
> >> > >> >
> >> > >> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> >> > >> > +		cr |= MX3_PWMCR_POUTC;
> >> > >> > +
> >> > >>
> >> > >> This seems wrong to me, the config callback is meant for
> >> > >> period/duty cycle only.
> > 
> > Unfortunately, it also resets the PWM IP block and setups it again
> > (by writing to PWMCR register). In that function we setup for
> > example MX3_PWMCR_DOZEEN
> > and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
> > 
> > 
> > I've double checked the backlight and pwm code flow.
> > 
> > Please find following snippet:
> > 
> > [    0.135545] ######### imx_pwm_probe
> > [    0.135581] PWM supports output inversion
> > [    0.136864] ######### pwm_backlight_probe
> > [    0.136913] backlight supply power not found, using dummy
> > regulator [    0.136984] ######### imx_pwm_set_polarity 1
> > [    0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> > 0x40000 0xf08f8000
> > [    0.137005] #########0 imx_pwm_config_v2 cr: 0x40000 
> > [    0.137683] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> > [    0.137693] #########2 imx_pwm_config_v2 cr: 0x1c20050
> > [    0.137702] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> > [    0.137711] @@@@@@@@@@ pwm_apply_state

Maybe a bit more logs:

[    0.135451] ######### imx_pwm_probe
[    0.135488] PWM supports output inversion
[    0.136777] ######### pwm_backlight_probe
[    0.136826] backlight supply power not found, using dummy regulator
[    0.136893] ********* pwm_apply_state state->enabled: 0
[    0.136902] ######### imx_pwm_set_polarity 1
[    0.136913] imx_pwm_set_polarity: polarity set to inverted cr: 0x40000 0xf08f8000
[    0.136923] #########0 imx_pwm_config_v2 cr: 0x40000
[    0.137692] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
[    0.137701] #########2 imx_pwm_config_v2 cr: 0x1c20050
[    0.137710] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
[    0.137720] @@@@@@@@@@ pwm_apply_state
[    0.137856] ********* pwm_apply_state state->enabled: 0
[    0.137869] #########0 imx_pwm_config_v2 cr: 0x1c20050
[    0.138904] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
[    0.138913] #########2 imx_pwm_config_v2 cr: 0x1c20050
[    0.138921] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
[    0.138928] @@@@@@@@@@ pwm_apply_state
[    0.138940] ********* pwm_apply_state state->enabled: 1
					 ^^^^^^^^^^^^^^^^^^ this is called from
			pwm_backlight_power_on() from pwm_bl probe function

The problem here is not the lack of ->apply() callback, but the requirement to
perform software reset on the pwm_v2 fifo when the pwm_v2 is NOT enabled (state->enabled: 0).

As fair as I can see the pwm_state has following members: period, duty cycle, polarity and enabled.
I'm fine to implement ->apply() callback, which would change above values.

However, there is a problem with ->config() (imx_pwm_config_v2 @ pwm-imx.c) and imx pwm_v2 software
FIFO reset.
We can set polarity in any other kernel subsystem, which uses PWM (backlight in this example) and 
then this setting would disappear when we call pwm_apply_state with state->enabled = 0 (as presented
in the log). This imposes setting polarity at ->config when we enable the PWM (as this patch does).



> > 
> > Here the pwm_backlight_probe calls set_polarity callback available
> > in pwm - the polarity is set (the 0x40000 value).
> > 
> > The above operation is performed in pwm_apply_state (@
> > drivers/pwm/core.c). In the same function, latter we call the
> > pwm->chip->ops->config(), which is the pointer to config_v2.
> > Since the PWM is not yet enabled, this function performs SW reset
> > and PWM inversion setting is cleared.
> 
> That function should not do that. 

I do agree that it shouldn't. Correct me if I'm wrong, but it seems like an 
PWM HW requirement to perform the reset.

>It was probably already problematic
> in the old times, it is definitely now with the atomic PWM stuff.

The "atomic"[*] code (with ->apply() provided) will not solve this issue.

> 
> > 
> > Possible solutions:
> > 
> > 1. Leave the original patch from Bhuvanchandra as it was (I'm for
> > this option)
> 
> That really seems like a hack to me, and makes transition to the
> atomic PWM API more complex.

Could you be more specific here? 

As I mentioned before, the problem is not with the lack of
"atomic" API.

For me the problem is with other subsystems (like pwm backlight)
which do require polarity inversion to work properly.

> 
> If we can't make it happen properly in the current state of affairs,

We can set polarity properly with this patch. The only thing which needs fixing
and another revision of the patch) is the removal of condition:

-       if (polarity == pwm->args.polarity)
-               return 0;

at imx_pwm_set_polarity() function.

> we probably should first move to the atomic API.
> 
> It really should not be that hard, since we already do almost
> everything in one function (imx_pwm_config_v2). We probably can
> almost just assign that function to the new apply function pointer
> and read information from the new state struct.

We do need to configure polarity according to DT value during the pwm
config_v2 configuration after PWM IP software reset.

> 
> There are examples for instance here:
> https://patchwork.kernel.org/patch/7228221/

The atomicity[*]:

- It seems to me that this "atomicity" is just embracing all the ->enable, ->disable
and ->set_polarity in one -> apply callback. Is this just conceptual clean up or is
there any other reason for it (I'm just curious) ?

Best regards,

Łukasz Majewski

> 
> --
> Stefan
> 
> > 
> > 2. Enable early PWM (in core, or in bl driver) so the config_v2 is
> > not calling SW reset on the PWM. (but this solutions seems _really_
> > bad to me)
> > 
> > 3. Perform defer probe of pwm backlight driver (pwm_bl.c) until the
> > pwm is fully configured (it might be a bit tricky).
> > 
> > 
> > Best regards,
> > Łukasz Majewski
> > 
> >> > >
> >> > > If it is meant only for that, then the polarity should be
> >> > > removed from it.
> >> > >
> >> > > However after very quick testing, at least on my setup, it
> >> > > turns out that removing this lines causes polarity to _not_
> >> > > being set (and the polarity is not inverted).
> >> > >
> >> > > I will investigate this further on my setup and hopefully sent
> >> > > proper patch.
> >> > >
> >> > >> The set_polarity callback should get called in case a
> >> > >> different polarity is requested.
> >> > >
> >> > > On my setup the pwm2 is set from DT and pwm_backlight_probe()
> >> > > calls pwm_apply_args(), so everything should work. However, as
> >> > > I mentioned above there still is some problem with inversion
> >> > > setting.
> >> > >
> >> > >>
> >> > >>
> >> > >> >  	writel(cr, imx->mmio_base + MX3_PWMCR);
> >> > >> >
> >> > >> >  	return 0;
> >> > >> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct
> >> > >> > pwm_chip *chip, struct pwm_device *pwm)
> >> > >> >  	clk_disable_unprepare(imx->clk_per);
> >> > >> >  }
> >> > >> >
> >> > >> > -static struct pwm_ops imx_pwm_ops = {
> >> > >> > +static int imx_pwm_set_polarity(struct pwm_chip *chip,
> >> > >> > struct pwm_device *pwm,
> >> > >> > +				enum pwm_polarity polarity)
> >> > >> > +{
> >> > >> > +	struct imx_chip *imx = to_imx_chip(chip);
> >> > >> > +	u32 val;
> >> > >> > +
> >> > >> > +	if (polarity == pwm->args.polarity)
> >> > >> > +		return 0;
> >> > >>
> >> > >> I don't think that this is right. Today, pwm_apply_args (in
> >> > >> include/linux/pwm.h) copies the polarity from args to
> >> > >> state.polarity, which is then passed as polarity argument to
> >> > >> this function. So this will always return 0 afaict.
> >> > >
> >> > > Yes, I've overlooked it (that the state is copied).
> >> > >
> >> > > It can be dropped.
> >> >
> >> > Did you do the above test with that line dropped?
> >>
> >> Yes. The above code has been also removed.
> >>
> >> Best regards,
> >> Łukasz Majewski
> >>
> >> >
> >> > >
> >> > >>
> >> > >> I would just drop that.
> >> > >>
> >> > >> There is probably one little problem in the current state of
> >> > >> affairs: If the bootloader makes use of a PWM channel with
> >> > >> inverted state, then the kernel would not know about that and
> >> > >> currently assume a wrong initial state... I guess at one
> >> > >> point in time we should implement the state retrieval
> >> > >> callback and move to the new atomic PWM API, which would mean
> >> > >> to implement apply callback.
> >> > >
> >> > > Are there any patches on the horizon?
> >> > >
> >> >
> >> > Not that I know of...
> >> >
> >> > --
> >> > Stefan
> >> >
> >> > >>
> >> > >> --
> >> > >> Stefan
> >> > >>
> >> > >>
> >> > >> > +
> >> > >> > +	val = readl(imx->mmio_base + MX3_PWMCR);
> >> > >> > +
> >> > >> > +	if (polarity == PWM_POLARITY_INVERSED)
> >> > >> > +		val |= MX3_PWMCR_POUTC;
> >> > >> > +	else
> >> > >> > +		val &= ~MX3_PWMCR_POUTC;
> >> > >> > +
> >> > >> > +	writel(val, imx->mmio_base + MX3_PWMCR);
> >> > >> > +
> >> > >> > +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n",
> >> > >> > __func__,
> >> > >> > +		polarity == PWM_POLARITY_INVERSED ?
> >> > >> > "inverted" : "normal"); +
> >> > >> > +	return 0;
> >> > >> > +}
> >> > >> > +
> >> > >> > +static struct pwm_ops imx_pwm_ops_v1 = {
> >> > >> >  	.enable = imx_pwm_enable,
> >> > >> >  	.disable = imx_pwm_disable,
> >> > >> >  	.config = imx_pwm_config,
> >> > >> >  	.owner = THIS_MODULE,
> >> > >> >  };
> >> > >> >
> >> > >> > +static struct pwm_ops imx_pwm_ops_v2 = {
> >> > >> > +	.enable = imx_pwm_enable,
> >> > >> > +	.disable = imx_pwm_disable,
> >> > >> > +	.set_polarity = imx_pwm_set_polarity,
> >> > >> > +	.config = imx_pwm_config,
> >> > >> > +	.owner = THIS_MODULE,
> >> > >> > +};
> >> > >> > +
> >> > >> >  struct imx_pwm_data {
> >> > >> >  	int (*config)(struct pwm_chip *chip,
> >> > >> >  		struct pwm_device *pwm, int duty_ns, int
> >> > >> > period_ns); void (*set_enable)(struct pwm_chip *chip, bool
> >> > >> > enable);
> >> > >> > +	struct pwm_ops *pwm_ops;
> >> > >> >  };
> >> > >> >
> >> > >> >  static struct imx_pwm_data imx_pwm_data_v1 = {
> >> > >> >  	.config = imx_pwm_config_v1,
> >> > >> >  	.set_enable = imx_pwm_set_enable_v1,
> >> > >> > +	.pwm_ops = &imx_pwm_ops_v1,
> >> > >> >  };
> >> > >> >
> >> > >> >  static struct imx_pwm_data imx_pwm_data_v2 = {
> >> > >> >  	.config = imx_pwm_config_v2,
> >> > >> >  	.set_enable = imx_pwm_set_enable_v2,
> >> > >> > +	.pwm_ops = &imx_pwm_ops_v2,
> >> > >> >  };
> >> > >> >
> >> > >> >  static const struct of_device_id imx_pwm_dt_ids[] = {
> >> > >> > @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct
> >> > >> > platform_device *pdev) if (!of_id)
> >> > >> >  		return -ENODEV;
> >> > >> >
> >> > >> > +	data = of_id->data;
> >> > >> > +
> >> > >> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx),
> >> > >> > GFP_KERNEL); if (imx == NULL)
> >> > >> >  		return -ENOMEM;
> >> > >> > @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct
> >> > >> > platform_device *pdev) return PTR_ERR(imx->clk_ipg);
> >> > >> >  	}
> >> > >> >
> >> > >> > -	imx->chip.ops = &imx_pwm_ops;
> >> > >> > +	imx->chip.ops = data->pwm_ops;
> >> > >> >  	imx->chip.dev = &pdev->dev;
> >> > >> >  	imx->chip.base = -1;
> >> > >> >  	imx->chip.npwm = 1;
> >> > >> >  	imx->chip.can_sleep = true;
> >> > >> > +	if (data->pwm_ops->set_polarity) {
> >> > >> > +		dev_dbg(&pdev->dev, "PWM supports output
> >> > >> > inversion\n");
> >> > >> > +		imx->chip.of_xlate =
> >> > >> > of_pwm_xlate_with_flags;
> >> > >> > +		imx->chip.of_pwm_n_cells = 3;
> >> > >> > +	}
> >> > >> >
> >> > >> >  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > >> >  	imx->mmio_base = devm_ioremap_resource(&pdev->dev,
> >> > >> > r); if (IS_ERR(imx->mmio_base))
> >> > >> >  		return PTR_ERR(imx->mmio_base);
> >> > >> >
> >> > >> > -	data = of_id->data;
> >> > >> >  	imx->config = data->config;
> >> > >> >  	imx->set_enable = data->set_enable;
> >> > >>
> >> > >
> >> > > Best regards,
> >> > >
> >> > > Łukasz Majewski
> >>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Boris Brezillon]

On Fri, 7 Oct 2016 20:41:26 +0530
Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> wrote:

> From: Lothar Wassmann <LW@KARO-electronics.de>
> 
> The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output
> polarity. This patch adds support to utilize this feature where available.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
>  drivers/pwm/pwm-imx.c                             | 51 +++++++++++++++++++++--
>  2 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> index e00c2e9..c61bdf8 100644
> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> @@ -6,8 +6,8 @@ Required properties:
>    - "fsl,imx1-pwm" for PWM compatible with the one integrated on i.MX1
>    - "fsl,imx27-pwm" for PWM compatible with the one integrated on i.MX27
>  - reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
> -  the cells format.
> +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See pwm.txt
> +  in this directory for a description of the cells format.
>  - clocks : Clock specifiers for both ipg and per clocks.
>  - clock-names : Clock names should include both "ipg" and "per"
>  See the clock consumer binding,
> @@ -17,7 +17,7 @@ See the clock consumer binding,
>  Example:
>  
>  pwm1: pwm@53fb4000 {
> -	#pwm-cells = <2>;
> +	#pwm-cells = <3>;
>  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
>  	reg = <0x53fb4000 0x4000>;
>  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..c37d223 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -38,6 +38,7 @@
>  #define MX3_PWMCR_DOZEEN		(1 << 24)
>  #define MX3_PWMCR_WAITEN		(1 << 23)
>  #define MX3_PWMCR_DBGEN			(1 << 22)
> +#define MX3_PWMCR_POUTC			(1 << 18)
>  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
>  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
>  #define MX3_PWMCR_SWR			(1 << 3)
> @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  	if (enable)
>  		cr |= MX3_PWMCR_EN;
>  
> +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> +		cr |= MX3_PWMCR_POUTC;
> +
>  	writel(cr, imx->mmio_base + MX3_PWMCR);
>  
>  	return 0;
> @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	clk_disable_unprepare(imx->clk_per);
>  }
>  
> -static struct pwm_ops imx_pwm_ops = {
> +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				enum pwm_polarity polarity)
> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	u32 val;
> +
> +	if (polarity == pwm->args.polarity)
> +		return 0;
> +
> +	val = readl(imx->mmio_base + MX3_PWMCR);
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		val |= MX3_PWMCR_POUTC;
> +	else
> +		val &= ~MX3_PWMCR_POUTC;
> +
> +	writel(val, imx->mmio_base + MX3_PWMCR);
> +
> +	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> +		polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
> +
> +	return 0;
> +}

I'll read the discussion Lukasz and Stefan had before reviewing this
part ;-).

> +
> +static struct pwm_ops imx_pwm_ops_v1 = {
>  	.enable = imx_pwm_enable,
>  	.disable = imx_pwm_disable,
>  	.config = imx_pwm_config,
>  	.owner = THIS_MODULE,
>  };
>  
> +static struct pwm_ops imx_pwm_ops_v2 = {
> +	.enable = imx_pwm_enable,
> +	.disable = imx_pwm_disable,
> +	.set_polarity = imx_pwm_set_polarity,
> +	.config = imx_pwm_config,
> +	.owner = THIS_MODULE,
> +};
> +
>  struct imx_pwm_data {
>  	int (*config)(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns);
>  	void (*set_enable)(struct pwm_chip *chip, bool enable);
> +	struct pwm_ops *pwm_ops;

Probably better to make that consistent: drop the ->config() and
->set_enable() hooks, and make both v1 and v2 define their own pwm_ops.
If you have common logic that is shared between v1 and v2, you can just
create helper functions implementing this common logic and call them
from your version-specific implementation.

>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v1 = {
>  	.config = imx_pwm_config_v1,
>  	.set_enable = imx_pwm_set_enable_v1,
> +	.pwm_ops = &imx_pwm_ops_v1,
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v2 = {
>  	.config = imx_pwm_config_v2,
>  	.set_enable = imx_pwm_set_enable_v2,
> +	.pwm_ops = &imx_pwm_ops_v2,
>  };
>  
>  static const struct of_device_id imx_pwm_dt_ids[] = {
> @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  	if (!of_id)
>  		return -ENODEV;
>  
> +	data = of_id->data;
> +
>  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
>  	if (imx == NULL)
>  		return -ENOMEM;
> @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  		return PTR_ERR(imx->clk_ipg);
>  	}
>  
> -	imx->chip.ops = &imx_pwm_ops;
> +	imx->chip.ops = data->pwm_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.base = -1;
>  	imx->chip.npwm = 1;
>  	imx->chip.can_sleep = true;
> +	if (data->pwm_ops->set_polarity) {
> +		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
> +		imx->chip.of_xlate = of_pwm_xlate_with_flags;
> +		imx->chip.of_pwm_n_cells = 3;
> +	}

Not that I really care, but you are breaking the DT ABI here. One
solution to avoid that would be to retrieve #pwm-cells here and
decide which ->of_xlate should be used based on this information.

>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
>  	if (IS_ERR(imx->mmio_base))
>  		return PTR_ERR(imx->mmio_base);
>  
> -	data = of_id->data;
>  	imx->config = data->config;
>  	imx->set_enable = data->set_enable;
>  

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Boris Brezillon]

Hi Lukasz,

On Fri, 21 Oct 2016 23:49:39 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> Hi Stefan,
> 
> > On 2016-10-20 01:30, Lukasz Majewski wrote:  
> > > Hi Stefan,
> > >   
> > >> Hi Stefan,
> > >>  
> > >> > On 2016-10-12 15:15, Lukasz Majewski wrote:  
> > >> > > Hi Stefan,
> > >> > >  
> > >> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:  
> > >> > >> > From: Lothar Wassmann <LW@KARO-electronics.de>
> > >> > >> >
> > >> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> > >> > >> > configurable output polarity. This patch adds support to
> > >> > >> > utilize this feature where available.
> > >> > >> >
> > >> > >> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > >> > >> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > >> > >> > Signed-off-by: Bhuvanchandra DV
> > >> > >> > <bhuvanchandra.dv@toradex.com> Acked-by: Shawn Guo
> > >> > >> > <shawn.guo@linaro.org> Reviewed-by: Sascha Hauer
> > >> > >> > <s.hauer@pengutronix.de> ---
> > >> > >> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
> > >> > >> >  drivers/pwm/pwm-imx.c                             | 51
> > >> > >> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> > >> > >> > deletions(-)
> > >> > >> >
> > >> > >> > diff --git
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> > >> > >> > e00c2e9..c61bdf8 100644 ---
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8
> > >> > >> > +6,8 @@ Required properties:
> > >> > >> >    - "fsl,imx1-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX1
> > >> > >> >    - "fsl,imx27-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX27
> > >> > >> >  - reg: physical base address and length of the controller's
> > >> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> > >> > >> > directory for a description of
> > >> > >> > -  the cells format.
> > >> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs.
> > >> > >> > See pwm.txt
> > >> > >> > +  in this directory for a description of the cells format.
> > >> > >> >  - clocks : Clock specifiers for both ipg and per clocks.
> > >> > >> >  - clock-names : Clock names should include both "ipg" and
> > >> > >> > "per" See the clock consumer binding,
> > >> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > >> > >> >  Example:
> > >> > >> >
> > >> > >> >  pwm1: pwm@53fb4000 {
> > >> > >> > -	#pwm-cells = <2>;
> > >> > >> > +	#pwm-cells = <3>;
> > >> > >> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > >> > >> >  	reg = <0x53fb4000 0x4000>;
> > >> > >> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > >> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > >> > >> > index d600fd5..c37d223 100644
> > >> > >> > --- a/drivers/pwm/pwm-imx.c
> > >> > >> > +++ b/drivers/pwm/pwm-imx.c
> > >> > >> > @@ -38,6 +38,7 @@
> > >> > >> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
> > >> > >> >  #define MX3_PWMCR_WAITEN		(1 << 23)
> > >> > >> >  #define MX3_PWMCR_DBGEN			(1 << 22)
> > >> > >> > +#define MX3_PWMCR_POUTC			(1 << 18)
> > >> > >> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> > >> > >> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> > >> > >> >  #define MX3_PWMCR_SWR			(1 << 3)
> > >> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> > >> > >> > pwm_chip *chip, if (enable)
> > >> > >> >  		cr |= MX3_PWMCR_EN;
> > >> > >> >
> > >> > >> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > >> > >> > +		cr |= MX3_PWMCR_POUTC;
> > >> > >> > +  
> > >> > >>
> > >> > >> This seems wrong to me, the config callback is meant for
> > >> > >> period/duty cycle only.  
> > > 
> > > Unfortunately, it also resets the PWM IP block and setups it again
> > > (by writing to PWMCR register).

Well, this is an implementation problem. If you were reading the
MX3_PWMCR register and masking it with MX3_PWMCR_POUTC to keep the
value set by ->set_polarity() that wouldn't be a problem.

> > > In that function we setup for
> > > example MX3_PWMCR_DOZEEN
> > > and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?

I think there's no real problem in setting the polarity again in
->config(), it's just that it feels weird to have it set twice (in
->set_polarity() and in ->config()).

> > > 
> > > 
> > > I've double checked the backlight and pwm code flow.
> > > 
> > > Please find following snippet:
> > > 
> > > [    0.135545] ######### imx_pwm_probe
> > > [    0.135581] PWM supports output inversion
> > > [    0.136864] ######### pwm_backlight_probe
> > > [    0.136913] backlight supply power not found, using dummy
> > > regulator [    0.136984] ######### imx_pwm_set_polarity 1
> > > [    0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> > > 0x40000 0xf08f8000
> > > [    0.137005] #########0 imx_pwm_config_v2 cr: 0x40000 
> > > [    0.137683] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> > > [    0.137693] #########2 imx_pwm_config_v2 cr: 0x1c20050
> > > [    0.137702] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> > > [    0.137711] @@@@@@@@@@ pwm_apply_state  
> 
> Maybe a bit more logs:
> 
> [    0.135451] ######### imx_pwm_probe
> [    0.135488] PWM supports output inversion
> [    0.136777] ######### pwm_backlight_probe
> [    0.136826] backlight supply power not found, using dummy regulator
> [    0.136893] ********* pwm_apply_state state->enabled: 0
> [    0.136902] ######### imx_pwm_set_polarity 1
> [    0.136913] imx_pwm_set_polarity: polarity set to inverted cr: 0x40000 0xf08f8000
> [    0.136923] #########0 imx_pwm_config_v2 cr: 0x40000
> [    0.137692] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [    0.137701] #########2 imx_pwm_config_v2 cr: 0x1c20050
> [    0.137710] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [    0.137720] @@@@@@@@@@ pwm_apply_state
> [    0.137856] ********* pwm_apply_state state->enabled: 0
> [    0.137869] #########0 imx_pwm_config_v2 cr: 0x1c20050
> [    0.138904] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [    0.138913] #########2 imx_pwm_config_v2 cr: 0x1c20050
> [    0.138921] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [    0.138928] @@@@@@@@@@ pwm_apply_state
> [    0.138940] ********* pwm_apply_state state->enabled: 1
> 					 ^^^^^^^^^^^^^^^^^^ this is called from
> 			pwm_backlight_power_on() from pwm_bl probe function
> 
> The problem here is not the lack of ->apply() callback, but the requirement to
> perform software reset on the pwm_v2 fifo when the pwm_v2 is NOT enabled (state->enabled: 0).

Not sure what a PWM fifo is, or which software reset you are talking
about. If you're talking about the disable()+set_polarity()+enable()
dance, then, this is required by some PWM controllers which do not
support changing the PWM polarity while the PWM is running.

> 
> As fair as I can see the pwm_state has following members: period, duty cycle, polarity and enabled.
> I'm fine to implement ->apply() callback, which would change above values.
> 
> However, there is a problem with ->config() (imx_pwm_config_v2 @ pwm-imx.c) and imx pwm_v2 software
> FIFO reset.

->config() will not be used if you implement ->apply(), and ->apply()
should take care of all the nasty details that should be taken care of
on your PWM controller.

Say for example that you need to disable the PWM before changing the
polarity, then your ->apply() function should check if the PWM is
enabled, if that's the case it should disable it, set the new polarity,
possibly re-apply the period and duty config and finally re-enable the
PWM.

> We can set polarity in any other kernel subsystem, which uses PWM (backlight in this example) and 
> then this setting would disappear when we call pwm_apply_state with state->enabled = 0 (as presented
> in the log). This imposes setting polarity at ->config when we enable the PWM (as this patch does).

That's really a driver problem, not a PWM user or PWM core problem. If
you have to take extra precautions when enabling the PWM (like
re-applying the polarity), then you can do that in your ->apply()
implementation.

> 
> 
> 
> > > 
> > > Here the pwm_backlight_probe calls set_polarity callback available
> > > in pwm - the polarity is set (the 0x40000 value).
> > > 
> > > The above operation is performed in pwm_apply_state (@
> > > drivers/pwm/core.c). In the same function, latter we call the
> > > pwm->chip->ops->config(), which is the pointer to config_v2.
> > > Since the PWM is not yet enabled, this function performs SW reset
> > > and PWM inversion setting is cleared.  
> > 
> > That function should not do that.   
> 
> I do agree that it shouldn't. Correct me if I'm wrong, but it seems like an 
> PWM HW requirement to perform the reset.
> 
> >It was probably already problematic
> > in the old times, it is definitely now with the atomic PWM stuff.  
> 
> The "atomic"[*] code (with ->apply() provided) will not solve this issue.

Of course it will solve the problem, because in ->apply() you're passed
all the information you need, and if you have to re-apply the polarity
setting, you can (which was not the case with the old pwm_ops
interface).

> 
> >   
> > > 
> > > Possible solutions:
> > > 
> > > 1. Leave the original patch from Bhuvanchandra as it was (I'm for
> > > this option)  
> > 
> > That really seems like a hack to me, and makes transition to the
> > atomic PWM API more complex.  
> 
> Could you be more specific here? 
> 
> As I mentioned before, the problem is not with the lack of
> "atomic" API.

And I really think it is.

> 
> For me the problem is with other subsystems (like pwm backlight)
> which do require polarity inversion to work properly.

Nope, it's just that, with the old pwm_ops interface, your driver was
not able to apply things in the right order. With the ->apply() it can.

> 
> > 
> > If we can't make it happen properly in the current state of affairs,  
> 
> We can set polarity properly with this patch. The only thing which needs fixing
> and another revision of the patch) is the removal of condition:
> 
> -       if (polarity == pwm->args.polarity)
> -               return 0;
> 
> at imx_pwm_set_polarity() function.

Yes, you can probably add even more hacks, but as Stefan said, it's
probably better to switch to the atomic interface, because by doing you
would greatly simplify the logic.

> 
> > we probably should first move to the atomic API.
> > 
> > It really should not be that hard, since we already do almost
> > everything in one function (imx_pwm_config_v2). We probably can
> > almost just assign that function to the new apply function pointer
> > and read information from the new state struct.  
> 
> We do need to configure polarity according to DT value during the pwm
> config_v2 configuration after PWM IP software reset.

And again, forget about ->config(), it should not be called if you
implement ->apply() (actually, you should even drop it completely). So
really, you should try implementing ->apply() and see what happens.

> 
> > 
> > There are examples for instance here:
> > https://patchwork.kernel.org/patch/7228221/  
> 
> The atomicity[*]:
> 
> - It seems to me that this "atomicity" is just embracing all the ->enable, ->disable
> and ->set_polarity in one -> apply callback. Is this just conceptual clean up or is
> there any other reason for it (I'm just curious) ?

No, it's not just a conceptual cleanup. Some hardware are capable of
updating the PWM config atomically (which means they will update the
config at the end of the current period cycle), and by doing that we
also prevent any glitches on the PWM signal.
With the old API, this was simply impossible to do, because the
enable/disable, set_polarity and config calls were separated. With the
atomic API, we can.

Now, let's say your controller is not guaranteeing atomicity. It's
still interesting to implement the ->apply() function, because this way
you can handle all those complex sequences that are required on some HW.

To sum-up, I really think you should listen to Stefan and try to
implement the atomic hook (AKA ->apply()).

Regards,

Boris

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Boris Brezillon]

On Fri, 21 Oct 2016 23:49:39 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> Hi Stefan,
> 
> > On 2016-10-20 01:30, Lukasz Majewski wrote:  
> > > Hi Stefan,
> > >   
> > >> Hi Stefan,
> > >>  
> > >> > On 2016-10-12 15:15, Lukasz Majewski wrote:  
> > >> > > Hi Stefan,
> > >> > >  
> > >> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:  
> > >> > >> > From: Lothar Wassmann <LW@KARO-electronics.de>
> > >> > >> >
> > >> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> > >> > >> > configurable output polarity. This patch adds support to
> > >> > >> > utilize this feature where available.
> > >> > >> >
> > >> > >> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > >> > >> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > >> > >> > Signed-off-by: Bhuvanchandra DV
> > >> > >> > <bhuvanchandra.dv@toradex.com> Acked-by: Shawn Guo
> > >> > >> > <shawn.guo@linaro.org> Reviewed-by: Sascha Hauer
> > >> > >> > <s.hauer@pengutronix.de> ---
> > >> > >> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
> > >> > >> >  drivers/pwm/pwm-imx.c                             | 51
> > >> > >> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> > >> > >> > deletions(-)
> > >> > >> >
> > >> > >> > diff --git
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> > >> > >> > e00c2e9..c61bdf8 100644 ---
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8
> > >> > >> > +6,8 @@ Required properties:
> > >> > >> >    - "fsl,imx1-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX1
> > >> > >> >    - "fsl,imx27-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX27
> > >> > >> >  - reg: physical base address and length of the controller's
> > >> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> > >> > >> > directory for a description of
> > >> > >> > -  the cells format.
> > >> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs.
> > >> > >> > See pwm.txt
> > >> > >> > +  in this directory for a description of the cells format.
> > >> > >> >  - clocks : Clock specifiers for both ipg and per clocks.
> > >> > >> >  - clock-names : Clock names should include both "ipg" and
> > >> > >> > "per" See the clock consumer binding,
> > >> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > >> > >> >  Example:
> > >> > >> >
> > >> > >> >  pwm1: pwm@53fb4000 {
> > >> > >> > -	#pwm-cells = <2>;
> > >> > >> > +	#pwm-cells = <3>;
> > >> > >> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > >> > >> >  	reg = <0x53fb4000 0x4000>;
> > >> > >> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > >> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > >> > >> > index d600fd5..c37d223 100644
> > >> > >> > --- a/drivers/pwm/pwm-imx.c
> > >> > >> > +++ b/drivers/pwm/pwm-imx.c
> > >> > >> > @@ -38,6 +38,7 @@
> > >> > >> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
> > >> > >> >  #define MX3_PWMCR_WAITEN		(1 << 23)
> > >> > >> >  #define MX3_PWMCR_DBGEN			(1 << 22)
> > >> > >> > +#define MX3_PWMCR_POUTC			(1 << 18)
> > >> > >> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> > >> > >> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> > >> > >> >  #define MX3_PWMCR_SWR			(1 << 3)
> > >> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> > >> > >> > pwm_chip *chip, if (enable)
> > >> > >> >  		cr |= MX3_PWMCR_EN;
> > >> > >> >
> > >> > >> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > >> > >> > +		cr |= MX3_PWMCR_POUTC;
> > >> > >> > +  
> > >> > >>
> > >> > >> This seems wrong to me, the config callback is meant for
> > >> > >> period/duty cycle only.  
> > > 
> > > Unfortunately, it also resets the PWM IP block and setups it again
> > > (by writing to PWMCR register). In that function we setup for
> > > example MX3_PWMCR_DOZEEN
> > > and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
> > > 
> > > 
> > > I've double checked the backlight and pwm code flow.
> > > 
> > > Please find following snippet:
> > > 
> > > [    0.135545] ######### imx_pwm_probe
> > > [    0.135581] PWM supports output inversion
> > > [    0.136864] ######### pwm_backlight_probe
> > > [    0.136913] backlight supply power not found, using dummy
> > > regulator [    0.136984] ######### imx_pwm_set_polarity 1
> > > [    0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> > > 0x40000 0xf08f8000
> > > [    0.137005] #########0 imx_pwm_config_v2 cr: 0x40000 
> > > [    0.137683] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> > > [    0.137693] #########2 imx_pwm_config_v2 cr: 0x1c20050
> > > [    0.137702] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> > > [    0.137711] @@@@@@@@@@ pwm_apply_state  
> 
> Maybe a bit more logs:
> 
> [    0.135451] ######### imx_pwm_probe
> [    0.135488] PWM supports output inversion
> [    0.136777] ######### pwm_backlight_probe
> [    0.136826] backlight supply power not found, using dummy regulator
> [    0.136893] ********* pwm_apply_state state->enabled: 0
> [    0.136902] ######### imx_pwm_set_polarity 1
> [    0.136913] imx_pwm_set_polarity: polarity set to inverted cr: 0x40000 0xf08f8000
> [    0.136923] #########0 imx_pwm_config_v2 cr: 0x40000
> [    0.137692] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [    0.137701] #########2 imx_pwm_config_v2 cr: 0x1c20050
> [    0.137710] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [    0.137720] @@@@@@@@@@ pwm_apply_state
> [    0.137856] ********* pwm_apply_state state->enabled: 0
> [    0.137869] #########0 imx_pwm_config_v2 cr: 0x1c20050
> [    0.138904] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [    0.138913] #########2 imx_pwm_config_v2 cr: 0x1c20050
> [    0.138921] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [    0.138928] @@@@@@@@@@ pwm_apply_state
> [    0.138940] ********* pwm_apply_state state->enabled: 1
> 					 ^^^^^^^^^^^^^^^^^^ this is called from
> 			pwm_backlight_power_on() from pwm_bl probe function
> 
> The problem here is not the lack of ->apply() callback, but the requirement to
> perform software reset on the pwm_v2 fifo when the pwm_v2 is NOT enabled (state->enabled: 0).
> 
> As fair as I can see the pwm_state has following members: period, duty cycle, polarity and enabled.
> I'm fine to implement ->apply() callback, which would change above values.
> 
> However, there is a problem with ->config() (imx_pwm_config_v2 @ pwm-imx.c) and imx pwm_v2 software
> FIFO reset.
> We can set polarity in any other kernel subsystem, which uses PWM (backlight in this example) and 
> then this setting would disappear when we call pwm_apply_state with state->enabled = 0 (as presented
> in the log). This imposes setting polarity at ->config when we enable the PWM (as this patch does).
> 
> 
> 
> > > 
> > > Here the pwm_backlight_probe calls set_polarity callback available
> > > in pwm - the polarity is set (the 0x40000 value).
> > > 
> > > The above operation is performed in pwm_apply_state (@
> > > drivers/pwm/core.c). In the same function, latter we call the
> > > pwm->chip->ops->config(), which is the pointer to config_v2.
> > > Since the PWM is not yet enabled, this function performs SW reset
> > > and PWM inversion setting is cleared.  
> > 
> > That function should not do that.   
> 
> I do agree that it shouldn't. Correct me if I'm wrong, but it seems like an 
> PWM HW requirement to perform the reset.
> 
> >It was probably already problematic
> > in the old times, it is definitely now with the atomic PWM stuff.  
> 
> The "atomic"[*] code (with ->apply() provided) will not solve this issue.
> 
> >   
> > > 
> > > Possible solutions:
> > > 
> > > 1. Leave the original patch from Bhuvanchandra as it was (I'm for
> > > this option)  
> > 
> > That really seems like a hack to me, and makes transition to the
> > atomic PWM API more complex.  
> 
> Could you be more specific here? 
> 
> As I mentioned before, the problem is not with the lack of
> "atomic" API.

Below is a quick and dirty I made on top of this patch to show you how
atomic update can be implemented in this driver. It's not tested, and
probably not working, but it should give you a better idea of what is
expected.

--->8---
>From f4dc9a368a951b619a9c67791cb9515ca70da0ee Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Sat, 22 Oct 2016 13:55:19 +0200
Subject: [PATCH] pwm: imx: switch to the atomic interface and related cleanup

This is just a crappy commit to show you how atomic update can be
implemented. Please do not submit the changes as is.

This commit is based on https://lkml.org/lkml/2016/10/7/454.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-imx.c | 262 +++++++++++++++++++++++---------------------------
 1 file changed, 119 insertions(+), 143 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index c37d22371848..c6d55f92dc40 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -66,8 +66,6 @@ struct imx_chip {
 static int imx_pwm_config_v1(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
-	struct imx_chip *imx = to_imx_chip(chip);
-
 	/*
 	 * The PWM subsystem allows for exact frequencies. However,
 	 * I cannot connect a scope on my device to the PWM line and
@@ -85,81 +83,118 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
 	 * both the prescaler (/1 .. /128) and then by CLKSEL
 	 * (/2 .. /16).
 	 */
+	struct imx_chip *imx = to_imx_chip(chip);
 	u32 max = readl(imx->mmio_base + MX1_PWMP);
 	u32 p = max * duty_ns / period_ns;
+	int ret;
+
+	ret = clk_prepare_enable(imx->clk_ipg);
+	if (ret)
+		return ret;
+
 	writel(max - p, imx->mmio_base + MX1_PWMS);
 
+	clk_disable_unprepare(imx->clk_ipg);
+
 	return 0;
 }
 
-static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
+static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
+	int ret;
 	u32 val;
 
+	ret = clk_prepare_enable(imx->clk_ipg);
+	if (ret)
+		return ret;
+
 	val = readl(imx->mmio_base + MX1_PWMC);
 
-	if (enable)
-		val |= MX1_PWMC_EN;
-	else
-		val &= ~MX1_PWMC_EN;
+	val |= MX1_PWMC_EN;
 
 	writel(val, imx->mmio_base + MX1_PWMC);
+
+	clk_disable_unprepare(imx->clk_per);
+
+	return 0;
 }
 
-static int imx_pwm_config_v2(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
+static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	u32 val;
+
+	val = readl(imx->mmio_base + MX1_PWMC);
+
+	val &= ~MX1_PWMC_EN;
+
+	writel(val, imx->mmio_base + MX1_PWMC);
+
+	clk_disable_unprepare(imx->clk_per);
+}
+
+static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
 	struct device *dev = chip->dev;
-	unsigned long long c;
-	unsigned long period_cycles, duty_cycles, prescale;
 	unsigned int period_ms;
-	bool enable = pwm_is_enabled(pwm);
-	int wait_count = 0, fifoav;
-	u32 cr, sr;
+	int fifoav;
+	u32 sr;
+
+	sr = readl(imx->mmio_base + MX3_PWMSR);
+	fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
+	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
+		period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
+					 NSEC_PER_MSEC);
+		msleep(period_ms);
 
-	/*
-	 * i.MX PWMv2 has a 4-word sample FIFO.
-	 * In order to avoid FIFO overflow issue, we do software reset
-	 * to clear all sample FIFO if the controller is disabled or
-	 * wait for a full PWM cycle to get a relinquished FIFO slot
-	 * when the controller is enabled and the FIFO is fully loaded.
-	 */
-	if (enable) {
 		sr = readl(imx->mmio_base + MX3_PWMSR);
-		fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
-		if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
-			period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
-						 NSEC_PER_MSEC);
-			msleep(period_ms);
-
-			sr = readl(imx->mmio_base + MX3_PWMSR);
-			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
-				dev_warn(dev, "there is no free FIFO slot\n");
-		}
-	} else {
-		writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
-		do {
-			usleep_range(200, 1000);
-			cr = readl(imx->mmio_base + MX3_PWMCR);
-		} while ((cr & MX3_PWMCR_SWR) &&
-			 (wait_count++ < MX3_PWM_SWR_LOOP));
-
-		if (cr & MX3_PWMCR_SWR)
-			dev_warn(dev, "software reset timeout\n");
+		if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
+			dev_warn(dev, "there is no free FIFO slot\n");
 	}
+}
+
+static void imx_pwm_empty_fifo(struct pwm_chip *chip)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	struct device *dev = chip->dev;
+	int wait_count = 0;
+	u32 cr;
+
+	writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
+	do {
+		usleep_range(200, 1000);
+		cr = readl(imx->mmio_base + MX3_PWMCR);
+	} while ((cr & MX3_PWMCR_SWR) &&
+		 (wait_count++ < MX3_PWM_SWR_LOOP));
+
+	if (cr & MX3_PWMCR_SWR)
+		dev_warn(dev, "software reset timeout\n");
+}
+
+static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
+			    struct pwm_state *state)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	unsigned long long c;
+	unsigned long period_cycles, duty_cycles, prescale;
+	struct pwm_state cstate;
+	int ret;
+	u32 cr = 0;
+
+	pwm_get_state(pwm, &cstate);
 
 	c = clk_get_rate(imx->clk_per);
-	c = c * period_ns;
+	c = c * state->period;
 	do_div(c, 1000000000);
 	period_cycles = c;
 
 	prescale = period_cycles / 0x10000 + 1;
 
 	period_cycles /= prescale;
-	c = (unsigned long long)period_cycles * duty_ns;
-	do_div(c, period_ns);
+	c = (unsigned long long)period_cycles * state->duty_cycle;
+	do_div(c, state->period);
 	duty_cycles = c;
 
 	/*
@@ -171,134 +206,72 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	else
 		period_cycles = 0;
 
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
-	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
-	cr = MX3_PWMCR_PRESCALER(prescale) |
-		MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-		MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
-
-	if (enable)
-		cr |= MX3_PWMCR_EN;
-
-	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
-		cr |= MX3_PWMCR_POUTC;
-
-	writel(cr, imx->mmio_base + MX3_PWMCR);
-
-	return 0;
-}
-
-static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	u32 val;
-
-	val = readl(imx->mmio_base + MX3_PWMCR);
-
-	if (enable)
-		val |= MX3_PWMCR_EN;
-	else
-		val &= ~MX3_PWMCR_EN;
-
-	writel(val, imx->mmio_base + MX3_PWMCR);
-}
-
-static int imx_pwm_config(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_ipg);
-	if (ret)
-		return ret;
-
-	ret = imx->config(chip, pwm, duty_ns, period_ns);
-
-	clk_disable_unprepare(imx->clk_ipg);
-
-	return ret;
-}
-
-static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	int ret;
-
 	ret = clk_prepare_enable(imx->clk_per);
 	if (ret)
 		return ret;
 
-	imx->set_enable(chip, true);
-
-	return 0;
-}
+	/* Enable the clock if the PWM is being enabled. */
+	if (state->enabled && !cstate.enabled) {
+		ret = clk_prepare_enable(imx->clk_per);
+		if (ret)
+			return ret;
+	}
 
-static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
+	/*
+	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
+	 * the FIFO if the PWM was disabled and is about to be enabled.
+	 */
+	if (cstate.enabled)
+		imx_pwm_wait_fifo_slot(chip, pwm);
+	else if (state->enabled)
+		imx_pwm_empty_fifo(chip);
 
-	imx->set_enable(chip, false);
+	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
-	clk_disable_unprepare(imx->clk_per);
-}
+	cr |= MX3_PWMCR_PRESCALER(prescale) |
+	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
+	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
 
-static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-				enum pwm_polarity polarity)
-{
-	struct imx_chip *imx = to_imx_chip(chip);
-	u32 val;
+	if (state->enabled)
+		cr |= MX3_PWMCR_EN;
 
-	if (polarity == pwm->args.polarity)
-		return 0;
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		cr |= MX3_PWMCR_POUTC;
 
-	val = readl(imx->mmio_base + MX3_PWMCR);
 
-	if (polarity == PWM_POLARITY_INVERSED)
-		val |= MX3_PWMCR_POUTC;
-	else
-		val &= ~MX3_PWMCR_POUTC;
+	writel(cr, imx->mmio_base + MX3_PWMCR);
 
-	writel(val, imx->mmio_base + MX3_PWMCR);
+	/* Disable the clock if the PWM is being disabled. */
+	if (!state->enabled && cstate.enabled)
+		clk_disable_unprepare(imx->clk_per);
 
-	dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
-		polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
+	clk_disable_unprepare(imx->clk_per);
 
 	return 0;
 }
 
 static struct pwm_ops imx_pwm_ops_v1 = {
-	.enable = imx_pwm_enable,
-	.disable = imx_pwm_disable,
-	.config = imx_pwm_config,
+	.enable = imx_pwm_enable_v1,
+	.disable = imx_pwm_disable_v1,
+	.config = imx_pwm_config_v1,
 	.owner = THIS_MODULE,
 };
 
 static struct pwm_ops imx_pwm_ops_v2 = {
-	.enable = imx_pwm_enable,
-	.disable = imx_pwm_disable,
-	.set_polarity = imx_pwm_set_polarity,
-	.config = imx_pwm_config,
+	.apply = imx_pwm_apply_v2,
 	.owner = THIS_MODULE,
 };
 
 struct imx_pwm_data {
-	int (*config)(struct pwm_chip *chip,
-		struct pwm_device *pwm, int duty_ns, int period_ns);
-	void (*set_enable)(struct pwm_chip *chip, bool enable);
 	struct pwm_ops *pwm_ops;
 };
 
 static struct imx_pwm_data imx_pwm_data_v1 = {
-	.config = imx_pwm_config_v1,
-	.set_enable = imx_pwm_set_enable_v1,
 	.pwm_ops = &imx_pwm_ops_v1,
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
-	.config = imx_pwm_config_v2,
-	.set_enable = imx_pwm_set_enable_v2,
 	.pwm_ops = &imx_pwm_ops_v2,
 };
 
@@ -317,6 +290,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	struct imx_chip *imx;
 	struct resource *r;
 	int ret = 0;
+	u32 cells;
 
 	if (!of_id)
 		return -ENODEV;
@@ -346,7 +320,12 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	imx->chip.base = -1;
 	imx->chip.npwm = 1;
 	imx->chip.can_sleep = true;
-	if (data->pwm_ops->set_polarity) {
+
+	ret = of_property_read_u32(pdev->dev.of_node, "#pwm-cells", &cells);
+	if (ret)
+		return ret;
+
+	if (cells == 3) {
 		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
 		imx->chip.of_xlate = of_pwm_xlate_with_flags;
 		imx->chip.of_pwm_n_cells = 3;
@@ -357,9 +336,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(imx->mmio_base))
 		return PTR_ERR(imx->mmio_base);
 
-	imx->config = data->config;
-	imx->set_enable = data->set_enable;
-
 	ret = pwmchip_add(&imx->chip);
 	if (ret < 0)
 		return ret;
-- 
2.7.4

Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

Hi Boris,

> > 
> > Could you be more specific here? 
> > 
> > As I mentioned before, the problem is not with the lack of
> > "atomic" API.  
> 
> Below is a quick and dirty I made on top of this patch to show you how
> atomic update can be implemented in this driver. 

Thank you for example patch.

I will implement the ->apply() callback and post patches very soon :-).

I had two issues with the ->apply() implementation:

- Do my work on top of this patch (https://lkml.org/lkml/2016/10/7/454
  as you did) to avoid rewriting work already done.

- In the example ->apply() implementation for rockchip
  (https://patchwork.kernel.org/patch/7228221/) the ->config() callback
  was not removed when ->apply() was implemented. I was confused with
  such approach, but as you explained in this mail, the solely ->apply()
  is enough.

> It's not tested, and
> probably not working, but it should give you a better idea of what is
> expected.

Thanks for explanation,

Łukasz Majewski

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]