[PATCHv2 0/2] pwm: imx: support polarity inversion

[PATCHv2 0/2] pwm: imx: support polarity inversion

[Lothar Waßmann]

The first patch does a minor source cleanup without any functional
change.

Changes wrt. v1:
- make PWM_POLARITY flag optional, so that the driver will work
  with either the existing DT binding or with support for polarity
  inversion.

[PATCHv2 1/2] pwm: imx: indentation cleanup

[Lothar Waßmann]

Consistently use TABs for indentation and the same indentation level.
No functional change.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/pwm/pwm-imx.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index cc47733..3b00a82 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -21,24 +21,24 @@
 
 /* i.MX1 and i.MX21 share the same PWM function block: */
 
-#define MX1_PWMC    0x00   /* PWM Control Register */
-#define MX1_PWMS    0x04   /* PWM Sample Register */
-#define MX1_PWMP    0x08   /* PWM Period Register */
+#define MX1_PWMC			0x00   /* PWM Control Register */
+#define MX1_PWMS			0x04   /* PWM Sample Register */
+#define MX1_PWMP			0x08   /* PWM Period Register */
 
-#define MX1_PWMC_EN		(1 << 4)
+#define MX1_PWMC_EN			(1 << 4)
 
 /* i.MX27, i.MX31, i.MX35 share the same PWM function block: */
 
-#define MX3_PWMCR                 0x00    /* PWM Control Register */
-#define MX3_PWMSAR                0x0C    /* PWM Sample Register */
-#define MX3_PWMPR                 0x10    /* PWM Period Register */
-#define MX3_PWMCR_PRESCALER(x)    (((x - 1) & 0xFFF) << 4)
-#define MX3_PWMCR_DOZEEN                (1 << 24)
-#define MX3_PWMCR_WAITEN                (1 << 23)
+#define MX3_PWMCR			0x00   /* PWM Control Register */
+#define MX3_PWMSAR			0x0C   /* PWM Sample Register */
+#define MX3_PWMPR			0x10   /* PWM Period Register */
+#define MX3_PWMCR_PRESCALER(x)		(((x - 1) & 0xFFF) << 4)
+#define MX3_PWMCR_DOZEEN		(1 << 24)
+#define MX3_PWMCR_WAITEN		(1 << 23)
 #define MX3_PWMCR_DBGEN			(1 << 22)
-#define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
-#define MX3_PWMCR_CLKSRC_IPG      (1 << 16)
-#define MX3_PWMCR_EN              (1 << 0)
+#define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
+#define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
+#define MX3_PWMCR_EN			(1 << 0)
 
 struct imx_chip {
 	struct clk	*clk_per;
-- 
1.7.2.5

[PATCHv2 2/2] pwm: imx: support polarity inversion

[Lothar Waßmann]

The i.MX PWM controller supports inverting the polarity of the PWM
output. Make this feature available in the pxm-imx driver.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 Documentation/devicetree/bindings/pwm/imx-pwm.txt |    5 +-
 drivers/pwm/pwm-imx.c                             |   42 +++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
index b50d7a6d..d0b04b5 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
@@ -3,8 +3,9 @@ Freescale i.MX PWM controller
 Required properties:
 - compatible: should be "fsl,<soc>-pwm"
 - 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: may be 2 for backwards compatibility or 3 to support
+  switching the output polarity. See pwm.txt in this directory for a
+  description of the cells format.
 - interrupts: The interrupt for the pwm controller
 
 Example:
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 3b00a82..05461bb 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -36,6 +36,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_EN			(1 << 0)
@@ -138,6 +139,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	if (test_bit(PWMF_ENABLED, &pwm->flags))
 		cr |= MX3_PWMCR_EN;
 
+	if (pwm->polarity == PWM_POLARITY_INVERSED)
+		cr |= MX3_PWMCR_POUTC;
+
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
 	return 0;
@@ -155,6 +159,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);
 }
 
@@ -198,6 +207,17 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	clk_disable_unprepare(imx->clk_per);
 }
 
