Skip to contentSkip to conversation

pwm: Add PWM driver for OMAP using dual-mode timers

PATCH In review Neil Armstrong 3 participants 4 messages Nov 2, 2015 – Dec 17, 2015

Subsystems (2)
Device tree bindings PWM
lore
Share & tools
lore

Neil Armstrong

author
#0 lore raw

PATCH 2/3 pwm: Add PWM driver for OMAP using dual-mode timers

Adds support for using a OMAP dual-mode timer with PWM capability as a Linux PWM device. The driver controls the timer by using the dmtimer API.

Add a platform_data structure for each pwm-omap-dmtimer nodes containing the dmtimers functions in order to get driver not rely on platform specific functions.

Cc Grant Erickson <[email protected]>
Cc NeilBrown <[email protected]>
Cc Joachim Eastwood <[email protected]>
Suggested-by Tony Lindgren <[email protected]>
Signed-off-by Neil Armstrong <[email protected]>
---
 .../devicetree/bindings/pwm/pwm-omap-dmtimer.txt   |  18 ++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-omap-dmtimer.c                     | 322 +++++++++++++++++++++
 include/linux/platform_data/pwm_omap_dmtimer.h     |  69 +++++
 5 files changed, 419 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt
 create mode 100644 drivers/pwm/pwm-omap-dmtimer.c
 create mode 100644 include/linux/platform_data/pwm_omap_dmtimer.h
Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt new file +18 -0
new file mode 100644
index 0000000..5befb53
@@ -0,0 +1,18 @@
+* OMAP PWM for dual-mode timers
+
+Required properties:
+- compatible: Shall contain "ti,omap-dmtimer-pwm".
+- ti,timers: phandle to PWM capable OMAP timer. See arm/omap/timer.txt for info
+  about these timers.
+- #pwm-cells: Should be 3. See pwm.txt in this directory for a description of
+  the cells format.
+
+Optional properties:
+- ti,prescaler: Should be a value between 0 and 7, see the timers datasheet
+
+Example:
+	pwm9: dmtimer-pwm at 9 {
+		compatible = "ti,omap-dmtimer-pwm";
+		ti,timers = <&timer9>;
+		#pwm-cells = <3>;
+	};
drivers/pwm/Kconfig +9 -0
index 062630a..2da60f8 100644
@@ -240,6 +240,15 @@ config PWM_MXS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-mxs.
 
+config PWM_OMAP_DMTIMER
+	tristate "OMAP Dual-Mode Timer PWM support"
+	depends on OF && ARCH_OMAP && OMAP_DM_TIMER
+	help
+	  Generic PWM framework driver for OMAP Dual-Mode Timer PWM output
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-omap-dmtimer
+
 config PWM_PCA9685
 	tristate "NXP PCA9685 PWM driver"
 	depends on OF && I2C
drivers/pwm/Makefile +1 -0
index a0e00c0..af14774 100644
@@ -21,6 +21,7 @@ obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
drivers/pwm/pwm-omap-dmtimer.c new file +322 -0

322 lines not on this page · raw message

include/linux/platform_data/pwm_omap_dmtimer.h new file +69 -0

69 lines not on this page · raw message

Signature · 1 line
1.9.1
Acked-by on v1 2/3
#1 lore raw
Quoted 8 lines · Neil Armstrong

Adds support for using a OMAP dual-mode timer with PWM capability as a Linux PWM device. The driver controls the timer by using the dmtimer API.

Add a platform_data structure for each pwm-omap-dmtimer nodes containing the dmtimers functions in order to get driver not rely on platform specific functions.

Quoted 5 lines
Cc Grant Erickson <[email protected]>
Cc NeilBrown <[email protected]>
Cc Joachim Eastwood <[email protected]>
Suggested-by Tony Lindgren <[email protected]>
Signed-off-by Neil Armstrong <[email protected]>

This looks good to me:

Acked-by Tony Lindgren <[email protected]>

Thierry, seems you can pick this driver patch separately if no more comments, I'll pick up the other two patches in this series.

Regards,

Tony

#2 lore raw
In reply to Neil Armstrong
Quoted 13 lines · Neil Armstrong

Adds support for using a OMAP dual-mode timer with PWM capability as a Linux PWM device. The driver controls the timer by using the dmtimer API.

Add a platform_data structure for each pwm-omap-dmtimer nodes containing the dmtimers functions in order to get driver not rely on platform specific functions.

