[PATCH/RFC] pwm: omap: Add PWM support using dual-mode timers

[PATCH/RFC] pwm: omap: Add PWM support using dual-mode timers

[NeilBrown]

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


I submitted this in December last year.  I got lots of good feedback
and fixed some things, but it never got accepted.  Not entirely sure
why, maybe I dropped the ball.

Anyway, here is again with device-tree support added.

This is only an RFC and not a real submission for two reasons, both of which
are really directed as Jon.

1/ I have to 

#include <../arch/arm/plat-omap/include/plat/dmtimer.h>

which is incredibly ugly.
Is there any reason this cannot be moved to include/linux/omap-dmtimer.h?

2/ I found that I need to call

	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);

 when using device-tree.  This is because with devicetree
 omap_timer_restore_context() is called much more often and it sets the counter
 register to 0 .. it takes a long time to count up to DM_TIMER_LOAD_MIN from there.

 Now I don't object to calling omap_dm_timer_write_counter (though it might be nice if
 omap_dm_timer_set_load wrote the one value to both LOAD_REG and COUNTER_REG).
 However it seems wrong that I have to call it *after* starting the counter.
 Currently _write_counter refuses to run if the timer hasn't been started.

 So Jon: 
   a/ can we change omap_dm_timer_write_counter to work when the timer isn't
      running?
   b/ can we have omap_dm_timer_set_load also set the counter?


For anyone else generous enough to read my code: is this otherwise acceptable?

Thanks,
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.

The choice of dmtimer to be used can be made through platform_data
by requesting a specific timer, or though devicetree by giving
the appropriate device-tree node.

Lots of clean-ups and improvements thanks to Thierry Reding
and Jon Hunter.

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

diff --git a/Documentation/devicetree/bindings/pwm/pwm-omap.txt b/Documentation/devicetree/bindings/pwm/pwm-omap.txt
new file mode 100644
index 0000000..5f03048
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-omap.txt
@@ -0,0 +1,32 @@
+* PWM for OMAP using dmtimers
+
+TI's OMAP SoCs contains dual mode timers (dmtimers), some of
+which can driver output pins and so provide services as
+a PWM.  When using this driver it will normally be necessary
+to programmer an appropriate pin to server as a timer output.
+
+Required properties:
+- compatible: must be
+   "ti,omap-pwm"
+
+- timers: a device-tree node for the dmtimer to use
+
+- #pwm-cells: must be <2>.
+
+Example:
+
+ pwm: omap-pwm {
+   compatible = "ti,omap-pwm";
+   timers = <&timer11>;
+   #pwm-cells = <2>;
+
+   pinctrl-names = "default";
+   pinctrl-0 = <&pwm-pins>;
+ };
+
+...
+ pwm-pins: pinmux_pwm_pins {
+   pinctrl-single,pins = <
+     0x08a (PIN_OUTPUT | MUX_MODE3) /* gpmc_ncs6.gpt11_pwm_evt */
+   >;
+ };
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 75840b5..0e3cf83 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -110,6 +110,15 @@ config PWM_PCA9685
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-pca9685.
 