+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 = {
 	.enable = imx_pwm_enable,
 	.disable = imx_pwm_disable,
@@ -209,6 +229,7 @@ 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);
+	unsigned output_polarity:1;
 };
 
 static struct imx_pwm_data imx_pwm_data_v1 = {
@@ -219,6 +240,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
 static struct imx_pwm_data imx_pwm_data_v2 = {
 	.config = imx_pwm_config_v2,
 	.set_enable = imx_pwm_set_enable_v2,
+	.output_polarity = 1,
 };
 
 static const struct of_device_id imx_pwm_dt_ids[] = {
@@ -271,6 +293,26 @@ static int imx_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(imx->mmio_base);
 
 	data = of_id->data;
+	if (data->output_polarity) {
+		const struct device_node *np = pdev->dev.of_node;
+		u32 num_cells;
+
+		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
+		ret = of_property_read_u32(np, "#pwm-cells", &num_cells);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "missing property '#pwm-cells'\n");
+			return ret;
+		}
+		if (num_cells == 3) {
+			imx_pwm_ops.set_polarity = imx_pwm_set_polarity;
+			imx->chip.of_xlate = of_pwm_xlate_with_flags;
+		} else if (num_cells != 2) {
+			dev_err(&pdev->dev, "'#pwm-cells' must be <2> or <3>\n");
+			return -EINVAL;
+		}
+		imx->chip.of_pwm_n_cells = num_cells;
+	}
+
 	imx->config = data->config;
 	imx->set_enable = data->set_enable;
 
-- 
1.7.2.5

Re: [PATCHv2 2/2] pwm: imx: support polarity inversion

[Sascha Hauer]

On Thu, Jan 16, 2014 at 09:06:25AM +0100, Lothar Waßmann wrote:
> The i.MX PWM controller supports inverting the polarity of the PWM
> output. Make this feature available in the pxm-imx driver.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  Documentation/devicetree/bindings/pwm/imx-pwm.txt |    5 +-
>  drivers/pwm/pwm-imx.c                             |   42 +++++++++++++++++++++
>  2 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> index b50d7a6d..d0b04b5 100644
> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> @@ -3,8 +3,9 @@ Freescale i.MX PWM controller
>  Required properties:
>  - compatible: should be "fsl,<soc>-pwm"
>  - 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: may be 2 for backwards compatibility or 3 to support
> +  switching the output polarity. See pwm.txt in this directory for a
> +  description of the cells format.
>  - interrupts: The interrupt for the pwm controller
>  
>  Example:
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 3b00a82..05461bb 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -36,6 +36,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_EN			(1 << 0)
> @@ -138,6 +139,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  	if (test_bit(PWMF_ENABLED, &pwm->flags))
>  		cr |= MX3_PWMCR_EN;
>  
> +	if (pwm->polarity == PWM_POLARITY_INVERSED)
> +		cr |= MX3_PWMCR_POUTC;
> +
>  	writel(cr, imx->mmio_base + MX3_PWMCR);
>  
>  	return 0;
> @@ -155,6 +159,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);
>  }
>  
> @@ -198,6 +207,17 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	clk_disable_unprepare(imx->clk_per);
>  }
>  
> +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 = {
>  	.enable = imx_pwm_enable,
>  	.disable = imx_pwm_disable,
> @@ -209,6 +229,7 @@ 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);
> +	unsigned output_polarity:1;
>  };
>  
>  static struct imx_pwm_data imx_pwm_data_v1 = {
> @@ -219,6 +240,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
>  static struct imx_pwm_data imx_pwm_data_v2 = {
>  	.config = imx_pwm_config_v2,
>  	.set_enable = imx_pwm_set_enable_v2,
> +	.output_polarity = 1,
>  };
>  
>  static const struct of_device_id imx_pwm_dt_ids[] = {
> @@ -271,6 +293,26 @@ static int imx_pwm_probe(struct platform_device *pdev)
>  		return PTR_ERR(imx->mmio_base);
>  
>  	data = of_id->data;
> +	if (data->output_polarity) {
> +		const struct device_node *np = pdev->dev.of_node;
> +		u32 num_cells;
> +
> +		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
> +		ret = of_property_read_u32(np, "#pwm-cells", &num_cells);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "missing property '#pwm-cells'\n");
> +			return ret;
> +		}
> +		if (num_cells == 3) {
> +			imx_pwm_ops.set_polarity = imx_pwm_set_polarity;
> +			imx->chip.of_xlate = of_pwm_xlate_with_flags;
> +		} else if (num_cells != 2) {
> +			dev_err(&pdev->dev, "'#pwm-cells' must be <2> or <3>\n");
> +			return -EINVAL;
> +		}
> +		imx->chip.of_pwm_n_cells = num_cells;
> +	}

Can't this be done in the PWM core? Right now the PWM core checks for
of_pwm_n_cells before calling ->of_xlate. IMO this check should be done
in the of_xlate hook. Then of_pwm_simple_xlate and
of_pwm_xlate_with_flags can be merged into something like:

