[PATCHv4 0/3] pwm: imx: support output polarity inversion

[PATCHv4 0/3] pwm: imx: support output polarity inversion

[Lothar Waßmann]

This patch series adds support for polarity inversion to the pwm-imx
driver. The patches have been tested on i.MX6, i.MX53 and with the
ti-ehrpwm.c driver.

Changes wrt. v2:
- make the use of '#pwm-cells = <3>' optional, so that the change does
  not break existing DT blobs as suggested by Arnd Bergmann and Sascha
  Hauer.

Changes wrt. v3:
- implemented the approach suggested by Sascha Hauer 
- don't modify struct pwm_ops in the imx_pwm probe function

Changes wrt. v4:
- rebased onto current linux-next
- dropped cleanup patch which is not necessary any more
- resent with ACK from Shawn Guo and Reviewed-By tag from Sascha Hauer

[PATCHv5 1/3] pwm: print error messages with pr_err() instead of pr_debug()

[Lothar Waßmann]

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>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 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 966497d..130bbea 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -525,13 +525,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;
 	}
-- 
1.7.10.4

[PATCHv5 2/3] pwm: make the PWM_POLARITY flag in DTB optional

[Lothar Waßmann]

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 Wa?mann <LW@KARO-electronics.de>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pwm/core.c            |   18 ++++++------------
 drivers/pwm/pwm-atmel-tcb.c   |    1 -
 drivers/pwm/pwm-atmel.c       |    4 +---
 drivers/pwm/pwm-pxa.c         |    5 ++---
 drivers/pwm/pwm-renesas-tpu.c |    1 -
 drivers/pwm/pwm-samsung.c     |    1 -
 drivers/pwm/pwm-tiecap.c      |    1 -
 drivers/pwm/pwm-tiehrpwm.c    |    1 -
 drivers/pwm/pwm-vt8500.c      |    1 -
 include/linux/pwm.h           |    1 -
 10 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 130bbea..ae1596f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -136,7 +136,7 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (pc->of_pwm_n_cells < 3)
+	if (args->args_count != 3)
 		return ERR_PTR(-EINVAL);
 
 	if (args->args[0] >= pc->npwm)
@@ -162,12 +162,15 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (pc->of_pwm_n_cells < 2)
+	if (args->args_count < 2)
 		return ERR_PTR(-EINVAL);
 
 	if (args->args[0] >= pc->npwm)
 		return ERR_PTR(-EINVAL);
 
+	if (args->args_count > 2)
+		return of_pwm_xlate_with_flags(pc, args);
+
 	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
 	if (IS_ERR(pwm))
 		return pwm;
@@ -182,10 +185,8 @@ static void of_pwmchip_add(struct pwm_chip *chip)
 	if (!chip->dev || !chip->dev->of_node)
 		return;
 
-	if (!chip->of_xlate) {
+	if (!chip->of_xlate)
 		chip->of_xlate = of_pwm_simple_xlate;
-		chip->of_pwm_n_cells = 2;
-	}
 
 	of_node_get(chip->dev->of_node);
 }
@@ -536,13 +537,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;
diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index d56e5b7..8ffa460 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -395,7 +395,6 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 	tcbpwm->chip.dev = &pdev->dev;
 	tcbpwm->chip.ops = &atmel_tcb_pwm_ops;
 	tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags;
-	tcbpwm->chip.of_pwm_n_cells = 3;
 	tcbpwm->chip.base = -1;
 	tcbpwm->chip.npwm = NPWM;
 	tcbpwm->tc = tc;
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index d3c22de..394c54f 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -348,10 +348,8 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 	atmel_pwm->chip.dev = &pdev->dev;
 	atmel_pwm->chip.ops = &atmel_pwm_ops;
 
-	if (pdev->dev.of_node) {
+	if (pdev->dev.of_node)
 		atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
-		atmel_pwm->chip.of_pwm_n_cells = 3;
-	}
 
 	atmel_pwm->chip.base = -1;
 	atmel_pwm->chip.npwm = 4;
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 0b312ec..56560529 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -191,10 +191,9 @@ static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.base = -1;
 	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
