[PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Grant Erickson]

This patch adds support to request and use one or more of the OMAP
dual-mode timers as a generic PWM device compatible with other generic
PWM drivers such as the PWM backlight or PWM beeper driver.

Boards can register such devices using platform data such as in the following
example:

	static struct omap2_pwm_platform_config pwm_config = {
	    .timer_id           = 10,   // GPT10_PWM_EVT
	    .polarity           = 1     // Active-high
	};

	static struct platform_device pwm_device = {
	    .name               = "omap-pwm",
	    .id                 = 0,
	    .dev                = {
	        .platform_data  = &pwm_config
	    }
	};

Signed-off-by: Grant Erickson <marathon96@gmail.com>

---

 arch/arm/plat-omap/Kconfig            |    9 +
 arch/arm/plat-omap/Makefile           |    2 +
 arch/arm/plat-omap/include/plat/pwm.h |   29 ++
 arch/arm/plat-omap/pwm.c              |  450 +++++++++++++++++++++++++++++++++
 4 files changed, 490 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 92c5bb7..4797b0e 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -164,6 +164,15 @@ config OMAP_DM_TIMER
 	help
 	 Select this option if you want to use OMAP Dual-Mode timers.
 
+config HAVE_PWM
+       bool "Use PWM timers"
+       depends on OMAP_DM_TIMER
+       help
+         Select this option if you want to be able to request and use
+         one or more of the OMAP dual-mode timers as a generic PWM device
+         compatible with other generic PWM drivers such as the backlight or
+         beeper driver.
+
 config OMAP_SERIAL_WAKE
 	bool "Enable wake-up events for serial ports"
 	depends on ARCH_OMAP1 && OMAP_MUX
diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
index a4a1285..9e5347b 100644
--- a/arch/arm/plat-omap/Makefile
+++ b/arch/arm/plat-omap/Makefile
@@ -32,3 +32,5 @@ obj-y += $(i2c-omap-m) $(i2c-omap-y)
 obj-$(CONFIG_OMAP_MBOX_FWK) += mailbox.o
 
 obj-$(CONFIG_OMAP_PM_NOOP) += omap-pm-noop.o