static struct pwm_device *
of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args
*args)
{
	struct pwm_device *pwm;
	int ret;

	if (pc->of_pwm_n_cells < 2)
		return ERR_PTR(-EINVAL);

	if (args->args[0] >= pc->npwm)
		return ERR_PTR(-EINVAL);

	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
	if (IS_ERR(pwm))
		return pwm;

	if (pc->of_pwm_n_cells >= 3) {
		if (args->args[2] & PWM_POLARITY_INVERTED)
			ret = pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
		else
			ret = pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
		if (ret)
			return ret;
	}

	pwm_set_period(pwm, args->args[1]);

	return pwm;
}

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCHv2 2/2] pwm: imx: support polarity inversion

[Lothar Waßmann]

Hi,

Sascha Hauer wrote:
> On Thu, Jan 16, 2014 at 09:06:25AM +0100, Lothar Waßmann wrote:
> > The i.MX PWM controller supports inverting the polarity of the PWM
> > output. Make this feature available in the pxm-imx driver.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |    5 +-
> >  drivers/pwm/pwm-imx.c                             |   42 +++++++++++++++++++++
> >  2 files changed, 45 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > index b50d7a6d..d0b04b5 100644
> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > @@ -3,8 +3,9 @@ Freescale i.MX PWM controller
> >  Required properties:
> >  - compatible: should be "fsl,<soc>-pwm"
> >  - 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: may be 2 for backwards compatibility or 3 to support
> > +  switching the output polarity. See pwm.txt in this directory for a
> > +  description of the cells format.
> >  - interrupts: The interrupt for the pwm controller
> >  
> >  Example:
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index 3b00a82..05461bb 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -36,6 +36,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_EN			(1 << 0)
> > @@ -138,6 +139,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> >  	if (test_bit(PWMF_ENABLED, &pwm->flags))
> >  		cr |= MX3_PWMCR_EN;
> >  
> > +	if (pwm->polarity == PWM_POLARITY_INVERSED)
> > +		cr |= MX3_PWMCR_POUTC;
> > +
> >  	writel(cr, imx->mmio_base + MX3_PWMCR);
> >  
> >  	return 0;
> > @@ -155,6 +159,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);
> >  }
> >  
> > @@ -198,6 +207,17 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  	clk_disable_unprepare(imx->clk_per);
> >  }
> >  
> > +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 = {
> >  	.enable = imx_pwm_enable,
> >  	.disable = imx_pwm_disable,
> > @@ -209,6 +229,7 @@ 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);
> > +	unsigned output_polarity:1;
> >  };
> >  
> >  static struct imx_pwm_data imx_pwm_data_v1 = {
> > @@ -219,6 +240,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> >  static struct imx_pwm_data imx_pwm_data_v2 = {
> >  	.config = imx_pwm_config_v2,
> >  	.set_enable = imx_pwm_set_enable_v2,
> > +	.output_polarity = 1,
> >  };
> >  
> >  static const struct of_device_id imx_pwm_dt_ids[] = {
> > @@ -271,6 +293,26 @@ static int imx_pwm_probe(struct platform_device *pdev)
> >  		return PTR_ERR(imx->mmio_base);
> >  
> >  	data = of_id->data;
> > +	if (data->output_polarity) {
> > +		const struct device_node *np = pdev->dev.of_node;
> > +		u32 num_cells;
> > +
> > +		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
> > +		ret = of_property_read_u32(np, "#pwm-cells", &num_cells);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "missing property '#pwm-cells'\n");
> > +			return ret;
> > +		}
> > +		if (num_cells == 3) {
> > +			imx_pwm_ops.set_polarity = imx_pwm_set_polarity;
> > +			imx->chip.of_xlate = of_pwm_xlate_with_flags;
> > +		} else if (num_cells != 2) {
> > +			dev_err(&pdev->dev, "'#pwm-cells' must be <2> or <3>\n");
> > +			return -EINVAL;
> > +		}
> > +		imx->chip.of_pwm_n_cells = num_cells;
> > +	}
> 
> Can't this be done in the PWM core? Right now the PWM core checks for
> of_pwm_n_cells before calling ->of_xlate. IMO this check should be done
> in the of_xlate hook. Then of_pwm_simple_xlate and
> of_pwm_xlate_with_flags can be merged into something like:
> 
This wouldn't buy much without a material change to of_pwm_get().
The function of_parse_phandle_with_args() called by of_pwm_get()
requires the number of args in the pwms property be greater or equal to
the #pwm-cells property in the pwm node. Thus, the interesting case of
having #pwm-cells = <3> without changing the existing users is
prohibited by of_parse_phandle_with_args().

