Skip to contentSkip to conversation

pwm: lpc32xx - Add a driver for the motor PWM

PATCH New Alban Bedel 3 participants 6 messages Nov 14, 2012 – Nov 16, 2012

Subsystems (3)
Device tree bindings PWM Arm/LPC32XX SoC support
lore
Share & tools
lore

Alban Bedel

author
#0 lore raw

v2 cover pwm: lpc32xx - Add a driver for the motor PWM

This is a new submission of the LPC32xx Motor PWM driver with the following changes:

* Splitted the arch part in a separate patch
* Removed the useless clock ref counting
* Fixed remove() to properly disable the clock

[PATCH 1/2] pwm: lpc32xx - Add a driver for the motor PWM [PATCH 2/2] ARM: LPC32xx: Add the motor PWM to base dts file

Alban

author
#1 lore raw

PATCH v2 1/2 pwm: lpc32xx - Add a driver for the motor PWM

Signed-off-by Alban Bedel <[email protected]>
---
 .../devicetree/bindings/pwm/lpc32xx-motor-pwm.txt  |   24 +++
 drivers/pwm/Kconfig                                |   10 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-lpc32xx-motor.c                    |  209 ++++++++++++++++++++
 4 files changed, 244 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt
 create mode 100644 drivers/pwm/pwm-lpc32xx-motor.c
Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file +24 -0
new file mode 100644
index 0000000..e19b0a4
@@ -0,0 +1,24 @@
+LPC32XX Motor PWM controller
+
+The LPC32xx motor PWMs have two output pin, A and B, with B=!A.
+Per default the output A should be used, if the output B is used the
+PWM polarity should be inverted using the linux,polarity property.
+
+Required properties:
+- compatible: should be "nxp,lpc3220-motor-pwm"
+- reg: physical base address and length of the controller's registers
+
+Optional properites:
+- linux,polarity: Bit mask of the polarity to use for each output,
+      a bit set to 0 indicate the default polarity, a bit set to 1
+      indicate an inverted polarity. In other word this set if output
+      A or output B has the correct polarity.
+
+Examples:
+
+mpwm at 400e8000 {
+	compatible = "nxp,lpc3220-motor-pwm";
+	reg = <0x400E8000 0x78>;
+	linux,polarity = 5; /* Use outputs B0, A1 and B2 */
+	#pwm-cells = <2>;
+};
drivers/pwm/Kconfig +10 -0
index 90c5c73..90fc167 100644
@@ -57,6 +57,16 @@ config PWM_LPC32XX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpc32xx.
 
+config PWM_LPC32XX_MOTOR
+	tristate "LPC32XX Motor PWM support"
+	depends on ARCH_LPC32XX
+	help
+	  Generic PWM framework driver for LPC32XX motor PWM. The LPC32XX SOC
+	  has one motor PWM controllers.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-motor-lpc32xx.
+
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
 	depends on ARCH_MXS && OF
drivers/pwm/Makefile +1 -0
index e4b2c89..510bad8 100644
@@ -2,6 +2,7 @@ obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
+obj-$(CONFIG_PWM_LPC32XX_MOTOR)	+= pwm-lpc32xx-motor.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
drivers/pwm/pwm-lpc32xx-motor.c new file +209 -0

209 lines not on this page · raw message

Signature · 1 line
1.7.0.4
#2 lore raw

PATCH 1/2 pwm: lpc32xx - Add a driver for the motor PWM

Quoted 1 line · Alban Bedel
Signed-off-by Alban Bedel <[email protected]>

Hi Alban,

This looks good. There could be some more informative commit message, maybe something similar to what the DT binding has in the first paragraph.

I have some other comments inline below.

Quoted 15 lines
---
 .../devicetree/bindings/pwm/lpc32xx-motor-pwm.txt  |   24 +++
 drivers/pwm/Kconfig                                |   10 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-lpc32xx-motor.c                    |  209 ++++++++++++++++++++
 4 files changed, 244 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt
 create mode 100644 drivers/pwm/pwm-lpc32xx-motor.c
Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file +0 -0
new file mode 100644
index 0000000..e19b0a4

Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt line 3