Cc Grant Erickson <[email protected]>
Cc NeilBrown <[email protected]>
Cc Joachim Eastwood <[email protected]>
Suggested-by Tony Lindgren <[email protected]>
Signed-off-by Neil Armstrong <[email protected]>
Quoted 10 lines
---
 .../devicetree/bindings/pwm/pwm-omap-dmtimer.txt   |  18 ++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-omap-dmtimer.c                     | 322 +++++++++++++++++++++
 include/linux/platform_data/pwm_omap_dmtimer.h     |  69 +++++
 5 files changed, 419 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt
 create mode 100644 drivers/pwm/pwm-omap-dmtimer.c
 create mode 100644 include/linux/platform_data/pwm_omap_dmtimer.h

I've applied this with some coding style bikeshedding applied. Also I think there's a timer leak in the probe function:

Quoted 1 line
drivers/pwm/pwm-omap-dmtimer.c +0 -0

[...]

Quoted 2 lines
drivers/pwm/pwm-omap-dmtimer.c +2 -0
+static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
+{

[...]

drivers/pwm/pwm-omap-dmtimer.c line 241

Quoted 3 lines
drivers/pwm/pwm-omap-dmtimer.c +3 -0
+	dm_timer = pdata->request_by_node(timer);
+	if (!dm_timer)
+		return -EPROBE_DEFER;

dm_timer holds the requested timer now.

drivers/pwm/pwm-omap-dmtimer.c line 245

Quoted 4 lines
drivers/pwm/pwm-omap-dmtimer.c +4 -0
+
+	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
+	if (!omap)
+		return -ENOMEM;

But it's not released when this allocation fails...

drivers/pwm/pwm-omap-dmtimer.c line 253

Quoted 8 lines
drivers/pwm/pwm-omap-dmtimer.c +8 -0
+
+	omap->pdata = pdata;
+	omap->dm_timer = dm_timer;
+	omap->dm_timer_pdev = of_find_device_by_node(timer);
+	if (!omap->dm_timer_pdev) {
+		dev_err(&pdev->dev, "Unable to find timer pdev\n");
+		return -EINVAL;
+	}

... nor when this lookup fails. I've taken the liberty of adding two calls to omap->pdata->free(dm_timer) to these error paths.

Please take a look at what's in the pwm/for-next branch to see if it still works correctly.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151216/3a336f85/attachment.sig>

author
#3 lore raw

Hi Thierry,

Quoted 2 lines · Thierry Reding

I've applied this with some coding style bikeshedding applied. Also I think there's a timer leak in the probe function:

Indeed, the coding style had some root for ameliorations ! Thanks ! I also missed this timer leak, thanks for the fix.

Quoted 19 lines
Quoted 2 lines
drivers/pwm/pwm-omap-dmtimer.c +0 -0

[...]

Quoted 2 lines
drivers/pwm/pwm-omap-dmtimer.c +2 -0
+static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
+{

[...]

drivers/pwm/pwm-omap-dmtimer.c line 241

Quoted 3 lines
drivers/pwm/pwm-omap-dmtimer.c +3 -0
+	dm_timer = pdata->request_by_node(timer);
+	if (!dm_timer)
+		return -EPROBE_DEFER;

dm_timer holds the requested timer now.

drivers/pwm/pwm-omap-dmtimer.c line 245

Quoted 4 lines
drivers/pwm/pwm-omap-dmtimer.c +4 -0
+
+	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
+	if (!omap)
+		return -ENOMEM;

But it's not released when this allocation fails...

Quoted 11 lines

drivers/pwm/pwm-omap-dmtimer.c line 253

Quoted 8 lines
drivers/pwm/pwm-omap-dmtimer.c +8 -0
+
+	omap->pdata = pdata;
+	omap->dm_timer = dm_timer;
+	omap->dm_timer_pdev = of_find_device_by_node(timer);
+	if (!omap->dm_timer_pdev) {
+		dev_err(&pdev->dev, "Unable to find timer pdev\n");
+		return -EINVAL;
+	}

... nor when this lookup fails. I've taken the liberty of adding two calls to omap->pdata->free(dm_timer) to these error paths.

Perfect !

Quoted 2 lines

Please take a look at what's in the pwm/for-next branch to see if it still works correctly.

I had a look against my original patch and it should be ok, I will still hook it up back on the real HW in case we forgot something.

Quoted 3 lines

Thanks,
Thierry

Thanks !
Neil

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151217/ee7ebea6/attachment.sig>