+
+obj-$(CONFIG_HAVE_PWM) += pwm.o
diff --git a/arch/arm/plat-omap/include/plat/pwm.h b/arch/arm/plat-omap/include/plat/pwm.h
new file mode 100644
index 0000000..04030cd
--- /dev/null
+++ b/arch/arm/plat-omap/include/plat/pwm.h
@@ -0,0 +1,29 @@
+/*
+ *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
+ *
+ *    This program is free software; you can redistribute it and/or
+ *    modify it under the terms of the GNU General Public License
+ *    version 2 as published by the Free Software Foundation.
+ *
+ *    Description:
+ *      This file is defines platform-specific configuration data for
+ *      the OMAP generic PWM platform driver.
+ */
+
+#ifndef _OMAP2_PWM_H
+#define _OMAP2_PWM_H
+
+/**
+ * struct omap2_pwm_platform_config - OMAP platform-specific data for PWMs
+ * @timer_id: the OMAP dual-mode timer ID.
+ * @polarity: the polarity (active-high or -low) of the PWM.
+ *
+ * This identifies the OMAP dual-mode timer (dmtimer) that will be bound
+ * to the PWM.
+ */
+struct omap2_pwm_platform_config {
+	int timer_id;
+	bool polarity;
+};
+
+#endif /* _OMAP2_PWM_H */
diff --git a/arch/arm/plat-omap/pwm.c b/arch/arm/plat-omap/pwm.c
new file mode 100644
index 0000000..c6d103d
--- /dev/null
+++ b/arch/arm/plat-omap/pwm.c
@@ -0,0 +1,450 @@
+/*
+ *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
+ *
+ *    This program is free software; you can redistribute it and/or
+ *    modify it under the terms of the GNU General Public License
+ *    version 2 as published by the Free Software Foundation.
+ *
+ *    Description:
+ *      This file is the core OMAP2/3 support for the generic, Linux
+ *      PWM driver / controller, using the OMAP's dual-mode timers.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/pwm.h>
+#include <mach/hardware.h>
+#include <plat/dmtimer.h>
+#include <plat/pwm.h>
+
+/* Preprocessor Definitions */
+
+#undef OMAP_PWM_DEBUG
+
+#if defined(OMAP_PWM_DEBUG)
+#define DBG(args...)			\
+	do {						\
+		pr_info(args);			\
+	} while (0)
+#define DEV_DBG(dev, args...)	\
+	do {						\
+		dev_info(dev, args);	\
+	} while (0)
+#else
+#define DBG(args...)			\
+	do { } while (0)
+#define DEV_DBG(dev, args...)	\
+	do { } while (0)
+#endif /* defined(OMAP_PWM_DEBUG) */
+
+#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
+
+/* Type Definitions */
+
+/**
+ * struct pwm_device - opaque internal PWM device instance state
+ * @head: list head for all PWMs managed by this driver.
+ * @pdev: corresponding platform device associated with this device instance.
+ * @dm_timer: corresponding dual-mode timer associated with this device
+ *  instance.
+ * @config: platform-specific configuration data.
+ * @label: description label.
+ * @use_count: use count.
+ * @pwm_id: generic PWM ID requested for this device instance.
+ *
+ * As far as clients of the PWM driver are concerned, PWM devices are
+ * opaque abstract objects. Consequently, this structure is used for
+ * tracking internal device instance state but is otherwise just a
+ * instance reference externally.
+ */
+
+struct pwm_device {
+	struct list_head				   head;
+	struct platform_device			  *pdev;
+	struct omap_dm_timer			  *dm_timer;
+	struct omap2_pwm_platform_config   config;
+	const char						  *label;
+	unsigned int					   use_count;
+	unsigned int					   pwm_id;
+};
+
+/* Function Prototypes */
+
+static int __devinit omap_pwm_probe(struct platform_device *pdev);
+static int __devexit omap_pwm_remove(struct platform_device *pdev);
+
+/* Global Variables */
+
+static struct platform_driver omap_pwm_driver = {
+	.driver		= {
+		.name	= "omap-pwm",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= omap_pwm_probe,
+	.remove		= __devexit_p(omap_pwm_remove)
+};
+
+/* List and associated lock for managing generic PWM devices bound to
+ * this driver.
+ */
+
+static DEFINE_MUTEX(pwm_lock);
+static LIST_HEAD(pwm_list);
+
+/**
+ * pwm_request - request and allocate the specified generic PWM device.
+ * @pwm_id: The identifier associated with the desired generic PWM device.
+ * @label: An optional pointer to a C string describing the usage of the
+ *         requested generic PWM device.
+ *
+ * Returns a pointer to the requested generic PWM device on success;
+ * otherwise, NULL on error.
+ */
+struct pwm_device *pwm_request(int pwm_id, const char *label)
+{
+	struct pwm_device *pwm = NULL;
+	bool found = false;
+
+	mutex_lock(&pwm_lock);
+
+	/* Walk the list of available PWMs and attempt to find a matching
+	 * ID, regardless of whether it is in use or not.
+	 */
+
+	list_for_each_entry(pwm, &pwm_list, head) {
+		if (pwm->pwm_id == pwm_id) {
+			found = true;
+			break;
+		}
+	}
+
+	if (found) {
+		if (pwm->use_count == 0) {
+			pwm->use_count++;
+			pwm->label = label;
+		} else {
+			pwm = ERR_PTR(-EBUSY);
+		}
+	} else {
+		pwm = ERR_PTR(-ENOENT);
+	}
+
+	mutex_unlock(&pwm_lock);
+
+	return pwm;
+}
+EXPORT_SYMBOL(pwm_request);
+
+/**
+ * pwm_free - deallocate/release a previously-requested generic PWM device.
+ * @pwm: A pointer to the generic PWM device to release.
+ */
+void pwm_free(struct pwm_device *pwm)
+{
+	mutex_lock(&pwm_lock);
+
+	if (pwm->use_count) {
+		pwm->use_count--;
+		pwm->label = NULL;
+	} else {
+		pr_err("PWM%d has already been freed.\n", pwm->pwm_id);
+	}
+
+	mutex_unlock(&pwm_lock);
+}
+EXPORT_SYMBOL(pwm_free);
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ *            counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+	const unsigned long nanoseconds_per_second = 1000000000;
+	int cycles;
+	__u64 c;
+
+	c = (__u64)clk_rate * ns;
+	do_div(c, nanoseconds_per_second);
+	cycles = c;
+
+	return DM_TIMER_LOAD_MIN - cycles;
+}
+
+/**
+ * pwm_config - configures the generic PWM device to the specified parameters.
+ * @pwm: A pointer to the PWM device to configure.
+ * @duty_ns: The duty period of the PWM, in nanoseconds.
+ * @period_ns: The overall period of the PWM, in nanoseconds.
+ *
+ * Returns 0 if the generic PWM device was successfully configured;
+ * otherwise, < 0 on error.
+ */
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+{
+	int status = 0;
+	const bool enable = true;
+	const bool autoreload = true;
+	const bool toggle = true;
+	const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
+	int load_value, match_value;
+	unsigned long clk_rate;
+
+	DEV_DBG(&pwm->pdev->dev,
+			"duty cycle: %d, period %d\n",
+			duty_ns, period_ns);
+
+	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(pwm->dm_timer));
+
+	/* Calculate the appropriate load and match values based on the
+	 * specified period and duty cycle. The load value determines the
+	 * cycle time and the match value determines the duty cycle.
+	 */
+
+	load_value = pwm_calc_value(clk_rate, period_ns);
+	match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
+
+	/* We MUST enable yet stop the associated dual-mode timer before
+	 * attempting to write its registers.
+	 */
+
+	omap_dm_timer_enable(pwm->dm_timer);
+	omap_dm_timer_stop(pwm->dm_timer);
+
+	omap_dm_timer_set_load(pwm->dm_timer, autoreload, load_value);
+	omap_dm_timer_set_match(pwm->dm_timer, enable, match_value);
+
+	DEV_DBG(&pwm->pdev->dev,
+			"load value: %#08x (%d), "
+			"match value: %#08x (%d)\n",
+			load_value, load_value,
+			match_value, match_value);
+
+	omap_dm_timer_set_pwm(pwm->dm_timer,
+						  !pwm->config.polarity,
+						  toggle,
+						  trigger);
+
+	/* Set the counter to generate an overflow event immediately. */
+
+	omap_dm_timer_write_counter(pwm->dm_timer, DM_TIMER_LOAD_MIN);
+
+	/* Now that we're done configuring the dual-mode timer, disable it
+	 * again. We'll enable and start it later, when requested.
+	 */
+
+	omap_dm_timer_disable(pwm->dm_timer);
+
+	return status;
+}
+EXPORT_SYMBOL(pwm_config);
+
+/**
+ * pwm_enable - enable the generic PWM device.
+ * @pwm: A pointer to the generic PWM device to enable.
+ *
+ * Returns 0 if the generic PWM device was successfully enabled;
+ * otherwise, < 0 on error.
+ */
+int pwm_enable(struct pwm_device *pwm)
+{
+	int status = 0;
+
+	/* Enable the counter--always--before attempting to write its
+	 * registers and then set the timer to its minimum load value to
+	 * ensure we get an overflow event right away once we start it.
+	 */
+
+	omap_dm_timer_enable(pwm->dm_timer);
+	omap_dm_timer_write_counter(pwm->dm_timer, DM_TIMER_LOAD_MIN);
+	omap_dm_timer_start(pwm->dm_timer);
+
+	return status;
+}
+EXPORT_SYMBOL(pwm_enable);
+
+/**
+ * pwm_disable - disable the generic PWM device.
+ * @pwm: A pointer to the generic PWM device to disable.
+ */
+void pwm_disable(struct pwm_device *pwm)
+{
+	omap_dm_timer_enable(pwm->dm_timer);
+	omap_dm_timer_stop(pwm->dm_timer);
+	omap_dm_timer_disable(pwm->dm_timer);
+}
+EXPORT_SYMBOL(pwm_disable);
+
+/**
+ * omap_pwm_probe - check for the PWM and bind it to the driver.
+ * @pdev: A pointer to the platform device node associated with the
+ *        PWM instance to be probed for driver binding.
+ *
+ * Returns 0 if the PWM instance was successfully bound to the driver;
+ * otherwise, < 0 on error.
+ */
+static int __devinit omap_pwm_probe(struct platform_device *pdev)
+{
+	struct pwm_device *pwm = NULL;
+	struct omap2_pwm_platform_config *pdata = NULL;
+	int status = 0;
+
+	pdata = ((struct omap2_pwm_platform_config *)(pdev->dev.platform_data));
+
+	BUG_ON(pdata == NULL);
+
+	if (pdata == NULL) {
+		dev_err(&pdev->dev, "Could not find required platform data.\n");
+		status = -ENOENT;
+		goto done;
+	}
+
+	/* Allocate memory for the driver-private PWM data and state */
+
+	pwm = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
+
+	if (pwm == NULL) {
+		dev_err(&pdev->dev, "Could not allocate memory.\n");
+		status = -ENOMEM;
+		goto done;
+	}
+
+	/* Request the OMAP dual-mode timer that will be bound to and
+	 * associated with this generic PWM.
+	 */
+
+	pwm->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);
+
+	if (pwm->dm_timer == NULL) {
+		status = -ENOENT;
+		goto err_free;
+	}
+
+	/* Configure the source for the dual-mode timer backing this
+	 * generic PWM device. The clock source will ultimately determine
+	 * how small or large the PWM frequency can be.
+	 *
+	 * At some point, it's probably worth revisiting moving this to
+	 * the configure method and choosing either the slow- or
+	 * system-clock source as appropriate for the desired PWM period.
+	 */
+
+	omap_dm_timer_set_source(pwm->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
+
+	/* Cache away other miscellaneous driver-private data and state
+	 * information and add the driver-private data to the platform
+	 * device.
+	 */
+
+	pwm->pdev = pdev;
+	pwm->pwm_id = pdev->id;
+	pwm->config = *pdata;
+
+	platform_set_drvdata(pdev, pwm);
+
+	/* Finally, push the added generic PWM device to the end of the
+	 * list of available generic PWM devices.
+	 */
+
+	mutex_lock(&pwm_lock);
+	list_add_tail(&pwm->head, &pwm_list);
+	mutex_unlock(&pwm_lock);
+
+	status = 0;
+	goto done;
+
+ err_free:
+	kfree(pwm);
+
+ done:
+	return status;
+}
+
+/**
+ * omap_pwm_remove - unbind the specified PWM platform device from the driver.
+ * @pdev: A pointer to the platform device node associated with the
+ *        PWM instance to be unbound/removed.
+ *
+ * Returns 0 if the PWM was successfully removed as a platform device;
+ * otherwise, < 0 on error.
+ */
+static int __devexit omap_pwm_remove(struct platform_device *pdev)
+{
+	struct pwm_device *pwm = NULL;
+	int status = 0;
+
+	/* Attempt to get the driver-private data from the platform device
+	 * node.
+	 */
+
+	pwm = platform_get_drvdata(pdev);
+
+	if (pwm == NULL) {
+		status = -ENODEV;
+		goto done;
+	}
+
+	/* Remove the generic PWM device from the list of available
+	 * generic PWM devices.
+	 */
+
+	mutex_lock(&pwm_lock);
+	list_del(&pwm->head);
+	mutex_unlock(&pwm_lock);
+
+	/* Unbind the OMAP dual-mode timer associated with the generic PWM
+	 * device.
+	 */
+
+	omap_dm_timer_free(pwm->dm_timer);
+
+	/* Finally, release the memory associated with the driver-private
+	 * data and state.
+	 */
+
+	kfree(pwm);
+
+ done:
+	return status;
+}
+
+/**
+ * omap_pwm_init - driver/module insertion entry point
+ *
+ * This routine is the driver/module insertion entry point. It
+ * registers the driver as a platform driver.
+ *
+ * Returns 0 if the driver/module was successfully registered as a
+ * platform driver driver; otherwise, < 0 on error.
+ */
+static int __init omap_pwm_init(void)
+{
+	return platform_driver_register(&omap_pwm_driver);
+}
+
+/**
+ * omap_pwm_exit - driver/module removal entry point
+ *
+ * This routine is the driver/module removal entry point. It
+ * unregisters the driver as a platform driver.
+ */
+static void __exit omap_pwm_exit(void)
+{
+	platform_driver_unregister(&omap_pwm_driver);
+}
+
+arch_initcall(omap_pwm_init);
+module_exit(omap_pwm_exit);
+
+MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
+MODULE_LICENSE("GPLv2");
+MODULE_VERSION("2010-11-09");

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Hemanth V]