+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 77a8c18..322ddf0 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.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_RENESAS_TPU)	+= pwm-renesas-tpu.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 0000000..5ff2b17
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,283 @@
+/*
+ *    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 <../arch/arm/plat-omap/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);
+	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
+
+	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_from_pdata(struct omap_chip *omap,
+			       struct omap_pwm_pdata *pdata)
+{
+
+	/*
+	 * 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)
+		return -EPROBE_DEFER;
+
+	/*
+	 * 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.
+	 */
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static inline int omap_pwm_from_dt(struct omap_chip *omap,
+				  struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *timer;
+	if (!np)
+		return -ENODEV;
+	timer = of_parse_phandle(np, "timers", 0);
+	if (!timer)
+		return -ENODEV;
+
+	omap->dm_timer = omap_dm_timer_request_by_node(timer);
+	if (!omap->dm_timer)
+		return -EPROBE_DEFER;
+	return 0;
+}
+#else
+static inline in omap_pwm_from_dt(struct omap_chip *omap,
+				  struct device *dev)
+{
+	return -ENODEV;
+}
+#endif
+
+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;
+
+	omap = devm_kzalloc(dev, sizeof(*omap), GFP_KERNEL);
+	if (omap == NULL) {
+		dev_err(dev, "Could not allocate memory.\n");
+		return -ENOMEM;
+	}
+	if (pdata)
+		status = omap_pwm_from_pdata(omap, pdata);
+	else
+		status = omap_pwm_from_dt(omap, dev);
+	if (status)
+		return status;
+
+	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);
+		return status;
+	}
+
+	platform_set_drvdata(pdev, omap);
+
+	return 0;
+}
+
+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);
+
+	return 0;
+}
+
+static const struct of_device_id omap_pwm_of_match[] = {
+	{.compatible = "ti,omap-pwm"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, omap_pwm_of_match);
+
+static struct platform_driver omap_pwm_driver = {
+	.driver = {
+		.name	= "omap-pwm",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(omap_pwm_of_match),
+	},
+	.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/RFC] pwm: omap: Add PWM support using dual-mode timers

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [131023 23:36]:
> 
> I submitted this in December last year.  I got lots of good feedback
> and fixed some things, but it never got accepted.  Not entirely sure
> why, maybe I dropped the ball.
> 
> Anyway, here is again with device-tree support added.
> 
> This is only an RFC and not a real submission for two reasons, both of which
> are really directed as Jon.
> 
> 1/ I have to 
> 
> #include <../arch/arm/plat-omap/include/plat/dmtimer.h>
> 
> which is incredibly ugly.
> Is there any reason this cannot be moved to include/linux/omap-dmtimer.h?

Yes that's what at least dw_apb_timer and sh_timer are doing.
 
> 2/ I found that I need to call
> 
> 	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> 
>  when using device-tree.  This is because with devicetree
>  omap_timer_restore_context() is called much more often and it sets the counter
>  register to 0 .. it takes a long time to count up to DM_TIMER_LOAD_MIN from there.
> 
>  Now I don't object to calling omap_dm_timer_write_counter (though it might be nice if
>  omap_dm_timer_set_load wrote the one value to both LOAD_REG and COUNTER_REG).
>  However it seems wrong that I have to call it *after* starting the counter.
>  Currently _write_counter refuses to run if the timer hasn't been started.
> 
>  So Jon: 
>    a/ can we change omap_dm_timer_write_counter to work when the timer isn't
>       running?
>    b/ can we have omap_dm_timer_set_load also set the counter?
> 
> 
> For anyone else generous enough to read my code: is this otherwise acceptable?

Did not look beyond the dmtimer stuff, but let's start by moving dmtimer.c to
live under drivers and move the header, then do the pwm patches.

If there's a bug with the omap_dm_timer_set_load(), then naturally that
should be done separately as a fix before anything.

Regards,

Tony

Re: [PATCH/RFC] pwm: omap: Add PWM support using dual-mode timers

[Thierry Reding]

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

On Tue, Oct 29, 2013 at 02:23:18PM -0700, Tony Lindgren wrote:
> * NeilBrown <neilb@suse.de> [131023 23:36]:
> > 
> > I submitted this in December last year.  I got lots of good feedback
> > and fixed some things, but it never got accepted.  Not entirely sure
> > why, maybe I dropped the ball.
> > 
> > Anyway, here is again with device-tree support added.
> > 
> > This is only an RFC and not a real submission for two reasons, both of which
> > are really directed as Jon.
> > 
> > 1/ I have to 
> > 
> > #include <../arch/arm/plat-omap/include/plat/dmtimer.h>
> > 
> > which is incredibly ugly.
> > Is there any reason this cannot be moved to include/linux/omap-dmtimer.h?
> 
> Yes that's what at least dw_apb_timer and sh_timer are doing.
>  
> > 2/ I found that I need to call
> > 
> > 	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > 
> >  when using device-tree.  This is because with devicetree
> >  omap_timer_restore_context() is called much more often and it sets the counter
> >  register to 0 .. it takes a long time to count up to DM_TIMER_LOAD_MIN from there.
> > 
> >  Now I don't object to calling omap_dm_timer_write_counter (though it might be nice if
> >  omap_dm_timer_set_load wrote the one value to both LOAD_REG and COUNTER_REG).
> >  However it seems wrong that I have to call it *after* starting the counter.
> >  Currently _write_counter refuses to run if the timer hasn't been started.
> > 
> >  So Jon: 
> >    a/ can we change omap_dm_timer_write_counter to work when the timer isn't
> >       running?
> >    b/ can we have omap_dm_timer_set_load also set the counter?
> > 
> > 
> > For anyone else generous enough to read my code: is this otherwise acceptable?
> 
> Did not look beyond the dmtimer stuff, but let's start by moving dmtimer.c to
> live under drivers and move the header, then do the pwm patches.