of_pwm_get() would have to be changed to something like below to allow
this:
struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
{
	struct pwm_device *pwm = NULL;
	struct of_phandle_args args;
	struct pwm_chip *pc;
	int index = 0;
	int err;
	struct property *prop;
	u32 num_cells;
	int i;
	const __be32 *list;

	if (con_id) {
		index = of_property_match_string(np, "pwm-names", con_id);
		if (index < 0)
			return ERR_PTR(index);
	}

	args.np = of_parse_phandle(np, "pwms", index);
	if (!args.np) {
		pr_err("%s(): property 'pwms' not found in '%s'\n",
			__func__, np->full_name);
		return ERR_PTR(-ENOENT);
	}

	err = of_property_read_u32(args.np, "#pwm-cells", &num_cells);
	if (err) {
		pr_err("%s(): could not read property '#pwm-cells' in '%s': %d\n",
			__func__, args.np->full_name, err);
		return ERR_PTR(err);
	}

	prop = of_find_property(np, "pwms", NULL);
	if (WARN_ON(!prop))
		return ERR_PTR(-EINVAL);

	args.args_count = prop->length / sizeof(u32) - 1;
	list = prop->value;
	for (i = 0; i < args.args_count; i++)
		args.args[i] = be32_to_cpup(++list);

	pc = of_node_to_pwmchip(args.np);
	if (IS_ERR(pc)) {
		pr_err("%s(): PWM chip not found\n", __func__);
		pwm = ERR_CAST(pc);
		goto put;
	}
[...]

 

Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCHv2 2/2] pwm: imx: support polarity inversion

[Sascha Hauer]

I thinking more of the following. I haven't tested it, but it has a negative
diffstat, so it must be good ;)

Sascha

----------------------------------------------------------------
Sascha Hauer (2):
      PWM: let of_xlate handlers check args count
      PWM: handle additional flags in of_pwm_simple_xlate

 drivers/pwm/core.c            | 41 +++++++----------------------------------
 drivers/pwm/pwm-atmel-tcb.c   |  2 --
 drivers/pwm/pwm-renesas-tpu.c |  2 --
 drivers/pwm/pwm-samsung.c     |  3 ---
 drivers/pwm/pwm-tiecap.c      |  2 --
 drivers/pwm/pwm-tiehrpwm.c    |  2 --
 drivers/pwm/pwm-vt8500.c      |  2 --
 include/linux/pwm.h           |  3 ---
 8 files changed, 7 insertions(+), 50 deletions(-)

[PATCH 1/2] PWM: let of_xlate handlers check args count

[Sascha Hauer]

of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip,
but it is only ever used by the of_xlate handler itsel. Remove
of_pwm_n_cells from struct pwm_chip and let the handler do the argument
count checking to simplify the code.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pwm/core.c            | 15 +++------------
 drivers/pwm/pwm-atmel-tcb.c   |  1 -
 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 -
 7 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2ca9504..c882051 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,7 +162,7 @@ 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)
@@ -182,10 +182,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 +534,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 f3dcd02..55fabf8 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-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index aff6ba9..0a8adb6 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -434,7 +434,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 b59639e..8d8dced 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -487,7 +487,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 4e5c3d1..4d7a01a 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -229,7 +229,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 a4d8f51..2c2621a 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -460,7 +460,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 323125a..5472051 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -219,7 +219,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;
 
-- 
1.8.5.2

[PATCH 2/2] PWM: handle additional flags in of_pwm_simple_xlate

[Sascha Hauer]

Let of_pwm_simple_xlate behave like of_pwm_xlate_with_flags when
the argument count is 3. This makes of_pwm_xlate_with_flags unncessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pwm/core.c            | 30 ++++++------------------------
 drivers/pwm/pwm-atmel-tcb.c   |  1 -
 drivers/pwm/pwm-renesas-tpu.c |  1 -
 drivers/pwm/pwm-samsung.c     |  2 --
 drivers/pwm/pwm-tiecap.c      |  1 -
 drivers/pwm/pwm-tiehrpwm.c    |  1 -
 drivers/pwm/pwm-vt8500.c      |  1 -
 include/linux/pwm.h           |  3 ---
 8 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c882051..329b1e5 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -131,12 +131,12 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 	return 0;
 }
 