----- Original Message ----- 
From: "Grant Erickson" <marathon96@gmail.com>
To: <linux-omap@vger.kernel.org>
Cc: "Tony Lindgren" <tony@atomide.com>
Sent: Monday, November 15, 2010 12:58 AM
Subject: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode 
Timers


> This patch adds support to request and use one or more of the OMAP
> dual-mode timers as a generic PWM device compatible with other generic
> PWM drivers such as the PWM backlight or PWM beeper driver.

Generally PWMs are supported on PMIC chip.  Is this driver intended for use
when OMAP is used standalone.

>
> Boards can register such devices using platform data such as in the 
> following
> example:
>
> static struct omap2_pwm_platform_config pwm_config = {
>     .timer_id           = 10,   // GPT10_PWM_EVT

Since only specific GPTs can be configured for PWM, can some check
we added in probe function.

>     .polarity           = 1     // Active-high
> };
>
> static struct platform_device pwm_device = {
>     .name               = "omap-pwm",
>     .id                 = 0,
>     .dev                = {
>         .platform_data  = &pwm_config
>     }
> };
>
> Signed-off-by: Grant Erickson <marathon96@gmail.com>
>
> ---
>
> arch/arm/plat-omap/Kconfig            |    9 +
> arch/arm/plat-omap/Makefile           |    2 +
> arch/arm/plat-omap/include/plat/pwm.h |   29 ++
> arch/arm/plat-omap/pwm.c              |  450 
> +++++++++++++++++++++++++++++++++

Can the above file  be part of mach-omap2, since comment say its 
specifically
for OMAP2/3 or will this work on OMAP1 also.


