[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lukasz Majewski]

[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Fri, Oct 07, 2016 at 08:41:26PM +0530, Bhuvanchandra DV wrote:
> From: Lothar Wassmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> 
> 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-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> Signed-off-by: Lukasz Majewski <l.majewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Reviewed-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>  drivers/pwm/pwm-imx.c                             | 51 +++++++++++++++++++++--
>  2 files changed, 51 insertions(+), 6 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org> wrote:

> From: Lothar Wassmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> 
> 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-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> Signed-off-by: Lukasz Majewski <l.majewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv-2KBjVHiyJgBBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Reviewed-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  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;
>  

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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