-struct pwm_device *
-of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
+static struct pwm_device *
+of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (args->args_count != 3)
+	if (args->args_count < 2)
 		return ERR_PTR(-EINVAL);
 
 	if (args->args[0] >= pc->npwm)
@@ -148,6 +148,9 @@ 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_count < 3)
+		return 0;
+
 	if (args->args[2] & PWM_POLARITY_INVERTED)
 		pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
 	else
@@ -155,27 +158,6 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 
 	return pwm;
 }
-EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
-
-static struct pwm_device *
-of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
-{
-	struct pwm_device *pwm;
-
-	if (args->args_count != 2)
-		return ERR_PTR(-EINVAL);
-
-	if (args->args[0] >= pc->npwm)
-		return ERR_PTR(-EINVAL);
-
-	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
-	if (IS_ERR(pwm))
-		return pwm;
-
-	pwm_set_period(pwm, args->args[1]);
-
-	return pwm;
-}
 
 static void of_pwmchip_add(struct pwm_chip *chip)
 {
diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 55fabf8..e1e5a20 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -394,7 +394,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.base = -1;
 	tcbpwm->chip.npwm = NPWM;
 	tcbpwm->tc = tc;
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 0a8adb6..82dcab9 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -433,7 +433,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.base = -1;
 	tpu->chip.npwm = TPU_CHANNEL_MAX;
 
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index 8d8dced..465f9ee 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -485,8 +485,6 @@ static int pwm_samsung_probe(struct platform_device *pdev)
 		ret = pwm_samsung_parse_dt(chip);
 		if (ret)
 			return ret;
-
-		chip->chip.of_xlate = of_pwm_xlate_with_flags;
 	} 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 4d7a01a..27cde03 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -228,7 +228,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.base = -1;
 	pc->chip.npwm = 1;
 
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 2c2621a..4e566df 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -459,7 +459,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.base = -1;
 	pc->chip.npwm = NUM_PWM_CHANNEL;
 
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index 5472051..30def61 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -218,7 +218,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.base = -1;
 	chip->chip.npwm = VT8500_NR_PWMS;
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f0feafd..2447d6f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -188,9 +188,6 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
 					 const char *label);
 
-struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
-		const struct of_phandle_args *args);
-
 struct pwm_device *pwm_get(struct device *dev, const char *con_id);
 struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id);
 void pwm_put(struct pwm_device *pwm);
-- 
1.8.5.2

Re: [PATCH 1/2] PWM: let of_xlate handlers check args count

[Lothar Waßmann]

Hi,

Sascha Hauer wrote:
> of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip,
> but it is only ever used by the of_xlate handler itsel. Remove
> of_pwm_n_cells from struct pwm_chip and let the handler do the argument
> count checking to simplify the code.
> 
This still does not make the PWM_POLARITY flag in the pwms node
optional as was the goal because of_parse_phandle_with_args() requires
at least #pwm-cells arguments in the node.

So, with a DT configuration like:
pwm0: pwm@0 {
	#pwm-cells = <3>;
};
backlight {
	pwms = <&pwm0 0 100000>;
};
the driver will bail out at of_parse_phandle_with_args() in
of_pwm_get() with the error message:
"/backlight: arguments longer than property" and never reach your
clever xlate function.

Thus you will still need to replace of_parse_phandle_with_args()
with different code that copies most but not all of the functionality.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH 1/2] PWM: let of_xlate handlers check args count

[Sascha Hauer]

On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Sascha Hauer wrote:
> > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip,
> > but it is only ever used by the of_xlate handler itsel. Remove
> > of_pwm_n_cells from struct pwm_chip and let the handler do the argument
> > count checking to simplify the code.
> > 
> This still does not make the PWM_POLARITY flag in the pwms node
> optional as was the goal because of_parse_phandle_with_args() requires
> at least #pwm-cells arguments in the node.
> 
> So, with a DT configuration like:
> pwm0: pwm@0 {
> 	#pwm-cells = <3>;
> };
> backlight {
> 	pwms = <&pwm0 0 100000>;
> };

We misunderstood each other. My goal was to allow the driver to also
work with old devicetrees which specify #pwm-cells = <2>, not to allow
inconsistent devicetrees like the snippet above.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCHv2 2/2] pwm: imx: support polarity inversion

[Russell King - ARM Linux]

