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@xxxxxxxxx>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
> 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@xxxxxxx>
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@xxxxxxxxx>");
> +MODULE_AUTHOR("NeilBrown <neilb@xxxxxxx>");
> +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@xxxxxxx>
> + *
> + * 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:
pgpuOp86QfqIg.pgp
Description: PGP signature
