OMAP: add pwm driver using dmtimers.

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.

drivers/pwm/pwm-omap.c +0 -0

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

drivers/pwm/pwm-omap.c +6 -0

+#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>

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

+static struct pwm_ops omap_pwm_ops = {

+static int __devinit omap_pwm_probe(struct platform_device *pdev)

drivers/pwm/pwm-omap.c +8 -0

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

drivers/pwm/pwm-omap.c +6 -0

+
+ err_free:
+	kfree(omap);
+ done:
+	return status;
+}

drivers/pwm/pwm-omap.c +8 -0

+}
+
+#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

drivers/pwm/pwm-omap.c +5 -0

+	.suspend	= omap_pwm_suspend,
+	.resume		= NULL,
+};
+
+static int __init omap_pwm_init(void)

+MODULE_LICENSE("GPLv2");

On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:

drivers/pwm/pwm-omap.c line 82

Quoted 9 lines

drivers/pwm/pwm-omap.c new file +9 -0

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

drivers/pwm/pwm-omap.c line 87

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

+
+	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?

Quoted 1 line · Thierry Reding

On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:

[snip]

Quoted 17 lines

drivers/pwm/pwm-omap.c line 82

Quoted 9 lines

drivers/pwm/pwm-omap.c new file +9 -0

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

Quoted 8 lines

drivers/pwm/pwm-omap.c line 87

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

+
+	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 :-)

Quoted 6 lines · NeilBrown

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.

Quoted 12 lines

Cc Grant Erickson <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/pwm/Kconfig +0 -0

index ed81720..7df573a 100644
@@ -85,6 +85,15 @@ config PWM_MXS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-mxs.
 

drivers/pwm/Kconfig config PWM_MXS · line 89

Quoted 2 lines

drivers/pwm/Kconfig +2 -0

+config PWM_OMAP
+	tristate "OMAP pwm support"

"OMAP PWM support"

Quoted 2 lines

drivers/pwm/pwm-omap.c +0 -0

[...]

Quoted 4 lines

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

Quoted 2 lines

+#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.

Quoted 11 lines

drivers/pwm/pwm-omap.c +10 -0

+#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>
+

drivers/pwm/pwm-omap.c line 39

Quoted 6 lines

drivers/pwm/pwm-omap.c +6 -0

+#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.

drivers/pwm/pwm-omap.c line 42

Quoted 3 lines

drivers/pwm/pwm-omap.c +2 -0

+	struct omap_dm_timer	*dm_timer;
+	unsigned int		polarity;

The PWM subsystem already has enum pwm_polarity for this.

drivers/pwm/pwm-omap.c line 43

Quoted 1 line

drivers/pwm/pwm-omap.c +1 -0

+	const char		*label;

This isn't used anywhere either.

drivers/pwm/pwm-omap.c line 51

Quoted 9 lines

drivers/pwm/pwm-omap.c +8 -0

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

drivers/pwm/pwm-omap.c line 54

Quoted 4 lines

drivers/pwm/pwm-omap.c +3 -0

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

Quoted 4 lines

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

drivers/pwm/pwm-omap.c line 63

Quoted 7 lines

+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */

drivers/pwm/pwm-omap.c +3 -0

+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?

drivers/pwm/pwm-omap.c line 65

Quoted 3 lines

drivers/pwm/pwm-omap.c +2 -0

+	int cycles;
+	__u64 c;

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

drivers/pwm/pwm-omap.c line 71

Quoted 7 lines

drivers/pwm/pwm-omap.c +6 -0

+
+	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?

drivers/pwm/pwm-omap.c line 82

Quoted 10 lines

drivers/pwm/pwm-omap.c +9 -0

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

drivers/pwm/pwm-omap.c line 87

Quoted 6 lines

drivers/pwm/pwm-omap.c +5 -0

+
+	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?

drivers/pwm/pwm-omap.c

Quoted 1 line

drivers/pwm/pwm-omap.c +1 -0

+	return status;

"return 0;" and drop the status variable.

drivers/pwm/pwm-omap.c line 103

Quoted 5 lines

drivers/pwm/pwm-omap.c +5 -0

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

drivers/pwm/pwm-omap.c line 107

Quoted 4 lines

drivers/pwm/pwm-omap.c +4 -0

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

drivers/pwm/pwm-omap.c line 113

