gpio: mvebu: Add PWM fan support

PATCH Superseded Ralph Sennhauser 2 participants 11 messages Mar 24, 2017 – Mar 27, 2017

Subsystems (5)

Device tree bindings Arm (32-bit) GPIO PWM +1 more subsystems

lore

Share & tools

lore

Ralph Sennhauser

author

2017-03-24 14:18 UTC #0 lore raw

v3 cover gpio: mvebu: Add PWM fan support

Hi everyone

This patch series was originally submitted by Andrew Lunn but got stalled. I picked up the series and addressed what was discussed for the earlier submission with some helpful input from Andrew. Hopefully this time support for the PWM fan as found on Linksys WRT1900AC (Mamba) will make it in.

Implementing as an MFD was discarded due to backward compatibility. The original discussion can be read at [1].

This series depends on two cleanup series. The first series [2] ("gpio:
mvebu: preparatory cleanup for pwm-fan support") is in linux next-20170320. The second one [3] ("gpio: mvebu: use BIT macro instead of bit shifting") got applied but didn't make it's way into linux-next yet.

See pending items below!

Ralph

[1] https://patchwork.ozlabs.org/patch/427287/
[2] https://lkml.org/lkml/2017/3/16/62
[3] https://lkml.org/lkml/2017/3/17/493

---

Changes v2->v3:
Patch 1/4 gpio: mvebu: Add limited PWM support:
* drop pin from mvebu_pwn, can be infered (suggested by Thierry Reding)
* rename pwm to mvpwm so pwm can be used for pwm_device as in the API, avoids some mental gymnastic.
* drop id from struct mvebu_gpio_chip, select blink counter in mvebu_pwm_probe for all lines instead. We do not care about the unused ones. I think a clear improvement in readability. Makes coming up with a good comment simple as well.
* Switch to new atomic PWM API (suggested by Thierry Reding)
* rename use mvebu_gpioreg_blink_select to mvebu_gpioreg_blink_counter_select.
* mark *_suspend() / *_resume() as __maybe_unused (suggested by Linus Walleij)
* document #pwm-cells = 1 (suggested by Thierry Reding)
Patch 2/4 mvebu: xp: Add PWM properties to .dtsi files
* add missing reg-names / #pwm-cell properties to armada-xp-mv78260.dtsi gpio1 node
* set pwm-cells = 1 (suggested by Thierry Reding)
All:
* always uppercase GPIO/PWM in prose (suggested by Thierry Reding)

Changes v1 -> v2:
Patch 1/4 gpio: mvebu: Add limited PWM support:
* use BIT macro (suggested by Linus Walleij)
* move id from struct mvebu_pwm to struct mvebu_gpio_chip, implement blink select as if else and comment on the chip id for code clarity (to accommodate Linus Walleijs request for a code clarification / comment. If you can word it better I'm all ears.)
* Move function comment mvebu_pwm_probe into the function itself.

---

Pending:
* npwm = 1? (suggested by Thierry Reding)
Like Andrew who commented on this item for v1 I'm not yet clear how to achieve that. So while a lie a rather convenient one.
* Tested-by: Andrew Lunn for v1 needs renewing
* Needs ACK from Thierry Reding to be merged via linux-gpio tree by Linus Walleij. (fine with the general approach, requested changes)

---

Andrew Lunn (4):
gpio: mvebu: Add limited PWM support
mvebu: xp: Add PWM properties to .dtsi files
ARM: mvebu: Enable SENSORS_PWM_FAN in defconfig
mvebu: wrt1900ac: Use pwm-fan rather than gpio-fan

 .../devicetree/bindings/gpio/gpio-mvebu.txt        |  29 ++
 MAINTAINERS                                        |   2 +
 arch/arm/boot/dts/armada-370.dtsi                  |  10 +-
 arch/arm/boot/dts/armada-xp-linksys-mamba.dts      |   8 +-
 arch/arm/boot/dts/armada-xp-mv78230.dtsi           |  10 +-
 arch/arm/boot/dts/armada-xp-mv78260.dtsi           |  10 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi           |  10 +-
 arch/arm/configs/mvebu_v7_defconfig                |   2 +
 drivers/gpio/gpio-mvebu.c                          | 317 ++++++++++++++++++++-
 9 files changed, 373 insertions(+), 25 deletions(-)

Signature · 1 line

2.10.2

Ralph Sennhauser

author

2017-03-24 14:18 UTC #1 lore raw

PATCH v3 1/4 gpio: mvebu: Add limited PWM support

From: Andrew Lunn <[email protected]>

Armada 370/XP devices can 'blink' GPIO lines with a configurable on and off period. This can be modelled as a PWM.

However, there are only two sets of PWM configuration registers for all the GPIO lines. This driver simply allows a single GPIO line per GPIO chip of 32 lines to be used as a PWM. Attempts to use more return EBUSY.

Due to the interleaving of registers it is not simple to separate the PWM driver from the GPIO driver. Thus the GPIO driver has been extended with a PWM driver.

Signed-off-by Andrew Lunn <[email protected]>

URL: https://patchwork.ozlabs.org/patch/427287/
URL: https://patchwork.ozlabs.org/patch/427295/
[Ralph Sennhauser:
* Port forward
* Merge PWM portion into gpio-mvebu.c
* Switch to atomic PWM API
* Merge documentation patch
* Update MAINTAINERS]

Signed-off-by Ralph Sennhauser <[email protected]>

---
 .../devicetree/bindings/gpio/gpio-mvebu.txt        |  29 ++
 MAINTAINERS                                        |   2 +
 drivers/gpio/gpio-mvebu.c                          | 317 ++++++++++++++++++++-
 3 files changed, 336 insertions(+), 12 deletions(-)

Documentation/devicetree/bindings/gpio/gpio-mvebu.txt +29 -0

index a6f3bec..748bbc7 100644
@@ -38,6 +38,21 @@ Required properties:
 - #gpio-cells: Should be two. The first cell is the pin number. The
   second cell is reserved for flags, unused at the moment.
 
+Optional properties:
+
+In order to use the gpio lines in PWM mode, some additional optional
+properties are required. Only Armada 370 and XP support these properties.
+
+- reg: an additional register set is needed, for the GPIO Blink
+  Counter on/off registers.
+
+- reg-names: Must contain an entry "pwm" corresponding to the
+  additional register range needed for pwm operation.
+
+- #pwm-cells: Should be one. The first cell is the pin number.
+
+- clocks: Must be a phandle to the clock for the gpio controller.
+
 Example:
 
 		gpio0: gpio at d0018100 {

@@ -51,3 +66,17 @@ Example:
 			#interrupt-cells = <2>;
 			interrupts = <16>, <17>, <18>, <19>;
 		};
+
+		gpio1: gpio at 18140 {
+			compatible = "marvell,orion-gpio";
+			reg = <0x18140 0x40>, <0x181c8 0x08>;
+			reg-names = "gpio", "pwm";
+			ngpios = <17>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			#pwm-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			interrupts = <87>, <88>, <89>;
+			clocks = <&coreclk 0>;
+		};

MAINTAINERS +2 -0

index c8f88d0..7fd2289 100644
@@ -10291,6 +10291,8 @@ F:	include/linux/pwm.h
 F:	drivers/pwm/
 F:	drivers/video/backlight/pwm_bl.c
 F:	include/linux/pwm_backlight.h
+F:	drivers/gpio/gpio-mvebu.c
+F:	Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
 
 PXA2xx/PXA3xx SUPPORT
 M:	Daniel Mack <[email protected]>

drivers/gpio/gpio-mvebu.c +305 ~~-12~~

392 lines not on this page · raw message

Signature · 1 line

2.10.2

Andrew Lunn

maintainer

2017-03-24 14:23 UTC #2 lore raw

Re: [PATCH v3 1/4] gpio: mvebu: Add limited PWM support

Hi Ralph

Documentation/devicetree/bindings/gpio/gpio-mvebu.txt Example: · line 77

Quoted 3 lines

+- #pwm-cells: Should be one. The first cell is the pin number.
Documentation/devicetree/bindings/gpio/gpio-mvebu.txt +1 -0
+			#pwm-cells = <2>;

You missed updating the example.

Andrew Lunn

maintainer

2017-03-24 14:28 UTC #3 lore raw

Re: [PATCH v3 1/4] gpio: mvebu: Add limited PWM support

In reply to Ralph Sennhauser

drivers/gpio/gpio-mvebu.c static void mvebu_gpio_irq_handler(struct irq_desc *desc) · line 627

Quoted 11 lines

drivers/gpio/gpio-mvebu.c +11 -0

+	if (val < state->duty_cycle)
+		state->period = 1;
+	else {
+		val -= state->duty_cycle;
+		if (val > UINT_MAX)
+			state->period = UINT_MAX;
+		else if (val)
+			state->period = val;
+		else
+			state->period = 1;
+	}

Hi Ralph

The coding style guide says that if one side of an if else clause has {}, the other side must as well, even if it is a single statement.

Andrew

Ralph Sennhauser

author

2017-03-24 14:18 UTC #4 lore raw

PATCH v3 2/4 mvebu: xp: Add PWM properties to .dtsi files

In reply to Ralph Sennhauser

From: Andrew Lunn <[email protected]>

Add properties to the GPIO nodes to allow them to be also used as PWM lines.

Signed-off-by Andrew Lunn <[email protected]>

URL: https://patchwork.ozlabs.org/patch/427294/
[Ralph Sennhauser:
* Use #pwm-cells = <1>
* Add missing reg-names / #pwm-cell properties to armada-xp-mv78260.dtsi

    'gpio1' node]

Signed-off-by Ralph Sennhauser <[email protected]>

---
 arch/arm/boot/dts/armada-370.dtsi        | 10 ++++++++--
 arch/arm/boot/dts/armada-xp-mv78230.dtsi | 10 ++++++++--
 arch/arm/boot/dts/armada-xp-mv78260.dtsi | 10 ++++++++--
 arch/arm/boot/dts/armada-xp-mv78460.dtsi | 10 ++++++++--
 4 files changed, 32 insertions(+), 8 deletions(-)

arch/arm/boot/dts/armada-370.dtsi +8 -2

index cc011c8..de7295c 100644
@@ -138,24 +138,30 @@
 
 			gpio0: gpio at 18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio at 18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>, <90>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio2: gpio at 18180 {

arch/arm/boot/dts/armada-xp-mv78230.dtsi +8 -2

index 07c5090..9ae856b 100644
@@ -203,24 +203,30 @@
 		internal-regs {
 			gpio0: gpio at 18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio at 18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <17>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>;
+				clocks = <&coreclk 0>;
 			};
 		};
 	};

arch/arm/boot/dts/armada-xp-mv78260.dtsi +8 -2

index 64e936a..11eb6ff 100644
@@ -286,24 +286,30 @@
 		internal-regs {
 			gpio0: gpio at 18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio at 18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>, <90>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio2: gpio at 18180 {

arch/arm/boot/dts/armada-xp-mv78460.dtsi +8 -2

index d1383dd..597b0d6 100644
@@ -324,24 +324,30 @@
 		internal-regs {
 			gpio0: gpio at 18100 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18100 0x40>;
+				reg = <0x18100 0x40>, <0x181c0 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <82>, <83>, <84>, <85>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio1: gpio at 18140 {
 				compatible = "marvell,orion-gpio";
-				reg = <0x18140 0x40>;
+				reg = <0x18140 0x40>, <0x181c8 0x08>;
+				reg-names = "gpio", "pwm";
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <1>;
 				interrupt-controller;
 				#interrupt-cells = <2>;
 				interrupts = <87>, <88>, <89>, <90>;
+				clocks = <&coreclk 0>;
 			};
 
 			gpio2: gpio at 18180 {

Signature · 1 line

2.10.2

Ralph Sennhauser

author

2017-03-24 14:18 UTC #5 lore raw

PATCH v3 3/4 ARM: mvebu: Enable SENSORS_PWM_FAN in defconfig

In reply to Ralph Sennhauser

From: Andrew Lunn <[email protected]>

Now that the GPIO driver also supports PWM operation, enable the PWM framework and fan driver in mvebu_v7_defconfig.

Signed-off-by Andrew Lunn <[email protected]>

URL: https://patchwork.ozlabs.org/patch/427297/

[Ralph Sennhauser: add fan driver to defconfig]

Signed-off-by Ralph Sennhauser <[email protected]>

---
 arch/arm/configs/mvebu_v7_defconfig | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/configs/mvebu_v7_defconfig +2 -0

index f1a0e25..6955370 100644
@@ -135,6 +135,8 @@ CONFIG_DMADEVICES=y
 CONFIG_MV_XOR=y
 # CONFIG_IOMMU_SUPPORT is not set
 CONFIG_MEMORY=y
+CONFIG_PWM=y
+CONFIG_SENSORS_PWM_FAN=y
 CONFIG_EXT4_FS=y
 CONFIG_ISO9660_FS=y
 CONFIG_JOLIET=y

Signature · 1 line

2.10.2

Ralph Sennhauser

author

2017-03-24 14:18 UTC #6 lore raw

PATCH v3 4/4 mvebu: wrt1900ac: Use pwm-fan rather than gpio-fan

In reply to Ralph Sennhauser

From: Andrew Lunn <[email protected]>

The mvebu GPIO driver can also perform PWM on some pins. Use the pwm-fan driver to control the fan of the WRT1900AC, giving us finer grained control over its speed and hence noise.

Signed-off-by Andrew Lunn <[email protected]>

URL: https://patchwork.ozlabs.org/patch/427291/

Signed-off-by Ralph Sennhauser <[email protected]>

---
 arch/arm/boot/dts/armada-xp-linksys-mamba.dts | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

arch/arm/boot/dts/armada-xp-linksys-mamba.dts +3 -5

index 3744ba3..836b275e 100644
@@ -293,13 +293,11 @@
 		};
 	};
 
-	gpio_fan {
+	pwm_fan {
 		/* SUNON HA4010V4-0000-C99 */
-		compatible = "gpio-fan";
-		gpios = <&gpio0 24 0>;
 
-		gpio-fan,speed-map = <0    0
-				      4500 1>;
+		compatible = "pwm-fan";
+		pwms = <&gpio0 24 4000 0>;
 	};
 
 	dsa {

Signature · 1 line

2.10.2

Andrew Lunn

maintainer

2017-03-24 14:35 UTC #7 lore raw

Re: [PATCH v3 4/4] mvebu: wrt1900ac: Use pwm-fan rather than gpio-fan

arch/arm/boot/dts/armada-xp-linksys-mamba.dts line 300

Quoted 9 lines

arch/arm/boot/dts/armada-xp-linksys-mamba.dts +3 -4

+	pwm_fan {
 		/* SUNON HA4010V4-0000-C99 */
-		compatible = "gpio-fan";
-		gpios = <&gpio0 24 0>;
 
-		gpio-fan,speed-map = <0    0
-				      4500 1>;
+		compatible = "pwm-fan";
+		pwms = <&gpio0 24 4000 0>;

Hi Ralph

I believe this last 0 is the flags parameter. Now that we have #pwm-cells = 1, i think this last 0 should be dropped.

Andrew

Ralph Sennhauser

author

2017-03-24 22:23 UTC #8 lore raw

Re: [PATCH v3 4/4] mvebu: wrt1900ac: Use pwm-fan rather than gpio-fan

Quoted 12 lines · On Fri, 24 Mar 2017 15:35:05 +0100 Andrew Lunn <[email protected]> wrote:

arch/arm/boot/dts/armada-xp-linksys-mamba.dts line 300
Quoted 9 lines
arch/arm/boot/dts/armada-xp-linksys-mamba.dts +3 -4
+	pwm_fan {
 		/* SUNON HA4010V4-0000-C99 */
-		compatible = "gpio-fan";
-		gpios = <&gpio0 24 0>;
 
-		gpio-fan,speed-map = <0    0
-				      4500 1>;
+		compatible = "pwm-fan";
+		pwms = <&gpio0 24 4000 0>;  
Hi Ralph

Quoted 4 lines

I believe this last 0 is the flags parameter. Now that we have #pwm-cells = 1, i think this last 0 should be dropped.
Andrew

Missed it. Duly noted like the other comments.

Thanks
Ralph

Ralph Sennhauser

author

2017-03-26 21:05 UTC #9 lore raw

Re: [PATCH v3 4/4] mvebu: wrt1900ac: Use pwm-fan rather than gpio-fan

In reply to Andrew Lunn

Quoted 12 lines · On Fri, 24 Mar 2017 15:35:05 +0100 Andrew Lunn <[email protected]> wrote:

arch/arm/boot/dts/armada-xp-linksys-mamba.dts line 300
Quoted 9 lines
arch/arm/boot/dts/armada-xp-linksys-mamba.dts +3 -4
+	pwm_fan {
 		/* SUNON HA4010V4-0000-C99 */
-		compatible = "gpio-fan";
-		gpios = <&gpio0 24 0>;
 
-		gpio-fan,speed-map = <0    0
-				      4500 1>;
+		compatible = "pwm-fan";
+		pwms = <&gpio0 24 4000 0>;  
Hi Ralph

Quoted 4 lines

I believe this last 0 is the flags parameter. Now that we have #pwm-cells = 1, i think this last 0 should be dropped.
Andrew

Hi Andrew,

isn't the 4000 (period?) pwm-cell #2 and 0 (flags?) pwm-cell #3? I actually expect "pwms = <&gpio0 24>;" here or "#pwm-cells = <2>;"

Ralph

Andrew Lunn

maintainer

2017-03-27 13:17 UTC #10 lore raw

Re: [PATCH v3 4/4] mvebu: wrt1900ac: Use pwm-fan rather than gpio-fan

Quoted 18 lines · Ralph Sennhauser

Quoted 15 lines · On Fri, 24 Mar 2017 15:35:05 +0100 Andrew Lunn <[email protected]> wrote:
arch/arm/boot/dts/armada-xp-linksys-mamba.dts line 300
Quoted 9 lines
arch/arm/boot/dts/armada-xp-linksys-mamba.dts +3 -4
+	pwm_fan {
 		/* SUNON HA4010V4-0000-C99 */
-		compatible = "gpio-fan";
-		gpios = <&gpio0 24 0>;
 
-		gpio-fan,speed-map = <0    0
-				      4500 1>;
+		compatible = "pwm-fan";
+		pwms = <&gpio0 24 4000 0>;  
Hi Ralph
I believe this last 0 is the flags parameter. Now that we have #pwm-cells = 1, i think this last 0 should be dropped.

Quoted 6 lines

Quoted 1 line
Andrew
Hi Andrew,
isn't the 4000 (period?) pwm-cell #2 and 0 (flags?) pwm-cell #3? I actually expect "pwms = <&gpio0 24>;" here or "#pwm-cells = <2>;"

Hi Ralph

Quoted 1 line

From Documentation/devicetree/bindings/pwm/pwm.txt:

pwm-list ::= <single-pwm> [pwm-list]
single-pwm ::= <pwm-phandle> <pwm-specifier> pwm-phandle : phandle to PWM controller node pwm-specifier : array of #pwm-cells specifying the given PWM
(controller specific)

Our pwm-list has a single single-pwm.
phandle is &gpio0.
The remaining parts are the specifier, or which there should be #pwm-calls.

Quoted 1 line

From Documentation/devicetree/bindings/pwm/pwm.txt again:

pwm-specifier typically encodes the chip-relative PWM number and the PWM period in nanoseconds.

Optionally, the pwm-specifier can encode a number of flags (defined in <dt-bindings/pwm/pwm.h>) in a third cell:
- PWM_POLARITY_INVERTED: invert the PWM signal polarity

So we are using the 24th PWM and 4000 nanosecod period. We don't want any flags.

pwms = <&gpio0 24 4000>;

has a phandle, and then 2 cells.

Andrew