-	if (IS_ENABLED(CONFIG_OF)) {
+	if (IS_ENABLED(CONFIG_OF))
 		pwm->chip.of_xlate = pxa_pwm_of_xlate;
-		pwm->chip.of_pwm_n_cells = 1;
-	}
+
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 3b71b42..5271da3 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -419,7 +419,6 @@ static int tpu_probe(struct platform_device *pdev)
 	tpu->chip.dev = &pdev->dev;
 	tpu->chip.ops = &tpu_pwm_ops;
 	tpu->chip.of_xlate = of_pwm_xlate_with_flags;
-	tpu->chip.of_pwm_n_cells = 3;
 	tpu->chip.base = -1;
 	tpu->chip.npwm = TPU_CHANNEL_MAX;
 
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index ba6b650..317d7d1 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -484,7 +484,6 @@ static int pwm_samsung_probe(struct platform_device *pdev)
 			return ret;
 
 		chip->chip.of_xlate = of_pwm_xlate_with_flags;
-		chip->chip.of_pwm_n_cells = 3;
 	} else {
 		if (!pdev->dev.platform_data) {
 			dev_err(&pdev->dev, "no platform data specified\n");
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index 74efbe7..a8e8282 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -227,7 +227,6 @@ static int ecap_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ecap_pwm_ops;
 	pc->chip.of_xlate = of_pwm_xlate_with_flags;
-	pc->chip.of_pwm_n_cells = 3;
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index cb75133..12a8ae4 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -458,7 +458,6 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ehrpwm_pwm_ops;
 	pc->chip.of_xlate = of_pwm_xlate_with_flags;
-	pc->chip.of_pwm_n_cells = 3;
 	pc->chip.base = -1;
 	pc->chip.npwm = NUM_PWM_CHANNEL;
 
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index 652e6b5..e04a3c0 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -217,7 +217,6 @@ static int vt8500_pwm_probe(struct platform_device *pdev)
 	chip->chip.dev = &pdev->dev;
 	chip->chip.ops = &vt8500_pwm_ops;
 	chip->chip.of_xlate = of_pwm_xlate_with_flags;
-	chip->chip.of_pwm_n_cells = 3;
 	chip->chip.base = -1;
 	chip->chip.npwm = VT8500_NR_PWMS;
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..4cf1569 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -174,7 +174,6 @@ struct pwm_chip {
 
 	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
 					    const struct of_phandle_args *args);
-	unsigned int		of_pwm_n_cells;
 	bool			can_sleep;
 };
 
-- 
1.7.10.4

[PATCHv5 3/3] pwm: imx: support output polarity inversion

[Lothar Waßmann]

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>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pwm/pwm-imx.c |   39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index f8b5f10..57aac836 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -179,6 +179,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	if (enable)
 		cr |= MX3_PWMCR_EN;
 
+	if (pwm->polarity == PWM_POLARITY_INVERSED)
+		cr |= MX3_PWMCR_POUTC;
+
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
 	return 0;
@@ -196,6 +199,11 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
 	else
 		val &= ~MX3_PWMCR_EN;
 
+	if (chip->pwms[0].polarity == PWM_POLARITY_INVERSED)
+		val |= MX3_PWMCR_POUTC;
+	else
+		val &= ~MX3_PWMCR_POUTC;
+
 	writel(val, imx->mmio_base + MX3_PWMCR);
 }
 