Why does the driver have to move? There is certainly some precedent for
having arch-specific code in arch/ but expose the public API via some
header in include/linux. Sometimes there's just no proper place for the
driver elsewhere, so rather than moving it somewhere more or less random
keeping it in arch/arm/plat-omap is just as good.

Thierry

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

Re: [PATCH/RFC] pwm: omap: Add PWM support using dual-mode timers

[Thierry Reding]

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

On Thu, Oct 24, 2013 at 05:36:20PM +1100, NeilBrown wrote:
> 
> I submitted this in December last year.  I got lots of good feedback
> and fixed some things, but it never got accepted.  Not entirely sure
> why, maybe I dropped the ball.
> 
> Anyway, here is again with device-tree support added.
> 
> This is only an RFC and not a real submission for two reasons, both of which
> are really directed as Jon.
> 
> 1/ I have to 
> 
> #include <../arch/arm/plat-omap/include/plat/dmtimer.h>
> 
> which is incredibly ugly.
> Is there any reason this cannot be moved to include/linux/omap-dmtimer.h?
> 
> 2/ I found that I need to call
> 
> 	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> 
>  when using device-tree.  This is because with devicetree
>  omap_timer_restore_context() is called much more often and it sets the counter
>  register to 0 .. it takes a long time to count up to DM_TIMER_LOAD_MIN from there.
> 
>  Now I don't object to calling omap_dm_timer_write_counter (though it might be nice if
>  omap_dm_timer_set_load wrote the one value to both LOAD_REG and COUNTER_REG).
>  However it seems wrong that I have to call it *after* starting the counter.
>  Currently _write_counter refuses to run if the timer hasn't been started.
> 
>  So Jon: 
>    a/ can we change omap_dm_timer_write_counter to work when the timer isn't
>       running?
>    b/ can we have omap_dm_timer_set_load also set the counter?
> 
> 
> For anyone else generous enough to read my code: is this otherwise acceptable?
> 
> Thanks,
> 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.
> 
> The choice of dmtimer to be used can be made through platform_data
> by requesting a specific timer, or though devicetree by giving
> the appropriate device-tree node.
> 
> Lots of clean-ups and improvements thanks to Thierry Reding
> and Jon Hunter.
> 
> Cc: Grant Erickson <marathon96@gmail.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-omap.txt b/Documentation/devicetree/bindings/pwm/pwm-omap.txt
> new file mode 100644
> index 0000000..5f03048
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-omap.txt
> @@ -0,0 +1,32 @@
> +* PWM for OMAP using dmtimers
> +
> +TI's OMAP SoCs contains dual mode timers (dmtimers), some of
> +which can driver output pins and so provide services as

s/driver/drive/

> +a PWM.  When using this driver it will normally be necessary
> +to programmer an appropriate pin to server as a timer output.

s/programmer/program/ and s/server/serve/

> +
> +Required properties:
> +- compatible: must be
> +   "ti,omap-pwm"
> +
> +- timers: a device-tree node for the dmtimer to use
> +
> +- #pwm-cells: must be <2>.

The canonical form to write this these days is:

- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
  the cells format.

> +
> +Example:
> +
> + pwm: omap-pwm {
> +   compatible = "ti,omap-pwm";
> +   timers = <&timer11>;
> +   #pwm-cells = <2>;
> +
> +   pinctrl-names = "default";
> +   pinctrl-0 = <&pwm-pins>;
> + };
> +
> +...
> + pwm-pins: pinmux_pwm_pins {

I don't think dashes work in labels or phandles. They do work in node
names, though, so this could be:

	pwm_pins: pinmux-pwm-pins {
		...
	};

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 75840b5..0e3cf83 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -110,6 +110,15 @@ config PWM_PCA9685
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-pca9685.
>  
> +config PWM_OMAP

This doesn't seem to be properly ordered now. I suspect that back when
you posted that last time PCA9685 support hadn't been merged yet and the
rebase messed this up.

I wonder: does the OMAP not have dedicated PWM hardware?

> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 77a8c18..322ddf0 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>  obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
> +obj-$(CONFIG_PWM_OMAP)		+= pwm-omap.o
>  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o

Same ordering issue here.

> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
[...]
> +/*
> + *    Copyright (c) 2012 NeilBrown <neilb@suse.de>

I guess that'd include 2013 now?

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

I'd prefer these to be sorted alphabetically.

> +#include <../arch/arm/plat-omap/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;

These should no longer be necessary. polarity, duty_ns and period_ns are
available in struct pwm_device nowadays.

> +	struct pwm_chip		chip;

Nit: If you sort the chip field first, then the to_omap_chip() below
essentially becomes a no-op.

> +};
> +
> +#define to_omap_chip(chip)	container_of(chip, struct omap_chip, chip)

This should be a static inline function for type checking.

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

Nit: perhaps rename ns to period to make the purpose clearer?

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

This condition easily fits on a single line.

> +		/* No change - don't cause any transients. */
> +		return 0;
> +
> +	clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer));