Quoted 7 lines

drivers/pwm/pwm-omap.c +6 -0

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

drivers/pwm/pwm-omap.c line 118

Quoted 6 lines

drivers/pwm/pwm-omap.c +5 -0

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

drivers/pwm/pwm-omap.c line 125

Quoted 7 lines

drivers/pwm/pwm-omap.c +6 -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.
+	 */

Again wrong block comment style.

drivers/pwm/pwm-omap.c line 133

Quoted 8 lines

drivers/pwm/pwm-omap.c +8 -0

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

Quoted 6 lines

drivers/pwm/pwm-omap.c +6 -0

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

drivers/pwm/pwm-omap.c line 146

Quoted 6 lines

drivers/pwm/pwm-omap.c +6 -0

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

drivers/pwm/pwm-omap.c line 151

Quoted 5 lines

drivers/pwm/pwm-omap.c +5 -0

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

drivers/pwm/pwm-omap.c line 159

Quoted 8 lines

drivers/pwm/pwm-omap.c +7 -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.

drivers/pwm/pwm-omap.c line 165

Quoted 5 lines

drivers/pwm/pwm-omap.c +5 -0

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

"return 0;"

drivers/pwm/pwm-omap.c line 78

Quoted 3 lines

drivers/pwm/pwm-omap.c +3 -0

+}
+
+

Gratuitous newline.

Quoted 1 line