@@ -239,27 +247,49 @@ 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);
+
+	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[] = {
@@ -281,6 +311,10 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	if (!of_id)
 		return -ENODEV;
 
+	data = of_id->data;
+	if (data->pwm_ops->set_polarity)
+		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
+
 	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
 	if (imx == NULL)
 		return -ENOMEM;
@@ -299,7 +333,7 @@ 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;
@@ -310,7 +344,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	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;
 
-- 
1.7.10.4

[PATCHv5 2/3] pwm: make the PWM_POLARITY flag in DTB optional

[Thierry Reding]

On Tue, Oct 07, 2014 at 03:55:33PM +0200, Lothar Wa?mann wrote:
> 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.

I don't like how this throws out the window the only sanity checking we
have in place for the #pwm-cells property.

As I understand it, the problem that you're trying to solve is one of
backwards-compatibility where existing device trees have #pwm-cells =
<2>, but the driver is extended to support flags as well.

In that case, can we not simply make of_pwm_xlate_with_flags() support
that case transparently? That is, if the driver sets .of_pwm_n_cells to
3, we can still support #pwm-cells = <2> and use the default (no) flags
instead.

Something like the below should do that (compile-tested only).

Thierry
-------------- next part --------------
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 966497d10c6e..89a5e309b0a3 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -136,9 +136,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
+	/* check that 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,10 +153,12 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 
 	pwm_set_period(pwm, args->args[1]);
 
-	if (args->args[2] & PWM_POLARITY_INVERTED)
-		pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
-	else
-		pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+	if (args->args_count > 2) {
+		if (args->args[2] & PWM_POLARITY_INVERTED)
+			pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
+		else
+			pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+	}
 
 	return pwm;
 }
@@ -162,9 +169,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);
 
@@ -536,13 +548,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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141009/da3e1b34/attachment.sig>

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Lothar Waßmann]

This patch series adds support for polarity inversion to the pwm-imx
driver. The patches have been tested on i.MX6, i.MX53 and with the
ti-ehrpwm.c driver.

Changes wrt. v2:
- make the use of '#pwm-cells = <3>' optional, so that the change does
  not break existing DT blobs as suggested by Arnd Bergmann and Sascha
  Hauer.

Changes wrt. v3:
- implemented the approach suggested by Sascha Hauer 
- don't modify struct pwm_ops in the imx_pwm probe function

Changes wrt. v4:
- rebased onto current linux-next
- dropped cleanup patch which is not necessary any more
- resent with ACK from Shawn Guo and Reviewed-By tag from Sascha Hauer

Changes wrt. v5:
- don't remove of_pwm_n_cells to keep sanity checks for #pwm-cells as
  suggested by Thierry Reding

[PATCHv6 1/3] pwm: print error messages with pr_err() instead of pr_debug()

[Lothar Waßmann]

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>
---
 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 966497d..130bbea 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -525,13 +525,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;
 	}
-- 
1.7.10.4

[PATCHv6 2/3] pwm: make the PWM_POLARITY flag in DTB optional

[Lothar Waßmann]

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 Wa?mann <LW@KARO-electronics.de>
---
 drivers/pwm/core.c |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 130bbea..0ff1bb0 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -136,9 +136,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,10 +153,12 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 
 	pwm_set_period(pwm, args->args[1]);
 
-	if (args->args[2] & PWM_POLARITY_INVERTED)
-		pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
-	else
-		pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+	if (args->args_count > 2) {
+		if (args->args[2] & PWM_POLARITY_INVERTED)
+			pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
+		else
+			pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+	}
 
 	return pwm;
 }
@@ -162,9 +169,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);
 
@@ -536,13 +548,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;
-- 
1.7.10.4

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

[Lothar Waßmann]

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>
---
 drivers/pwm/pwm-imx.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index f8b5f10..10e0018 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)
@@ -179,6 +180,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	if (enable)
 		cr |= MX3_PWMCR_EN;
 
+	if (pwm->polarity == PWM_POLARITY_INVERSED)
+		cr |= MX3_PWMCR_POUTC;
+
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
 	return 0;
@@ -196,6 +200,11 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
 	else
 		val &= ~MX3_PWMCR_EN;
 
+	if (chip->pwms[0].polarity == PWM_POLARITY_INVERSED)
+		val |= MX3_PWMCR_POUTC;
+	else
+		val &= ~MX3_PWMCR_POUTC;
+
 	writel(val, imx->mmio_base + MX3_PWMCR);
 }
 
@@ -239,27 +248,49 @@ 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);
+
+	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[] = {
@@ -281,6 +312,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;
@@ -299,18 +332,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;
 
-- 
1.7.10.4

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Stefan Agner]

Hi Lothar,

On 2014-10-10 07:22, Lothar Wa?mann wrote:
> This patch series adds support for polarity inversion to the pwm-imx
> driver. The patches have been tested on i.MX6, i.MX53 and with the
> ti-ehrpwm.c driver.

Do you know what prevented this patchset from getting merged?

We are looking for Polarity support in PWM for too, this is especially
useful for backlight control.

--
Stefan

> 
> Changes wrt. v2:
> - make the use of '#pwm-cells = <3>' optional, so that the change does
>   not break existing DT blobs as suggested by Arnd Bergmann and Sascha
>   Hauer.
> 
> Changes wrt. v3:
> - implemented the approach suggested by Sascha Hauer 
> - don't modify struct pwm_ops in the imx_pwm probe function
> 
> Changes wrt. v4:
> - rebased onto current linux-next
> - dropped cleanup patch which is not necessary any more
> - resent with ACK from Shawn Guo and Reviewed-By tag from Sascha Hauer
> 
> Changes wrt. v5:
> - don't remove of_pwm_n_cells to keep sanity checks for #pwm-cells as
>   suggested by Thierry Reding
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Lothar Waßmann]

Hi,

On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote:
> On 2014-10-10 07:22, Lothar Wa?mann wrote:
> > This patch series adds support for polarity inversion to the pwm-imx
> > driver. The patches have been tested on i.MX6, i.MX53 and with the
> > ti-ehrpwm.c driver.
> 
> Do you know what prevented this patchset from getting merged?
> 
No idea.

> We are looking for Polarity support in PWM for too, this is especially
> useful for backlight control.
> 
Actually the PWM driver may be the wrong place to achieve this. When
the backlight driver sets the brightness to 0 to switch the backlight
off, it will disable the PWM. This will make the PWM pin go LOW and
thus turn the backlight to full brightness rather than off (unless there
is an additional GPIO that controls a backlight enable pin on the LCD).


Lothar Wa?mann

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Alexandre Belloni]

On 09/09/2016 at 09:18:57 +0200, Lothar Wa?mann wrote :
> Hi,
> 
> On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote:
> > On 2014-10-10 07:22, Lothar Wa?mann wrote:
> > > This patch series adds support for polarity inversion to the pwm-imx
> > > driver. The patches have been tested on i.MX6, i.MX53 and with the
> > > ti-ehrpwm.c driver.
> > 
> > Do you know what prevented this patchset from getting merged?
> > 
> No idea.
> 
> > We are looking for Polarity support in PWM for too, this is especially
> > useful for backlight control.
> > 
> Actually the PWM driver may be the wrong place to achieve this. When
> the backlight driver sets the brightness to 0 to switch the backlight
> off, it will disable the PWM. This will make the PWM pin go LOW and
> thus turn the backlight to full brightness rather than off (unless there
> is an additional GPIO that controls a backlight enable pin on the LCD).
> 

Isn't a properly designed PWM putting a high level on its pin when
disabled and configured with inversed polarity ?
If the HW is capable of it, the driver should be fixed.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Vladimir Zapolskiy]

Hi Lothar,

On 09/09/2016 10:18 AM, Lothar Wa?mann wrote:
> Hi,
>
> On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote:
>> On 2014-10-10 07:22, Lothar Wa?mann wrote:
>>> This patch series adds support for polarity inversion to the pwm-imx
>>> driver. The patches have been tested on i.MX6, i.MX53 and with the
>>> ti-ehrpwm.c driver.
>>
>> Do you know what prevented this patchset from getting merged?
>>
> No idea.
>
>> We are looking for Polarity support in PWM for too, this is especially
>> useful for backlight control.
>>
> Actually the PWM driver may be the wrong place to achieve this. When
> the backlight driver sets the brightness to 0 to switch the backlight
> off, it will disable the PWM. This will make the PWM pin go LOW and
> thus turn the backlight to full brightness rather than off (unless there
> is an additional GPIO that controls a backlight enable pin on the LCD).
>

I've just realized that I had submitted practically the same change
(excluding iMX specifics) and about the same time in October 2014, but
my v1 is one and a half hours later than yours preceding v6 :)

Since I've subscribed to the linux-pwm right before sending my changes,
I don't have your changes in my mailbox. Would you mind to review my
v3 "pwm: support backward compatibility of DTB extending PWM args":

   http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/303833.html

then incorporate anything you find useful into your series and resend
v7? Or just resend the rebased v6 if nothing is found attracting?

In my turn I'll spend time to review the series and test it on iMX.

--
With best wishes,
Vladimir

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Uwe Kleine-König]

Hello,

On Mon, Sep 12, 2016 at 02:45:53PM +0200, Alexandre Belloni wrote:
> Isn't a properly designed PWM putting a high level on its pin when
> disabled and configured with inversed polarity ?

it's not well defined. When trying several times over the years to
properly define and document it, I didn't manage to agree with Thierry
what is the right thing to define.

IMHO it would be sensible to make it explicitly undefined what happens
when a PMW is disabled. This would simplify drivers from

	pwm_config(mypwm, value, period);
	if (!value)
		pwm_disable(mypwm)
	else
		pwm_enable(led_dat->pwm);

to

	pwm_config(mypwm, value, period);

and let the pwm driver disable it's clock (or whatever) when value is 0
and there are energy saving benefits that don't hurt the expected
behaviour of the pin. So the hardware specific stuff is handled in the
hardware specific driver and usage in pwm-consumers is simplified.
Moreover this also simplifies some pwm drivers because they don't have
to catch in software the cases where the hardware differs from the
expectation[1].
Looking at drivers/leds/leds-pwm.c it doesn't ensure that each
pwm_enable is paired by an pwm_disable (e.g. on .remove). Is this a bug?
With my purposed semantics of .config and .disable this would be much
easier to fix.

Regarding your question: Yeah, maybe all properly designed PWMs behave
like you expect. But reality isn't only about properly designed
hardware, so I wouldn't expect all hardware to behave. The inverse
property might be software emulated and so on pwm_disable the pin might
become 0.

The obvious downside of my suggestion is that this is a change in what
most people expect (because it was "safe" to call pwm_enable before),
but the resulting code is simpler and cleaner.

Today it's a (maybe small) bug, when a pwm consumer calls pwm_config with
value=0 and doesn't disable it afterwards. IMHO that's a bug in the pwm
API that pwm_config with value=0 doesn't imply (the wanted effects of)
pwm_disable.

Best regards
Uwe

[1] This might even be impossible: Consider a PWM that gets 0 (or
high-z) on hw-disable independent of configured duty or inversion. The
driver now sees for an inverted pwm: pwm_config(this, 0, 100);
pwm_disable(this); The driver cannot know if it should continue to drive
the pin at 1, or if the pwm consumer stopped caring about the pwm and
disabling the hardware is OK.

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

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Stefan Agner]

Hi,

Thanks for that insight Uwe!

On 2016-09-12 07:04, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Mon, Sep 12, 2016 at 02:45:53PM +0200, Alexandre Belloni wrote:
>> Isn't a properly designed PWM putting a high level on its pin when
>> disabled and configured with inversed polarity ?
> 
> it's not well defined. When trying several times over the years to
> properly define and document it, I didn't manage to agree with Thierry
> what is the right thing to define.
> 
> IMHO it would be sensible to make it explicitly undefined what happens
> when a PMW is disabled. This would simplify drivers from
> 
> 	pwm_config(mypwm, value, period);
> 	if (!value)
> 		pwm_disable(mypwm)
> 	else
> 		pwm_enable(led_dat->pwm);
> 
> to
> 
> 	pwm_config(mypwm, value, period);
> 
> and let the pwm driver disable it's clock (or whatever) when value is 0
> and there are energy saving benefits that don't hurt the expected
> behaviour of the pin. So the hardware specific stuff is handled in the
> hardware specific driver and usage in pwm-consumers is simplified.
> Moreover this also simplifies some pwm drivers because they don't have
> to catch in software the cases where the hardware differs from the
> expectation[1].

That sounds like a sane definition to me and what I would have expected
from the PWM framework. That the pin is not defined after pwm_disable is
totally understandable. It is usually a case which the board designer
anyway needs to take care of (e.g. what is the state right after power
on? If the designer cares about, he will put a pull-up/down in place).

And it seems also Sascha suggested that:
https://lkml.org/lkml/2013/1/4/139

I did not found where Thierry disagreed to that...?

> Looking at drivers/leds/leds-pwm.c it doesn't ensure that each
> pwm_enable is paired by an pwm_disable (e.g. on .remove). Is this a bug?
> With my purposed semantics of .config and .disable this would be much
> easier to fix.

That looks like a bug to me.

> 
> Regarding your question: Yeah, maybe all properly designed PWMs behave
> like you expect. But reality isn't only about properly designed
> hardware, so I wouldn't expect all hardware to behave. The inverse
> property might be software emulated and so on pwm_disable the pin might
> become 0.
> 
> The obvious downside of my suggestion is that this is a change in what
> most people expect (because it was "safe" to call pwm_enable before),
> but the resulting code is simpler and cleaner.
> 
> Today it's a (maybe small) bug, when a pwm consumer calls pwm_config with
> value=0 and doesn't disable it afterwards. IMHO that's a bug in the pwm
> API that pwm_config with value=0 doesn't imply (the wanted effects of)
> pwm_disable.

I don't quite get what you are saying here. What wanted effects of
pwm_disable would you like to move into pwm_config with value=0?

--
Stefan

> 
> Best regards
> Uwe
> 
> [1] This might even be impossible: Consider a PWM that gets 0 (or
> high-z) on hw-disable independent of configured duty or inversion. The
> driver now sees for an inverted pwm: pwm_config(this, 0, 100);
> pwm_disable(this); The driver cannot know if it should continue to drive
> the pin at 1, or if the pwm consumer stopped caring about the pwm and
> disabling the hardware is OK.

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Uwe Kleine-König]

Hello Stefan,

On Mon, Sep 12, 2016 at 09:51:26AM -0700, Stefan Agner wrote:
> On 2016-09-12 07:04, Uwe Kleine-K?nig wrote:
> > Hello,
> > 
> > On Mon, Sep 12, 2016 at 02:45:53PM +0200, Alexandre Belloni wrote:
> >> Isn't a properly designed PWM putting a high level on its pin when
> >> disabled and configured with inversed polarity ?
> > 
> > it's not well defined. When trying several times over the years to
> > properly define and document it, I didn't manage to agree with Thierry
> > what is the right thing to define.
> > 
> > IMHO it would be sensible to make it explicitly undefined what happens
> > when a PMW is disabled. This would simplify drivers from
> > 
> > 	pwm_config(mypwm, value, period);
> > 	if (!value)
> > 		pwm_disable(mypwm)
> > 	else
> > 		pwm_enable(led_dat->pwm);
> > 
> > to
> > 
> > 	pwm_config(mypwm, value, period);
> > 
> > and let the pwm driver disable it's clock (or whatever) when value is 0
> > and there are energy saving benefits that don't hurt the expected
> > behaviour of the pin. So the hardware specific stuff is handled in the
> > hardware specific driver and usage in pwm-consumers is simplified.
> > Moreover this also simplifies some pwm drivers because they don't have
> > to catch in software the cases where the hardware differs from the
> > expectation[1].
> 
> That sounds like a sane definition to me and what I would have expected
> from the PWM framework. That the pin is not defined after pwm_disable is
> totally understandable. It is usually a case which the board designer
> anyway needs to take care of (e.g. what is the state right after power
> on? If the designer cares about, he will put a pull-up/down in place).
> 
> And it seems also Sascha suggested that:
> https://lkml.org/lkml/2013/1/4/139

actually I talked to Sascha in private before ranting :-)

> I did not found where Thierry disagreed to that...?

Hmm, I used gmane links before, most of them are dead today. :-|
http://www.spinics.net/lists/linux-leds/msg03237.html is one example.

> > Today it's a (maybe small) bug, when a pwm consumer calls pwm_config with
> > value=0 and doesn't disable it afterwards. IMHO that's a bug in the pwm
> > API that pwm_config with value=0 doesn't imply (the wanted effects of)
> > pwm_disable.
> 
> I don't quite get what you are saying here. What wanted effects of
> pwm_disable would you like to move into pwm_config with value=0?

I want that the pwm driver disables its clock on pwm_config(mypwm, 0,
someperiod) such that the consumer doesn't need to call
pwm_disable(mypwm) to save power (assuming it's safe to do so, which
only the pwm provider knows).

Best regards
Uwe

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

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Clemens Gruber]

Hi Uwe,

On Mon, Sep 12, 2016 at 10:00:21PM +0200, Uwe Kleine-K?nig wrote:
> I want that the pwm driver disables its clock on pwm_config(mypwm, 0,
> someperiod) such that the consumer doesn't need to call
> pwm_disable(mypwm) to save power (assuming it's safe to do so, which
> only the pwm provider knows).

I am not sure if this is such a good idea, because there are use cases
where you want to keep the PWM driver enabled the whole time but still
be able to change the duty cycle to 0 for some time without adding
unnecessary delays when changing the duty cycle back to something else.

We have an application where we control fluid valves in a beer
dispensing system through PWMs and these valves are pulsed with
different PWM duty cycles for a short time. In-between the duty cycle
is also 0. For example: Start at 0%, 100ms 90%, 200ms 70%, 300ms 0%,
100ms 90%, and so on..
There it is critical that the change from and to 0 duty cycle is not
delayed by disabling and reenabling the clock.
The oscillator (if there is one) should be up and running, only the duty
cycle should be 0 for a short time.

This would not be possible anymore with your API change, right?

Regards,
Clemens

[PATCHv6 0/3] pwm: imx: support output polarity inversion

[Uwe Kleine-König]

Hello Clemens,

On Mon, Sep 12, 2016 at 11:12:54PM +0200, Clemens Gruber wrote:
> On Mon, Sep 12, 2016 at 10:00:21PM +0200, Uwe Kleine-K?nig wrote:
> > I want that the pwm driver disables its clock on pwm_config(mypwm, 0,
> > someperiod) such that the consumer doesn't need to call
> > pwm_disable(mypwm) to save power (assuming it's safe to do so, which
> > only the pwm provider knows).
> 
> I am not sure if this is such a good idea, because there are use cases
> where you want to keep the PWM driver enabled the whole time but still
> be able to change the duty cycle to 0 for some time without adding
> unnecessary delays when changing the duty cycle back to something else.
> 
> We have an application where we control fluid valves in a beer
> dispensing system through PWMs and these valves are pulsed with
> different PWM duty cycles for a short time. In-between the duty cycle
> is also 0. For example: Start at 0%, 100ms 90%, 200ms 70%, 300ms 0%,
> 100ms 90%, and so on..
> There it is critical that the change from and to 0 duty cycle is not
> delayed by disabling and reenabling the clock.
> The oscillator (if there is one) should be up and running, only the duty
> cycle should be 0 for a short time.

I don't think it is sensible to map this requirement in the pwm api. The
trade-off between performance and power saving is common between all
types of devices and there are other mechanisms to handle this.

Also only some pwms are affected by this because disabling the clock
doesn't introduce a measurable overhead for all of them.

With write(2) there is also no way to define if the hard disk should
spin down after the request is completed. And this wouldn't make sense
for an ssd.

So yes, there would be no way to prohibit stopping the pwm with the pwm
API, but you could implement runtime pm for your device.

Best regards
Uwe

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