> 4 files changed, 490 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
> index 92c5bb7..4797b0e 100644
> --- a/arch/arm/plat-omap/Kconfig
> +++ b/arch/arm/plat-omap/Kconfig
> @@ -164,6 +164,15 @@ config OMAP_DM_TIMER
>  help
>  Select this option if you want to use OMAP Dual-Mode timers.
>
> +config HAVE_PWM
> +       bool "Use PWM timers"
> +       depends on OMAP_DM_TIMER
> +       help
> +         Select this option if you want to be able to request and use
> +         one or more of the OMAP dual-mode timers as a generic PWM device
> +         compatible with other generic PWM drivers such as the backlight 
> or
> +         beeper driver.
> +
> config OMAP_SERIAL_WAKE
>  bool "Enable wake-up events for serial ports"
>  depends on ARCH_OMAP1 && OMAP_MUX
> diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
> index a4a1285..9e5347b 100644
> --- a/arch/arm/plat-omap/Makefile
> +++ b/arch/arm/plat-omap/Makefile
> @@ -32,3 +32,5 @@ obj-y += $(i2c-omap-m) $(i2c-omap-y)
> obj-$(CONFIG_OMAP_MBOX_FWK) += mailbox.o
>
> obj-$(CONFIG_OMAP_PM_NOOP) += omap-pm-noop.o
> +
> +obj-$(CONFIG_HAVE_PWM) += pwm.o
> diff --git a/arch/arm/plat-omap/include/plat/pwm.h 
> b/arch/arm/plat-omap/include/plat/pwm.h
> new file mode 100644
> index 0000000..04030cd
> --- /dev/null
> +++ b/arch/arm/plat-omap/include/plat/pwm.h
> @@ -0,0 +1,29 @@
> +/*
> + *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
> + *
> + *    This program is free software; you can redistribute it and/or
> + *    modify it under the terms of the GNU General Public License
> + *    version 2 as published by the Free Software Foundation.
> + *
> + *    Description:
> + *      This file is defines platform-specific configuration data for
> + *      the OMAP generic PWM platform driver.
> + */
> +
> +#ifndef _OMAP2_PWM_H
> +#define _OMAP2_PWM_H
> +
> +/**
> + * struct omap2_pwm_platform_config - OMAP platform-specific data for 
> PWMs
> + * @timer_id: the OMAP dual-mode timer ID.
> + * @polarity: the polarity (active-high or -low) of the PWM.
> + *
> + * This identifies the OMAP dual-mode timer (dmtimer) that will be bound
> + * to the PWM.
> + */
> +struct omap2_pwm_platform_config {
> + int timer_id;
> + bool polarity;
> +};
> +
> +#endif /* _OMAP2_PWM_H */
> diff --git a/arch/arm/plat-omap/pwm.c b/arch/arm/plat-omap/pwm.c
> new file mode 100644
> index 0000000..c6d103d
> --- /dev/null
> +++ b/arch/arm/plat-omap/pwm.c
> @@ -0,0 +1,450 @@
> +/*
> + *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
> + *
> + *    This program is free software; you can redistribute it and/or
> + *    modify it under the terms of the GNU General Public License
> + *    version 2 as published by the Free Software Foundation.
> + *
> + *    Description:
> + *      This file is the core OMAP2/3 support for the generic, Linux
> + *      PWM driver / controller, using the OMAP's dual-mode timers.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/pwm.h>
> +#include <mach/hardware.h>
> +#include <plat/dmtimer.h>
> +#include <plat/pwm.h>
> +
> +/* Preprocessor Definitions */
> +
> +#undef OMAP_PWM_DEBUG
> +
> +#if defined(OMAP_PWM_DEBUG)
> +#define DBG(args...) \
> + do { \
> + pr_info(args); \
> + } while (0)
> +#define DEV_DBG(dev, args...) \
> + do { \
> + dev_info(dev, args); \
> + } while (0)
> +#else
> +#define DBG(args...) \
> + do { } while (0)
> +#define DEV_DBG(dev, args...) \
> + do { } while (0)
> +#endif /* defined(OMAP_PWM_DEBUG) */
> +
> +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE
> +
> +/* Type Definitions */
> +
> +/**
> + * struct pwm_device - opaque internal PWM device instance state
> + * @head: list head for all PWMs managed by this driver.
> + * @pdev: corresponding platform device associated with this device 
> instance.
> + * @dm_timer: corresponding dual-mode timer associated with this device
> + *  instance.
> + * @config: platform-specific configuration data.
> + * @label: description label.
> + * @use_count: use count.
> + * @pwm_id: generic PWM ID requested for this device instance.
> + *
> + * As far as clients of the PWM driver are concerned, PWM devices are
> + * opaque abstract objects. Consequently, this structure is used for
> + * tracking internal device instance state but is otherwise just a
> + * instance reference externally.
> + */
> +
> +struct pwm_device {
> + struct list_head    head;
> + struct platform_device   *pdev;
> + struct omap_dm_timer   *dm_timer;
> + struct omap2_pwm_platform_config   config;
> + const char   *label;
> + unsigned int    use_count;
> + unsigned int    pwm_id;
> +};
> +
> +/* Function Prototypes */
> +
> +static int __devinit omap_pwm_probe(struct platform_device *pdev);
> +static int __devexit omap_pwm_remove(struct platform_device *pdev);
> +
> +/* Global Variables */
> +
> +static struct platform_driver omap_pwm_driver = {
> + .driver = {
> + .name = "omap-pwm",
> + .owner = THIS_MODULE,
> + },
> + .probe = omap_pwm_probe,
> + .remove = __devexit_p(omap_pwm_remove)
> +};
> +
> +/* List and associated lock for managing generic PWM devices bound to
> + * this driver.
> + */
> +
> +static DEFINE_MUTEX(pwm_lock);
> +static LIST_HEAD(pwm_list);
> +
> +/**
> + * pwm_request - request and allocate the specified generic PWM device.
> + * @pwm_id: The identifier associated with the desired generic PWM 
> device.
> + * @label: An optional pointer to a C string describing the usage of the
> + *         requested generic PWM device.
> + *
> + * Returns a pointer to the requested generic PWM device on success;
> + * otherwise, NULL on error.
> + */
> +struct pwm_device *pwm_request(int pwm_id, const char *label)
> +{
> + struct pwm_device *pwm = NULL;
> + bool found = false;
> +
> + mutex_lock(&pwm_lock);
> +
> + /* Walk the list of available PWMs and attempt to find a matching
> + * ID, regardless of whether it is in use or not.
> + */
> +
> + list_for_each_entry(pwm, &pwm_list, head) {
> + if (pwm->pwm_id == pwm_id) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (found) {
> + if (pwm->use_count == 0) {
> + pwm->use_count++;
> + pwm->label = label;
> + } else {
> + pwm = ERR_PTR(-EBUSY);
> + }
> + } else {
> + pwm = ERR_PTR(-ENOENT);
> + }
> +
> + mutex_unlock(&pwm_lock);
> +
> + return pwm;
> +}
> +EXPORT_SYMBOL(pwm_request);
> +
> +/**
> + * pwm_free - deallocate/release a previously-requested generic PWM 
> device.
> + * @pwm: A pointer to the generic PWM device to release.
> + */
> +void pwm_free(struct pwm_device *pwm)
> +{
> + mutex_lock(&pwm_lock);
> +
> + if (pwm->use_count) {
> + pwm->use_count--;
> + pwm->label = NULL;
> + } else {
> + pr_err("PWM%d has already been freed.\n", pwm->pwm_id);
> + }
> +
> + mutex_unlock(&pwm_lock);
> +}
> +EXPORT_SYMBOL(pwm_free);
> +
> +/**
> + * pwm_calc_value - determines the counter value for a clock rate and 
> period.
> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute 
> the
> + *            counter value for.
> + * @ns: The period, in nanoseconds, to computer the counter value for.
> + *
> + * Returns the PWM counter value for the specified clock rate and period.
> + */
> +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
> +{
> + const unsigned long nanoseconds_per_second = 1000000000;
> + int cycles;
> + __u64 c;
> +
> + c = (__u64)clk_rate * ns;
> + do_div(c, nanoseconds_per_second);
> + cycles = c;
> +
> + return DM_TIMER_LOAD_MIN - cycles;
> +}
> +
> +/**
> + * pwm_config - configures the generic PWM device to the specified 
> parameters.
> + * @pwm: A pointer to the PWM device to configure.
> + * @duty_ns: The duty period of the PWM, in nanoseconds.
> + * @period_ns: The overall period of the PWM, in nanoseconds.
> + *
> + * Returns 0 if the generic PWM device was successfully configured;
> + * otherwise, < 0 on error.
> + */
> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> +{
> + int status = 0;
> + const bool enable = true;
> + const bool autoreload = true;
> + const bool toggle = true;
> + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
> + int load_value, match_value;
> + unsigned long clk_rate;
> +
> + DEV_DBG(&pwm->pdev->dev,
> + "duty cycle: %d, period %d\n",
> + duty_ns, period_ns);
> +
> + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(pwm->dm_timer));
> +
> + /* Calculate the appropriate load and match values based on the
> + * specified period and duty cycle. The load value determines the
> + * cycle time and the match value determines the duty cycle.
> + */
> +
> + load_value = pwm_calc_value(clk_rate, period_ns);
> + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
> +
> + /* We MUST enable yet stop the associated dual-mode timer before
> + * attempting to write its registers.
> + */
> +
> + omap_dm_timer_enable(pwm->dm_timer);
> + omap_dm_timer_stop(pwm->dm_timer);
> +
> + omap_dm_timer_set_load(pwm->dm_timer, autoreload, load_value);
> + omap_dm_timer_set_match(pwm->dm_timer, enable, match_value);
> +
> + DEV_DBG(&pwm->pdev->dev,
> + "load value: %#08x (%d), "
> + "match value: %#08x (%d)\n",
> + load_value, load_value,
> + match_value, match_value);
> +
> + omap_dm_timer_set_pwm(pwm->dm_timer,
> +   !pwm->config.polarity,
> +   toggle,
> +   trigger);
> +
> + /* Set the counter to generate an overflow event immediately. */
> +
> + omap_dm_timer_write_counter(pwm->dm_timer, DM_TIMER_LOAD_MIN);
> +
> + /* Now that we're done configuring the dual-mode timer, disable it
> + * again. We'll enable and start it later, when requested.
> + */
> +
> + omap_dm_timer_disable(pwm->dm_timer);
> +
> + return status;
> +}
> +EXPORT_SYMBOL(pwm_config);
> +
> +/**
> + * pwm_enable - enable the generic PWM device.
> + * @pwm: A pointer to the generic PWM device to enable.
> + *
> + * Returns 0 if the generic PWM device was successfully enabled;
> + * otherwise, < 0 on error.
> + */
> +int pwm_enable(struct pwm_device *pwm)
> +{
> + int status = 0;
> +
> + /* Enable the counter--always--before attempting to write its
> + * registers and then set the timer to its minimum load value to
> + * ensure we get an overflow event right away once we start it.
> + */
> +
> + omap_dm_timer_enable(pwm->dm_timer);
> + omap_dm_timer_write_counter(pwm->dm_timer, DM_TIMER_LOAD_MIN);
> + omap_dm_timer_start(pwm->dm_timer);
> +
> + return status;
> +}
> +EXPORT_SYMBOL(pwm_enable);
> +
> +/**
> + * pwm_disable - disable the generic PWM device.
> + * @pwm: A pointer to the generic PWM device to disable.
> + */
> +void pwm_disable(struct pwm_device *pwm)
> +{
> + omap_dm_timer_enable(pwm->dm_timer);
> + omap_dm_timer_stop(pwm->dm_timer);
> + omap_dm_timer_disable(pwm->dm_timer);
> +}
> +EXPORT_SYMBOL(pwm_disable);
> +
> +/**
> + * omap_pwm_probe - check for the PWM and bind it to the driver.
> + * @pdev: A pointer to the platform device node associated with the
> + *        PWM instance to be probed for driver binding.
> + *
> + * Returns 0 if the PWM instance was successfully bound to the driver;
> + * otherwise, < 0 on error.
> + */
> +static int __devinit omap_pwm_probe(struct platform_device *pdev)
> +{
> + struct pwm_device *pwm = NULL;
> + struct omap2_pwm_platform_config *pdata = NULL;
> + int status = 0;
> +
> + pdata = ((struct omap2_pwm_platform_config *)(pdev->dev.platform_data));
> +
> + BUG_ON(pdata == NULL);
> +
> + if (pdata == NULL) {
> + dev_err(&pdev->dev, "Could not find required platform data.\n");
> + status = -ENOENT;
> + goto done;
> + }
> +
> + /* Allocate memory for the driver-private PWM data and state */
> +
> + pwm = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
> +
> + if (pwm == NULL) {
> + dev_err(&pdev->dev, "Could not allocate memory.\n");
> + status = -ENOMEM;
> + goto done;
> + }
> +
> + /* Request the OMAP dual-mode timer that will be bound to and
> + * associated with this generic PWM.
> + */
> +
> + pwm->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);
> +
> + if (pwm->dm_timer == NULL) {
> + status = -ENOENT;
> + goto err_free;
> + }
> +
> + /* Configure the source for the dual-mode timer backing this
> + * generic PWM device. The clock source will ultimately determine
> + * how small or large the PWM frequency can be.
> + *
> + * At some point, it's probably worth revisiting moving this to
> + * the configure method and choosing either the slow- or
> + * system-clock source as appropriate for the desired PWM period.
> + */
> +
> + omap_dm_timer_set_source(pwm->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
> +
> + /* Cache away other miscellaneous driver-private data and state
> + * information and add the driver-private data to the platform
> + * device.
> + */
> +
> + pwm->pdev = pdev;
> + pwm->pwm_id = pdev->id;
> + pwm->config = *pdata;
> +
> + platform_set_drvdata(pdev, pwm);
> +
> + /* Finally, push the added generic PWM device to the end of the
> + * list of available generic PWM devices.
> + */
> +
> + mutex_lock(&pwm_lock);
> + list_add_tail(&pwm->head, &pwm_list);
> + mutex_unlock(&pwm_lock);
> +
> + status = 0;
> + goto done;
> +
> + err_free:
> + kfree(pwm);
> +
> + done:
> + return status;
> +}
> +
> +/**
> + * omap_pwm_remove - unbind the specified PWM platform device from the 
> driver.
> + * @pdev: A pointer to the platform device node associated with the
> + *        PWM instance to be unbound/removed.
> + *
> + * Returns 0 if the PWM was successfully removed as a platform device;
> + * otherwise, < 0 on error.
> + */
> +static int __devexit omap_pwm_remove(struct platform_device *pdev)
> +{
> + struct pwm_device *pwm = NULL;
> + int status = 0;
> +
> + /* Attempt to get the driver-private data from the platform device
> + * node.
> + */
> +
> + pwm = platform_get_drvdata(pdev);
> +
> + if (pwm == NULL) {
> + status = -ENODEV;
> + goto done;
> + }
> +
> + /* Remove the generic PWM device from the list of available
> + * generic PWM devices.
> + */
> +
> + mutex_lock(&pwm_lock);
> + list_del(&pwm->head);
> + mutex_unlock(&pwm_lock);
> +
> + /* Unbind the OMAP dual-mode timer associated with the generic PWM
> + * device.
> + */
> +
> + omap_dm_timer_free(pwm->dm_timer);
> +
> + /* Finally, release the memory associated with the driver-private
> + * data and state.
> + */
> +
> + kfree(pwm);
> +
> + done:
> + return status;
> +}
> +
> +/**
> + * omap_pwm_init - driver/module insertion entry point
> + *
> + * This routine is the driver/module insertion entry point. It
> + * registers the driver as a platform driver.
> + *
> + * Returns 0 if the driver/module was successfully registered as a
> + * platform driver driver; otherwise, < 0 on error.
> + */
> +static int __init omap_pwm_init(void)
> +{
> + return platform_driver_register(&omap_pwm_driver);
> +}
> +
> +/**
> + * omap_pwm_exit - driver/module removal entry point
> + *
> + * This routine is the driver/module removal entry point. It
> + * unregisters the driver as a platform driver.
> + */
> +static void __exit omap_pwm_exit(void)
> +{
> + platform_driver_unregister(&omap_pwm_driver);
> +}
> +
> +arch_initcall(omap_pwm_init);
> +module_exit(omap_pwm_exit);
> +
> +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
> +MODULE_LICENSE("GPLv2");
> +MODULE_VERSION("2010-11-09");
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Felipe Balbi]