On Thu, Jan 23, 2014 at 08:37:14AM +0100, Lothar Waßmann wrote:
> This wouldn't buy much without a material change to of_pwm_get().
> The function of_parse_phandle_with_args() called by of_pwm_get()
> requires the number of args in the pwms property be greater or equal to
> the #pwm-cells property in the pwm node. Thus, the interesting case of
> having #pwm-cells = <3> without changing the existing users is
> prohibited by of_parse_phandle_with_args().

I really don't think that's a problem we need to be concerned with at
the moment.  What we need is for the kernel to be able to parse files
with #pwm-cells = <2> with the pwms property containing two arguments,
and when they're updated to #pwm-cells = <3> with the pwms property
containing three arguments.

Yes, that means all the board dt files need to be updated at the same
time to include the additional argument, but I don't see that as a big
problem.

What we do need to do is to adjust the PWM parsing code such that it's
possible to use either specification without causing any side effects.

I would test this, but as u-boot is rather fscked at the moment and the
networking has broken on my cubox-i as a result... and it seems that the
u-boot developers have pissed off cubox-i u-boot hackers soo much that
they've dropped u-boot in favour of barebox...

 drivers/pwm/core.c  | 59 +++++++++++++++++++++++++++++------------------------
 include/linux/pwm.h |  2 ++
 2 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2ca95042a0b9..40adbce8ef0c 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -132,14 +132,11 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 }
 
 struct pwm_device *
-of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
+of_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (pc->of_pwm_n_cells < 3)
-		return ERR_PTR(-EINVAL);
-
-	if (args->args[0] >= pc->npwm)
+	if (args->args_count < 2)
 		return ERR_PTR(-EINVAL);
 
 	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
@@ -148,33 +145,45 @@ 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) {
+		int err;
+
+		if (args->args[2] & PWM_POLARITY_INVERTED)
+			err = pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
+		else
+			err = pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+
+		pwm_put(pwm);
+		return ERR_PTR(err);
+	}
 
 	return pwm;
 }
+EXPORT_SYMBOL(of_pwm_xlate);
+
+struct pwm_device *
+of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	if (pc->of_pwm_n_cells < 3)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args_count != pc->of_pwm_n_cells)
+		return ERR_PTR(-EINVAL);
+
+	return of_pwm_xlate(pc, args);
+}
 EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
 
 static struct pwm_device *
 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)
 		return ERR_PTR(-EINVAL);
 
-	if (args->args[0] >= pc->npwm)
+	if (args->args_count != pc->of_pwm_n_cells)
 		return ERR_PTR(-EINVAL);
 
-	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
-	if (IS_ERR(pwm))
-		return pwm;
-
-	pwm_set_period(pwm, args->args[1]);
-
-	return pwm;
+	return of_pwm_xlate(pc, args);
 }
 
 static void of_pwmchip_add(struct pwm_chip *chip)