+static struct pwm_ops omap_pwm_ops = {

Should be "static const".

drivers/pwm/pwm-omap.c line 171

Quoted 2 lines

drivers/pwm/pwm-omap.c +2 -0

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

There should be a space between .disable and =.

drivers/pwm/pwm-omap.c line 183

Quoted 13 lines

drivers/pwm/pwm-omap.c +12 -0

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

Quoted 2 lines

+static int __devinit omap_pwm_probe(struct platform_device *pdev)

No more __devinit, please.

drivers/pwm/pwm-omap.c line 190

Quoted 7 lines

drivers/pwm/pwm-omap.c +6 -0

+{
+	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.

drivers/pwm/pwm-omap.c line 193

Quoted 3 lines

drivers/pwm/pwm-omap.c +3 -0

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

Gratuituous newline.

drivers/pwm/pwm-omap.c line 202

Quoted 9 lines

drivers/pwm/pwm-omap.c +9 -0

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

drivers/pwm/pwm-omap.c line 205

Quoted 3 lines

drivers/pwm/pwm-omap.c +3 -0

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

Gratuitous newline.

Quoted 4 lines

drivers/pwm/pwm-omap.c +3 -0

+	if (omap->dm_timer == NULL) {
+		status = -EPROBE_DEFER;
+		goto err_free;

drivers/pwm/pwm-omap.c line 218

Quoted 10 lines

drivers/pwm/pwm-omap.c +10 -0

+	}
+
+	/* 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.

drivers/pwm/pwm-omap.c line 225

Quoted 7 lines

drivers/pwm/pwm-omap.c +7 -0

+
+	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. =)

Quoted 12 lines

drivers/pwm/pwm-omap.c +12 -0

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

drivers/pwm/pwm-omap.c line 243

Quoted 6 lines

drivers/pwm/pwm-omap.c +6 -0

+	}
+
+	platform_set_drvdata(pdev, omap);
+
+	status = 0;
+	goto done;

This can just be "return 0;".

Quoted 9 lines

drivers/pwm/pwm-omap.c +8 -0

+
+ err_free:
+	kfree(omap);
+ done:
+	return status;
+}
+
+/**

drivers/pwm/pwm-omap.c line 259

Quoted 10 lines

drivers/pwm/pwm-omap.c +8 -0

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

drivers/pwm/pwm-omap.c line 262

Quoted 3 lines

drivers/pwm/pwm-omap.c +3 -0

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

Just drop this.

drivers/pwm/pwm-omap.c line 266

Quoted 4 lines

drivers/pwm/pwm-omap.c +4 -0

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

"return status;"

drivers/pwm/pwm-omap.c line 273

Quoted 8 lines

drivers/pwm/pwm-omap.c +7 -0

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

Just "return 0;". No label required.

Quoted 8 lines

drivers/pwm/pwm-omap.c +8 -0

+}
+
+#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

drivers/pwm/pwm-omap.c line 291

Quoted 10 lines

drivers/pwm/pwm-omap.c +10 -0

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

drivers/pwm/pwm-omap.c line 295

Quoted 4 lines

drivers/pwm/pwm-omap.c +3 -0

+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,
	},

drivers/pwm/pwm-omap.c line 297

Quoted 2 lines

drivers/pwm/pwm-omap.c +2 -0

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

No __devexit_p anymore.

Quoted 5 lines

drivers/pwm/pwm-omap.c +5 -0

+	.suspend	= omap_pwm_suspend,
+	.resume		= NULL,
+};
+
+static int __init omap_pwm_init(void)

drivers/pwm/pwm-omap.c line 312

Quoted 10 lines

drivers/pwm/pwm-omap.c +10 -0

+{
+	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

drivers/pwm/pwm-omap.c line 316

Quoted 4 lines

drivers/pwm/pwm-omap.c +4 -0

+module_exit(omap_pwm_exit);
+
+MODULE_AUTHOR("Grant Erickson <[email protected]>");
+MODULE_AUTHOR("NeilBrown <[email protected]>");

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?

Quoted 2 lines

+MODULE_LICENSE("GPLv2");

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

Thierry

Quoted 1 line · On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding…

On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:

Quoted 5 lines

drivers/pwm/pwm-omap.c line 42

Quoted 2 lines

drivers/pwm/pwm-omap.c new file +2 -0

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

Quoted 6 lines

drivers/pwm/pwm-omap.c line 118

Quoted 4 lines

drivers/pwm/pwm-omap.c new file +4 -0

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

Quoted 11 lines

drivers/pwm/pwm-omap.c line 151

Quoted 4 lines

drivers/pwm/pwm-omap.c new file +4 -0

+	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:

Quoted 4 lines

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

(omap->polarity == PWM_POLARITY_NORMAL)

would have the same effect.

Quoted 3 lines

Quoted 1 line

+static int __devinit omap_pwm_probe(struct platform_device *pdev)

No more __devinit, please.

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

Quoted 22 lines

Quoted 6 lines

drivers/pwm/pwm-omap.c new file +6 -0

+#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

drivers/pwm/pwm-omap.c line 291

Quoted 10 lines

drivers/pwm/pwm-omap.c new file +10 -0

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

Quoted 3 lines

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.

Quoted 5 lines

drivers/pwm/pwm-omap.c line 316

Quoted 2 lines

drivers/pwm/pwm-omap.c new file +2 -0

+MODULE_AUTHOR("Grant Erickson <[email protected]>");
+MODULE_AUTHOR("NeilBrown <[email protected]>");

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 :-)

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 <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/pwm/Kconfig +0 -0

index ed81720..7df573a 100644

drivers/pwm/Kconfig +6 -0

+	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

drivers/pwm/Makefile +1 -0

index acfe482..f5d200d 100644
@@ -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

drivers/pwm/pwm-omap.c new file +3 -0

new file mode 100644
index 0000000..e3dbce3
@@ -0,0 +1,318 @@
+/*
+ *    Copyright (c) 2012 NeilBrown <[email protected]>
+ *    Heavily based on earlier code which is:

drivers/pwm/pwm-omap.c new file +11 -0

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

drivers/pwm/pwm-omap.c new file +42 -0

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

drivers/pwm/pwm-omap.c new file +40 -0

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

drivers/pwm/pwm-omap.c new file +13 -0

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

drivers/pwm/pwm-omap.c new file +13 -0

+	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,
+};

drivers/pwm/pwm-omap.c new file +10 -0

+{
+	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) {

drivers/pwm/pwm-omap.c new file +45 -0

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

drivers/pwm/pwm-omap.c new file +7 -0

+
+	kfree(omap);
+
+ done:
+	return status;
+}
+

Hi Neil,

Quoted 18 lines · NeilBrown

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 <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/pwm/Kconfig +0 -0

index ed81720..7df573a 100644
@@ -85,6 +85,15 @@ config PWM_MXS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-mxs.
 

drivers/pwm/Kconfig config PWM_MXS · line 90

Quoted 3 lines

drivers/pwm/Kconfig +3 -0

+config PWM_OMAP
+	tristate "OMAP pwm support"
+	depends on ARCH_OMAP

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

Quoted 36 lines

drivers/pwm/Kconfig +6 -0

+	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

drivers/pwm/Makefile +1 -0

index acfe482..f5d200d 100644
@@ -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

drivers/pwm/pwm-omap.c new file +8 -0

new file mode 100644
index 0000000..e3dbce3
@@ -0,0 +1,318 @@
+/*
+ *    Copyright (c) 2012 NeilBrown <[email protected]>
+ *    Heavily based on earlier code which is:
+ *    Copyright (c) 2010 Grant Erickson <[email protected]>
+ *
+ *    Also based on pwm-samsung.c
+ *
+ *    This program is free software; you can redistribute it and/or

drivers/pwm/pwm-omap.c line 13

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

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

Quoted 19 lines

drivers/pwm/pwm-omap.c new file +19 -0

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

drivers/pwm/pwm-omap.c line 34

Quoted 2 lines

drivers/pwm/pwm-omap.c new file +2 -0

+
+#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 ...

Quoted 48 lines

drivers/pwm/pwm-omap.c new file +47 -0

+#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.
+	 */

drivers/pwm/pwm-omap.c line 87

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

+
+	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?

Quoted 46 lines

drivers/pwm/pwm-omap.c new file +45 -0

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

drivers/pwm/pwm-omap.c line 137

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

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

Quoted 14 lines

drivers/pwm/pwm-omap.c new file +13 -0

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

drivers/pwm/pwm-omap.c line 161

Quoted 10 lines

drivers/pwm/pwm-omap.c new file +10 -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.
+	 */
+
+	omap_dm_timer_disable(omap->dm_timer);

Similarly the disable should not be needed here either.

Quoted 13 lines

drivers/pwm/pwm-omap.c new file +13 -0

+	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,
+};

drivers/pwm/pwm-omap.c line 184

Quoted 10 lines

drivers/pwm/pwm-omap.c new file +10 -0

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

Quoted 17 lines

drivers/pwm/pwm-omap.c new file +17 -0

+{
+	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.

drivers/pwm/pwm-omap.c line 204

Quoted 3 lines

drivers/pwm/pwm-omap.c new file +3 -0

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

Quoted 46 lines

drivers/pwm/pwm-omap.c new file +45 -0

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

drivers/pwm/pwm-omap.c line 259

Quoted 10 lines

drivers/pwm/pwm-omap.c new file +10 -0

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

drivers/pwm/pwm-omap.c line 268

Quoted 9 lines

drivers/pwm/pwm-omap.c new file +9 -0

+{
+	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?

Quoted 15 lines

drivers/pwm/pwm-omap.c new file +14 -0

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

drivers/pwm/pwm-omap.c line 285

Quoted 3 lines

drivers/pwm/pwm-omap.c new file +3 -0

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

drivers/pwm/pwm-omap.c line 297

Quoted 11 lines

drivers/pwm/pwm-omap.c new file +11 -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),

I believe that __devexit_p is no longer required.

Otherwise it looks good to me. Thanks for sending!

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

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

Quoted 20 lines

Quoted 20 lines

drivers/pwm/pwm-omap.c new file +18 -0

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

Quoted 8 lines

drivers/pwm/pwm-omap.c line 161

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

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

Quoted 5 lines · On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <[email protected]> wrote:

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

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

Quoted 2 lines

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?

Quoted 7 lines · NeilBrown

Quoted 5 lines · On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <[email protected]> wrote:

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

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

Quoted 8 lines

Quoted 2 lines

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?

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

Quoted 3 lines · On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <[email protected]> wrote:

Hi Neil,

On 12/12/2012 02:24 AM, NeilBrown wrote:

Quoted 11 lines

drivers/pwm/pwm-omap.c line 268

Quoted 9 lines

drivers/pwm/pwm-omap.c new file +9 -0

+{
+	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?

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

Quoted 21 lines · On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <[email protected]> wrote:

Hi Neil,

Quoted 18 lines · NeilBrown

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 <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/pwm/Kconfig +0 -0

index ed81720..7df573a 100644
@@ -85,6 +85,15 @@ config PWM_MXS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-mxs.
 

Quoted 5 lines

drivers/pwm/Kconfig config PWM_MXS · line 90

Quoted 3 lines

drivers/pwm/Kconfig +3 -0

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

Quoted 36 lines

Quoted 36 lines

drivers/pwm/Kconfig +6 -0

+	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

drivers/pwm/Makefile +1 -0

index acfe482..f5d200d 100644
@@ -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

drivers/pwm/pwm-omap.c new file +8 -0

new file mode 100644
index 0000000..e3dbce3
@@ -0,0 +1,318 @@
+/*
+ *    Copyright (c) 2012 NeilBrown <[email protected]>
+ *    Heavily based on earlier code which is:
+ *    Copyright (c) 2010 Grant Erickson <[email protected]>
+ *
+ *    Also based on pwm-samsung.c
+ *
+ *    This program is free software; you can redistribute it and/or

Quoted 9 lines

drivers/pwm/pwm-omap.c line 13

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

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

Quoted 19 lines

Quoted 19 lines

drivers/pwm/pwm-omap.c new file +19 -0

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

Quoted 8 lines

drivers/pwm/pwm-omap.c line 34

Quoted 2 lines

drivers/pwm/pwm-omap.c new file +2 -0

+
+#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.

Quoted 48 lines

Quoted 48 lines

drivers/pwm/pwm-omap.c new file +47 -0

+#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.
+	 */

Quoted 8 lines

drivers/pwm/pwm-omap.c line 87

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

+
+	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 :-)

Quoted 46 lines

Quoted 46 lines

drivers/pwm/pwm-omap.c new file +45 -0

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

Quoted 8 lines

drivers/pwm/pwm-omap.c line 137

Quoted 5 lines

drivers/pwm/pwm-omap.c new file +5 -0

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

Quoted 70 lines

Quoted 14 lines

drivers/pwm/pwm-omap.c new file +13 -0

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

drivers/pwm/pwm-omap.c line 161

Quoted 10 lines

drivers/pwm/pwm-omap.c new file +10 -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.
+	 */
+
+	omap_dm_timer_disable(omap->dm_timer);

Similarly the disable should not be needed here either.

Quoted 13 lines

drivers/pwm/pwm-omap.c new file +13 -0

+	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,
+};

drivers/pwm/pwm-omap.c line 184

Quoted 10 lines

drivers/pwm/pwm-omap.c new file +10 -0

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

Quoted 17 lines

drivers/pwm/pwm-omap.c new file +17 -0

+{
+	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.

Quoted 8 lines

drivers/pwm/pwm-omap.c line 204

Quoted 3 lines

drivers/pwm/pwm-omap.c new file +3 -0

+	 */
+
+	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?

Quoted 60 lines

Quoted 47 lines

drivers/pwm/pwm-omap.c new file +45 -0

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

drivers/pwm/pwm-omap.c line 259

Quoted 10 lines

drivers/pwm/pwm-omap.c new file +10 -0

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

Quoted 11 lines

drivers/pwm/pwm-omap.c line 268

Quoted 9 lines

drivers/pwm/pwm-omap.c new file +9 -0

+{
+	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?

Quoted 15 lines

Quoted 15 lines

drivers/pwm/pwm-omap.c new file +14 -0

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

Quoted 8 lines

drivers/pwm/pwm-omap.c line 285

Quoted 3 lines

drivers/pwm/pwm-omap.c new file +3 -0

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

Quoted 18 lines

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.

drivers/pwm/pwm-omap.c line 297

Quoted 11 lines

drivers/pwm/pwm-omap.c new file +11 -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),

I believe that __devexit_p is no longer required.

Quoted 1 line

Otherwise it looks good to me. Thanks for sending!

And thanks a lot for reading and reviewing!

Quoted 11 lines · NeilBrown

Quoted 9 lines · On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <[email protected]> wrote:

Review

Quoted 2 lines · NeilBrown

(quoted diff) +2 -0

+
+#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 ...

Quoted 1 line

But that doesn't work with devicetree does it?

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

Quoted 1 line

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.

Quoted 19 lines · NeilBrown

Quoted 18 lines

Review

Quoted 13 lines

(quoted diff) +11 -0

+
+#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?

Quoted 6 lines

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 <[email protected]>
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 <[email protected]>

arch/arm/plat-omap/dmtimer.c +0 -0

index 938b50a..c216696 100644
@@ -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);

Quoted 12 lines · On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <[email protected]> wrote:

drivers/pwm/pwm-omap.c line 204

Quoted 6 lines · NeilBrown

drivers/pwm/pwm-omap.c new file +6 -0

+
+	/* 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?