Quoted 3 lines
Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file +3 -0
@@ -0,0 +1,24 @@
+LPC32XX Motor PWM controller
+
+The LPC32xx motor PWMs have two output pin, A and B, with B=!A.

You use two different spellings: LPC32XX and LPC32xx. Can we settle on one, please? Also "two output pins".

Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt line 4

Quoted 1 line
Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file +1 -0
+Per default the output A should be used, if the output B is used the

"By default, output A..."

Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt line 15

Quoted 11 lines
Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file +11 -0
+PWM polarity should be inverted using the linux,polarity property.
+
+Required properties:
+- compatible: should be "nxp,lpc3220-motor-pwm"
+- reg: physical base address and length of the controller's registers
+
+Optional properites:
+- linux,polarity: Bit mask of the polarity to use for each output,
+      a bit set to 0 indicate the default polarity, a bit set to 1
+      indicate an inverted polarity. In other word this set if output
+      A or output B has the correct polarity.

Maybe this should state explicitly which bits are used for which outputs.

Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt line 22

Quoted 7 lines
Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file +7 -0
+
+Examples:
+
+mpwm at 400e8000 {
+	compatible = "nxp,lpc3220-motor-pwm";
+	reg = <0x400E8000 0x78>;
+	linux,polarity = 5; /* Use outputs B0, A1 and B2 */

Also since this is a bitmask it could make more sense to write it in hexadecimal. And you may want to add <> around for consistency.

drivers/pwm/Kconfig config PWM_LPC32XX · line 61

Quoted 12 lines
Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file +2 -0
+	#pwm-cells = <2>;
+};
drivers/pwm/Kconfig +2 -0
index 90c5c73..90fc167 100644
@@ -57,6 +57,16 @@ config PWM_LPC32XX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpc32xx.
 
+config PWM_LPC32XX_MOTOR
+	tristate "LPC32XX Motor PWM support"

Again the different spelling.

drivers/pwm/Kconfig config PWM_LPC32XX · line 64

Quoted 3 lines
drivers/pwm/Kconfig +3 -0
+	depends on ARCH_LPC32XX
+	help
+	  Generic PWM framework driver for LPC32XX motor PWM. The LPC32XX SOC

LPC32XX seems to be your preferred spelling.

drivers/pwm/Kconfig config PWM_LPC32XX · line 68

Quoted 4 lines
drivers/pwm/Kconfig +4 -0
+	  has one motor PWM controllers.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-motor-lpc32xx.

According to the Makefile, the module will be called pwm-lpc32xx-motor.

Quoted 17 lines
drivers/pwm/Kconfig +1 -0
+
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
 	depends on ARCH_MXS && OF
drivers/pwm/Makefile +1 -0
index e4b2c89..510bad8 100644
@@ -2,6 +2,7 @@ obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
+obj-$(CONFIG_PWM_LPC32XX_MOTOR)	+= pwm-lpc32xx-motor.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
drivers/pwm/pwm-lpc32xx-motor.c +0 -0

[...]

drivers/pwm/pwm-lpc32xx-motor.c line 32

Quoted 2 lines
drivers/pwm/pwm-lpc32xx-motor.c +2 -0
+#define to_lpc32xx_motor_pwm_chip(_chip) \
+	container_of(_chip, struct lpc32xx_motor_pwm_chip, chip)

That's an awfully long name for this macro. Maybe something like to_motor_pwm_chip() would be long enough already.

drivers/pwm/pwm-lpc32xx-motor.c line 53