I'd prefer this to be split up into getting a struct clk * and then
querying that.

> +static struct pwm_ops omap_pwm_ops = {

static const, please.

> +	.enable		= omap_pwm_enable,
> +	.disable	= omap_pwm_disable,
> +	.config		= omap_pwm_config,
> +	.set_polarity	= omap_pwm_set_polarity,
> +	.owner		= THIS_MODULE,
> +};

I prefer these not to be aligned at all. It doesn't add to the
readability and makes a mess when a new function is added with a name
longer than "set_polarity".

> +static int omap_pwm_from_pdata(struct omap_chip *omap,
> +			       struct omap_pwm_pdata *pdata)
> +{
> +

There's a spurious blank line here.

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

For consistency with the remainder of this driver I'd prefer this to be:

	if (!omap->dm_timer)

> +		return -EPROBE_DEFER;

Can there ever be another failure? Perhaps an invalid timer_id? I
suppose that if the omap_dm_timer API can't provide more details this is
as good as it gets. Ideally omap_dm_timer_request_specific() would
return an ERR_PTR()-encoded error code which you could then simply
propagate.

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

Move what "to the configure method"? There's nothing here that could be
moved.

> +#ifdef CONFIG_OF
> +static inline int omap_pwm_from_dt(struct omap_chip *omap,
> +				  struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;

I don't think you necessarily need this temporary variable. You only use
it twice. If you make the change I propose a little further down, you'll
only reference it once so keeping it around isn't useful.

> +	struct device_node *timer;
> +	if (!np)
> +		return -ENODEV;
> +	timer = of_parse_phandle(np, "timers", 0);

Could use a blank line to separate the above two. Although with the
change proposed below the if (!np) check can actually go away.

> +	if (!timer)
> +		return -ENODEV;
> +
> +	omap->dm_timer = omap_dm_timer_request_by_node(timer);
> +	if (!omap->dm_timer)
> +		return -EPROBE_DEFER;
> +	return 0;

Could use another blank line to separate the above two lines. Again it
would be nicer if omap_dm_timer_request_by_node() returned some kind of
precise error. Unless there really are no errors other than the device
not being there yet (therefore assuming deferred probe will eventually
succeed).

> +}
> +#else
> +static inline in omap_pwm_from_dt(struct omap_chip *omap,
> +				  struct device *dev)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
> +static int omap_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct omap_chip *omap;
> +	int status = 0;

There should be no need to initialize this.

> +	struct omap_pwm_pdata *pdata = dev->platform_data;
> +
> +	omap = devm_kzalloc(dev, sizeof(*omap), GFP_KERNEL);
> +	if (omap == NULL) {

"if (!omap)", please.

> +		dev_err(dev, "Could not allocate memory.\n");

We don't need an error message here.

> +		return -ENOMEM;
> +	}
> +	if (pdata)
> +		status = omap_pwm_from_pdata(omap, pdata);
> +	else
> +		status = omap_pwm_from_dt(omap, dev);

I'd like to propose that this be rewritten as:

	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
		status = omap_pwm_from_dt(omap, dev);

The reason is that you can now simply drop the #ifdeffery around the
function's implementation, remove the dummy and have the compiler
automatically discard the function for !OF. That gives you compile
coverage for free.

> +	if (status)
> +		return status;
> +
> +	omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
> +	/*

Could use another blank line.

> +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);
> +
> +	return 0;
> +}

Perhaps call pwmchip_remove() last so that omap_dm_timer_free() is
always called, even if pwmchip_remove() fails?

That way you can also make it somewhat shorter using:

	return pwmchip_remove(&omap->chip);

> +
> +static const struct of_device_id omap_pwm_of_match[] = {
> +	{.compatible = "ti,omap-pwm"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, omap_pwm_of_match);

I think this will need #ifdef protection, otherwise the usage of
of_match_ptr() below will make this static and unused.

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

.owner no longer needs to be assigned, since the core takes care of that
nowadays.

> +		.of_match_table = of_match_ptr(omap_pwm_of_match),
> +	},
> +	.probe		= omap_pwm_probe,
> +	.remove		= omap_pwm_remove,

The alignment here is also not necessary. Note how .name and .owner have
been aligned with tabs, but then .of_match_table messes it all up by
being so long. Better not artificially align at all.

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

Hehe, haven't seen one of these in a while. Do we really need it?

> diff --git a/include/linux/platform_data/omap-pwm.h b/include/linux/platform_data/omap-pwm.h
[...]
> @@ -0,0 +1,20 @@
> +/*
> + * omap-pwm.h

I don't think we really need the filename here.

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

Nit: s/id/ID/

> + */
> +
> +#ifndef _OMAP_PWM_H_
> +#define _OMAP_PWM_H_
> +
> +struct omap_pwm_pdata {
> +	int	timer_id;

More needless alignment.

Thierry

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

Re: [PATCH/RFC] pwm: omap: Add PWM support using dual-mode timers

[Tony Lindgren]

* Thierry Reding <thierry.reding@gmail.com> [131212 05:02]:
> On Tue, Oct 29, 2013 at 02:23:18PM -0700, Tony Lindgren wrote:
> > * NeilBrown <neilb@suse.de> [131023 23:36]:
> > > 
> > > I submitted this in December last year.  I got lots of good feedback
> > > and fixed some things, but it never got accepted.  Not entirely sure
> > > why, maybe I dropped the ball.
> > > 
> > > Anyway, here is again with device-tree support added.
> > > 
> > > This is only an RFC and not a real submission for two reasons, both of which
> > > are really directed as Jon.
> > > 
> > > 1/ I have to 
> > > 
> > > #include <../arch/arm/plat-omap/include/plat/dmtimer.h>
> > > 
> > > which is incredibly ugly.
> > > Is there any reason this cannot be moved to include/linux/omap-dmtimer.h?
> > 
> > Yes that's what at least dw_apb_timer and sh_timer are doing.
> >  
> > > 2/ I found that I need to call
> > > 
> > > 	omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > > 
> > >  when using device-tree.  This is because with devicetree
> > >  omap_timer_restore_context() is called much more often and it sets the counter
> > >  register to 0 .. it takes a long time to count up to DM_TIMER_LOAD_MIN from there.
> > > 
> > >  Now I don't object to calling omap_dm_timer_write_counter (though it might be nice if
> > >  omap_dm_timer_set_load wrote the one value to both LOAD_REG and COUNTER_REG).
> > >  However it seems wrong that I have to call it *after* starting the counter.
> > >  Currently _write_counter refuses to run if the timer hasn't been started.
> > > 
> > >  So Jon: 
> > >    a/ can we change omap_dm_timer_write_counter to work when the timer isn't
> > >       running?
> > >    b/ can we have omap_dm_timer_set_load also set the counter?
> > > 
> > > 
> > > For anyone else generous enough to read my code: is this otherwise acceptable?
> > 
> > Did not look beyond the dmtimer stuff, but let's start by moving dmtimer.c to
> > live under drivers and move the header, then do the pwm patches.
> 
> Why does the driver have to move? There is certainly some precedent for
> having arch-specific code in arch/ but expose the public API via some
> header in include/linux. Sometimes there's just no proper place for the
> driver elsewhere, so rather than moving it somewhere more or less random
> keeping it in arch/arm/plat-omap is just as good.

There's actually been some developments regarding the dmtimer usage after
these comments, see below.

Basically we don't want to export custom functions as we've seen what kind
of mess exporting custom interfaces makes. See the current state of omap
DMA for example.

We've figured out a way to access the timers by doing a request_irq()
on a timer, then do a clk_set_rate() to select the timer source clock.

And then we need only about five custom functions to program the timer,
which can be eventually replaced with some Linux generic hardware timer
framework.

For more info, see the discussion in thread "[PATCH 6/8] devicetree: doc:
Document ti,timer-parent property".

Regards,

Tony