@@ -536,16 +545,12 @@ 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))
+	if (IS_ERR(pwm)) {
+		pr_debug("%s: of_xlate failed for %s: %d\n", np->full_name,
+			 args.np->full_name, (int)PTR_ERR(pwm));
 		goto put;
+	}
 
 	/*
 	 * If a consumer name was not given, try to look it up from the
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f0feafd184a0..14a823f77c31 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -188,6 +188,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
 					 const char *label);
 
+struct pwm_device *of_pwm_xlate(struct pwm_chip *pc,
+		const struct of_phandle_args *args);
 struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
 		const struct of_phandle_args *args);
 


-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCHv2 2/2] pwm: imx: support polarity inversion

[Russell King - ARM Linux]

On Thu, Jan 23, 2014 at 11:52:03AM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 23, 2014 at 08:37:14AM +0100, Lothar Waßmann wrote:
> > This wouldn't buy much without a material change to of_pwm_get().
> > The function of_parse_phandle_with_args() called by of_pwm_get()
> > requires the number of args in the pwms property be greater or equal to
> > the #pwm-cells property in the pwm node. Thus, the interesting case of
> > having #pwm-cells = <3> without changing the existing users is
> > prohibited by of_parse_phandle_with_args().
> 
> I really don't think that's a problem we need to be concerned with at
> the moment.  What we need is for the kernel to be able to parse files
> with #pwm-cells = <2> with the pwms property containing two arguments,
> and when they're updated to #pwm-cells = <3> with the pwms property
> containing three arguments.
> 
> Yes, that means all the board dt files need to be updated at the same
> time to include the additional argument, but I don't see that as a big
> problem.
> 
> What we do need to do is to adjust the PWM parsing code such that it's
> possible to use either specification without causing any side effects.
> 
> I would test this, but as u-boot is rather fscked at the moment and the
> networking has broken on my cubox-i as a result... and it seems that the
> u-boot developers have pissed off cubox-i u-boot hackers soo much that
> they've dropped u-boot in favour of barebox...

Oh, and another reason... the u-boot video settings are totally and utterly
buggered to the point that it doesn't produce correct timings, and it seems
that u-boot people have zero interest in fixing that, so u-boot mainline is
basically refusing to fix this - another reason to stay away from it.

(1024x768 @ 60Hz produces 70Hz refresh on iMX6Q here - I've seen it produce
51Hz on iMX6S, both of which are far enough out that lots of display devices
will not accept it as a valid signal.)

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCHv2 2/2] pwm: imx: support polarity inversion

[Russell King - ARM Linux]

On Thu, Jan 23, 2014 at 11:52:03AM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 23, 2014 at 08:37:14AM +0100, Lothar Waßmann wrote:
> > This wouldn't buy much without a material change to of_pwm_get().
> > The function of_parse_phandle_with_args() called by of_pwm_get()
> > requires the number of args in the pwms property be greater or equal to
> > the #pwm-cells property in the pwm node. Thus, the interesting case of
> > having #pwm-cells = <3> without changing the existing users is
> > prohibited by of_parse_phandle_with_args().
> 
> I really don't think that's a problem we need to be concerned with at
> the moment.  What we need is for the kernel to be able to parse files
> with #pwm-cells = <2> with the pwms property containing two arguments,
> and when they're updated to #pwm-cells = <3> with the pwms property
> containing three arguments.
> 
> Yes, that means all the board dt files need to be updated at the same
> time to include the additional argument, but I don't see that as a big
> problem.
> 
> What we do need to do is to adjust the PWM parsing code such that it's
> possible to use either specification without causing any side effects.
> 
> I would test this, but as u-boot is rather fscked at the moment and the
> networking has broken on my cubox-i as a result... and it seems that the
> u-boot developers have pissed off cubox-i u-boot hackers soo much that
> they've dropped u-boot in favour of barebox...

Okay, finally confirmed that this works with #pwm-cells = 2.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH 1/2] PWM: let of_xlate handlers check args count

[Russell King - ARM Linux]

On Thu, Jan 23, 2014 at 12:04:44PM +0100, Sascha Hauer wrote:
> On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > Sascha Hauer wrote:
> > > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip,
> > > but it is only ever used by the of_xlate handler itsel. Remove
> > > of_pwm_n_cells from struct pwm_chip and let the handler do the argument
> > > count checking to simplify the code.
> > > 
> > This still does not make the PWM_POLARITY flag in the pwms node
> > optional as was the goal because of_parse_phandle_with_args() requires
> > at least #pwm-cells arguments in the node.
> > 
> > So, with a DT configuration like:
> > pwm0: pwm@0 {
> > 	#pwm-cells = <3>;
> > };
> > backlight {
> > 	pwms = <&pwm0 0 100000>;
> > };
> 
> We misunderstood each other. My goal was to allow the driver to also
> work with old devicetrees which specify #pwm-cells = <2>, not to allow
> inconsistent devicetrees like the snippet above.

In which case, the patch I've posted seems to do that job too... I'm
just about to test out the three-cell version.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH 1/2] PWM: let of_xlate handlers check args count

[Russell King - ARM Linux]

On Thu, Jan 23, 2014 at 04:53:50PM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 23, 2014 at 12:04:44PM +0100, Sascha Hauer wrote:
> > On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Sascha Hauer wrote:
> > > > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip,
> > > > but it is only ever used by the of_xlate handler itsel. Remove
> > > > of_pwm_n_cells from struct pwm_chip and let the handler do the argument
> > > > count checking to simplify the code.
> > > > 
> > > This still does not make the PWM_POLARITY flag in the pwms node
> > > optional as was the goal because of_parse_phandle_with_args() requires
> > > at least #pwm-cells arguments in the node.
> > > 
> > > So, with a DT configuration like:
> > > pwm0: pwm@0 {
> > > 	#pwm-cells = <3>;
> > > };
> > > backlight {
> > > 	pwms = <&pwm0 0 100000>;
> > > };
> > 
> > We misunderstood each other. My goal was to allow the driver to also
> > work with old devicetrees which specify #pwm-cells = <2>, not to allow
> > inconsistent devicetrees like the snippet above.
> 
> In which case, the patch I've posted seems to do that job too... I'm
> just about to test out the three-cell version.

Okay, this works, but there's a problem with pwm-leds.

When the duty cycle is set to zero (when you set the brightness to zero)
pwm-leds decides to disable the PWM after configuring it.  This causes
the PWM output to be driven low, causing the LED to go to maximum
brightness.

So, using the inversion at PWM level doesn't work.

To make this work correctly, we really need pwm-leds to do the inversion
rather than setting the inversion bit in hardware.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH 1/2] PWM: let of_xlate handlers check args count

[Lothar Waßmann]

Hi,

Russell King - ARM Linux wrote:
> On Thu, Jan 23, 2014 at 04:53:50PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Jan 23, 2014 at 12:04:44PM +0100, Sascha Hauer wrote:
> > > On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote:
> > > > Hi,
> > > > 
> > > > Sascha Hauer wrote:
> > > > > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip,
> > > > > but it is only ever used by the of_xlate handler itsel. Remove
> > > > > of_pwm_n_cells from struct pwm_chip and let the handler do the argument
> > > > > count checking to simplify the code.
> > > > > 
> > > > This still does not make the PWM_POLARITY flag in the pwms node
> > > > optional as was the goal because of_parse_phandle_with_args() requires
> > > > at least #pwm-cells arguments in the node.
> > > > 
> > > > So, with a DT configuration like:
> > > > pwm0: pwm@0 {
> > > > 	#pwm-cells = <3>;
> > > > };
> > > > backlight {
> > > > 	pwms = <&pwm0 0 100000>;
> > > > };
> > > 
> > > We misunderstood each other. My goal was to allow the driver to also
> > > work with old devicetrees which specify #pwm-cells = <2>, not to allow
> > > inconsistent devicetrees like the snippet above.
> > 
> > In which case, the patch I've posted seems to do that job too... I'm
> > just about to test out the three-cell version.
> 
> Okay, this works, but there's a problem with pwm-leds.
> 
> When the duty cycle is set to zero (when you set the brightness to zero)
> pwm-leds decides to disable the PWM after configuring it.  This causes
> the PWM output to be driven low, causing the LED to go to maximum
> brightness.
> 
> So, using the inversion at PWM level doesn't work.
> 
The problem is that the driver calls pwm_disable() when the duty cycle is 0.
This sets the PWM output low independent from the output polarity setting.

> To make this work correctly, we really need pwm-leds to do the inversion
> rather than setting the inversion bit in hardware.
> 
The same holds for the pwm-backlight driver.

The easiest fix would be not to call pwm_disable() even for a zero duty
cycle.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH 1/2] PWM: let of_xlate handlers check args count

[Sascha Hauer]

On Fri, Jan 24, 2014 at 06:42:54AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> > Okay, this works, but there's a problem with pwm-leds.
> > 
> > When the duty cycle is set to zero (when you set the brightness to zero)
> > pwm-leds decides to disable the PWM after configuring it.  This causes
> > the PWM output to be driven low, causing the LED to go to maximum
> > brightness.
> > 
> > So, using the inversion at PWM level doesn't work.
> > 
> The problem is that the driver calls pwm_disable() when the duty cycle is 0.
> This sets the PWM output low independent from the output polarity setting.
> 
> > To make this work correctly, we really need pwm-leds to do the inversion
> > rather than setting the inversion bit in hardware.
> > 
> The same holds for the pwm-backlight driver.
> 
> The easiest fix would be not to call pwm_disable() even for a zero duty
> cycle.

IMO that's the right thing to do anyway due to the different PWM
hardware controllers we have. I'm thinking about the following patch
for some time.

Sascha

--------------------8<--------------------------

>From 9ebbc3d72c71bd97d7fc4458f60ae3ecd5876984 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Fri, 24 Jan 2014 08:34:16 +0100
Subject: [PATCH] PWM: Document disabled PWM output as undefined

When disabled PWM hardware reacts differently. Some controllers
just stop with their current value, others produce a constant high
or low output, sometimes depending on the output inversion bit. Update
the documentation to reflect that and request from the PWM consumer
drivers to set a constant high/low value with duty cycles of 0/100%
instead of disabling the PWM.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/pwm.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 93cb979..b7e8a31 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -44,6 +44,8 @@ After being requested, a PWM has to be configured using:
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
 
 To start/stop toggling the PWM output use pwm_enable()/pwm_disable().
+The output of a disabled PWM is undefined. Set the duty cycle to 100%
+for a constant high output and to 0 for constant low output.
 
 Using PWMs with the sysfs interface
 -----------------------------------
-- 
1.8.5.2

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |