[PATCH] OMAP: add pwm driver using dmtimers.

[PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 10621 bytes --]



This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.

This driver instead uses the new framework interface.

Cc: Grant Erickson <marathon96@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-mxs.
 
+config PWM_OMAP
+	tristate "OMAP pwm support"
+	depends on ARCH_OMAP
+	help
+	  Generic PWM framework driver for OMAP
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-omap
+
 config PWM_PUV3
 	tristate "PKUnity NetBook-0916 PWM support"
 	depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP)		+= pwm-omap.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 0000000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *    Copyright (c) 2012 NeilBrown <neilb@suse.de>
+ *    Heavily based on earlier code which is:
+ *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
+ *
+ *    Also based on pwm-samsung.c
+ *
+ *    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.
+ *
+ *    The 'id' number for the device encodes the number of the dm timer
+ *    to use, and the polarity of the output.
+ *    lsb is '1' of active-high, and '0' for active low
+ *    remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) "pwm-omap: " fmt
+
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+#include <linux/module.h>
+
+#include <plat/dmtimer.h>
+
+#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
+
+struct omap_chip {
+	struct platform_device	*pdev;
+
+	struct omap_dm_timer	*dm_timer;
+	unsigned int		polarity;
+	const char		*label;
+
+	unsigned int		duty_ns, period_ns;
+	struct pwm_chip		chip;
+};
+
+#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)
+
+#define	pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
+
+/**
+ * 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;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct omap_chip *omap = to_omap_chip(chip);
+	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(omap->dm_timer);
+	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
+	omap_dm_timer_start(omap->dm_timer);
+	omap_dm_timer_disable(omap->dm_timer);
+
+	return status;
+}
+
+static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct omap_chip *omap = to_omap_chip(chip);
+
+	omap_dm_timer_stop(omap->dm_timer);
+}
+
+static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			   int duty_ns, int period_ns)
+{
+	struct omap_chip *omap = to_omap_chip(chip);
+	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(chip->dev,
+		"duty cycle: %d, period %d\n",
+		duty_ns, period_ns);
+
+	if (omap->duty_ns == duty_ns &&
+	    omap->period_ns == period_ns)
+		/* No change - don't cause any transients */
+		return 0;
+
+	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->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.  Hopefully it is already
+	 * disabled, but call the (idempotent) pwm_disable just in case
+	 */
+
+	pwm_disable(pwm);
+
+	omap_dm_timer_enable(omap->dm_timer);
+
+	omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
+	omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
+
+	dev_dbg(chip->dev,
+			"load value: %#08x (%d), "
+			"match value: %#08x (%d)\n",
+			load_value, load_value,
+			match_value, match_value);
+
+	omap_dm_timer_set_pwm(omap->dm_timer,
+			      !omap->polarity,
+			      toggle,
+			      trigger);
+
+	/* Set the counter to generate an overflow event immediately. */
+
+	omap_dm_timer_write_counter(omap->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(omap->dm_timer);
+	omap->duty_ns = duty_ns;
+	omap->period_ns = period_ns;
+
+	return status;
+}
+
+
+static struct pwm_ops omap_pwm_ops = {
+	.enable	= omap_pwm_enable,
+	.disable= omap_pwm_disable,
+	.config	= omap_pwm_config,
+	.owner	= THIS_MODULE,
+};
+
+/**
+ * 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 device *dev = &pdev->dev;
+	struct omap_chip *omap;
+	int status = 0;
+	unsigned int id = pdev->id;
+	unsigned int timer = id >> 1; /* lsb is polarity */
+
+	omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
+
+	if (omap == NULL) {
+		dev_err(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.
+	 */
+
+	omap->dm_timer = omap_dm_timer_request_specific(timer);
+
+	if (omap->dm_timer == NULL) {
+		status = -EPROBE_DEFER;
+		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(omap->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.
+	 */
+
+	omap->chip.dev = dev;
+	omap->chip.ops = &omap_pwm_ops;
+	omap->chip.base = -1;
+	omap->chip.npwm = 1;
+	omap->polarity = id & 1;
+
+	status = pwmchip_add(&omap->chip);
+	if (status < 0) {
+		dev_err(dev, "failed to register pwm\n");
+		omap_dm_timer_free(omap->dm_timer);
+		goto err_free;
+	}
+
+	platform_set_drvdata(pdev, omap);
+
+	status = 0;
+	goto done;
+
+ err_free:
+	kfree(omap);
+ 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 omap_chip *omap = platform_get_drvdata(pdev);
+	int status = 0;
+
+	status = pwmchip_remove(&omap->chip);
+	if (status < 0)
+		goto done;
+
+	omap_dm_timer_free(omap->dm_timer);
+
+	kfree(omap);
+
+ done:
+	return status;
+}
+
+#if CONFIG_PM
+static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct omap_chip *omap = platform_get_drvdata(pdev);
+	/* No one preserve these values during suspend so reset them
+	 * Otherwise driver leaves PWM unconfigured if same values
+	 * passed to pwm_config
+	 */
+	omap->period_ns = 0;
+	omap->duty_ns = 0;
+
+	return 0;
+}
+#else
+#define omap_pwm_suspend	NULL
+#endif
+
+static struct platform_driver omap_pwm_driver = {
+	.driver.name	= "omap-pwm",
+	.driver.owner	= THIS_MODULE,
+	.probe		= omap_pwm_probe,
+	.remove		= __devexit_p(omap_pwm_remove),
+	.suspend	= omap_pwm_suspend,
+	.resume		= NULL,
+};
+
+static int __init omap_pwm_init(void)
+{
+	return platform_driver_register(&omap_pwm_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_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_LICENSE("GPLv2");
+MODULE_VERSION("2012-12-01");

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 13988 bytes --]

On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
> 
> 
> This patch is based on an earlier patch by Grant Erickson
> which provided pwm devices using the 'legacy' interface.
> 
> This driver instead uses the new framework interface.

I'd prefer some kind of description about the driver here. Also the
subject should be something like:

	pwm: Add OMAP support using dual-mode timers

or

	pwm: omap: Add PWM support using dual-mode timers

I take this description to mean that OMAP doesn't have dedicated PWM
hardware? Otherwise it might be bad to call this pwm-omap.

Also please use all-caps when referring to PWM devices in prose. A few
other comments inline below.

> Cc: Grant Erickson <marathon96@gmail.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..7df573a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -85,6 +85,15 @@ config PWM_MXS
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-mxs.
>  
> +config PWM_OMAP
> +	tristate "OMAP pwm support"

"OMAP PWM support"

> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
[...]
> + *    The 'id' number for the device encodes the number of the dm timer
> + *    to use, and the polarity of the output.
> + *    lsb is '1' of active-high, and '0' for active low
> + *    remaining bit a timer number and need to be shifted down before use.

I don't know if this is such a good idea. Usually you number platform
devices sequentially, while this would leave gaps in the numbering. I
know that adding platform data may sound a bit like overkill, but I
really think the added clarity and consistency is worth it.

> +#define pr_fmt(fmt) "pwm-omap: " fmt

You don't seem to be using any of the pr_*() logging functions, so this
isn't needed.

> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/pwm.h>
> +#include <linux/module.h>
> +
> +#include <plat/dmtimer.h>
> +
> +#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
> +
> +struct omap_chip {
> +	struct platform_device	*pdev;

I don't see this field being used anywhere.

> +	struct omap_dm_timer	*dm_timer;
> +	unsigned int		polarity;

The PWM subsystem already has enum pwm_polarity for this.

> +	const char		*label;

This isn't used anywhere either.

> +
> +	unsigned int		duty_ns, period_ns;
> +	struct pwm_chip		chip;
> +};
> +
> +#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)
> +
> +#define	pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)

This is never used.

> +
> +/**
> + * pwm_calc_value - determines the counter value for a clock rate and period.

Nit: You should either start the sentence with a capital or not
terminate it with a full stop.

> + * @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.

"compute"

> + *
> + * 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;

Maybe use NSEC_PER_SEC instead?

> +	int cycles;
> +	__u64 c;

I think for in-kernel use, the custom is to stick with simply u64.

> +
> +	c = (__u64)clk_rate * ns;
> +	do_div(c, nanoseconds_per_second);
> +	cycles = c;
> +
> +	return DM_TIMER_LOAD_MIN - cycles;

Can't you just do "DM_TIMER_LOAD_MIN - c" and get rid of the cycles
variable altogether?

> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct omap_chip *omap = to_omap_chip(chip);
> +	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.
> +	 */

Block comments should be in the following format:

	/*
	 * foo...
	 * bar...
	 */

> +
> +	omap_dm_timer_enable(omap->dm_timer);
> +	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> +	omap_dm_timer_start(omap->dm_timer);
> +	omap_dm_timer_disable(omap->dm_timer);

So omap_dm_timer_disable() doesn't actually stop the timer? It just
disables the access to the registers?

> +	return status;

"return 0;" and drop the status variable.

> +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)
> +{
> +	struct omap_chip *omap = to_omap_chip(chip);
> +	int status = 0;

This one can be dropped as well.

> +	const bool enable = true;
> +	const bool autoreload = true;
> +	const bool toggle = true;
> +	const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;

I understand that these extra variables are supposed to "document" the
parameters of the functions below. I'm not a huge fan of this approach
because instead the API would better be designed to make it obvious what
the parameters are. Or people can just look at the prototypes to find
out the meaning. But if you really prefer this way I won't object.

> +	int load_value, match_value;
> +	unsigned long clk_rate;
> +
> +	dev_dbg(chip->dev,
> +		"duty cycle: %d, period %d\n",
> +		duty_ns, period_ns);

This all fits on a single line, so no need to break it up.

> +
> +	if (omap->duty_ns == duty_ns &&
> +	    omap->period_ns == period_ns)
> +		/* No change - don't cause any transients */
> +		return 0;

Note to self: this might be a candidate to put in the core.

> +	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->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.
> +	 */

Again wrong block comment style.

> +
> +	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.  Hopefully it is already
> +	 * disabled, but call the (idempotent) pwm_disable just in case
> +	 */

And here as well. While at it you might want to terminate the last
sentence with a full stop.

> +	pwm_disable(pwm);
> +
> +	omap_dm_timer_enable(omap->dm_timer);
> +
> +	omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> +	omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> +
> +	dev_dbg(chip->dev,
> +			"load value: %#08x (%d), "
> +			"match value: %#08x (%d)\n",
> +			load_value, load_value,
> +			match_value, match_value);

Again this doesn't need so much wrapping.

> +
> +	omap_dm_timer_set_pwm(omap->dm_timer,
> +			      !omap->polarity,
> +			      toggle,
> +			      trigger);

This doesn't either. Also you should be explicit about the polarity
parameter, since enum pwm_polarity is an enum and therefore negating it
isn't very nice (it should work though).

You could solve this by doing something like:

	if (omap->polarity == PWM_POLARITY_NORMAL)
		polarity = 1;
	else
		polarity = 0;

> +	/* Set the counter to generate an overflow event immediately. */
> +
> +	omap_dm_timer_write_counter(omap->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.
> +	 */

Also wrong style.

> +	omap_dm_timer_disable(omap->dm_timer);
> +	omap->duty_ns = duty_ns;
> +	omap->period_ns = period_ns;
> +
> +	return status;

"return 0;"

> +}
> +
> +

Gratuitous newline.

> +static struct pwm_ops omap_pwm_ops = {

Should be "static const".

> +	.enable	= omap_pwm_enable,
> +	.disable= omap_pwm_disable,

There should be a space between .disable and =.

> +	.config	= omap_pwm_config,
> +	.owner	= THIS_MODULE,
> +};
> +
> +/**
> + * 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.
> + */

I'm not sure how useful this kerneldoc comment really is. It isn't about
an exported function and developers usually know what the .probe() does.

> +static int __devinit omap_pwm_probe(struct platform_device *pdev)

No more __devinit, please.

> +{
> +	struct device *dev = &pdev->dev;
> +	struct omap_chip *omap;
> +	int status = 0;
> +	unsigned int id = pdev->id;
> +	unsigned int timer = id >> 1; /* lsb is polarity */

I've said this before, I don't think it's a good idea.

> +
> +	omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
> +

Gratuituous newline.

> +	if (omap == NULL) {
> +		dev_err(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.
> +	 */

Block comment style again.

> +
> +	omap->dm_timer = omap_dm_timer_request_specific(timer);
> +

Gratuitous newline.

> +	if (omap->dm_timer == NULL) {
> +		status = -EPROBE_DEFER;
> +		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.
> +	 */

And again.

> +
> +	omap_dm_timer_set_source(omap->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.
> +	 */

And again. =)

> +
> +	omap->chip.dev = dev;
> +	omap->chip.ops = &omap_pwm_ops;
> +	omap->chip.base = -1;
> +	omap->chip.npwm = 1;
> +	omap->polarity = id & 1;
> +
> +	status = pwmchip_add(&omap->chip);
> +	if (status < 0) {
> +		dev_err(dev, "failed to register pwm\n");
> +		omap_dm_timer_free(omap->dm_timer);
> +		goto err_free;
> +	}
> +
> +	platform_set_drvdata(pdev, omap);
> +
> +	status = 0;
> +	goto done;

This can just be "return 0;".

> +
> + err_free:
> +	kfree(omap);
> + 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)

No __devexit, please.

> +{
> +	struct omap_chip *omap = platform_get_drvdata(pdev);
> +	int status = 0;

Just drop this.

> +
> +	status = pwmchip_remove(&omap->chip);
> +	if (status < 0)
> +		goto done;

"return status;"

> +
> +	omap_dm_timer_free(omap->dm_timer);
> +
> +	kfree(omap);
> +
> + done:
> +	return status;

Just "return 0;". No label required.

> +}
> +
> +#if CONFIG_PM
> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct omap_chip *omap = platform_get_drvdata(pdev);
> +	/* No one preserve these values during suspend so reset them
> +	 * Otherwise driver leaves PWM unconfigured if same values
> +	 * passed to pwm_config
> +	 */
> +	omap->period_ns = 0;
> +	omap->duty_ns = 0;
> +
> +	return 0;
> +}
> +#else
> +#define omap_pwm_suspend	NULL
> +#endif

This doesn't look right. You should implement .resume() if you really
care, in which case the resume callback would have to reconfigure with
the cached values. In that case maybe you should switch to dev_pm_ops
and SIMPLE_DEV_PM_OPS() as well.

If you don't, just resetting these values will not make the PWM work
properly after resume either since it will have to be explicitly
reconfigured.

> +static struct platform_driver omap_pwm_driver = {
> +	.driver.name	= "omap-pwm",
> +	.driver.owner	= THIS_MODULE,

The more idiomatic way of writing this is:

	.driver = {
		.name = "omap-pwm",
		.owner = THIS_MODULE,
	},

> +	.probe		= omap_pwm_probe,
> +	.remove		= __devexit_p(omap_pwm_remove),

No __devexit_p anymore.

> +	.suspend	= omap_pwm_suspend,
> +	.resume		= NULL,
> +};
> +
> +static int __init omap_pwm_init(void)
> +{
> +	return platform_driver_register(&omap_pwm_driver);
> +}
> +
> +static void __exit omap_pwm_exit(void)
> +{
> +	platform_driver_unregister(&omap_pwm_driver);
> +}
> +
> +arch_initcall(omap_pwm_init);

This should probably be module_init() instead. You already return
-EPROBE_DEFER if the dual-mode timer isn't ready yet so you should bite
the bullet and get all the dependencies to behave properly as well. New
code shouldn't be using this kind of dependency handling.

In that case you could also just run module_platform_driver().

Thierry

> +module_exit(omap_pwm_exit);
> +
> +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");

Shouldn't this be "Neil Brown"? I noticed you use the concatenated form
in the email address as well, so maybe that's on purpose?

> +MODULE_LICENSE("GPLv2");

This should be "GPL v2". Maybe MODULE_DESCRIPTION() would be nice too.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Jon Hunter]

Hi Neil,

On 12/12/2012 02:24 AM, NeilBrown wrote:
> 
> 
> This patch is based on an earlier patch by Grant Erickson
> which provided pwm devices using the 'legacy' interface.
> 
> This driver instead uses the new framework interface.
> 
> Cc: Grant Erickson <marathon96@gmail.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..7df573a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -85,6 +85,15 @@ config PWM_MXS
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-mxs.
>  
> +config PWM_OMAP
> +	tristate "OMAP pwm support"
> +	depends on ARCH_OMAP

We should probably have depends on or selects OMAP_DM_TIMER here too.

> +	help
> +	  Generic PWM framework driver for OMAP
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-omap
> +
>  config PWM_PUV3
>  	tristate "PKUnity NetBook-0916 PWM support"
>  	depends on ARCH_PUV3
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..f5d200d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
> +obj-$(CONFIG_PWM_OMAP)		+= pwm-omap.o
>  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> new file mode 100644
> index 0000000..e3dbce3
> --- /dev/null
> +++ b/drivers/pwm/pwm-omap.c
> @@ -0,0 +1,318 @@
> +/*
> + *    Copyright (c) 2012 NeilBrown <neilb@suse.de>
> + *    Heavily based on earlier code which is:
> + *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
> + *
> + *    Also based on pwm-samsung.c
> + *
> + *    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

I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.

> + *      PWM driver / controller, using the OMAP's dual-mode timers.
> + *
> + *    The 'id' number for the device encodes the number of the dm timer
> + *    to use, and the polarity of the output.
> + *    lsb is '1' of active-high, and '0' for active low
> + *    remaining bit a timer number and need to be shifted down before use.
> + */
> +
> +#define pr_fmt(fmt) "pwm-omap: " fmt
> +
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/pwm.h>
> +#include <linux/module.h>
> +
> +#include <plat/dmtimer.h>

This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...

> +#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
> +
> +struct omap_chip {
> +	struct platform_device	*pdev;
> +
> +	struct omap_dm_timer	*dm_timer;
> +	unsigned int		polarity;
> +	const char		*label;
> +
> +	unsigned int		duty_ns, period_ns;
> +	struct pwm_chip		chip;
> +};
> +
> +#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)
> +
> +#define	pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
> +
> +/**
> + * 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;
> +}
> +
> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct omap_chip *omap = to_omap_chip(chip);
> +	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(omap->dm_timer);
> +	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> +	omap_dm_timer_start(omap->dm_timer);
> +	omap_dm_timer_disable(omap->dm_timer);

Why not just use omap_dm_timer_load_start() here instead of the above 4
APIs?

> +
> +	return status;
> +}
> +
> +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct omap_chip *omap = to_omap_chip(chip);
> +
> +	omap_dm_timer_stop(omap->dm_timer);
> +}
> +
> +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)
> +{
> +	struct omap_chip *omap = to_omap_chip(chip);
> +	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(chip->dev,
> +		"duty cycle: %d, period %d\n",
> +		duty_ns, period_ns);
> +
> +	if (omap->duty_ns == duty_ns &&
> +	    omap->period_ns == period_ns)
> +		/* No change - don't cause any transients */
> +		return 0;
> +
> +	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->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.  Hopefully it is already
> +	 * disabled, but call the (idempotent) pwm_disable just in case
> +	 */
> +
> +	pwm_disable(pwm);
> +
> +	omap_dm_timer_enable(omap->dm_timer);

Do you need to call omap_dm_timer_enable here? _set_load and _set_match
will enable the timer. So this should not be necessary.

> +	omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> +	omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> +
> +	dev_dbg(chip->dev,
> +			"load value: %#08x (%d), "
> +			"match value: %#08x (%d)\n",
> +			load_value, load_value,
> +			match_value, match_value);
> +
> +	omap_dm_timer_set_pwm(omap->dm_timer,
> +			      !omap->polarity,
> +			      toggle,
> +			      trigger);
> +
> +	/* Set the counter to generate an overflow event immediately. */
> +
> +	omap_dm_timer_write_counter(omap->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(omap->dm_timer);

Similarly the disable should not be needed here either.

> +	omap->duty_ns = duty_ns;
> +	omap->period_ns = period_ns;
> +
> +	return status;
> +}
> +
> +
> +static struct pwm_ops omap_pwm_ops = {
> +	.enable	= omap_pwm_enable,
> +	.disable= omap_pwm_disable,
> +	.config	= omap_pwm_config,
> +	.owner	= THIS_MODULE,
> +};
> +
> +/**
> + * 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)

I believe that __devinit is no longer required.

> +{
> +	struct device *dev = &pdev->dev;
> +	struct omap_chip *omap;
> +	int status = 0;
> +	unsigned int id = pdev->id;
> +	unsigned int timer = id >> 1; /* lsb is polarity */
> +
> +	omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
> +
> +	if (omap == NULL) {
> +		dev_err(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.
> +	 */
> +
> +	omap->dm_timer = omap_dm_timer_request_specific(timer);

I would recommend that you use omap_dm_timer_request_by_cap() (new for
v3.8 so you should be able to use once v3.8-rc1 is out) here to request
a timer that supports the PWM output. The above function will not be
supported when booting with device-tree.

> +
> +	if (omap->dm_timer == NULL) {
> +		status = -EPROBE_DEFER;
> +		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(omap->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.
> +	 */
> +
> +	omap->chip.dev = dev;
> +	omap->chip.ops = &omap_pwm_ops;
> +	omap->chip.base = -1;
> +	omap->chip.npwm = 1;
> +	omap->polarity = id & 1;
> +
> +	status = pwmchip_add(&omap->chip);
> +	if (status < 0) {
> +		dev_err(dev, "failed to register pwm\n");
> +		omap_dm_timer_free(omap->dm_timer);
> +		goto err_free;
> +	}
> +
> +	platform_set_drvdata(pdev, omap);
> +
> +	status = 0;
> +	goto done;
> +
> + err_free:
> +	kfree(omap);
> + 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)

I believe that __devexit is no longer required.

> +{
> +	struct omap_chip *omap = platform_get_drvdata(pdev);
> +	int status = 0;
> +
> +	status = pwmchip_remove(&omap->chip);
> +	if (status < 0)
> +		goto done;
> +
> +	omap_dm_timer_free(omap->dm_timer);

Is it guaranteed that the timer will be disabled at this point?

> +
> +	kfree(omap);
> +
> + done:
> +	return status;
> +}
> +
> +#if CONFIG_PM
> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct omap_chip *omap = platform_get_drvdata(pdev);
> +	/* No one preserve these values during suspend so reset them
> +	 * Otherwise driver leaves PWM unconfigured if same values
> +	 * passed to pwm_config
> +	 */
> +	omap->period_ns = 0;
> +	omap->duty_ns = 0;


Hmmm, looks like you are trying to force a reconfiguration after suspend
if the same values are used. Is there an underlying problem here that
you are trying to workaround?

Please note that I am not familiar with the PWM sub-system to know how
suspend-resume is typically handled and if this is normal or not.

> +	return 0;
> +}
> +#else
> +#define omap_pwm_suspend	NULL
> +#endif
> +
> +static struct platform_driver omap_pwm_driver = {
> +	.driver.name	= "omap-pwm",
> +	.driver.owner	= THIS_MODULE,
> +	.probe		= omap_pwm_probe,
> +	.remove		= __devexit_p(omap_pwm_remove),

I believe that __devexit_p is no longer required.

Otherwise it looks good to me. Thanks for sending!

Cheers
Jon

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Jon Hunter]

On 12/12/2012 05:31 AM, Thierry Reding wrote:
> On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:

[snip]

>> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +	struct omap_chip *omap = to_omap_chip(chip);
>> +	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.
>> +	 */
> 
> Block comments should be in the following format:
> 
> 	/*
> 	 * foo...
> 	 * bar...
> 	 */
> 
>> +
>> +	omap_dm_timer_enable(omap->dm_timer);
>> +	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>> +	omap_dm_timer_start(omap->dm_timer);
>> +	omap_dm_timer_disable(omap->dm_timer);
> 
> So omap_dm_timer_disable() doesn't actually stop the timer? It just
> disables the access to the registers?

I thought this looked odd too ;-)

So what is going on here is that omap_dm_timer_start() calls
omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
last disable really just complements the first enable (ie. decrements
the use count), but the timer will not actually be disabled, because the
start has called an extra enable.

These four function calls can be replaced by one call to
omap_dm_timer_set_load_start() and I think that will be much clearer and
concise.

In general, it should not be necessary to call these
omap_dm_timer_enable/disable APIs directly. I am not sure what the
history is or if there is a use-case that really requires this. So in
the future may be I should make them static so they cannot be used
directly :-)

Cheers
Jon

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 17455 bytes --]

On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
<thierry.reding@avionic-design.de> wrote:

> On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
> > 
> > 
> > This patch is based on an earlier patch by Grant Erickson
> > which provided pwm devices using the 'legacy' interface.
> > 
> > This driver instead uses the new framework interface.
> 
> I'd prefer some kind of description about the driver here.

I'm not really sure what more there is to say.  There was a bit of text in a
comment at the top of the file which I've copied to the commit comment.


>                                                            Also the
> subject should be something like:
> 
> 	pwm: Add OMAP support using dual-mode timers
> 
> or
> 
> 	pwm: omap: Add PWM support using dual-mode timers

Done - I chose the second.

> 
> I take this description to mean that OMAP doesn't have dedicated PWM
> hardware? Otherwise it might be bad to call this pwm-omap.

Correct.  The timers can be used for a number of things which explicitly
includes PWM.

> 
> Also please use all-caps when referring to PWM devices in prose. A few
> other comments inline below.

OK.

> 
> > Cc: Grant Erickson <marathon96@gmail.com>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..7df573a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -85,6 +85,15 @@ config PWM_MXS
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-mxs.
> >  
> > +config PWM_OMAP
> > +	tristate "OMAP pwm support"
> 
> "OMAP PWM support"

Fixed.

> 
> > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> [...]
> > + *    The 'id' number for the device encodes the number of the dm timer
> > + *    to use, and the polarity of the output.
> > + *    lsb is '1' of active-high, and '0' for active low
> > + *    remaining bit a timer number and need to be shifted down before use.
> 
> I don't know if this is such a good idea. Usually you number platform
> devices sequentially, while this would leave gaps in the numbering. I
> know that adding platform data may sound a bit like overkill, but I
> really think the added clarity and consistency is worth it.

I guess so.  No other PWM driver seems to use platform data, and I needed so
little...
I'll see what I can do.


> 
> > +#define pr_fmt(fmt) "pwm-omap: " fmt
> 
> You don't seem to be using any of the pr_*() logging functions, so this
> isn't needed.

Gone now, thanks.


> 
> > +#include <linux/export.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/pwm.h>
> > +#include <linux/module.h>
> > +
> > +#include <plat/dmtimer.h>
> > +
> > +#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
> > +
> > +struct omap_chip {
> > +	struct platform_device	*pdev;
>
> I don't see this field being used anywhere.

No.  Gone.

> 
> > +	struct omap_dm_timer	*dm_timer;
> > +	unsigned int		polarity;
> 
> The PWM subsystem already has enum pwm_polarity for this.
> 

I'll use that then .... and as there  is a pwm_set_polarity() interface, that
probably means that I don't need to configure the polarity via the platform
data?  That would be a lot cleaner.


> > +	const char		*label;
> 
> This isn't used anywhere either.

Gone.

> 
> > +
> > +	unsigned int		duty_ns, period_ns;
> > +	struct pwm_chip		chip;
> > +};
> > +
> > +#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)
> > +
> > +#define	pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
> 
> This is never used.

:-)  There is a theme here.


> 
> > +
> > +/**
> > + * pwm_calc_value - determines the counter value for a clock rate and period.
> 
> Nit: You should either start the sentence with a capital or not
> terminate it with a full stop.

In this case the sentence really includes the function name which is
case-sensitive so cannot be capitalised ;-)
I'll rephrase a bit and find something to capitalise.

> 
> > + * @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.
> 
> "compute"

Yep.

> 
> > + *
> > + * 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;
> 
> Maybe use NSEC_PER_SEC instead?

Good idea, thanks.

> 
> > +	int cycles;
> > +	__u64 c;
> 
> I think for in-kernel use, the custom is to stick with simply u64.

It is, yes.


> 
> > +
> > +	c = (__u64)clk_rate * ns;
> > +	do_div(c, nanoseconds_per_second);
> > +	cycles = c;
> > +
> > +	return DM_TIMER_LOAD_MIN - cycles;
> 
> Can't you just do "DM_TIMER_LOAD_MIN - c" and get rid of the cycles
> variable altogether?

Yep.

> 
> > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct omap_chip *omap = to_omap_chip(chip);
> > +	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.
> > +	 */
> 
> Block comments should be in the following format:
> 
> 	/*
> 	 * foo...
> 	 * bar...
> 	 */

OK, I fixed all of those.

> 
> > +
> > +	omap_dm_timer_enable(omap->dm_timer);
> > +	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > +	omap_dm_timer_start(omap->dm_timer);
> > +	omap_dm_timer_disable(omap->dm_timer);
> 
> So omap_dm_timer_disable() doesn't actually stop the timer? It just
> disables the access to the registers?
> 
> > +	return status;
> 
> "return 0;" and drop the status variable.
> 

Done.

> > +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			   int duty_ns, int period_ns)
> > +{
> > +	struct omap_chip *omap = to_omap_chip(chip);
> > +	int status = 0;
> 
> This one can be dropped as well.
> 
> > +	const bool enable = true;
> > +	const bool autoreload = true;
> > +	const bool toggle = true;
> > +	const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
> 
> I understand that these extra variables are supposed to "document" the
> parameters of the functions below. I'm not a huge fan of this approach
> because instead the API would better be designed to make it obvious what
> the parameters are. Or people can just look at the prototypes to find
> out the meaning. But if you really prefer this way I won't object.

I've removed the 'trigger' const as the name is self-descriptive.
I might leave the others.

> 
> > +	int load_value, match_value;
> > +	unsigned long clk_rate;
> > +
> > +	dev_dbg(chip->dev,
> > +		"duty cycle: %d, period %d\n",
> > +		duty_ns, period_ns);
> 
> This all fits on a single line, so no need to break it up.

Indeed.

> 
> > +
> > +	if (omap->duty_ns == duty_ns &&
> > +	    omap->period_ns == period_ns)
> > +		/* No change - don't cause any transients */
> > +		return 0;
> 
> Note to self: this might be a candidate to put in the core.

might be useful, though the core doesn't currently "know" the current values.

> 
> > +	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->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.
> > +	 */
> 
> Again wrong block comment style.
> 
> > +
> > +	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.  Hopefully it is already
> > +	 * disabled, but call the (idempotent) pwm_disable just in case
> > +	 */
> 
> And here as well. While at it you might want to terminate the last
> sentence with a full stop.
> 

I check the sentences in other comments too.

> > +	pwm_disable(pwm);
> > +
> > +	omap_dm_timer_enable(omap->dm_timer);
> > +
> > +	omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> > +	omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> > +
> > +	dev_dbg(chip->dev,
> > +			"load value: %#08x (%d), "
> > +			"match value: %#08x (%d)\n",
> > +			load_value, load_value,
> > +			match_value, match_value);
> 
> Again this doesn't need so much wrapping.
> 

Nope.

> > +
> > +	omap_dm_timer_set_pwm(omap->dm_timer,
> > +			      !omap->polarity,
> > +			      toggle,
> > +			      trigger);
> 
> This doesn't either. Also you should be explicit about the polarity
> parameter, since enum pwm_polarity is an enum and therefore negating it
> isn't very nice (it should work though).
> 
> You could solve this by doing something like:
> 
> 	if (omap->polarity == PWM_POLARITY_NORMAL)
> 		polarity = 1;
> 	else
> 		polarity = 0;

(omap->polarity == PWM_POLARITY_NORMAL)

would have the same effect.

> 
> > +	/* Set the counter to generate an overflow event immediately. */
> > +
> > +	omap_dm_timer_write_counter(omap->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.
> > +	 */
> 
> Also wrong style.
> 
> > +	omap_dm_timer_disable(omap->dm_timer);
> > +	omap->duty_ns = duty_ns;
> > +	omap->period_ns = period_ns;
> > +
> > +	return status;
> 
> "return 0;"
> 
> > +}
> > +
> > +
> 
> Gratuitous newline.
> 
> > +static struct pwm_ops omap_pwm_ops = {
> 
> Should be "static const".
> 
> > +	.enable	= omap_pwm_enable,
> > +	.disable= omap_pwm_disable,
> 
> There should be a space between .disable and =.

OK.

> 
> > +	.config	= omap_pwm_config,
> > +	.owner	= THIS_MODULE,
> > +};
> > +
> > +/**
> > + * 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.
> > + */
> 
> I'm not sure how useful this kerneldoc comment really is. It isn't about
> an exported function and developers usually know what the .probe() does.

True.  Gone.

> 
> > +static int __devinit omap_pwm_probe(struct platform_device *pdev)
> 
> No more __devinit, please.

If you say so (having no idea what it did :-)


> 
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct omap_chip *omap;
> > +	int status = 0;
> > +	unsigned int id = pdev->id;
> > +	unsigned int timer = id >> 1; /* lsb is polarity */
> 
> I've said this before, I don't think it's a good idea.
> 
> > +
> > +	omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
> > +
> 
> Gratuituous newline.
> 
> > +	if (omap == NULL) {
> > +		dev_err(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.
> > +	 */
> 
> Block comment style again.
> 
> > +
> > +	omap->dm_timer = omap_dm_timer_request_specific(timer);
> > +
> 
> Gratuitous newline.

Gone.

> 
> > +	if (omap->dm_timer == NULL) {
> > +		status = -EPROBE_DEFER;
> > +		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.
> > +	 */
> 
> And again.
> 
> > +
> > +	omap_dm_timer_set_source(omap->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.
> > +	 */
> 
> And again. =)
> 
> > +
> > +	omap->chip.dev = dev;
> > +	omap->chip.ops = &omap_pwm_ops;
> > +	omap->chip.base = -1;
> > +	omap->chip.npwm = 1;
> > +	omap->polarity = id & 1;
> > +
> > +	status = pwmchip_add(&omap->chip);
> > +	if (status < 0) {
> > +		dev_err(dev, "failed to register pwm\n");
> > +		omap_dm_timer_free(omap->dm_timer);
> > +		goto err_free;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, omap);
> > +
> > +	status = 0;
> > +	goto done;
> 
> This can just be "return 0;".

Yep.

> 
> > +
> > + err_free:
> > +	kfree(omap);
> > + 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)
> 
> No __devexit, please.
> 
> > +{
> > +	struct omap_chip *omap = platform_get_drvdata(pdev);
> > +	int status = 0;
> 
> Just drop this.
> 
> > +
> > +	status = pwmchip_remove(&omap->chip);
> > +	if (status < 0)
> > +		goto done;
> 
> "return status;"

Fixed.

> 
> > +
> > +	omap_dm_timer_free(omap->dm_timer);
> > +
> > +	kfree(omap);
> > +
> > + done:
> > +	return status;
> 
> Just "return 0;". No label required.
> 
> > +}
> > +
> > +#if CONFIG_PM
> > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	struct omap_chip *omap = platform_get_drvdata(pdev);
> > +	/* No one preserve these values during suspend so reset them
> > +	 * Otherwise driver leaves PWM unconfigured if same values
> > +	 * passed to pwm_config
> > +	 */
> > +	omap->period_ns = 0;
> > +	omap->duty_ns = 0;
> > +
> > +	return 0;
> > +}
> > +#else
> > +#define omap_pwm_suspend	NULL
> > +#endif
> 
> This doesn't look right. You should implement .resume() if you really
> care, in which case the resume callback would have to reconfigure with
> the cached values. In that case maybe you should switch to dev_pm_ops
> and SIMPLE_DEV_PM_OPS() as well.
> 
> If you don't, just resetting these values will not make the PWM work
> properly after resume either since it will have to be explicitly
> reconfigured.

I just copied that from pwm-samsung.c

I think the point is to avoid the "no transients" short-circuit in
omap_pwm_config if the config is unchanged.

The assumption is that pwm_disable() will be called before suspend and
pwm_config()/pwm_enable() after resume.  So there is no point actually
configuring anything in .resume() - it makes sense to wait until pwm_config()
is called (if ever).  But we want to make sure that pwm_config actually does
something.

But yes, I should change to dev_pm_ops.

> 
> > +static struct platform_driver omap_pwm_driver = {
> > +	.driver.name	= "omap-pwm",
> > +	.driver.owner	= THIS_MODULE,
> 
> The more idiomatic way of writing this is:
> 
> 	.driver = {
> 		.name = "omap-pwm",
> 		.owner = THIS_MODULE,
> 	},
> 

I guess... though I think my way is a lot neater.  But conformity is good.

> > +	.probe		= omap_pwm_probe,
> > +	.remove		= __devexit_p(omap_pwm_remove),
> 
> No __devexit_p anymore.
> 

OK.

> > +	.suspend	= omap_pwm_suspend,
> > +	.resume		= NULL,
> > +};
> > +
> > +static int __init omap_pwm_init(void)
> > +{
> > +	return platform_driver_register(&omap_pwm_driver);
> > +}
> > +
> > +static void __exit omap_pwm_exit(void)
> > +{
> > +	platform_driver_unregister(&omap_pwm_driver);
> > +}
> > +
> > +arch_initcall(omap_pwm_init);
> 
> This should probably be module_init() instead. You already return
> -EPROBE_DEFER if the dual-mode timer isn't ready yet so you should bite
> the bullet and get all the dependencies to behave properly as well. New
> code shouldn't be using this kind of dependency handling.
> 
> In that case you could also just run module_platform_driver().
> 
> Thierry
> 
> > +module_exit(omap_pwm_exit);
> > +
> > +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
> > +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> 
> Shouldn't this be "Neil Brown"? I noticed you use the concatenated form
> in the email address as well, so maybe that's on purpose?

Yes, it is on purpose.  With a surname like "Brown", one likes finding ways
to be distinctive :-)

> 
> > +MODULE_LICENSE("GPLv2");
> 
> This should be "GPL v2". Maybe MODULE_DESCRIPTION() would be nice too.

Fixed.

> 
> Thierry

Thanks for your very thorough review!  I'll send an updated version once I've
resolved other comments and tested again.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]

On Wed, 12 Dec 2012 10:20:34 -0600 Jon Hunter <jon-hunter@ti.com> wrote:

> 
> On 12/12/2012 05:31 AM, Thierry Reding wrote:
> > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
> 
> [snip]
> 
> >> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> +	struct omap_chip *omap = to_omap_chip(chip);
> >> +	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.
> >> +	 */
> > 
> > Block comments should be in the following format:
> > 
> > 	/*
> > 	 * foo...
> > 	 * bar...
> > 	 */
> > 
> >> +
> >> +	omap_dm_timer_enable(omap->dm_timer);
> >> +	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> >> +	omap_dm_timer_start(omap->dm_timer);
> >> +	omap_dm_timer_disable(omap->dm_timer);
> > 
> > So omap_dm_timer_disable() doesn't actually stop the timer? It just
> > disables the access to the registers?
> 
> I thought this looked odd too ;-)
> 
> So what is going on here is that omap_dm_timer_start() calls
> omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
> last disable really just complements the first enable (ie. decrements
> the use count), but the timer will not actually be disabled, because the
> start has called an extra enable.
> 
> These four function calls can be replaced by one call to
> omap_dm_timer_set_load_start() and I think that will be much clearer and
> concise.