Quoted 3 lines
drivers/pwm/pwm-lpc32xx-motor.c +3 -0
+#define PWM_EN_MASK(pwm)		BIT(0 + (pwm)->hwpwm*8)
+#define MCLIM_REG_OFFSET(pwm)		(LPC32XX_MCPWM_MCLIM0 + (pwm)->hwpwm*4)
+#define MCMAT_REG_OFFSET(pwm)		(LPC32XX_MCPWM_MCMAT0 + (pwm)->hwpwm*4)

CodingStyle mandates that you put spaces around binary operators.

drivers/pwm/pwm-lpc32xx-motor.c line 60

Quoted 7 lines
drivers/pwm/pwm-lpc32xx-motor.c +7 -0
+
+static int lpc32xx_motor_pwm_config(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    int duty_ns, int period_ns)
+{
+	struct lpc32xx_motor_pwm_chip *lpc32xx =
+		to_lpc32xx_motor_pwm_chip(chip);

If you make the macro name shorter this will actually fit on one line. =)

drivers/pwm/pwm-lpc32xx-motor.c line 61

Quoted 1 line
drivers/pwm/pwm-lpc32xx-motor.c +1 -0
+	u64 rate, per, duty;

Maybe rename per to period to make it explicit.

Quoted 1 line

+static int lpc32xx_motor_pwm_probe(struct platform_device *pdev)

[...]

drivers/pwm/pwm-lpc32xx-motor.c line 148

Quoted 6 lines
drivers/pwm/pwm-lpc32xx-motor.c +6 -0
+	/* Configure the pins polarity */
+	ret = of_property_read_u32(pdev->dev.of_node, "linux,polarity",
+				   &lpc32xx->pins);
+	if (!ret) {
+		u32 set = 0, clr = 0;
+		int i;

This could use a blank line for readability.

drivers/pwm/pwm-lpc32xx-motor.c line 149

Quoted 1 line
drivers/pwm/pwm-lpc32xx-motor.c +1 -0
+		for (i = 0 ; i < LPC32XX_MCPWM_COUNT ; i += 1)

Can we make that i++?

drivers/pwm/pwm-lpc32xx-motor.c line 151

Quoted 2 lines
drivers/pwm/pwm-lpc32xx-motor.c +2 -0
+			if (lpc32xx->pins & BIT(i))
+				set |= BIT(2 + i*8);

Spaces around '*'.

drivers/pwm/pwm-lpc32xx-motor.c line 153

Quoted 2 lines
drivers/pwm/pwm-lpc32xx-motor.c +2 -0
+			else
+				clr |= BIT(2 + i*8);

And another blank line.

drivers/pwm/pwm-lpc32xx-motor.c line 156

Quoted 3 lines
drivers/pwm/pwm-lpc32xx-motor.c +3 -0
+		ret = clk_enable(lpc32xx->clk);
+		if (ret)
+			return ret;

And here.

Quoted 1 line

+static int __devexit lpc32xx_motor_pwm_remove(struct platform_device *pdev)

__devexit is now officially obsolete.

drivers/pwm/pwm-lpc32xx-motor.c line 183

Quoted 5 lines
drivers/pwm/pwm-lpc32xx-motor.c +5 -0
+{
+	struct lpc32xx_motor_pwm_chip *lpc32xx = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0 ; i < lpc32xx->chip.npwm ; i += 1)

Again, i++.

drivers/pwm/pwm-lpc32xx-motor.c line 189

Quoted 6 lines
drivers/pwm/pwm-lpc32xx-motor.c +6 -0
+		pwm_disable(&lpc32xx->chip.pwms[i]);
+
+	return pwmchip_remove(&lpc32xx->chip);
+}
+
+static const struct of_device_id lpc32xx_motor_pwm_dt_ids[] __devinitconst = {

__devinitconst is equally obsolete.

drivers/pwm/pwm-lpc32xx-motor.c line 202

Quoted 13 lines
drivers/pwm/pwm-lpc32xx-motor.c +13 -0
+	{ .compatible = "nxp,lpc3220-motor-pwm", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lpc32xx_motor_pwm_dt_ids);
+
+static struct platform_driver lpc32xx_motor_pwm_driver = {
+	.driver = {
+		.name = "lpc32xx-motor-pwm",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(lpc32xx_motor_pwm_dt_ids),
+	},
+	.probe = lpc32xx_motor_pwm_probe,
+	.remove = __devexit_p(lpc32xx_motor_pwm_remove),

__devexit_p as well.

drivers/pwm/pwm-lpc32xx-motor.c line 208

Quoted 6 lines
drivers/pwm/pwm-lpc32xx-motor.c +6 -0
+};
+module_platform_driver(lpc32xx_motor_pwm_driver);
+
+MODULE_ALIAS("platform:lpc32xx-motor-pwm");
+MODULE_AUTHOR("Alban Bedel <[email protected]>");
+MODULE_DESCRIPTION("LPC32XX Motor PWM Driver");

And the spelling again.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/7d21b78d/attachment.sig>

#3 lore raw

PATCH 1/2 pwm: lpc32xx - Add a driver for the motor PWM

Quoted 9 lines · Thierry Reding
Quoted 9 lines
.../devicetree/bindings/pwm/lpc32xx-motor-pwm.txt  |   24 +++ 
drivers/pwm/Kconfig                                |   10 + 
drivers/pwm/Makefile                               |    1 + 
drivers/pwm/pwm-lpc32xx-motor.c                    |  209

++++++++++++++++++++ 4 files changed, 244 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt create mode 100644 drivers/pwm/pwm-lpc32xx-motor.c

Quoted 7 lines
Quoted 7 lines

diff --git a/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file mode 100644 index 0000000..e19b0a4 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt @@ -0,0 +1,24 @@ +LPC32XX Motor PWM controller + +The LPC32xx motor PWMs have two output pin, A and B, with B=!A.

We have LPC32xx in most places in the kernel, except where upper case is mandated, like in CONFIG_ options.

Roland

author
#4 lore raw

PATCH v2 2/2 ARM: LPC32xx: Add the motor PWM to base dts file

In reply to Alban Bedel
Signed-off-by Alban Bedel <[email protected]>
---
 arch/arm/boot/dts/lpc32xx.dtsi |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/boot/dts/lpc32xx.dtsi +7 -0
index ef0fdc7..a1e6a9b 100644
@@ -182,6 +182,13 @@
 				pnx,timeout = <0x64>;
 			};
 
