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

[Thierry Reding <thierry.reding@gmail.com>]

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