So it now reads:


	/*
	 * Set the timer to its minimum load value to ensure we get an
	 * overflow event right away once we start it.
	 */

	omap_dm_timer_set_load_start(omap->dm_timer, true, DM_TIMER_LOAD_MIN);


Certainly more concise - thanks.


> 
> In general, it should not be necessary to call these
> omap_dm_timer_enable/disable APIs directly. I am not sure what the
> history is or if there is a use-case that really requires this. So in
> the future may be I should make them static so they cannot be used
> directly :-)

I've removed the other instance of these calls - in omap_pwm_config.


Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 14876 bytes --]


[Thierry: question for you near the end - thanks]

On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:

> Hi Neil,
> 
> On 12/12/2012 02:24 AM, NeilBrown wrote:
> > 
> > 
> > This patch is based on an earlier patch by Grant Erickson
> > which provided pwm devices using the 'legacy' interface.
> > 
> > This driver instead uses the new framework interface.
> > 
> > Cc: Grant Erickson <marathon96@gmail.com>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..7df573a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -85,6 +85,15 @@ config PWM_MXS
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-mxs.
> >  
> > +config PWM_OMAP
> > +	tristate "OMAP pwm support"
> > +	depends on ARCH_OMAP
> 
> We should probably have depends on or selects OMAP_DM_TIMER here too.