+			mpwm: mpwm at 400E8000 {
+				compatible = "nxp,lpc3220-motor-pwm";
+				reg = <0x400E8000 0x78>;
+				status = "disabled";
+				#pwm-cells = <2>;
+			};
+
 			i2cusb: i2c at 31020300 {
 				compatible = "nxp,pnx-i2c";
 				reg = <0x31020300 0x100>;
Signature · 1 line
1.7.0.4
#5 lore raw

PATCH 2/2 ARM: LPC32xx: Add the motor PWM to base dts file

Quoted 13 lines · Alban Bedel
Signed-off-by Alban Bedel <[email protected]>
---
 arch/arm/boot/dts/lpc32xx.dtsi |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
arch/arm/boot/dts/lpc32xx.dtsi +0 -0
index ef0fdc7..a1e6a9b 100644
@@ -182,6 +182,13 @@
 				pnx,timeout = <0x64>;
 			};

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

Quoted 10 lines
arch/arm/boot/dts/lpc32xx.dtsi +7 -0
+			mpwm: mpwm at 400E8000 {
+				compatible = "nxp,lpc3220-motor-pwm";
+				reg = <0x400E8000 0x78>;
+				status = "disabled";
+				#pwm-cells = <2>;
+			};
+
 			i2cusb: i2c at 31020300 {
 				compatible = "nxp,pnx-i2c";
 				reg = <0x31020300 0x100>;

Will take this into my lpc32xx/dts branch.

Thanks,

Roland