On Sun, Nov 14, 2010 at 01:28:49PM -0600, Grant Erickson wrote:
>+MODULE_LICENSE("GPLv2");

module license should be GPL v2

just read the comment above MODULE_LICENSE definition:

/*
  * The following license idents are currently accepted as indicating free
  * software modules
  *
  *	"GPL"				[GNU Public License v2 or later]
  *	"GPL v2"			[GNU Public License v2]
  *	"GPL and additional rights"	[GNU Public License v2 rights and more]
  *	"Dual BSD/GPL"			[GNU Public License v2
  *					 or BSD license choice]
  *	"Dual MIT/GPL"			[GNU Public License v2
  *					 or MIT license choice]
  *	"Dual MPL/GPL"			[GNU Public License v2
  *					 or Mozilla license choice]
  *
  * The following other idents are available
  *
  *	"Proprietary"			[Non free products]
  *
  * There are dual licensed components, but when running with Linux it is the
  * GPL that is relevant so this is a non issue. Similarly LGPL linked with GPL
  * is a GPL combined work.
  *
  * This exists for several reasons
  * 1.	So modinfo can show license info for users wanting to vet their setup 
  *	is free
  * 2.	So the community can ignore bug reports including proprietary modules
  * 3.	So vendors can do likewise based on their own policies
  */

-- 
balbi

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Kevin Hilman]

Grant Erickson <marathon96@gmail.com> writes:

> This patch adds support to request and use one or more of the OMAP
> dual-mode timers as a generic PWM device compatible with other generic
> PWM drivers such as the PWM backlight or PWM beeper driver.

How will this co-exist with the PWM on the twl6030 PMIC
(drivers/mfd/twl6030-pwm.c.)    Both are exporting the same API.

Regarding multilpe PWM drivers, there has been discussion on the
linux-embedded list[1] regarding a more generic PWM framework, and a few
other projects working on PWM for OMAP[2].  Someone needs to step up and
consolidate/push this work forward as there is a clear need for a more
generic PWM infrastructure on various SoCs.  Your work is another
example of similar work that needs to be coordinated with the other projects.

> Boards can register such devices using platform data such as in the following
> example:
>
> 	static struct omap2_pwm_platform_config pwm_config = {
> 	    .timer_id           = 10,   // GPT10_PWM_EVT
> 	    .polarity           = 1     // Active-high
> 	};

Board code should not have to know which timers are PWM capable.  This
information is fixed at the SoC level.  

This information is available and is part of the in-progress conversion
of the timers to use the omap_hwmod infrastructure.  There are subsets
of the timers that have various features (1ms timers, PWM capable, etc.)
Using hwmod classes, they can be grouped together such that board code
does not have to know which timers on which SoC are PWM capable.   It
can simply request a timer with certain capabilities.

Kevin

[1] http://marc.info/?l=linux-embedded&m=128594628918269&w=2
[2] http://marc.info/?l=linux-embedded&m=128599642406671&w=2

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Grant Erickson]

On 11/16/10 4:37 AM, Hemanth V wrote:
> ----- Original Message -----
> From: "Grant Erickson" <marathon96@gmail.com>
> Sent: Monday, November 15, 2010 12:58 AM
> 
>> This patch adds support to request and use one or more of the OMAP
>> dual-mode timers as a generic PWM device compatible with other generic
>> PWM drivers such as the PWM backlight or PWM beeper driver.
> 
> Generally PWMs are supported on PMIC chip.  Is this driver intended for use
> when OMAP is used standalone?

Correct. 

>> Boards can register such devices using platform data such as in the
>> following
>> example:
>> 
>> static struct omap2_pwm_platform_config pwm_config = {
>>     .timer_id           = 10,   // GPT10_PWM_EVT
> 
> Since only specific GPTs can be configured for PWM, can some check
> we added in probe function?

Yes, that check is already present in the probe function:

    pwm->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);

    if (pwm->dm_timer == NULL) {
        status = -ENOENT;
        goto err_free;
    }

> Can the above file  be part of mach-omap2, since comment say its
> specifically for OMAP2/3 or will this work on OMAP1 also?

It should work wherever dmtimers are supported. To the extent that dmtimer.c
is in plat-omap/, I located pwm.c there as well.

Thanks for the feedback!

Best,

Grant

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Grant Erickson]

On 11/16/10 4:42 AM, Felipe Balbi wrote:
> On Sun, Nov 14, 2010 at 01:28:49PM -0600, Grant Erickson wrote:
>> +MODULE_LICENSE("GPLv2");
> 
> module license should be GPL v2

Thanks for the feedback. I'll make that adjustment.

Best,

Grant

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Grant Erickson]

On 11/16/10 9:01 AM, Kevin Hilman wrote:
> Grant Erickson <marathon96@gmail.com> writes:
>> This patch adds support to request and use one or more of the OMAP
>> dual-mode timers as a generic PWM device compatible with other generic
>> PWM drivers such as the PWM backlight or PWM beeper driver.
> 
> How will this co-exist with the PWM on the twl6030 PMIC
> (drivers/mfd/twl6030-pwm.c.)? Both are exporting the same API.

That's an excellent question. This driver started life in the 2.6.32 tree
where twl6030-pwm.c didn't exist. Thanks for the heads-up.

The right short-term solution is to probably change the configuration from:

    config HAVE_PWM

to:

    config OMAP_PWM
        select HAVE_PWM

and then have it conflict with TWL6030_PWM if that's enabled.