Sounds sensible - fixed.

> 
> > +	help
> > +	  Generic PWM framework driver for OMAP
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-omap
> > +
> >  config PWM_PUV3
> >  	tristate "PKUnity NetBook-0916 PWM support"
> >  	depends on ARCH_PUV3
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..f5d200d 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
> >  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> >  obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
> >  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
> > +obj-$(CONFIG_PWM_OMAP)		+= pwm-omap.o
> >  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
> >  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> > new file mode 100644
> > index 0000000..e3dbce3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-omap.c
> > @@ -0,0 +1,318 @@
> > +/*
> > + *    Copyright (c) 2012 NeilBrown <neilb@suse.de>
> > + *    Heavily based on earlier code which is:
> > + *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
> > + *
> > + *    Also based on pwm-samsung.c
> > + *
> > + *    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
> 
> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
> work for OMAP1-5.
> 

Done.


> > + *      PWM driver / controller, using the OMAP's dual-mode timers.
> > + *
> > + *    The 'id' number for the device encodes the number of the dm timer
> > + *    to use, and the polarity of the output.
> > + *    lsb is '1' of active-high, and '0' for active low
> > + *    remaining bit a timer number and need to be shifted down before use.
> > + */
> > +
> > +#define pr_fmt(fmt) "pwm-omap: " fmt
> > +
> > +#include <linux/export.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/pwm.h>
> > +#include <linux/module.h>
> > +
> > +#include <plat/dmtimer.h>
> 
> This is going to be a problem for the single zImage work, because we
> cannot include any plat headers in driver code any more. Therefore,
> although it is not ideal, one way to handle this is pass function
> pointers to the various dmtimer APIs that are needed via the platform
> data. Painful I know ...

