ARM: dts: lpc32xx: add pwm-cells to base dts file

PATCH New Sylvain Lemieux 2 participants 4 messages Sep 26, 2016 – Oct 5, 2016

Subsystems (3)

Device tree bindings PWM Arm/LPC32XX SoC support

lore

Share & tools

lore

Sylvain Lemieux

author

2016-09-26 18:47 UTC #0 lore raw

PATCH 1/1 ARM: dts: lpc32xx: add pwm-cells to base dts file

From: Sylvain Lemieux <[email protected]>

There is no need to define the "pwm-cells" in the board specific dts file; move the entry to the base dts file.

Signed-off-by Sylvain Lemieux <[email protected]>

---
Note:
* This patch should be apply after
"ARM: dts: lpc32xx: set default parent clock for pwm1 & pwm2"
http://www.spinics.net/lists/arm-kernel/msg530277.html
- There is no dependency between the patches.

 arch/arm/boot/dts/lpc32xx.dtsi | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/boot/dts/lpc32xx.dtsi +2 -0

index 218d9fa..c031c94 100644
@@ -472,6 +472,7 @@
 				assigned-clocks = <&clk LPC32XX_CLK_PWM1>;
 				assigned-clock-parents = <&clk LPC32XX_CLK_PERIPH>;
 				status = "disabled";