> Regarding multilpe PWM drivers, there has been discussion on the
> linux-embedded list[1] regarding a more generic PWM framework, and a few
> other projects working on PWM for OMAP[2].  Someone needs to step up and
> consolidate/push this work forward as there is a clear need for a more
> generic PWM infrastructure on various SoCs.  Your work is another
> example of similar work that needs to be coordinated with the other projects.
> 
>> Boards can register such devices using platform data such as in the following
>> example:
>> 
>> static struct omap2_pwm_platform_config pwm_config = {
>>    .timer_id           = 10,   // GPT10_PWM_EVT
>>    .polarity           = 1     // Active-high
>> };
> 
> Board code should not have to know which timers are PWM capable.  This
> information is fixed at the SoC level.

Fair enough; however, board code does have to know which PWM outputs are
tied to which piece of hardware being driven by it. In the OMAP3, there
isn't an any-to-any mapping of PWM timers to PWM-capable output pins.

So, in this case, I explicitly:

1) configure that pin multiplexer as GPT10_PWM_EVT.
2) make available GPT10_PWM_EVT as a generic PWM because that's the one
   precisely tied to the display backlight.
3) ensure that the backlight driver asks for the right generic PWM
   associated with it.
  
> This information is available and is part of the in-progress conversion
> of the timers to use the omap_hwmod infrastructure.  There are subsets
> of the timers that have various features (1ms timers, PWM capable, etc.)
> Using hwmod classes, they can be grouped together such that board code
> does not have to know which timers on which SoC are PWM capable.   It
> can simply request a timer with certain capabilities.
> 
> Kevin
> 
> [1] http://marc.info/?l=linux-embedded&m=128594628918269&w=2
> [2] http://marc.info/?l=linux-embedded&m=128599642406671&w=2

I'll take a look at those references.

With the appropriate configuration change to avoid the conflict with
TWL6030-PWM, it's probably better to have this driver in-tree than not.
It'll give those working on the PWM abstraction and rework another working
use case to integrate and work from.

Thanks for the feedback and the references.

Best,

Grant

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Kevin Hilman]

Grant Erickson <marathon96@gmail.com> writes:

> On 11/16/10 9:01 AM, Kevin Hilman wrote:
>> Grant Erickson <marathon96@gmail.com> writes:
>>> This patch adds support to request and use one or more of the OMAP
>>> dual-mode timers as a generic PWM device compatible with other generic
>>> PWM drivers such as the PWM backlight or PWM beeper driver.
>> 
>> How will this co-exist with the PWM on the twl6030 PMIC
>> (drivers/mfd/twl6030-pwm.c.)? Both are exporting the same API.
>
> That's an excellent question. This driver started life in the 2.6.32 tree
> where twl6030-pwm.c didn't exist. Thanks for the heads-up.
>
> The right short-term solution is to probably change the configuration from:
>
>     config HAVE_PWM
>
> to:
>
>     config OMAP_PWM
>         select HAVE_PWM
>
> and then have it conflict with TWL6030_PWM if that's enabled.

And what happens when other PWM sources are added?

>> Regarding multilpe PWM drivers, there has been discussion on the
>> linux-embedded list[1] regarding a more generic PWM framework, and a few
>> other projects working on PWM for OMAP[2].  Someone needs to step up and
>> consolidate/push this work forward as there is a clear need for a more
>> generic PWM infrastructure on various SoCs.  Your work is another
>> example of similar work that needs to be coordinated with the other projects.
>> 
>>> Boards can register such devices using platform data such as in the following
>>> example:
>>> 
>>> static struct omap2_pwm_platform_config pwm_config = {
>>>    .timer_id           = 10,   // GPT10_PWM_EVT
>>>    .polarity           = 1     // Active-high
>>> };
>> 
>> Board code should not have to know which timers are PWM capable.  This
>> information is fixed at the SoC level.
>
> Fair enough; however, board code does have to know which PWM outputs are
> tied to which piece of hardware being driven by it. In the OMAP3, there
> isn't an any-to-any mapping of PWM timers to PWM-capable output pins.
>
> So, in this case, I explicitly:
>
> 1) configure that pin multiplexer as GPT10_PWM_EVT.
> 2) make available GPT10_PWM_EVT as a generic PWM because that's the one
>    precisely tied to the display backlight.
> 3) ensure that the backlight driver asks for the right generic PWM
>    associated with it.
>   
>> This information is available and is part of the in-progress conversion
>> of the timers to use the omap_hwmod infrastructure.  There are subsets
>> of the timers that have various features (1ms timers, PWM capable, etc.)
>> Using hwmod classes, they can be grouped together such that board code
>> does not have to know which timers on which SoC are PWM capable.   It
>> can simply request a timer with certain capabilities.
>> 
>> Kevin
>> 
>> [1] http://marc.info/?l=linux-embedded&m=128594628918269&w=2
>> [2] http://marc.info/?l=linux-embedded&m=128599642406671&w=2
>
> I'll take a look at those references.
>
> With the appropriate configuration change to avoid the conflict with
> TWL6030-PWM, it's probably better to have this driver in-tree than not.

Well, Tony will make the final call here, but I disagree that this
should merge in its current form.

Before something like this can merge, I would rather see

1) generic PWM framework pushed along and merged
2) the dmtimer hwmod conversion finished

Yes, I know it's a lot more work to fix the core/framework code before
having a feature included, but having something more generic that can
actually support multiple PWM sources is clearly needed. 

Kevin

> It'll give those working on the PWM abstraction and rework another working
> use case to integrate and work from.
>
> Thanks for the feedback and the references.
>
> Best,
>
> Grant

RE: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Grant Erickson
> Sent: Monday, November 15, 2010 12:59 AM
> To: linux-omap@vger.kernel.org
> Cc: Tony Lindgren
> Subject: [PATCH] Add OMAP Support for Generic PWM Devices 
> using Dual-mode Timers
> 
> This patch adds support to request and use one or more of the OMAP
> dual-mode timers as a generic PWM device compatible with other generic
> PWM drivers such as the PWM backlight or PWM beeper driver.
> 
> Boards can register such devices using platform data such as 
> in the following
> example:
> 
> 	static struct omap2_pwm_platform_config pwm_config = {
> 	    .timer_id           = 10,   // GPT10_PWM_EVT
> 	    .polarity           = 1     // Active-high
> 	};
> 
> 	static struct platform_device pwm_device = {
> 	    .name               = "omap-pwm",
> 	    .id                 = 0,
> 	    .dev                = {
> 	        .platform_data  = &pwm_config
> 	    }
> 	};
> 
> Signed-off-by: Grant Erickson <marathon96@gmail.com>

[snip]...[snip]

[sp] Few additional comments that should be considered while submitting
     next patch revision.

> +/* Preprocessor Definitions */
> +
> +#undef OMAP_PWM_DEBUG
> +
> +#if defined(OMAP_PWM_DEBUG)
> +#define DBG(args...)			\
> +	do {						\
> +		pr_info(args);			\
> +	} while (0)
> +#define DEV_DBG(dev, args...)	\
> +	do {						\
> +		dev_info(dev, args);	\
> +	} while (0)
> +#else
> +#define DBG(args...)			\
> +	do { } while (0)
> +#define DEV_DBG(dev, args...)	\
> +	do { } while (0)
> +#endif /* defined(OMAP_PWM_DEBUG) */

[sp] pr_info(), dev_info() are already macros - conditional to DEBUG.
     We should use them instead of defining another layer.

> +
> +#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
> +
> +/* Type Definitions */
> +
> +/**
> + * struct pwm_device - opaque internal PWM device instance state
> + * @head: list head for all PWMs managed by this driver.
> + * @pdev: corresponding platform device associated with this 
> device instance.
> + * @dm_timer: corresponding dual-mode timer associated with 
> this device
> + *  instance.
> + * @config: platform-specific configuration data.
> + * @label: description label.
> + * @use_count: use count.
> + * @pwm_id: generic PWM ID requested for this device instance.
> + *
> + * As far as clients of the PWM driver are concerned, PWM devices are
> + * opaque abstract objects. Consequently, this structure is used for
> + * tracking internal device instance state but is otherwise just a
> + * instance reference externally.
> + */
> +
> +struct pwm_device {
> +	struct list_head				   head;
> +	struct platform_device			  *pdev;
> +	struct omap_dm_timer			  *dm_timer;
> +	struct omap2_pwm_platform_config   config;
> +	const char						
>   *label;
> +	unsigned int					   use_count;
> +	unsigned int					   pwm_id;
> +};