But that doesn't work with devicetree does it?

Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?

It only included other things from include/linux, so it should be safe.

> 
> > +#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
> > +
> > +struct omap_chip {
> > +	struct platform_device	*pdev;
> > +
> > +	struct omap_dm_timer	*dm_timer;
> > +	unsigned int		polarity;
> > +	const char		*label;
> > +
> > +	unsigned int		duty_ns, period_ns;
> > +	struct pwm_chip		chip;
> > +};
> > +
> > +#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)
> > +
> > +#define	pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
> > +
> > +/**
> > + * 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;
> > +}
> > +
> > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct omap_chip *omap = to_omap_chip(chip);
> > +	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(omap->dm_timer);
> > +	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > +	omap_dm_timer_start(omap->dm_timer);
> > +	omap_dm_timer_disable(omap->dm_timer);
> 
> Why not just use omap_dm_timer_load_start() here instead of the above 4
> APIs?

Because I didn't know about it.  I do now :-)

> 
> > +
> > +	return status;
> > +}
> > +
> > +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct omap_chip *omap = to_omap_chip(chip);
> > +
> > +	omap_dm_timer_stop(omap->dm_timer);
> > +}
> > +
> > +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			   int duty_ns, int period_ns)
> > +{
> > +	struct omap_chip *omap = to_omap_chip(chip);
> > +	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(chip->dev,
> > +		"duty cycle: %d, period %d\n",
> > +		duty_ns, period_ns);
> > +
> > +	if (omap->duty_ns == duty_ns &&
> > +	    omap->period_ns == period_ns)
> > +		/* No change - don't cause any transients */
> > +		return 0;
> > +
> > +	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->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.  Hopefully it is already
> > +	 * disabled, but call the (idempotent) pwm_disable just in case
> > +	 */
> > +
> > +	pwm_disable(pwm);
> > +
> > +	omap_dm_timer_enable(omap->dm_timer);
> 
> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> will enable the timer. So this should not be necessary.