+				#pwm-cells = <2>;
 			};
 
 			pwm2: pwm at 4005C004 {

@@ -481,6 +482,7 @@
 				assigned-clocks = <&clk LPC32XX_CLK_PWM2>;
 				assigned-clock-parents = <&clk LPC32XX_CLK_PERIPH>;
 				status = "disabled";
+				#pwm-cells = <2>;
 			};
 
 			timer3: timer at 40060000 {

Signature · 1 line

1.8.3.1

Vladimir Zapolskiy

maintainer

2016-09-26 22:07 UTC #1 lore raw

PATCH 1/1 ARM: dts: lpc32xx: add pwm-cells to base dts file

Hi Sylvain,

Quoted 30 lines · Sylvain Lemieux

From: Sylvain Lemieux <[email protected]>
There is no need to define the "pwm-cells" in the board specific dts file; move the entry to the base dts file.
Signed-off-by Sylvain Lemieux <[email protected]>
---
Note:
* This patch should be apply after
"ARM: dts: lpc32xx: set default parent clock for pwm1 & pwm2"
http://www.spinics.net/lists/arm-kernel/msg530277.html
- There is no dependency between the patches.
 arch/arm/boot/dts/lpc32xx.dtsi | 2 ++
 1 file changed, 2 insertions(+)
arch/arm/boot/dts/lpc32xx.dtsi +1 -0
index 218d9fa..c031c94 100644
@@ -472,6 +472,7 @@
 				assigned-clocks = <&clk LPC32XX_CLK_PWM1>;
 				assigned-clock-parents = <&clk LPC32XX_CLK_PERIPH>;
 				status = "disabled";
+				#pwm-cells = <2>;
 			};
 
 			pwm2: pwm at 4005C004 {

arch/arm/boot/dts/lpc32xx.dtsi line 488

Quoted 7 lines

arch/arm/boot/dts/lpc32xx.dtsi +1 -0

@@ -481,6 +482,7 @@
 				assigned-clocks = <&clk LPC32XX_CLK_PWM2>;
 				assigned-clock-parents = <&clk LPC32XX_CLK_PERIPH>;
 				status = "disabled";
+				#pwm-cells = <2>;
 			};
 
 			timer3: timer at 40060000 {

that's something I have done locally and in a different manner, but I haven't published it yet, please find below a draft.

First of all from multiple places in the User's Manual you can find that there are "two single output PWM blocks" or "the LPC32x0 provides two 8-bit PWMs" etc.

In this case it does not make sense to set PWM cells to 2 (there is only one channel), and 1 cell for frequency is good enough, and that's the proposed change to support it:

drivers/pwm/pwm-lpc32xx.c +16 -0

index a9b3cff..447ae44 100644
@@ -99,6 +99,22 @@ static const struct pwm_ops lpc32xx_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static struct pwm_device *lpc32xx_pwm_of_xlate(struct pwm_chip *pc,
+				       const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	pwm = pwm_request_from_chip(pc, 0, NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm->args.period = args->args[0];
+
+	return pwm;
+}
+
 static int lpc32xx_pwm_probe(struct platform_device *pdev)
 {
 	struct lpc32xx_pwm_chip *lpc32xx;

@@ -123,6 +139,8 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 	lpc32xx->chip.ops = &lpc32xx_pwm_ops;
 	lpc32xx->chip.npwm = 1;
 	lpc32xx->chip.base = -1;
+	lpc32xx->chip.of_xlate = lpc32xx_pwm_of_xlate;
+	lpc32xx->chip.of_pwm_n_cells = 1;
 
 	ret = pwmchip_add(&lpc32xx->chip);
 	if (ret < 0) {

What is your opinion about this proposal?

If this change is applied, then lpc32xx.dtsi should contain #pwm-cells = <1>.

--
With best wishes,
Vladimir

Sylvain Lemieux (gmail)

author

2016-09-27 12:34 UTC #2 lore raw

PATCH 1/1 ARM: dts: lpc32xx: add pwm-cells to base dts file

Hi Vladimir,

Quoted 19 lines · Vladimir Zapolskiy

Hi Sylvain,
Quoted 16 lines · Sylvain Lemieux
From: Sylvain Lemieux <[email protected]>
There is no need to define the "pwm-cells" in the board specific dts file; move the entry to the base dts file.
Signed-off-by Sylvain Lemieux <[email protected]>
---
Note:
* This patch should be apply after
"ARM: dts: lpc32xx: set default parent clock for pwm1 & pwm2"
http://www.spinics.net/lists/arm-kernel/msg530277.html
- There is no dependency between the patches.
 arch/arm/boot/dts/lpc32xx.dtsi | 2 ++
 1 file changed, 2 insertions(+)

[...]

Quoted 47 lines

that's something I have done locally and in a different manner, but I haven't published it yet, please find below a draft.
First of all from multiple places in the User's Manual you can find that there are "two single output PWM blocks" or "the LPC32x0 provides two 8-bit PWMs" etc.
In this case it does not make sense to set PWM cells to 2 (there is only one channel), and 1 cell for frequency is good enough, and that's the proposed change to support it:
drivers/pwm/pwm-lpc32xx.c +16 -0
index a9b3cff..447ae44 100644
@@ -99,6 +99,22 @@ static const struct pwm_ops lpc32xx_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static struct pwm_device *lpc32xx_pwm_of_xlate(struct pwm_chip *pc,
+				       const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	pwm = pwm_request_from_chip(pc, 0, NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm->args.period = args->args[0];
+
+	return pwm;
+}
+
 static int lpc32xx_pwm_probe(struct platform_device *pdev)
 {
 	struct lpc32xx_pwm_chip *lpc32xx;
@@ -123,6 +139,8 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 	lpc32xx->chip.ops = &lpc32xx_pwm_ops;
 	lpc32xx->chip.npwm = 1;
 	lpc32xx->chip.base = -1;
+	lpc32xx->chip.of_xlate = lpc32xx_pwm_of_xlate;
+	lpc32xx->chip.of_pwm_n_cells = 1;
 
 	ret = pwmchip_add(&lpc32xx->chip);
 	if (ret < 0) {

Quoted 2 lines

What is your opinion about this proposal?

I agree with you, this clean-up make sense; the PWM cell should be 1.

Quoted 2 lines

If this change is applied, then lpc32xx.dtsi should contain #pwm-cells = <1>.

Can you submit your change on the mailing list?
I will send a version 2 of this patch after.

If it is helping, I can take care of submitting a patch to update the documentation (lpc32xx-pwm.txt).

Did you have a change to look at the 2 others PWM related change:
* clk: lpc32xx: fix pwm clock divider computation
http://www.spinics.net/lists/arm-kernel/msg534048.html
* ARM: dts: lpc32xx: set pwm1 & pwm2 default clock rate
http://www.spinics.net/lists/arm-kernel/msg534051.html

Quoted 3 lines

--
With best wishes,
Vladimir

Sylvain

Sylvain Lemieux

author

2016-10-05 15:07 UTC #3 lore raw

PATCH 1/1 ARM: dts: lpc32xx: add pwm-cells to base dts file

Hi Vladimir,

Quoted 34 lines · Sylvain Lemieux (gmail)

Hi Vladimir,
Quoted 19 lines · Vladimir Zapolskiy
Hi Sylvain,
Quoted 16 lines · Sylvain Lemieux
From: Sylvain Lemieux <[email protected]>
There is no need to define the "pwm-cells" in the board specific dts file; move the entry to the base dts file.
Signed-off-by Sylvain Lemieux <[email protected]>
---
Note:
* This patch should be apply after
"ARM: dts: lpc32xx: set default parent clock for pwm1 & pwm2"
http://www.spinics.net/lists/arm-kernel/msg530277.html
- There is no dependency between the patches.
 arch/arm/boot/dts/lpc32xx.dtsi | 2 ++
 1 file changed, 2 insertions(+)
[...]
Quoted 11 lines
that's something I have done locally and in a different manner, but I haven't published it yet, please find below a draft.
First of all from multiple places in the User's Manual you can find that there are "two single output PWM blocks" or "the LPC32x0 provides two 8-bit PWMs" etc.
In this case it does not make sense to set PWM cells to 2 (there is only one channel), and 1 cell for frequency is good enough, and that's the proposed change to support it:

[..]

Quoted 5 lines

Quoted 3 lines
What is your opinion about this proposal?
I agree with you, this clean-up make sense; the PWM cell should be 1.

Quoted 8 lines

Quoted 2 lines
If this change is applied, then lpc32xx.dtsi should contain #pwm-cells = <1>.
Can you submit your change on the mailing list?
I will send a version 2 of this patch after.
If it is helping, I can take care of submitting a patch to update the documentation (lpc32xx-pwm.txt).

Just doing a quick follow-up;
when do you think you can submit your change to the mailling list?

Quoted 6 lines

Quoted 3 lines
--
With best wishes,
Vladimir
Sylvain

Regards,
Sylvain