[sp] This block has mix of tabs and white spaces. We should be consistent.
     There are similar instances below as well...

     You may also want to line up the field names - they start at different columns.

> +
> +/* Function Prototypes */
> +
> +static int __devinit omap_pwm_probe(struct platform_device *pdev);
> +static int __devexit omap_pwm_remove(struct platform_device *pdev);
> +
> +/* Global Variables */
> +
> +static struct platform_driver omap_pwm_driver = {
> +	.driver		= {
> +		.name	= "omap-pwm",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= omap_pwm_probe,
> +	.remove		= __devexit_p(omap_pwm_remove)
> +};
> +
> +/* List and associated lock for managing generic PWM devices bound to
> + * this driver.
> + */
> +
[sp] White space not needed. Similar instances below as well.

> +static DEFINE_MUTEX(pwm_lock);
> +static LIST_HEAD(pwm_list);
> +

[snip]...[snip]

> +/**
> + * pwm_config - configures the generic PWM device to the 
> specified parameters.
> + * @pwm: A pointer to the PWM device to configure.
> + * @duty_ns: The duty period of the PWM, in nanoseconds.
> + * @period_ns: The overall period of the PWM, in nanoseconds.
> + *
> + * Returns 0 if the generic PWM device was successfully configured;
> + * otherwise, < 0 on error.
> + */
> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> +{
> +	int status = 0;
> +	const bool enable = true;
> +	const bool autoreload = true;
> +	const bool toggle = true;
> +	const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;

[sp] Based on code abobe, I believe that OMAP...COMPARE is macro;
     shouldn't we use it as is - where trigger is used in code below?

> +	int load_value, match_value;
> +	unsigned long clk_rate;
> +
> +	DEV_DBG(&pwm->pdev->dev,
> +			"duty cycle: %d, period %d\n",
> +			duty_ns, period_ns);
> +
> +	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(pwm->dm_timer));
> +
> +	/* Calculate the appropriate load and match values based on the
> +	 * specified period and duty cycle. The load value determines the
> +	 * cycle time and the match value determines the duty cycle.
> +	 */
> +
> +	load_value = pwm_calc_value(clk_rate, period_ns);
> +	match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
> +
> +	/* We MUST enable yet stop the associated dual-mode timer before
> +	 * attempting to write its registers.
> +	 */
> +
> +	omap_dm_timer_enable(pwm->dm_timer);
> +	omap_dm_timer_stop(pwm->dm_timer);
> +
> +	omap_dm_timer_set_load(pwm->dm_timer, autoreload, load_value);
> +	omap_dm_timer_set_match(pwm->dm_timer, enable, match_value);
> +
> +	DEV_DBG(&pwm->pdev->dev,
> +			"load value: %#08x (%d), "
> +			"match value: %#08x (%d)\n",
> +			load_value, load_value,
> +			match_value, match_value);
> +
> +	omap_dm_timer_set_pwm(pwm->dm_timer,
> +						  !pwm->config.polarity,
> +						  toggle,
> +						  trigger);
> +
> +	/* Set the counter to generate an overflow event immediately. */
> +
> +	omap_dm_timer_write_counter(pwm->dm_timer, DM_TIMER_LOAD_MIN);

[sp] I am assuming that call to this function would be followed by
     another call to pwm_enable().

     omap_dm_timer_write_counter() is being called in pwm_enable()
     with exactly same arguments. Is is necessary duplication?
     If no, call here should be removed.

> +
> +	/* Now that we're done configuring the dual-mode timer, disable it
> +	 * again. We'll enable and start it later, when requested.
> +	 */
> +
> +	omap_dm_timer_disable(pwm->dm_timer);
> +
> +	return status;
> +}
> +EXPORT_SYMBOL(pwm_config);
> +
> +/**
> + * pwm_enable - enable the generic PWM device.
> + * @pwm: A pointer to the generic PWM device to enable.
> + *
> + * Returns 0 if the generic PWM device was successfully enabled;
> + * otherwise, < 0 on error.
> + */
> +int pwm_enable(struct pwm_device *pwm)
> +{
> +	int status = 0;

[sp] This variable isn't assigned/modified below. Not required.
     function can simply return 0.

> +
> +	/* Enable the counter--always--before attempting to write its
> +	 * registers and then set the timer to its minimum load value to
> +	 * ensure we get an overflow event right away once we start it.
> +	 */
> +
> +	omap_dm_timer_enable(pwm->dm_timer);
> +	omap_dm_timer_write_counter(pwm->dm_timer, DM_TIMER_LOAD_MIN);
> +	omap_dm_timer_start(pwm->dm_timer);
> +
> +	return status;
> +}
> +EXPORT_SYMBOL(pwm_enable);
> +
> +/**
> + * pwm_disable - disable the generic PWM device.
> + * @pwm: A pointer to the generic PWM device to disable.
> + */
> +void pwm_disable(struct pwm_device *pwm)
> +{
> +	omap_dm_timer_enable(pwm->dm_timer);
> +	omap_dm_timer_stop(pwm->dm_timer);
> +	omap_dm_timer_disable(pwm->dm_timer);
> +}
> +EXPORT_SYMBOL(pwm_disable);
> +
> +/**
> + * omap_pwm_probe - check for the PWM and bind it to the driver.
> + * @pdev: A pointer to the platform device node associated with the
> + *        PWM instance to be probed for driver binding.
> + *
> + * Returns 0 if the PWM instance was successfully bound to 
> the driver;
> + * otherwise, < 0 on error.
> + */
> +static int __devinit omap_pwm_probe(struct platform_device *pdev)
> +{
> +	struct pwm_device *pwm = NULL;
> +	struct omap2_pwm_platform_config *pdata = NULL;
> +	int status = 0;
> +
> +	pdata = ((struct omap2_pwm_platform_config 
> *)(pdev->dev.platform_data));
> +
> +	BUG_ON(pdata == NULL);
> +
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "Could not find required 
> platform data.\n");
> +		status = -ENOENT;
> +		goto done;

[sp] We can simply return status from here. goto and label can be avoided.

> +	}
> +
> +	/* Allocate memory for the driver-private PWM data and state */
> +
> +	pwm = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
> +
> +	if (pwm == NULL) {
> +		dev_err(&pdev->dev, "Could not allocate memory.\n");
> +		status = -ENOMEM;
> +		goto done;

[sp] We can simply return status from here. goto and label can be avoided.

> +	}
> +
> +	/* Request the OMAP dual-mode timer that will be bound to and
> +	 * associated with this generic PWM.
> +	 */
> +
> +	pwm->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);
> +
> +	if (pwm->dm_timer == NULL) {
> +		status = -ENOENT;
> +		goto err_free;
> +	}
> +
> +	/* Configure the source for the dual-mode timer backing this
> +	 * generic PWM device. The clock source will ultimately 
> determine
> +	 * how small or large the PWM frequency can be.
> +	 *
> +	 * At some point, it's probably worth revisiting moving this to
> +	 * the configure method and choosing either the slow- or
> +	 * system-clock source as appropriate for the desired 
> PWM period.
> +	 */
> +
> +	omap_dm_timer_set_source(pwm->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
> +
> +	/* Cache away other miscellaneous driver-private data and state
> +	 * information and add the driver-private data to the platform
> +	 * device.
> +	 */
> +
> +	pwm->pdev = pdev;
> +	pwm->pwm_id = pdev->id;
> +	pwm->config = *pdata;
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	/* Finally, push the added generic PWM device to the end of the
> +	 * list of available generic PWM devices.
> +	 */
> +
> +	mutex_lock(&pwm_lock);
> +	list_add_tail(&pwm->head, &pwm_list);
> +	mutex_unlock(&pwm_lock);
> +
> +	status = 0;
> +	goto done;
> +
> + err_free:
> +	kfree(pwm);
> +
> + done:
> +	return status;
> +}
> +
> +/**
> + * omap_pwm_remove - unbind the specified PWM platform 
> device from the driver.
> + * @pdev: A pointer to the platform device node associated with the
> + *        PWM instance to be unbound/removed.
> + *
> + * Returns 0 if the PWM was successfully removed as a 
> platform device;
> + * otherwise, < 0 on error.
> + */
> +static int __devexit omap_pwm_remove(struct platform_device *pdev)
> +{
> +	struct pwm_device *pwm = NULL;
> +	int status = 0;
> +
> +	/* Attempt to get the driver-private data from the 
> platform device
> +	 * node.
> +	 */
> +
> +	pwm = platform_get_drvdata(pdev);
> +
> +	if (pwm == NULL) {
> +		status = -ENODEV;
> +		goto done;

[sp] We can simply return status from here. goto and label can be avoided.

> +	}
> +
> +	/* Remove the generic PWM device from the list of available
> +	 * generic PWM devices.
> +	 */
> +
> +	mutex_lock(&pwm_lock);
> +	list_del(&pwm->head);
> +	mutex_unlock(&pwm_lock);
> +
> +	/* Unbind the OMAP dual-mode timer associated with the 
> generic PWM
> +	 * device.
> +	 */
> +
> +	omap_dm_timer_free(pwm->dm_timer);
> +
> +	/* Finally, release the memory associated with the 
> driver-private
> +	 * data and state.
> +	 */
> +
> +	kfree(pwm);
> +
> + done:
> +	return status;
> +}