True.  That is what you get for copying someone else's code and not
understanding it fully.

> 
> > +	omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> > +	omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> > +
> > +	dev_dbg(chip->dev,
> > +			"load value: %#08x (%d), "
> > +			"match value: %#08x (%d)\n",
> > +			load_value, load_value,
> > +			match_value, match_value);
> > +
> > +	omap_dm_timer_set_pwm(omap->dm_timer,
> > +			      !omap->polarity,
> > +			      toggle,
> > +			      trigger);
> > +
> > +	/* Set the counter to generate an overflow event immediately. */
> > +
> > +	omap_dm_timer_write_counter(omap->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(omap->dm_timer);
> 
> Similarly the disable should not be needed here either.
> 
> > +	omap->duty_ns = duty_ns;
> > +	omap->period_ns = period_ns;
> > +
> > +	return status;
> > +}
> > +
> > +
> > +static struct pwm_ops omap_pwm_ops = {
> > +	.enable	= omap_pwm_enable,
> > +	.disable= omap_pwm_disable,
> > +	.config	= omap_pwm_config,
> > +	.owner	= THIS_MODULE,
> > +};
> > +
> > +/**
> > + * 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)
> 
> I believe that __devinit is no longer required.
> 
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct omap_chip *omap;
> > +	int status = 0;
> > +	unsigned int id = pdev->id;
> > +	unsigned int timer = id >> 1; /* lsb is polarity */
> > +
> > +	omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
> > +
> > +	if (omap == NULL) {
> > +		dev_err(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.
> > +	 */
> > +
> > +	omap->dm_timer = omap_dm_timer_request_specific(timer);
> 
> I would recommend that you use omap_dm_timer_request_by_cap() (new for
> v3.8 so you should be able to use once v3.8-rc1 is out) here to request
> a timer that supports the PWM output. The above function will not be
> supported when booting with device-tree.

I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do
this now.
Would you object to the patch being submitted with the current call, then an
update when I do move on to 3.8?

However.... I may be misunderstanding something, but I want a timer to drive
a particular output pin - GPIO-57.  And I thought that it could only be
driver by GPT11.  So I need to explicitly request number 11 don't I?


> 
> > +
> > +	if (omap->dm_timer == NULL) {
> > +		status = -EPROBE_DEFER;
> > +		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(omap->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.
> > +	 */
> > +
> > +	omap->chip.dev = dev;
> > +	omap->chip.ops = &omap_pwm_ops;
> > +	omap->chip.base = -1;
> > +	omap->chip.npwm = 1;
> > +	omap->polarity = id & 1;
> > +
> > +	status = pwmchip_add(&omap->chip);
> > +	if (status < 0) {
> > +		dev_err(dev, "failed to register pwm\n");
> > +		omap_dm_timer_free(omap->dm_timer);
> > +		goto err_free;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, omap);
> > +
> > +	status = 0;
> > +	goto done;
> > +
> > + err_free:
> > +	kfree(omap);
> > + 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)
> 
> I believe that __devexit is no longer required.
> 
> > +{
> > +	struct omap_chip *omap = platform_get_drvdata(pdev);
> > +	int status = 0;
> > +
> > +	status = pwmchip_remove(&omap->chip);
> > +	if (status < 0)
> > +		goto done;
> > +
> > +	omap_dm_timer_free(omap->dm_timer);
> 
> Is it guaranteed that the timer will be disabled at this point?

Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
might not be disabled.
Thierry: should pwm_put do that, or do I need a 'free' function in my chip
ops to do that?


> 
> > +
> > +	kfree(omap);
> > +
> > + done:
> > +	return status;
> > +}
> > +
> > +#if CONFIG_PM
> > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	struct omap_chip *omap = platform_get_drvdata(pdev);
> > +	/* No one preserve these values during suspend so reset them
> > +	 * Otherwise driver leaves PWM unconfigured if same values
> > +	 * passed to pwm_config
> > +	 */
> > +	omap->period_ns = 0;
> > +	omap->duty_ns = 0;
> 
> 
> Hmmm, looks like you are trying to force a reconfiguration after suspend
> if the same values are used. Is there an underlying problem here that
> you are trying to workaround?

I copied that from pwm-samsung.c.

The key question is: does a dmtimer preserve all register values over suspend.
If so, then I guess we don't need this.
If not, we do (because omap_pwm_config short circuits if it thinks the config
hasn't changed).

Maybe I should test and see - though as my backlight always blanks before
suspend that might not be straight forward...

> 
> Please note that I am not familiar with the PWM sub-system to know how
> suspend-resume is typically handled and if this is normal or not.
> 
> > +	return 0;
> > +}
> > +#else
> > +#define omap_pwm_suspend	NULL
> > +#endif
> > +
> > +static struct platform_driver omap_pwm_driver = {
> > +	.driver.name	= "omap-pwm",
> > +	.driver.owner	= THIS_MODULE,
> > +	.probe		= omap_pwm_probe,
> > +	.remove		= __devexit_p(omap_pwm_remove),
> 
> I believe that __devexit_p is no longer required.
> 
> Otherwise it looks good to me. Thanks for sending!

And thanks a lot for reading and reviewing!

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:

> > > +	omap_dm_timer_enable(omap->dm_timer);
> > 
> > Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> > will enable the timer. So this should not be necessary.
> 
> True.  That is what you get for copying someone else's code and not
> understanding it fully.

However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
explicitly checks that it is already runtime-enabled.

Does that mean I don't need to call omap_dm_timer_write_counter here?  Or
does it mean that I do need the enable/disable pair?

> 
> > 
> > > +	omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> > > +	omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> > > +
> > > +	dev_dbg(chip->dev,
> > > +			"load value: %#08x (%d), "
> > > +			"match value: %#08x (%d)\n",
> > > +			load_value, load_value,
> > > +			match_value, match_value);
> > > +
> > > +	omap_dm_timer_set_pwm(omap->dm_timer,
> > > +			      !omap->polarity,
> > > +			      toggle,
> > > +			      trigger);
> > > +
> > > +	/* Set the counter to generate an overflow event immediately. */
> > > +
> > > +	omap_dm_timer_write_counter(omap->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(omap->dm_timer);
> > 
> > Similarly the disable should not be needed here either.
> > 


Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Thu, Dec 13, 2012 at 02:06:35PM +1100, NeilBrown wrote:
> 
> [Thierry: question for you near the end - thanks]
> 
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
> 
> > Hi Neil,
> > 
> > On 12/12/2012 02:24 AM, NeilBrown wrote:
[...]
> > > +{
> > > +	struct omap_chip *omap = platform_get_drvdata(pdev);
> > > +	int status = 0;
> > > +
> > > +	status = pwmchip_remove(&omap->chip);
> > > +	if (status < 0)
> > > +		goto done;
> > > +
> > > +	omap_dm_timer_free(omap->dm_timer);
> > 
> > Is it guaranteed that the timer will be disabled at this point?
> 
> Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
> might not be disabled.
> Thierry: should pwm_put do that, or do I need a 'free' function in my chip
> ops to do that?

To be honest, I haven't decided yet. =) There have been discussions that
resulted in a request to run pwm_disable() from pwmchip_remove() on all
PWM devices a chip provides.

This isn't implemented yet and I'm not sure about all the side-effects.
I think for now the best way would be to implement .free() within this
driver, or even do an explicit pwm_disable() in the driver's .remove()
function to do this. When I've come to a decision I'll refactor all of
that in one patch across the whole subsystem.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5206 bytes --]

On Thu, Dec 13, 2012 at 01:38:28PM +1100, NeilBrown wrote:
> On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> 
> > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[...]
> > > +	struct omap_dm_timer	*dm_timer;
> > > +	unsigned int		polarity;
> > 
> > The PWM subsystem already has enum pwm_polarity for this.
> > 
> 
> I'll use that then .... and as there  is a pwm_set_polarity() interface, that
> probably means that I don't need to configure the polarity via the platform
> data?  That would be a lot cleaner.

I guess the answer to that question is: it depends. If the user can set
the polarity (via platform or other means), then yes, you don't have to
pass it in here. However there may be users that don't support setting
the polarity or there may even be situations where the PWM goes through
an additional inverter on the board and therefore doesn't need the
polarity inversed after all, even if the user driver requests it.

Generally though I think that it is up to the user drivers to take care
of this and call pwm_set_polarity() as appropriate, so yes, I don't
think you have to explicitly pass it via platform data at all.

> > > +	if (omap->duty_ns == duty_ns &&
> > > +	    omap->period_ns == period_ns)
> > > +		/* No change - don't cause any transients */
> > > +		return 0;
> > 
> > Note to self: this might be a candidate to put in the core.
> 
> might be useful, though the core doesn't currently "know" the current values.

Yes, but that can be changed. PWM is still a very young subsystem and
I'm trying to be cautious not to add too much cruft to it unless it's
really worth it.

> > > +	omap_dm_timer_set_pwm(omap->dm_timer,
> > > +			      !omap->polarity,
> > > +			      toggle,
> > > +			      trigger);
> > 
> > This doesn't either. Also you should be explicit about the polarity
> > parameter, since enum pwm_polarity is an enum and therefore negating it
> > isn't very nice (it should work though).
> > 
> > You could solve this by doing something like:
> > 
> > 	if (omap->polarity == PWM_POLARITY_NORMAL)
> > 		polarity = 1;
> > 	else
> > 		polarity = 0;
> 
> (omap->polarity == PWM_POLARITY_NORMAL)
> 
> would have the same effect.

Yes, that should work as well. However I'm not a friend of using such
expressions in a function call. But since you'll probably be reworking
this anyway because of the pwm_set_polarity() comments from above you
might just want to stick the proper value into omap->polarity in your
.set_polarity() implementation and not need the extra negation here.

> > > +static int __devinit omap_pwm_probe(struct platform_device *pdev)
> > 
> > No more __devinit, please.
> 
> If you say so (having no idea what it did :-)

This was used to mark functions depending on whether HOTPLUG was enabled
or not. For instance functions marked __devinit could be discarded after
the init stage if HOTPLUG was disabled because it would be guaranteed to
not be called after the init stage. Recently however HOTPLUG was changed
to be always enabled because the gains were very small and most people
would get them wrong anyway.

> > > +#if CONFIG_PM
> > > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> > > +{
> > > +	struct omap_chip *omap = platform_get_drvdata(pdev);
> > > +	/* No one preserve these values during suspend so reset them
> > > +	 * Otherwise driver leaves PWM unconfigured if same values
> > > +	 * passed to pwm_config
> > > +	 */
> > > +	omap->period_ns = 0;
> > > +	omap->duty_ns = 0;
> > > +
> > > +	return 0;
> > > +}
> > > +#else
> > > +#define omap_pwm_suspend	NULL
> > > +#endif
> > 
> > This doesn't look right. You should implement .resume() if you really
> > care, in which case the resume callback would have to reconfigure with
> > the cached values. In that case maybe you should switch to dev_pm_ops
> > and SIMPLE_DEV_PM_OPS() as well.
> > 
> > If you don't, just resetting these values will not make the PWM work
> > properly after resume either since it will have to be explicitly
> > reconfigured.
> 
> I just copied that from pwm-samsung.c
> 
> I think the point is to avoid the "no transients" short-circuit in
> omap_pwm_config if the config is unchanged.
> 
> The assumption is that pwm_disable() will be called before suspend and
> pwm_config()/pwm_enable() after resume.  So there is no point actually
> configuring anything in .resume() - it makes sense to wait until pwm_config()
> is called (if ever).  But we want to make sure that pwm_config actually does
> something.

Okay, that makes sense. User drivers should actually be better suited to
reset PWM devices to their proper state on resume.

> > > +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
> > > +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> > 
> > Shouldn't this be "Neil Brown"? I noticed you use the concatenated form
> > in the email address as well, so maybe that's on purpose?
> 
> Yes, it is on purpose.  With a surname like "Brown", one likes finding ways
> to be distinctive :-)

Hehe, alright then =).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Jon Hunter]

On 12/12/2012 09:06 PM, NeilBrown wrote:
> 
> [Thierry: question for you near the end - thanks]
> 
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
> 
>> Hi Neil,
>>
>> On 12/12/2012 02:24 AM, NeilBrown wrote:
>>>
>>>
>>> This patch is based on an earlier patch by Grant Erickson
>>> which provided pwm devices using the 'legacy' interface.
>>>
>>> This driver instead uses the new framework interface.
>>>
>>> Cc: Grant Erickson <marathon96@gmail.com>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index ed81720..7df573a 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -85,6 +85,15 @@ config PWM_MXS
>>>  	  To compile this driver as a module, choose M here: the module
>>>  	  will be called pwm-mxs.
>>>  
>>> +config PWM_OMAP
>>> +	tristate "OMAP pwm support"
>>> +	depends on ARCH_OMAP
>>
>> We should probably have depends on or selects OMAP_DM_TIMER here too.
> 
> Sounds sensible - fixed.
> 
>>
>>> +	help
>>> +	  Generic PWM framework driver for OMAP
>>> +
>>> +	  To compile this driver as a module, choose M here: the module
>>> +	  will be called pwm-omap
>>> +
>>>  config PWM_PUV3
>>>  	tristate "PKUnity NetBook-0916 PWM support"
>>>  	depends on ARCH_PUV3
>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>> index acfe482..f5d200d 100644
>>> --- a/drivers/pwm/Makefile
>>> +++ b/drivers/pwm/Makefile
>>> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
>>>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>>>  obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
>>>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>>> +obj-$(CONFIG_PWM_OMAP)		+= pwm-omap.o
>>>  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
>>>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>>>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>>> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
>>> new file mode 100644
>>> index 0000000..e3dbce3
>>> --- /dev/null
>>> +++ b/drivers/pwm/pwm-omap.c
>>> @@ -0,0 +1,318 @@
>>> +/*
>>> + *    Copyright (c) 2012 NeilBrown <neilb@suse.de>
>>> + *    Heavily based on earlier code which is:
>>> + *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
>>> + *
>>> + *    Also based on pwm-samsung.c
>>> + *
>>> + *    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
>>
>> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
>> work for OMAP1-5.
>>
> 
> Done.
> 
> 
>>> + *      PWM driver / controller, using the OMAP's dual-mode timers.
>>> + *
>>> + *    The 'id' number for the device encodes the number of the dm timer
>>> + *    to use, and the polarity of the output.
>>> + *    lsb is '1' of active-high, and '0' for active low
>>> + *    remaining bit a timer number and need to be shifted down before use.
>>> + */
>>> +
>>> +#define pr_fmt(fmt) "pwm-omap: " fmt
>>> +
>>> +#include <linux/export.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/err.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/pwm.h>
>>> +#include <linux/module.h>
>>> +
>>> +#include <plat/dmtimer.h>
>>
>> This is going to be a problem for the single zImage work, because we
>> cannot include any plat headers in driver code any more. Therefore,
>> although it is not ideal, one way to handle this is pass function
>> pointers to the various dmtimer APIs that are needed via the platform
>> data. Painful I know ...
> 
> But that doesn't work with devicetree does it?

Ugh, you are right! This is becoming an increasing problem.

> Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?

I can ask Tony if he thinks we could do that.

> It only included other things from include/linux, so it should be safe.
> 
>>
>>> +#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
>>> +
>>> +struct omap_chip {
>>> +	struct platform_device	*pdev;
>>> +
>>> +	struct omap_dm_timer	*dm_timer;
>>> +	unsigned int		polarity;
>>> +	const char		*label;
>>> +
>>> +	unsigned int		duty_ns, period_ns;
>>> +	struct pwm_chip		chip;
>>> +};
>>> +
>>> +#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)
>>> +
>>> +#define	pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
>>> +
>>> +/**
>>> + * 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;
>>> +}
>>> +
>>> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> +	struct omap_chip *omap = to_omap_chip(chip);
>>> +	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(omap->dm_timer);
>>> +	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>>> +	omap_dm_timer_start(omap->dm_timer);
>>> +	omap_dm_timer_disable(omap->dm_timer);
>>
>> Why not just use omap_dm_timer_load_start() here instead of the above 4
>> APIs?
> 
> Because I didn't know about it.  I do now :-)

No problem.

>>
>>> +
>>> +	return status;
>>> +}
>>> +
>>> +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> +	struct omap_chip *omap = to_omap_chip(chip);
>>> +
>>> +	omap_dm_timer_stop(omap->dm_timer);
>>> +}
>>> +
>>> +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>> +			   int duty_ns, int period_ns)
>>> +{
>>> +	struct omap_chip *omap = to_omap_chip(chip);
>>> +	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(chip->dev,
>>> +		"duty cycle: %d, period %d\n",
>>> +		duty_ns, period_ns);
>>> +
>>> +	if (omap->duty_ns == duty_ns &&
>>> +	    omap->period_ns == period_ns)
>>> +		/* No change - don't cause any transients */
>>> +		return 0;
>>> +
>>> +	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->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.  Hopefully it is already
>>> +	 * disabled, but call the (idempotent) pwm_disable just in case
>>> +	 */
>>> +
>>> +	pwm_disable(pwm);
>>> +
>>> +	omap_dm_timer_enable(omap->dm_timer);
>>
>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
>> will enable the timer. So this should not be necessary.
> 
> True.  That is what you get for copying someone else's code and not
> understanding it fully.
> 
>>
>>> +	omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
>>> +	omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
>>> +
>>> +	dev_dbg(chip->dev,
>>> +			"load value: %#08x (%d), "
>>> +			"match value: %#08x (%d)\n",
>>> +			load_value, load_value,
>>> +			match_value, match_value);
>>> +
>>> +	omap_dm_timer_set_pwm(omap->dm_timer,
>>> +			      !omap->polarity,
>>> +			      toggle,
>>> +			      trigger);
>>> +
>>> +	/* Set the counter to generate an overflow event immediately. */
>>> +
>>> +	omap_dm_timer_write_counter(omap->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(omap->dm_timer);
>>
>> Similarly the disable should not be needed here either.
>>
>>> +	omap->duty_ns = duty_ns;
>>> +	omap->period_ns = period_ns;
>>> +
>>> +	return status;
>>> +}
>>> +
>>> +
>>> +static struct pwm_ops omap_pwm_ops = {
>>> +	.enable	= omap_pwm_enable,
>>> +	.disable= omap_pwm_disable,
>>> +	.config	= omap_pwm_config,
>>> +	.owner	= THIS_MODULE,
>>> +};
>>> +
>>> +/**
>>> + * 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)
>>
>> I believe that __devinit is no longer required.
>>
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct omap_chip *omap;
>>> +	int status = 0;
>>> +	unsigned int id = pdev->id;
>>> +	unsigned int timer = id >> 1; /* lsb is polarity */
>>> +
>>> +	omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
>>> +
>>> +	if (omap == NULL) {
>>> +		dev_err(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.
>>> +	 */
>>> +
>>> +	omap->dm_timer = omap_dm_timer_request_specific(timer);
>>
>> I would recommend that you use omap_dm_timer_request_by_cap() (new for
>> v3.8 so you should be able to use once v3.8-rc1 is out) here to request
>> a timer that supports the PWM output. The above function will not be
>> supported when booting with device-tree.
> 
> I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do
> this now.
> Would you object to the patch being submitted with the current call, then an
> update when I do move on to 3.8?

I don't have strong objections, but I did not think Thierry liked the
use of the id.

> However.... I may be misunderstanding something, but I want a timer to drive
> a particular output pin - GPIO-57.  And I thought that it could only be
> driver by GPT11.  So I need to explicitly request number 11 don't I?

Ugh, good point. Looks like for dev-tree we will need a way to request a
timer by IO and this is missing today!

> 
>>
>>> +
>>> +	if (omap->dm_timer == NULL) {
>>> +		status = -EPROBE_DEFER;
>>> +		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(omap->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.
>>> +	 */
>>> +
>>> +	omap->chip.dev = dev;
>>> +	omap->chip.ops = &omap_pwm_ops;
>>> +	omap->chip.base = -1;
>>> +	omap->chip.npwm = 1;
>>> +	omap->polarity = id & 1;
>>> +
>>> +	status = pwmchip_add(&omap->chip);
>>> +	if (status < 0) {
>>> +		dev_err(dev, "failed to register pwm\n");
>>> +		omap_dm_timer_free(omap->dm_timer);
>>> +		goto err_free;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, omap);
>>> +
>>> +	status = 0;
>>> +	goto done;
>>> +
>>> + err_free:
>>> +	kfree(omap);
>>> + 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)
>>
>> I believe that __devexit is no longer required.
>>
>>> +{
>>> +	struct omap_chip *omap = platform_get_drvdata(pdev);
>>> +	int status = 0;
>>> +
>>> +	status = pwmchip_remove(&omap->chip);
>>> +	if (status < 0)
>>> +		goto done;
>>> +
>>> +	omap_dm_timer_free(omap->dm_timer);
>>
>> Is it guaranteed that the timer will be disabled at this point?
> 
> Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
> might not be disabled.
> Thierry: should pwm_put do that, or do I need a 'free' function in my chip
> ops to do that?
> 
> 
>>
>>> +
>>> +	kfree(omap);
>>> +
>>> + done:
>>> +	return status;
>>> +}
>>> +
>>> +#if CONFIG_PM
>>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
>>> +{
>>> +	struct omap_chip *omap = platform_get_drvdata(pdev);
>>> +	/* No one preserve these values during suspend so reset them
>>> +	 * Otherwise driver leaves PWM unconfigured if same values
>>> +	 * passed to pwm_config
>>> +	 */
>>> +	omap->period_ns = 0;
>>> +	omap->duty_ns = 0;
>>
>>
>> Hmmm, looks like you are trying to force a reconfiguration after suspend
>> if the same values are used. Is there an underlying problem here that
>> you are trying to workaround?
> 
> I copied that from pwm-samsung.c.
> 
> The key question is: does a dmtimer preserve all register values over suspend.
> If so, then I guess we don't need this.
> If not, we do (because omap_pwm_config short circuits if it thinks the config
> hasn't changed).

I gave it a quick test on omap3/4 when just operating the timer as a
counter (not driving a pwm output) and suspend/resume works fine.
However, it does not work if I enable off mode (via the debugfs). This
is not enabled by default and may be I should put that on my to-do list
as well.

> Maybe I should test and see - though as my backlight always blanks before
> suspend that might not be straight forward...

Yes it would be great if you can test too. I have been finding that
certain features of the timer are not that well tested.

>>
>> Please note that I am not familiar with the PWM sub-system to know how
>> suspend-resume is typically handled and if this is normal or not.
>>
>>> +	return 0;
>>> +}
>>> +#else
>>> +#define omap_pwm_suspend	NULL
>>> +#endif
>>> +
>>> +static struct platform_driver omap_pwm_driver = {
>>> +	.driver.name	= "omap-pwm",
>>> +	.driver.owner	= THIS_MODULE,
>>> +	.probe		= omap_pwm_probe,
>>> +	.remove		= __devexit_p(omap_pwm_remove),
>>
>> I believe that __devexit_p is no longer required.
>>
>> Otherwise it looks good to me. Thanks for sending!
> 
> And thanks a lot for reading and reviewing!

No problem.

Cheers
Jon

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [121213 09:11]:
> On 12/12/2012 09:06 PM, NeilBrown wrote:
> > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
> >> On 12/12/2012 02:24 AM, NeilBrown wrote:
> >>> +
> >>> +#include <plat/dmtimer.h>
> >>
> >> This is going to be a problem for the single zImage work, because we
> >> cannot include any plat headers in driver code any more. Therefore,
> >> although it is not ideal, one way to handle this is pass function
> >> pointers to the various dmtimer APIs that are needed via the platform
> >> data. Painful I know ...
> > 
> > But that doesn't work with devicetree does it?
> 
> Ugh, you are right! This is becoming an increasing problem.
> 
> > Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
> 
> I can ask Tony if he thinks we could do that.

Yeah we need to fix this somehow. First we need to limit that header
to the minimum and have most of it in a local header file for the
clocksource and clockevent. Then let's move the minimal header to
include/linux/omap-dmtimer.h until we have something Linux generic
available for doing things like this.

Regards,

Tony

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [121212 19:09]:
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
> > On 12/12/2012 02:24 AM, NeilBrown wrote:
> > > +
> > > +	/* Request the OMAP dual-mode timer that will be bound to and
> > > +	 * associated with this generic PWM.
> > > +	 */
> > > +
> > > +	omap->dm_timer = omap_dm_timer_request_specific(timer);
> > 
> > I would recommend that you use omap_dm_timer_request_by_cap() (new for
> > v3.8 so you should be able to use once v3.8-rc1 is out) here to request
> > a timer that supports the PWM output. The above function will not be
> > supported when booting with device-tree.
> 
> I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do
> this now.
> Would you object to the patch being submitted with the current call, then an
> update when I do move on to 3.8?
> 
> However.... I may be misunderstanding something, but I want a timer to drive
> a particular output pin - GPIO-57.  And I thought that it could only be
> driver by GPT11.  So I need to explicitly request number 11 don't I?

Yes I believe it needs to tied to a specific GPT instance to have access
to the pin. We should not export omap_dm_timer_request_specific(), that
information should become from platform_data and device tree.

Regards,

Tony

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Jon Hunter]

On 12/12/2012 10:33 PM, NeilBrown wrote:
> On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
> 
>>>> +	omap_dm_timer_enable(omap->dm_timer);
>>>
>>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
>>> will enable the timer. So this should not be necessary.
>>
>> True.  That is what you get for copying someone else's code and not
>> understanding it fully.
> 
> However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
> explicitly checks that it is already runtime-enabled.
> 
> Does that mean I don't need to call omap_dm_timer_write_counter here?  Or
> does it mean that I do need the enable/disable pair?

Typically, omap_dm_timer_write_counter() is used to update the counter
value while the counter is running and hence is enabled.

Looking at the code, some more I now see what they are trying to do. It
seems that they are trying to force an overflow to occur as soon as they
enable the timer. This will cause the timer to load the count value from
the timer load register into the timer counter register. So that does
make sense to me. However, this should not be necessary as
omap_dm_timer_set_load should do this for you. Therefore, I think that
you could accomplish the same thing by doing ...

omap_pwm_config
	--> omap_dm_timer_set_load()
	--> omap_dm_timer_set_match()
	--> omap_dm_timer_set_pwm()

omap_pwm_enable
	--> omap_dm_timer_start()

If we call _set_load in config then we don't need to call _load_start in
the enable, we can just call _start.

Can you try this and see if this is working ok?

Cheers
Jon

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 12175 bytes --]

On Thu, 13 Dec 2012 11:42:18 -0600 Jon Hunter <jon-hunter@ti.com> wrote:

> 
> On 12/12/2012 10:33 PM, NeilBrown wrote:
> > On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
> > 
> >>>> +	omap_dm_timer_enable(omap->dm_timer);
> >>>
> >>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> >>> will enable the timer. So this should not be necessary.
> >>
> >> True.  That is what you get for copying someone else's code and not
> >> understanding it fully.
> > 
> > However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
> > explicitly checks that it is already runtime-enabled.
> > 
> > Does that mean I don't need to call omap_dm_timer_write_counter here?  Or
> > does it mean that I do need the enable/disable pair?
> 
> Typically, omap_dm_timer_write_counter() is used to update the counter
> value while the counter is running and hence is enabled.
> 
> Looking at the code, some more I now see what they are trying to do. It
> seems that they are trying to force an overflow to occur as soon as they
> enable the timer. This will cause the timer to load the count value from
> the timer load register into the timer counter register. So that does
> make sense to me. However, this should not be necessary as
> omap_dm_timer_set_load should do this for you. Therefore, I think that
> you could accomplish the same thing by doing ...
> 
> omap_pwm_config
> 	--> omap_dm_timer_set_load()
> 	--> omap_dm_timer_set_match()
> 	--> omap_dm_timer_set_pwm()
> 
> omap_pwm_enable
> 	--> omap_dm_timer_start()
> 
> If we call _set_load in config then we don't need to call _load_start in
> the enable, we can just call _start.
> 
> Can you try this and see if this is working ok?

Seems to work, and is much neater.  Thanks.

Below is my current patch.
Unresolved issues are:
 - it uses
    omap_dm_timer_request_specific()
   which apparently isn't ideal.
 - It still zeros things in the suspend routine.  I haven't explored this at
   all yet

Thanks,
NeilBrown

From 69ed735d1bc377e8e65345792997f809e60b5fbf Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Sun, 2 Dec 2012 14:53:20 +1100
Subject: [PATCH] pwm: omap: Add PWM support using dual-mode timers

This patch is based on an earlier patch by Grant Erickson
which provided PWM devices using the 'legacy' interface.

This driver instead uses the new framework interface.

Platform data must be provided to identify which dmtimer to use.

Lots of cleanup and inprovements thanks to Thierry Reding
and Jon Hunter.

Cc: Grant Erickson <marathon96@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..32c1253 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-mxs.
 
+config PWM_OMAP
+	tristate "OMAP PWM support"
+	depends on ARCH_OMAP && OMAP_DM_TIMER
+	help
+	  Generic PWM framework driver for OMAP
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-omap
+
 config PWM_PUV3
 	tristate "PKUnity NetBook-0916 PWM support"
 	depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP)		+= pwm-omap.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 0000000..344072c
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,271 @@
+/*
+ *    Copyright (c) 2012 NeilBrown <neilb@suse.de>
+ *    Heavily based on earlier code which is:
+ *    Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
+ *
+ *    Also based on pwm-samsung.c
+ *
+ *    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 OMAP support for the generic, Linux
+ *      PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ */
+
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+#include <linux/module.h>
+#include <linux/platform_data/omap-pwm.h>
+
+#include <plat/dmtimer.h>
+
+#define DM_TIMER_LOAD_MIN		0xFFFFFFFE
+
+struct omap_chip {
+	struct omap_dm_timer	*dm_timer;
+	enum pwm_polarity	polarity;
+	unsigned int		duty_ns, period_ns;
+	struct pwm_chip		chip;
+};
+
+#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)
+
+/**
+ * pwm_calc_value - Determine 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 compute 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)
+{
+	u64 c;
+
+	c = (u64)clk_rate * ns;
+	do_div(c, NSEC_PER_SEC);
+
+	return DM_TIMER_LOAD_MIN - c;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct omap_chip *omap = to_omap_chip(chip);
+
+	omap_dm_timer_start(omap->dm_timer);
+
+	return 0;
+}
+
+static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct omap_chip *omap = to_omap_chip(chip);
+
+	omap_dm_timer_stop(omap->dm_timer);
+}
+
+static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			   int duty_ns, int period_ns)
+{
+	struct omap_chip *omap = to_omap_chip(chip);
+	int load_value, match_value;
+	unsigned long clk_rate;
+
+	dev_dbg(chip->dev, "duty cycle: %d, period %d\n", duty_ns, period_ns);
+
+	if (omap->duty_ns == duty_ns &&
+	    omap->period_ns == period_ns)
+		/* No change - don't cause any transients. */
+		return 0;
+
+	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->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.  Hopefully it is already
+	 * disabled, but call the (idempotent) pwm_disable just in case.
+	 */
+
+	pwm_disable(pwm);
+
+	omap_dm_timer_set_load(omap->dm_timer, true, load_value);
+	omap_dm_timer_set_match(omap->dm_timer, true, match_value);
+
+	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
+		load_value, load_value,	match_value, match_value);
+
+	omap_dm_timer_set_pwm(omap->dm_timer,
+			      omap->polarity == PWM_POLARITY_INVERSED,
+			      true,
+			      OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+
+	omap->duty_ns = duty_ns;
+	omap->period_ns = period_ns;
+
+	return 0;
+}
+
+static int omap_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				 enum pwm_polarity polarity)
+{
+	struct omap_chip *omap = to_omap_chip(chip);
+
+	if (omap->polarity == polarity)
+		return 0;
+
+	omap->polarity = polarity;
+
+	omap_dm_timer_set_pwm(omap->dm_timer,
+			      omap->polarity == PWM_POLARITY_INVERSED,
+			      true,
+			      OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+	return 0;
+}
+
+static struct pwm_ops omap_pwm_ops = {
+	.enable		= omap_pwm_enable,
+	.disable	= omap_pwm_disable,
+	.config		= omap_pwm_config,
+	.set_polarity	= omap_pwm_set_polarity,
+	.owner		= THIS_MODULE,
+};
+
+static int omap_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct omap_chip *omap;
+	int status = 0;
+	struct omap_pwm_pdata *pdata = dev->platform_data;
+
+	if (!pdata) {
+		dev_err(dev, "No platform data provided\n");
+		return -ENODEV;
+	}
+
+	omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
+	if (omap == NULL) {
+		dev_err(dev, "Could not allocate memory.\n");
+		return -ENOMEM;
+	}
+
+	/*
+	 * Request the OMAP dual-mode timer that will be bound to and
+	 * associated with this generic PWM.
+	 */
+
+	omap->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);
+	if (omap->dm_timer == NULL) {
+		status = -EPROBE_DEFER;
+		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(omap->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.
+	 */
+
+	omap->chip.dev = dev;
+	omap->chip.ops = &omap_pwm_ops;
+	omap->chip.base = -1;
+	omap->chip.npwm = 1;
+	omap->polarity = PWM_POLARITY_NORMAL;
+
+	status = pwmchip_add(&omap->chip);
+	if (status < 0) {
+		dev_err(dev, "failed to register PWM\n");
+		omap_dm_timer_free(omap->dm_timer);
+		goto err_free;
+	}
+
+	platform_set_drvdata(pdev, omap);
+
+	return 0;
+
+ err_free:
+	kfree(omap);
+	return status;
+}
+
+static int omap_pwm_remove(struct platform_device *pdev)
+{
+	struct omap_chip *omap = platform_get_drvdata(pdev);
+	int status;
+
+	omap_dm_timer_stop(omap->dm_timer);
+	status = pwmchip_remove(&omap->chip);
+	if (status < 0)
+		return status;
+
+	omap_dm_timer_free(omap->dm_timer);
+	kfree(omap);
+
+	return 0;
+}
+
+#if CONFIG_PM
+static int omap_pwm_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_chip *omap = platform_get_drvdata(pdev);
+	/*
+	 * No one preserve these values during suspend so reset them,
+	 * otherwise driver leaves PWM unconfigured if same values
+	 * passed to pwm_config.
+	 */
+	omap->period_ns = 0;
+	omap->duty_ns = 0;
+
+	return 0;
+}
+#else
+#define omap_pwm_suspend	NULL
+#endif
+
+static SIMPLE_DEV_PM_OPS(omap_pwm_pm, omap_pwm_suspend, NULL);
+static struct platform_driver omap_pwm_driver = {
+	.driver = {
+		.name	= "omap-pwm",
+		.owner	= THIS_MODULE,
+		.pm	= &omap_pwm_pm,
+	},
+	.probe		= omap_pwm_probe,
+	.remove		= omap_pwm_remove,
+};
+module_platform_driver(omap_pwm_driver);
+
+MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("2012-12-01");
+MODULE_DESCRIPTION("OMAP PWM Driver using Dual-mode Timers");
diff --git a/include/linux/platform_data/omap-pwm.h b/include/linux/platform_data/omap-pwm.h
new file mode 100644
index 0000000..169fc3c
--- /dev/null
+++ b/include/linux/platform_data/omap-pwm.h
@@ -0,0 +1,20 @@
+/*
+ * omap-pwm.h
+ *
+ *    Copyright (c) 2012 NeilBrown <neilb@suse.de>
+ *
+ *    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.
+ *
+ * Set the timer id to use for a PWM
+ */
+
+#ifndef _OMAP_PWM_H_
+#define _OMAP_PWM_H_
+
+struct omap_pwm_pdata {
+	int	timer_id;
+};
+
+#endif /* _OMAP_PWM_H_ */


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]

On Thu, 13 Dec 2012 11:07:25 -0600 Jon Hunter <jon-hunter@ti.com> wrote:

> 
> On 12/12/2012 09:06 PM, NeilBrown wrote:
> > 

> >>> +
> >>> +#if CONFIG_PM
> >>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> >>> +{
> >>> +	struct omap_chip *omap = platform_get_drvdata(pdev);
> >>> +	/* No one preserve these values during suspend so reset them
> >>> +	 * Otherwise driver leaves PWM unconfigured if same values
> >>> +	 * passed to pwm_config
> >>> +	 */
> >>> +	omap->period_ns = 0;
> >>> +	omap->duty_ns = 0;
> >>
> >>
> >> Hmmm, looks like you are trying to force a reconfiguration after suspend
> >> if the same values are used. Is there an underlying problem here that
> >> you are trying to workaround?
> > 
> > I copied that from pwm-samsung.c.
> > 
> > The key question is: does a dmtimer preserve all register values over suspend.
> > If so, then I guess we don't need this.
> > If not, we do (because omap_pwm_config short circuits if it thinks the config
> > hasn't changed).
> 
> I gave it a quick test on omap3/4 when just operating the timer as a
> counter (not driving a pwm output) and suspend/resume works fine.
> However, it does not work if I enable off mode (via the debugfs). This
> is not enabled by default and may be I should put that on my to-do list
> as well.

I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.  
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).

NeilBrown


From: NeilBrown <neilb@suse.de>
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.

The context loss handling in dmtimer appears to assume that
   omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
   omap_dm_timer_stop()

bracket all interactions.  Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.

So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).

Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
 void omap_dm_timer_enable(struct omap_dm_timer *timer)
 {
 	pm_runtime_get_sync(&timer->pdev->dev);
+
+	if (!(timer->capability & OMAP_TIMER_ALWON)) {
+		int loss_count =
+			omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
+		if (loss_count != timer->ctx_loss_count) {
+			omap_timer_restore_context(timer);
+			timer->ctx_loss_count = loss_count;
+		}
+	}
 }
 EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
 
@@ -347,12 +356,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
 
 	omap_dm_timer_enable(timer);
 
-	if (!(timer->capability & OMAP_TIMER_ALWON)) {
-		if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
-				timer->ctx_loss_count)
-			omap_timer_restore_context(timer);
-	}
-
 	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
 	if (!(l & OMAP_TIMER_CTRL_ST)) {
 		l |= OMAP_TIMER_CTRL_ST;
@@ -377,10 +380,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
 
 	__omap_dm_timer_stop(timer, timer->posted, rate);
 
-	if (!(timer->capability & OMAP_TIMER_ALWON))
-		timer->ctx_loss_count =
-			omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
-
 	/*
 	 * Since the register values are computed and written within
 	 * __omap_dm_timer_stop, we need to use read to retrieve the
@@ -494,12 +493,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
 
 	omap_dm_timer_enable(timer);
 
-	if (!(timer->capability & OMAP_TIMER_ALWON)) {
-		if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
-				timer->ctx_loss_count)
-			omap_timer_restore_context(timer);
-	}
-
 	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
 	if (autoreload) {
 		l |= OMAP_TIMER_CTRL_AR;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Jon Hunter]

On 01/06/2013 03:12 PM, NeilBrown wrote:

[snip]

> I've been playing with off-mode and discovered that the first attempt to set
> the PWM after resume didn't work, but subsequent ones did.
> I did some digging and came up with the following patch.  
> With this in place, the omap_pwm_suspend() above is definitely pointless (was
> wasn't really useful even without it).

Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(

Some minor comments below ...
 
> NeilBrown
> 
> 
> From: NeilBrown <neilb@suse.de>
> Date: Mon, 7 Jan 2013 07:53:03 +1100
> Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.

Nit, subject should formatted "ARM: OMAP: blah blah blah"

Also, may be worth calling this "fix context-loss" as this is really
fixing something that is broken.
 
> The context loss handling in dmtimer appears to assume that
>    omap_dm_timer_set_load_start() or omap_dm_timer_start()
> and
>    omap_dm_timer_stop()
> 
> bracket all interactions.  Only the first two restore the context and
> the last updates the context loss counter.
> However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
> reasonably be called outside this bracketing, and the fact that they
> call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
> is expected.
> 
> So if, after a transition into and out of off-mode which would cause
> the dm timer to loose all state, omap_dm_timer_set_match() is called
> before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
> will be 'wrong' and this wrong value will be stored context.tclr so
> a subsequent omap_dm_timer_start() can fail (As the control register
> is wrong).
> 
> Simplify this be doing the restore-from-context in
> omap_dm_timer_enable() so that whenever the timer is enabled, the
> context is correct.
> Also update the ctx_loss_count at the same time as we notice it is
> wrong - these is no value in delaying this until the
> omap_dm_timer_disable() as it cannot change while the timer is
> enabled.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index 938b50a..c216696 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
>  void omap_dm_timer_enable(struct omap_dm_timer *timer)
>  {
>  	pm_runtime_get_sync(&timer->pdev->dev);
> +
> +	if (!(timer->capability & OMAP_TIMER_ALWON)) {
> +		int loss_count =
> +			omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
> +		if (loss_count != timer->ctx_loss_count) {
> +			omap_timer_restore_context(timer);
> +			timer->ctx_loss_count = loss_count;
> +		}
> +	}
>  }

Can you rebase on v3.8-rc2? We no longer call 
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
 
 void omap_dm_timer_enable(struct omap_dm_timer *timer)
 {
+       int c;
+
        pm_runtime_get_sync(&timer->pdev->dev);
+
+       if (!(timer->capability & OMAP_TIMER_ALWON)) {
+               if (timer->get_context_loss_count) {
+                       c = timer->get_context_loss_count(&timer->pdev->dev);
+                       if (c != timer->ctx_loss_count) {
+                               omap_timer_restore_context(timer);
+                               timer->ctx_loss_count = c;
+                       }
+               }
+       }

Cheers
Jon