[snip]...[snip]

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Grant Erickson]

On 11/16/10 10:54 AM, Premi, Sanjeev wrote:
> [sp] pr_info(), dev_info() are already macros - conditional to DEBUG.
>      We should use them instead of defining another layer.

The macros pr_devel, pr_dbg, and dev_dbg are so conditionalized; however,
*_info are straight pass-throughs to printk and dev_printk, respectively.

>> +struct pwm_device {
>> + struct list_head       head;
>> + struct platform_device     *pdev;
>> + struct omap_dm_timer     *dm_timer;
>> + struct omap2_pwm_platform_config   config;
>> + const char      
>>   *label;
>> + unsigned int        use_count;
>> + unsigned int        pwm_id;
>> +};
> 
> [sp] This block has mix of tabs and white spaces. We should be consistent.
>      There are similar instances below as well...
> 
>      You may also want to line up the field names - they start at different
> columns.

Will do. Oddly, these sailed through checkpatch.pl.

>> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
>> +{
>> + int status = 0;
>> + const bool enable = true;
>> + const bool autoreload = true;
>> + const bool toggle = true;
>> + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
> 
> [sp] Based on code abobe, I believe that OMAP...COMPARE is macro;
>      shouldn't we use it as is - where trigger is used in code below?

We certainly could; however, I found the above constants to make the code
far more self-documenting.

>> + if (pdata == NULL) {
>> +  dev_err(&pdev->dev, "Could not find required
>> platform data.\n");
>> +  status = -ENOENT;
>> +  goto done;
> 
> [sp] We can simply return status from here. goto and label can be avoided.
> 
>> + }
>> +
>> + /* Allocate memory for the driver-private PWM data and state */
>> +
>> + pwm = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
>> +
>> + if (pwm == NULL) {
>> +  dev_err(&pdev->dev, "Could not allocate memory.\n");
>> +  status = -ENOMEM;
>> +  goto done;
> 
> [sp] We can simply return status from here. goto and label can be avoided.

We certainly can; however, I prefer to code my functions so there's a single
exit point or block, even if it's a trivial return. See chapter 7 in
Documentation/CodingStyle.txt.

Thanks for your feedback and comments.

Regards,

Grant

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Grant Erickson]

On 11/16/10 10:43 AM, Kevin Hilman wrote:
> Grant Erickson <marathon96@gmail.com> writes:
>> On 11/16/10 9:01 AM, Kevin Hilman wrote:
>>> Grant Erickson <marathon96@gmail.com> writes:
>>>> This patch adds support to request and use one or more of the OMAP
>>>> dual-mode timers as a generic PWM device compatible with other generic
>>>> PWM drivers such as the PWM backlight or PWM beeper driver.
>>> 
>>> How will this co-exist with the PWM on the twl6030 PMIC
>>> (drivers/mfd/twl6030-pwm.c.)? Both are exporting the same API.
>> 
>> That's an excellent question. This driver started life in the 2.6.32 tree
>> where twl6030-pwm.c didn't exist. Thanks for the heads-up.
>> 
>> The right short-term solution is to probably change the configuration from:
>> 
>>     config HAVE_PWM
>> 
>> to:
>> 
>>     config OMAP_PWM
>>         select HAVE_PWM
>> 
>> and then have it conflict with TWL6030_PWM if that's enabled.
> 
> And what happens when other PWM sources are added?

More conflicts, of course.

>> With the appropriate configuration change to avoid the conflict with
>> TWL6030-PWM, it's probably better to have this driver in-tree than not.
> 
> Well, Tony will make the final call here, but I disagree that this
> should merge in its current form.

Understood.

> Before something like this can merge, I would rather see
> 
> 1) generic PWM framework pushed along and merged
> 2) the dmtimer hwmod conversion finished
> 
> Yes, I know it's a lot more work to fix the core/framework code before
> having a feature included, but having something more generic that can
> actually support multiple PWM sources is clearly needed.

No disagreement on the long-term architectural and design goals. All good
stuff.

However, patches have to be submitted against the repository and branch we
have today, not those we might have tomorrow.

When (1) is in place in the linux-omap GIT, I am happy to work on
refactoring the driver as necessary.

Thanks again for your feedback and input.

Best,

Grant 

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Kevin Hilman]

Grant Erickson <marathon96@gmail.com> writes:

[...]

>> Before something like this can merge, I would rather see
>> 
>> 1) generic PWM framework pushed along and merged
>> 2) the dmtimer hwmod conversion finished
>> 
>> Yes, I know it's a lot more work to fix the core/framework code before
>> having a feature included, but having something more generic that can
>> actually support multiple PWM sources is clearly needed.
>
> No disagreement on the long-term architectural and design goals. All good
> stuff.
>
> However, patches have to be submitted against the repository and branch we
> have today, not those we might have tomorrow.

This comment is where we diverge. 

An alternative to waiting for the generic framework cleanup/refactor
work is to contribute to it and help it along.

Kevin

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Tony Lindgren]

* Kevin Hilman <khilman@deeprootsystems.com> [101116 11:43]:
> Grant Erickson <marathon96@gmail.com> writes:
> 
> [...]
> 
> >> Before something like this can merge, I would rather see
> >> 
> >> 1) generic PWM framework pushed along and merged
> >> 2) the dmtimer hwmod conversion finished
> >> 
> >> Yes, I know it's a lot more work to fix the core/framework code before
> >> having a feature included, but having something more generic that can
> >> actually support multiple PWM sources is clearly needed.
> >
> > No disagreement on the long-term architectural and design goals. All good
> > stuff.
> >
> > However, patches have to be submitted against the repository and branch we
> > have today, not those we might have tomorrow.
> 
> This comment is where we diverge. 
> 
> An alternative to waiting for the generic framework cleanup/refactor
> work is to contribute to it and help it along.

Yeah I agree, with Kevin. We should not export any more omap specific
functions in arch/arm/*omap*/ as they always lead into maintenance
nightmares later on. We still have several merge cycles of work just
to get rid of the existing exported functions.

Regards,

Tony

Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Hemanth V]

----- Original Message ----- 
From: "Grant Erickson" <marathon96@gmail.com>
To: "Hemanth V" <hemanthv@ti.com>
Cc: "Tony Lindgren" <tony@atomide.com>; <linux-omap@vger.kernel.org>
Sent: Tuesday, November 16, 2010 11:15 PM
Subject: Re: [PATCH] Add OMAP Support for Generic PWM Devices using 
Dual-mode Timers


>
>>> Boards can register such devices using platform data such as in the
>>> following
>>> example:
>>>
>>> static struct omap2_pwm_platform_config pwm_config = {
>>>     .timer_id           = 10,   // GPT10_PWM_EVT
>>
>> Since only specific GPTs can be configured for PWM, can some check
>> we added in probe function?
>
> Yes, that check is already present in the probe function:
>
>    pwm->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);
>
>    if (pwm->dm_timer == NULL) {
>        status = -ENOENT;
>        goto err_free;
>    }
>

Since only GPT8 to GPT11 can be configured for PWM output on OMAP3,
I was thinking if a suitable check can be added.

Thanks
Hemanth