Message ID | 20121212192430.50cea126@notabene.brown (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote: > > > This patch is based on an earlier patch by Grant Erickson > which provided pwm devices using the 'legacy' interface. > > This driver instead uses the new framework interface. I'd prefer some kind of description about the driver here. Also the subject should be something like: pwm: Add OMAP support using dual-mode timers or pwm: omap: Add PWM support using dual-mode timers I take this description to mean that OMAP doesn't have dedicated PWM hardware? Otherwise it might be bad to call this pwm-omap. Also please use all-caps when referring to PWM devices in prose. A few other comments inline below. > Cc: Grant Erickson <marathon96@gmail.com> > Signed-off-by: NeilBrown <neilb@suse.de> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index ed81720..7df573a 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -85,6 +85,15 @@ config PWM_MXS > To compile this driver as a module, choose M here: the module > will be called pwm-mxs. > > +config PWM_OMAP > + tristate "OMAP pwm support" "OMAP PWM support" > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c [...] > + * The 'id' number for the device encodes the number of the dm timer > + * to use, and the polarity of the output. > + * lsb is '1' of active-high, and '0' for active low > + * remaining bit a timer number and need to be shifted down before use. I don't know if this is such a good idea. Usually you number platform devices sequentially, while this would leave gaps in the numbering. I know that adding platform data may sound a bit like overkill, but I really think the added clarity and consistency is worth it. > +#define pr_fmt(fmt) "pwm-omap: " fmt You don't seem to be using any of the pr_*() logging functions, so this isn't needed. > +#include <linux/export.h> > +#include <linux/kernel.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/pwm.h> > +#include <linux/module.h> > + > +#include <plat/dmtimer.h> > + > +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE > + > +struct omap_chip { > + struct platform_device *pdev; I don't see this field being used anywhere. > + struct omap_dm_timer *dm_timer; > + unsigned int polarity; The PWM subsystem already has enum pwm_polarity for this. > + const char *label; This isn't used anywhere either. > + > + unsigned int duty_ns, period_ns; > + struct pwm_chip chip; > +}; > + > +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip) > + > +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg) This is never used. > + > +/** > + * pwm_calc_value - determines the counter value for a clock rate and period. Nit: You should either start the sentence with a capital or not terminate it with a full stop. > + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the > + * counter value for. > + * @ns: The period, in nanoseconds, to computer the counter value for. "compute" > + * > + * Returns the PWM counter value for the specified clock rate and period. > + */ > +static inline int pwm_calc_value(unsigned long clk_rate, int ns) > +{ > + const unsigned long nanoseconds_per_second = 1000000000; Maybe use NSEC_PER_SEC instead? > + int cycles; > + __u64 c; I think for in-kernel use, the custom is to stick with simply u64. > + > + c = (__u64)clk_rate * ns; > + do_div(c, nanoseconds_per_second); > + cycles = c; > + > + return DM_TIMER_LOAD_MIN - cycles; Can't you just do "DM_TIMER_LOAD_MIN - c" and get rid of the cycles variable altogether? > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct omap_chip *omap = to_omap_chip(chip); > + int status = 0; > + > + /* Enable the counter--always--before attempting to write its > + * registers and then set the timer to its minimum load value to > + * ensure we get an overflow event right away once we start it. > + */ Block comments should be in the following format: /* * foo... * bar... */ > + > + omap_dm_timer_enable(omap->dm_timer); > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > + omap_dm_timer_start(omap->dm_timer); > + omap_dm_timer_disable(omap->dm_timer); So omap_dm_timer_disable() doesn't actually stop the timer? It just disables the access to the registers? > + return status; "return 0;" and drop the status variable. > +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct omap_chip *omap = to_omap_chip(chip); > + int status = 0; This one can be dropped as well. > + const bool enable = true; > + const bool autoreload = true; > + const bool toggle = true; > + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE; I understand that these extra variables are supposed to "document" the parameters of the functions below. I'm not a huge fan of this approach because instead the API would better be designed to make it obvious what the parameters are. Or people can just look at the prototypes to find out the meaning. But if you really prefer this way I won't object. > + int load_value, match_value; > + unsigned long clk_rate; > + > + dev_dbg(chip->dev, > + "duty cycle: %d, period %d\n", > + duty_ns, period_ns); This all fits on a single line, so no need to break it up. > + > + if (omap->duty_ns == duty_ns && > + omap->period_ns == period_ns) > + /* No change - don't cause any transients */ > + return 0; Note to self: this might be a candidate to put in the core. > + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer)); > + > + /* Calculate the appropriate load and match values based on the > + * specified period and duty cycle. The load value determines the > + * cycle time and the match value determines the duty cycle. > + */ Again wrong block comment style. > + > + load_value = pwm_calc_value(clk_rate, period_ns); > + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns); > + > + /* We MUST enable yet stop the associated dual-mode timer before > + * attempting to write its registers. Hopefully it is already > + * disabled, but call the (idempotent) pwm_disable just in case > + */ And here as well. While at it you might want to terminate the last sentence with a full stop. > + pwm_disable(pwm); > + > + omap_dm_timer_enable(omap->dm_timer); > + > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); > + > + dev_dbg(chip->dev, > + "load value: %#08x (%d), " > + "match value: %#08x (%d)\n", > + load_value, load_value, > + match_value, match_value); Again this doesn't need so much wrapping. > + > + omap_dm_timer_set_pwm(omap->dm_timer, > + !omap->polarity, > + toggle, > + trigger); This doesn't either. Also you should be explicit about the polarity parameter, since enum pwm_polarity is an enum and therefore negating it isn't very nice (it should work though). You could solve this by doing something like: if (omap->polarity == PWM_POLARITY_NORMAL) polarity = 1; else polarity = 0; > + /* Set the counter to generate an overflow event immediately. */ > + > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > + > + /* Now that we're done configuring the dual-mode timer, disable it > + * again. We'll enable and start it later, when requested. > + */ Also wrong style. > + omap_dm_timer_disable(omap->dm_timer); > + omap->duty_ns = duty_ns; > + omap->period_ns = period_ns; > + > + return status; "return 0;" > +} > + > + Gratuitous newline. > +static struct pwm_ops omap_pwm_ops = { Should be "static const". > + .enable = omap_pwm_enable, > + .disable= omap_pwm_disable, There should be a space between .disable and =. > + .config = omap_pwm_config, > + .owner = THIS_MODULE, > +}; > + > +/** > + * omap_pwm_probe - check for the PWM and bind it to the driver. > + * @pdev: A pointer to the platform device node associated with the > + * PWM instance to be probed for driver binding. > + * > + * Returns 0 if the PWM instance was successfully bound to the driver; > + * otherwise, < 0 on error. > + */ I'm not sure how useful this kerneldoc comment really is. It isn't about an exported function and developers usually know what the .probe() does. > +static int __devinit omap_pwm_probe(struct platform_device *pdev) No more __devinit, please. > +{ > + struct device *dev = &pdev->dev; > + struct omap_chip *omap; > + int status = 0; > + unsigned int id = pdev->id; > + unsigned int timer = id >> 1; /* lsb is polarity */ I've said this before, I don't think it's a good idea. > + > + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL); > + Gratuituous newline. > + if (omap == NULL) { > + dev_err(dev, "Could not allocate memory.\n"); > + status = -ENOMEM; > + goto done; > + } > + > + /* Request the OMAP dual-mode timer that will be bound to and > + * associated with this generic PWM. > + */ Block comment style again. > + > + omap->dm_timer = omap_dm_timer_request_specific(timer); > + Gratuitous newline. > + if (omap->dm_timer == NULL) { > + status = -EPROBE_DEFER; > + goto err_free; > + } > + > + /* Configure the source for the dual-mode timer backing this > + * generic PWM device. The clock source will ultimately determine > + * how small or large the PWM frequency can be. > + * > + * At some point, it's probably worth revisiting moving this to > + * the configure method and choosing either the slow- or > + * system-clock source as appropriate for the desired PWM period. > + */ And again. > + > + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK); > + > + /* Cache away other miscellaneous driver-private data and state > + * information and add the driver-private data to the platform > + * device. > + */ And again. =) > + > + omap->chip.dev = dev; > + omap->chip.ops = &omap_pwm_ops; > + omap->chip.base = -1; > + omap->chip.npwm = 1; > + omap->polarity = id & 1; > + > + status = pwmchip_add(&omap->chip); > + if (status < 0) { > + dev_err(dev, "failed to register pwm\n"); > + omap_dm_timer_free(omap->dm_timer); > + goto err_free; > + } > + > + platform_set_drvdata(pdev, omap); > + > + status = 0; > + goto done; This can just be "return 0;". > + > + err_free: > + kfree(omap); > + done: > + return status; > +} > + > +/** > + * omap_pwm_remove - unbind the specified PWM platform device from the driver. > + * @pdev: A pointer to the platform device node associated with the > + * PWM instance to be unbound/removed. > + * > + * Returns 0 if the PWM was successfully removed as a platform device; > + * otherwise, < 0 on error. > + */ > +static int __devexit omap_pwm_remove(struct platform_device *pdev) No __devexit, please. > +{ > + struct omap_chip *omap = platform_get_drvdata(pdev); > + int status = 0; Just drop this. > + > + status = pwmchip_remove(&omap->chip); > + if (status < 0) > + goto done; "return status;" > + > + omap_dm_timer_free(omap->dm_timer); > + > + kfree(omap); > + > + done: > + return status; Just "return 0;". No label required. > +} > + > +#if CONFIG_PM > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct omap_chip *omap = platform_get_drvdata(pdev); > + /* No one preserve these values during suspend so reset them > + * Otherwise driver leaves PWM unconfigured if same values > + * passed to pwm_config > + */ > + omap->period_ns = 0; > + omap->duty_ns = 0; > + > + return 0; > +} > +#else > +#define omap_pwm_suspend NULL > +#endif This doesn't look right. You should implement .resume() if you really care, in which case the resume callback would have to reconfigure with the cached values. In that case maybe you should switch to dev_pm_ops and SIMPLE_DEV_PM_OPS() as well. If you don't, just resetting these values will not make the PWM work properly after resume either since it will have to be explicitly reconfigured. > +static struct platform_driver omap_pwm_driver = { > + .driver.name = "omap-pwm", > + .driver.owner = THIS_MODULE, The more idiomatic way of writing this is: .driver = { .name = "omap-pwm", .owner = THIS_MODULE, }, > + .probe = omap_pwm_probe, > + .remove = __devexit_p(omap_pwm_remove), No __devexit_p anymore. > + .suspend = omap_pwm_suspend, > + .resume = NULL, > +}; > + > +static int __init omap_pwm_init(void) > +{ > + return platform_driver_register(&omap_pwm_driver); > +} > + > +static void __exit omap_pwm_exit(void) > +{ > + platform_driver_unregister(&omap_pwm_driver); > +} > + > +arch_initcall(omap_pwm_init); This should probably be module_init() instead. You already return -EPROBE_DEFER if the dual-mode timer isn't ready yet so you should bite the bullet and get all the dependencies to behave properly as well. New code shouldn't be using this kind of dependency handling. In that case you could also just run module_platform_driver(). Thierry > +module_exit(omap_pwm_exit); > + > +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>"); > +MODULE_AUTHOR("NeilBrown <neilb@suse.de>"); Shouldn't this be "Neil Brown"? I noticed you use the concatenated form in the email address as well, so maybe that's on purpose? > +MODULE_LICENSE("GPLv2"); This should be "GPL v2". Maybe MODULE_DESCRIPTION() would be nice too. Thierry
Hi Neil, On 12/12/2012 02:24 AM, NeilBrown wrote: > > > This patch is based on an earlier patch by Grant Erickson > which provided pwm devices using the 'legacy' interface. > > This driver instead uses the new framework interface. > > Cc: Grant Erickson <marathon96@gmail.com> > Signed-off-by: NeilBrown <neilb@suse.de> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index ed81720..7df573a 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -85,6 +85,15 @@ config PWM_MXS > To compile this driver as a module, choose M here: the module > will be called pwm-mxs. > > +config PWM_OMAP > + tristate "OMAP pwm support" > + depends on ARCH_OMAP We should probably have depends on or selects OMAP_DM_TIMER here too. > + help > + Generic PWM framework driver for OMAP > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-omap > + > config PWM_PUV3 > tristate "PKUnity NetBook-0916 PWM support" > depends on ARCH_PUV3 > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index acfe482..f5d200d 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o > obj-$(CONFIG_PWM_MXS) += pwm-mxs.o > +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o > obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c > new file mode 100644 > index 0000000..e3dbce3 > --- /dev/null > +++ b/drivers/pwm/pwm-omap.c > @@ -0,0 +1,318 @@ > +/* > + * Copyright (c) 2012 NeilBrown <neilb@suse.de> > + * Heavily based on earlier code which is: > + * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com> > + * > + * Also based on pwm-samsung.c > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * version 2 as published by the Free Software Foundation. > + * > + * Description: > + * This file is the core OMAP2/3 support for the generic, Linux I would drop the OMAP2/3 and just say OMAP here. Potentially this should work for OMAP1-5. > + * PWM driver / controller, using the OMAP's dual-mode timers. > + * > + * The 'id' number for the device encodes the number of the dm timer > + * to use, and the polarity of the output. > + * lsb is '1' of active-high, and '0' for active low > + * remaining bit a timer number and need to be shifted down before use. > + */ > + > +#define pr_fmt(fmt) "pwm-omap: " fmt > + > +#include <linux/export.h> > +#include <linux/kernel.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/pwm.h> > +#include <linux/module.h> > + > +#include <plat/dmtimer.h> This is going to be a problem for the single zImage work, because we cannot include any plat headers in driver code any more. Therefore, although it is not ideal, one way to handle this is pass function pointers to the various dmtimer APIs that are needed via the platform data. Painful I know ... > +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE > + > +struct omap_chip { > + struct platform_device *pdev; > + > + struct omap_dm_timer *dm_timer; > + unsigned int polarity; > + const char *label; > + > + unsigned int duty_ns, period_ns; > + struct pwm_chip chip; > +}; > + > +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip) > + > +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg) > + > +/** > + * pwm_calc_value - determines the counter value for a clock rate and period. > + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the > + * counter value for. > + * @ns: The period, in nanoseconds, to computer the counter value for. > + * > + * Returns the PWM counter value for the specified clock rate and period. > + */ > +static inline int pwm_calc_value(unsigned long clk_rate, int ns) > +{ > + const unsigned long nanoseconds_per_second = 1000000000; > + int cycles; > + __u64 c; > + > + c = (__u64)clk_rate * ns; > + do_div(c, nanoseconds_per_second); > + cycles = c; > + > + return DM_TIMER_LOAD_MIN - cycles; > +} > + > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct omap_chip *omap = to_omap_chip(chip); > + int status = 0; > + > + /* Enable the counter--always--before attempting to write its > + * registers and then set the timer to its minimum load value to > + * ensure we get an overflow event right away once we start it. > + */ > + > + omap_dm_timer_enable(omap->dm_timer); > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > + omap_dm_timer_start(omap->dm_timer); > + omap_dm_timer_disable(omap->dm_timer); Why not just use omap_dm_timer_load_start() here instead of the above 4 APIs? > + > + return status; > +} > + > +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct omap_chip *omap = to_omap_chip(chip); > + > + omap_dm_timer_stop(omap->dm_timer); > +} > + > +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct omap_chip *omap = to_omap_chip(chip); > + int status = 0; > + const bool enable = true; > + const bool autoreload = true; > + const bool toggle = true; > + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE; > + int load_value, match_value; > + unsigned long clk_rate; > + > + dev_dbg(chip->dev, > + "duty cycle: %d, period %d\n", > + duty_ns, period_ns); > + > + if (omap->duty_ns == duty_ns && > + omap->period_ns == period_ns) > + /* No change - don't cause any transients */ > + return 0; > + > + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer)); > + > + /* Calculate the appropriate load and match values based on the > + * specified period and duty cycle. The load value determines the > + * cycle time and the match value determines the duty cycle. > + */ > + > + load_value = pwm_calc_value(clk_rate, period_ns); > + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns); > + > + /* We MUST enable yet stop the associated dual-mode timer before > + * attempting to write its registers. Hopefully it is already > + * disabled, but call the (idempotent) pwm_disable just in case > + */ > + > + pwm_disable(pwm); > + > + omap_dm_timer_enable(omap->dm_timer); Do you need to call omap_dm_timer_enable here? _set_load and _set_match will enable the timer. So this should not be necessary. > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); > + > + dev_dbg(chip->dev, > + "load value: %#08x (%d), " > + "match value: %#08x (%d)\n", > + load_value, load_value, > + match_value, match_value); > + > + omap_dm_timer_set_pwm(omap->dm_timer, > + !omap->polarity, > + toggle, > + trigger); > + > + /* Set the counter to generate an overflow event immediately. */ > + > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > + > + /* Now that we're done configuring the dual-mode timer, disable it > + * again. We'll enable and start it later, when requested. > + */ > + > + omap_dm_timer_disable(omap->dm_timer); Similarly the disable should not be needed here either. > + omap->duty_ns = duty_ns; > + omap->period_ns = period_ns; > + > + return status; > +} > + > + > +static struct pwm_ops omap_pwm_ops = { > + .enable = omap_pwm_enable, > + .disable= omap_pwm_disable, > + .config = omap_pwm_config, > + .owner = THIS_MODULE, > +}; > + > +/** > + * omap_pwm_probe - check for the PWM and bind it to the driver. > + * @pdev: A pointer to the platform device node associated with the > + * PWM instance to be probed for driver binding. > + * > + * Returns 0 if the PWM instance was successfully bound to the driver; > + * otherwise, < 0 on error. > + */ > +static int __devinit omap_pwm_probe(struct platform_device *pdev) I believe that __devinit is no longer required. > +{ > + struct device *dev = &pdev->dev; > + struct omap_chip *omap; > + int status = 0; > + unsigned int id = pdev->id; > + unsigned int timer = id >> 1; /* lsb is polarity */ > + > + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL); > + > + if (omap == NULL) { > + dev_err(dev, "Could not allocate memory.\n"); > + status = -ENOMEM; > + goto done; > + } > + > + /* Request the OMAP dual-mode timer that will be bound to and > + * associated with this generic PWM. > + */ > + > + omap->dm_timer = omap_dm_timer_request_specific(timer); I would recommend that you use omap_dm_timer_request_by_cap() (new for v3.8 so you should be able to use once v3.8-rc1 is out) here to request a timer that supports the PWM output. The above function will not be supported when booting with device-tree. > + > + if (omap->dm_timer == NULL) { > + status = -EPROBE_DEFER; > + goto err_free; > + } > + > + /* Configure the source for the dual-mode timer backing this > + * generic PWM device. The clock source will ultimately determine > + * how small or large the PWM frequency can be. > + * > + * At some point, it's probably worth revisiting moving this to > + * the configure method and choosing either the slow- or > + * system-clock source as appropriate for the desired PWM period. > + */ > + > + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK); > + > + /* Cache away other miscellaneous driver-private data and state > + * information and add the driver-private data to the platform > + * device. > + */ > + > + omap->chip.dev = dev; > + omap->chip.ops = &omap_pwm_ops; > + omap->chip.base = -1; > + omap->chip.npwm = 1; > + omap->polarity = id & 1; > + > + status = pwmchip_add(&omap->chip); > + if (status < 0) { > + dev_err(dev, "failed to register pwm\n"); > + omap_dm_timer_free(omap->dm_timer); > + goto err_free; > + } > + > + platform_set_drvdata(pdev, omap); > + > + status = 0; > + goto done; > + > + err_free: > + kfree(omap); > + done: > + return status; > +} > + > +/** > + * omap_pwm_remove - unbind the specified PWM platform device from the driver. > + * @pdev: A pointer to the platform device node associated with the > + * PWM instance to be unbound/removed. > + * > + * Returns 0 if the PWM was successfully removed as a platform device; > + * otherwise, < 0 on error. > + */ > +static int __devexit omap_pwm_remove(struct platform_device *pdev) I believe that __devexit is no longer required. > +{ > + struct omap_chip *omap = platform_get_drvdata(pdev); > + int status = 0; > + > + status = pwmchip_remove(&omap->chip); > + if (status < 0) > + goto done; > + > + omap_dm_timer_free(omap->dm_timer); Is it guaranteed that the timer will be disabled at this point? > + > + kfree(omap); > + > + done: > + return status; > +} > + > +#if CONFIG_PM > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct omap_chip *omap = platform_get_drvdata(pdev); > + /* No one preserve these values during suspend so reset them > + * Otherwise driver leaves PWM unconfigured if same values > + * passed to pwm_config > + */ > + omap->period_ns = 0; > + omap->duty_ns = 0; Hmmm, looks like you are trying to force a reconfiguration after suspend if the same values are used. Is there an underlying problem here that you are trying to workaround? Please note that I am not familiar with the PWM sub-system to know how suspend-resume is typically handled and if this is normal or not. > + return 0; > +} > +#else > +#define omap_pwm_suspend NULL > +#endif > + > +static struct platform_driver omap_pwm_driver = { > + .driver.name = "omap-pwm", > + .driver.owner = THIS_MODULE, > + .probe = omap_pwm_probe, > + .remove = __devexit_p(omap_pwm_remove), I believe that __devexit_p is no longer required. Otherwise it looks good to me. Thanks for sending! Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/2012 05:31 AM, Thierry Reding wrote: > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote: [snip] >> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + struct omap_chip *omap = to_omap_chip(chip); >> + int status = 0; >> + >> + /* Enable the counter--always--before attempting to write its >> + * registers and then set the timer to its minimum load value to >> + * ensure we get an overflow event right away once we start it. >> + */ > > Block comments should be in the following format: > > /* > * foo... > * bar... > */ > >> + >> + omap_dm_timer_enable(omap->dm_timer); >> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); >> + omap_dm_timer_start(omap->dm_timer); >> + omap_dm_timer_disable(omap->dm_timer); > > So omap_dm_timer_disable() doesn't actually stop the timer? It just > disables the access to the registers? I thought this looked odd too ;-) So what is going on here is that omap_dm_timer_start() calls omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the last disable really just complements the first enable (ie. decrements the use count), but the timer will not actually be disabled, because the start has called an extra enable. These four function calls can be replaced by one call to omap_dm_timer_set_load_start() and I think that will be much clearer and concise. In general, it should not be necessary to call these omap_dm_timer_enable/disable APIs directly. I am not sure what the history is or if there is a use-case that really requires this. So in the future may be I should make them static so they cannot be used directly :-) Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote: > > > > > > This patch is based on an earlier patch by Grant Erickson > > which provided pwm devices using the 'legacy' interface. > > > > This driver instead uses the new framework interface. > > I'd prefer some kind of description about the driver here. I'm not really sure what more there is to say. There was a bit of text in a comment at the top of the file which I've copied to the commit comment. > Also the > subject should be something like: > > pwm: Add OMAP support using dual-mode timers > > or > > pwm: omap: Add PWM support using dual-mode timers Done - I chose the second. > > I take this description to mean that OMAP doesn't have dedicated PWM > hardware? Otherwise it might be bad to call this pwm-omap. Correct. The timers can be used for a number of things which explicitly includes PWM. > > Also please use all-caps when referring to PWM devices in prose. A few > other comments inline below. OK. > > > Cc: Grant Erickson <marathon96@gmail.com> > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index ed81720..7df573a 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -85,6 +85,15 @@ config PWM_MXS > > To compile this driver as a module, choose M here: the module > > will be called pwm-mxs. > > > > +config PWM_OMAP > > + tristate "OMAP pwm support" > > "OMAP PWM support" Fixed. > > > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c > [...] > > + * The 'id' number for the device encodes the number of the dm timer > > + * to use, and the polarity of the output. > > + * lsb is '1' of active-high, and '0' for active low > > + * remaining bit a timer number and need to be shifted down before use. > > I don't know if this is such a good idea. Usually you number platform > devices sequentially, while this would leave gaps in the numbering. I > know that adding platform data may sound a bit like overkill, but I > really think the added clarity and consistency is worth it. I guess so. No other PWM driver seems to use platform data, and I needed so little... I'll see what I can do. > > > +#define pr_fmt(fmt) "pwm-omap: " fmt > > You don't seem to be using any of the pr_*() logging functions, so this > isn't needed. Gone now, thanks. > > > +#include <linux/export.h> > > +#include <linux/kernel.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/err.h> > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/pwm.h> > > +#include <linux/module.h> > > + > > +#include <plat/dmtimer.h> > > + > > +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE > > + > > +struct omap_chip { > > + struct platform_device *pdev; > > I don't see this field being used anywhere. No. Gone. > > > + struct omap_dm_timer *dm_timer; > > + unsigned int polarity; > > The PWM subsystem already has enum pwm_polarity for this. > I'll use that then .... and as there is a pwm_set_polarity() interface, that probably means that I don't need to configure the polarity via the platform data? That would be a lot cleaner. > > + const char *label; > > This isn't used anywhere either. Gone. > > > + > > + unsigned int duty_ns, period_ns; > > + struct pwm_chip chip; > > +}; > > + > > +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip) > > + > > +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg) > > This is never used. :-) There is a theme here. > > > + > > +/** > > + * pwm_calc_value - determines the counter value for a clock rate and period. > > Nit: You should either start the sentence with a capital or not > terminate it with a full stop. In this case the sentence really includes the function name which is case-sensitive so cannot be capitalised ;-) I'll rephrase a bit and find something to capitalise. > > > + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the > > + * counter value for. > > + * @ns: The period, in nanoseconds, to computer the counter value for. > > "compute" Yep. > > > + * > > + * Returns the PWM counter value for the specified clock rate and period. > > + */ > > +static inline int pwm_calc_value(unsigned long clk_rate, int ns) > > +{ > > + const unsigned long nanoseconds_per_second = 1000000000; > > Maybe use NSEC_PER_SEC instead? Good idea, thanks. > > > + int cycles; > > + __u64 c; > > I think for in-kernel use, the custom is to stick with simply u64. It is, yes. > > > + > > + c = (__u64)clk_rate * ns; > > + do_div(c, nanoseconds_per_second); > > + cycles = c; > > + > > + return DM_TIMER_LOAD_MIN - cycles; > > Can't you just do "DM_TIMER_LOAD_MIN - c" and get rid of the cycles > variable altogether? Yep. > > > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct omap_chip *omap = to_omap_chip(chip); > > + int status = 0; > > + > > + /* Enable the counter--always--before attempting to write its > > + * registers and then set the timer to its minimum load value to > > + * ensure we get an overflow event right away once we start it. > > + */ > > Block comments should be in the following format: > > /* > * foo... > * bar... > */ OK, I fixed all of those. > > > + > > + omap_dm_timer_enable(omap->dm_timer); > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > > + omap_dm_timer_start(omap->dm_timer); > > + omap_dm_timer_disable(omap->dm_timer); > > So omap_dm_timer_disable() doesn't actually stop the timer? It just > disables the access to the registers? > > > + return status; > > "return 0;" and drop the status variable. > Done. > > +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > > + int duty_ns, int period_ns) > > +{ > > + struct omap_chip *omap = to_omap_chip(chip); > > + int status = 0; > > This one can be dropped as well. > > > + const bool enable = true; > > + const bool autoreload = true; > > + const bool toggle = true; > > + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE; > > I understand that these extra variables are supposed to "document" the > parameters of the functions below. I'm not a huge fan of this approach > because instead the API would better be designed to make it obvious what > the parameters are. Or people can just look at the prototypes to find > out the meaning. But if you really prefer this way I won't object. I've removed the 'trigger' const as the name is self-descriptive. I might leave the others. > > > + int load_value, match_value; > > + unsigned long clk_rate; > > + > > + dev_dbg(chip->dev, > > + "duty cycle: %d, period %d\n", > > + duty_ns, period_ns); > > This all fits on a single line, so no need to break it up. Indeed. > > > + > > + if (omap->duty_ns == duty_ns && > > + omap->period_ns == period_ns) > > + /* No change - don't cause any transients */ > > + return 0; > > Note to self: this might be a candidate to put in the core. might be useful, though the core doesn't currently "know" the current values. > > > + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer)); > > + > > + /* Calculate the appropriate load and match values based on the > > + * specified period and duty cycle. The load value determines the > > + * cycle time and the match value determines the duty cycle. > > + */ > > Again wrong block comment style. > > > + > > + load_value = pwm_calc_value(clk_rate, period_ns); > > + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns); > > + > > + /* We MUST enable yet stop the associated dual-mode timer before > > + * attempting to write its registers. Hopefully it is already > > + * disabled, but call the (idempotent) pwm_disable just in case > > + */ > > And here as well. While at it you might want to terminate the last > sentence with a full stop. > I check the sentences in other comments too. > > + pwm_disable(pwm); > > + > > + omap_dm_timer_enable(omap->dm_timer); > > + > > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); > > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); > > + > > + dev_dbg(chip->dev, > > + "load value: %#08x (%d), " > > + "match value: %#08x (%d)\n", > > + load_value, load_value, > > + match_value, match_value); > > Again this doesn't need so much wrapping. > Nope. > > + > > + omap_dm_timer_set_pwm(omap->dm_timer, > > + !omap->polarity, > > + toggle, > > + trigger); > > This doesn't either. Also you should be explicit about the polarity > parameter, since enum pwm_polarity is an enum and therefore negating it > isn't very nice (it should work though). > > You could solve this by doing something like: > > if (omap->polarity == PWM_POLARITY_NORMAL) > polarity = 1; > else > polarity = 0; (omap->polarity == PWM_POLARITY_NORMAL) would have the same effect. > > > + /* Set the counter to generate an overflow event immediately. */ > > + > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > > + > > + /* Now that we're done configuring the dual-mode timer, disable it > > + * again. We'll enable and start it later, when requested. > > + */ > > Also wrong style. > > > + omap_dm_timer_disable(omap->dm_timer); > > + omap->duty_ns = duty_ns; > > + omap->period_ns = period_ns; > > + > > + return status; > > "return 0;" > > > +} > > + > > + > > Gratuitous newline. > > > +static struct pwm_ops omap_pwm_ops = { > > Should be "static const". > > > + .enable = omap_pwm_enable, > > + .disable= omap_pwm_disable, > > There should be a space between .disable and =. OK. > > > + .config = omap_pwm_config, > > + .owner = THIS_MODULE, > > +}; > > + > > +/** > > + * omap_pwm_probe - check for the PWM and bind it to the driver. > > + * @pdev: A pointer to the platform device node associated with the > > + * PWM instance to be probed for driver binding. > > + * > > + * Returns 0 if the PWM instance was successfully bound to the driver; > > + * otherwise, < 0 on error. > > + */ > > I'm not sure how useful this kerneldoc comment really is. It isn't about > an exported function and developers usually know what the .probe() does. True. Gone. > > > +static int __devinit omap_pwm_probe(struct platform_device *pdev) > > No more __devinit, please. If you say so (having no idea what it did :-) > > > +{ > > + struct device *dev = &pdev->dev; > > + struct omap_chip *omap; > > + int status = 0; > > + unsigned int id = pdev->id; > > + unsigned int timer = id >> 1; /* lsb is polarity */ > > I've said this before, I don't think it's a good idea. > > > + > > + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL); > > + > > Gratuituous newline. > > > + if (omap == NULL) { > > + dev_err(dev, "Could not allocate memory.\n"); > > + status = -ENOMEM; > > + goto done; > > + } > > + > > + /* Request the OMAP dual-mode timer that will be bound to and > > + * associated with this generic PWM. > > + */ > > Block comment style again. > > > + > > + omap->dm_timer = omap_dm_timer_request_specific(timer); > > + > > Gratuitous newline. Gone. > > > + if (omap->dm_timer == NULL) { > > + status = -EPROBE_DEFER; > > + goto err_free; > > + } > > + > > + /* Configure the source for the dual-mode timer backing this > > + * generic PWM device. The clock source will ultimately determine > > + * how small or large the PWM frequency can be. > > + * > > + * At some point, it's probably worth revisiting moving this to > > + * the configure method and choosing either the slow- or > > + * system-clock source as appropriate for the desired PWM period. > > + */ > > And again. > > > + > > + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK); > > + > > + /* Cache away other miscellaneous driver-private data and state > > + * information and add the driver-private data to the platform > > + * device. > > + */ > > And again. =) > > > + > > + omap->chip.dev = dev; > > + omap->chip.ops = &omap_pwm_ops; > > + omap->chip.base = -1; > > + omap->chip.npwm = 1; > > + omap->polarity = id & 1; > > + > > + status = pwmchip_add(&omap->chip); > > + if (status < 0) { > > + dev_err(dev, "failed to register pwm\n"); > > + omap_dm_timer_free(omap->dm_timer); > > + goto err_free; > > + } > > + > > + platform_set_drvdata(pdev, omap); > > + > > + status = 0; > > + goto done; > > This can just be "return 0;". Yep. > > > + > > + err_free: > > + kfree(omap); > > + done: > > + return status; > > +} > > + > > +/** > > + * omap_pwm_remove - unbind the specified PWM platform device from the driver. > > + * @pdev: A pointer to the platform device node associated with the > > + * PWM instance to be unbound/removed. > > + * > > + * Returns 0 if the PWM was successfully removed as a platform device; > > + * otherwise, < 0 on error. > > + */ > > +static int __devexit omap_pwm_remove(struct platform_device *pdev) > > No __devexit, please. > > > +{ > > + struct omap_chip *omap = platform_get_drvdata(pdev); > > + int status = 0; > > Just drop this. > > > + > > + status = pwmchip_remove(&omap->chip); > > + if (status < 0) > > + goto done; > > "return status;" Fixed. > > > + > > + omap_dm_timer_free(omap->dm_timer); > > + > > + kfree(omap); > > + > > + done: > > + return status; > > Just "return 0;". No label required. > > > +} > > + > > +#if CONFIG_PM > > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state) > > +{ > > + struct omap_chip *omap = platform_get_drvdata(pdev); > > + /* No one preserve these values during suspend so reset them > > + * Otherwise driver leaves PWM unconfigured if same values > > + * passed to pwm_config > > + */ > > + omap->period_ns = 0; > > + omap->duty_ns = 0; > > + > > + return 0; > > +} > > +#else > > +#define omap_pwm_suspend NULL > > +#endif > > This doesn't look right. You should implement .resume() if you really > care, in which case the resume callback would have to reconfigure with > the cached values. In that case maybe you should switch to dev_pm_ops > and SIMPLE_DEV_PM_OPS() as well. > > If you don't, just resetting these values will not make the PWM work > properly after resume either since it will have to be explicitly > reconfigured. I just copied that from pwm-samsung.c I think the point is to avoid the "no transients" short-circuit in omap_pwm_config if the config is unchanged. The assumption is that pwm_disable() will be called before suspend and pwm_config()/pwm_enable() after resume. So there is no point actually configuring anything in .resume() - it makes sense to wait until pwm_config() is called (if ever). But we want to make sure that pwm_config actually does something. But yes, I should change to dev_pm_ops. > > > +static struct platform_driver omap_pwm_driver = { > > + .driver.name = "omap-pwm", > > + .driver.owner = THIS_MODULE, > > The more idiomatic way of writing this is: > > .driver = { > .name = "omap-pwm", > .owner = THIS_MODULE, > }, > I guess... though I think my way is a lot neater. But conformity is good. > > + .probe = omap_pwm_probe, > > + .remove = __devexit_p(omap_pwm_remove), > > No __devexit_p anymore. > OK. > > + .suspend = omap_pwm_suspend, > > + .resume = NULL, > > +}; > > + > > +static int __init omap_pwm_init(void) > > +{ > > + return platform_driver_register(&omap_pwm_driver); > > +} > > + > > +static void __exit omap_pwm_exit(void) > > +{ > > + platform_driver_unregister(&omap_pwm_driver); > > +} > > + > > +arch_initcall(omap_pwm_init); > > This should probably be module_init() instead. You already return > -EPROBE_DEFER if the dual-mode timer isn't ready yet so you should bite > the bullet and get all the dependencies to behave properly as well. New > code shouldn't be using this kind of dependency handling. > > In that case you could also just run module_platform_driver(). > > Thierry > > > +module_exit(omap_pwm_exit); > > + > > +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>"); > > +MODULE_AUTHOR("NeilBrown <neilb@suse.de>"); > > Shouldn't this be "Neil Brown"? I noticed you use the concatenated form > in the email address as well, so maybe that's on purpose? Yes, it is on purpose. With a surname like "Brown", one likes finding ways to be distinctive :-) > > > +MODULE_LICENSE("GPLv2"); > > This should be "GPL v2". Maybe MODULE_DESCRIPTION() would be nice too. Fixed. > > Thierry Thanks for your very thorough review! I'll send an updated version once I've resolved other comments and tested again. NeilBrown
On Wed, 12 Dec 2012 10:20:34 -0600 Jon Hunter <jon-hunter@ti.com> wrote: > > On 12/12/2012 05:31 AM, Thierry Reding wrote: > > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote: > > [snip] > > >> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > >> +{ > >> + struct omap_chip *omap = to_omap_chip(chip); > >> + int status = 0; > >> + > >> + /* Enable the counter--always--before attempting to write its > >> + * registers and then set the timer to its minimum load value to > >> + * ensure we get an overflow event right away once we start it. > >> + */ > > > > Block comments should be in the following format: > > > > /* > > * foo... > > * bar... > > */ > > > >> + > >> + omap_dm_timer_enable(omap->dm_timer); > >> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > >> + omap_dm_timer_start(omap->dm_timer); > >> + omap_dm_timer_disable(omap->dm_timer); > > > > So omap_dm_timer_disable() doesn't actually stop the timer? It just > > disables the access to the registers? > > I thought this looked odd too ;-) > > So what is going on here is that omap_dm_timer_start() calls > omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the > last disable really just complements the first enable (ie. decrements > the use count), but the timer will not actually be disabled, because the > start has called an extra enable. > > These four function calls can be replaced by one call to > omap_dm_timer_set_load_start() and I think that will be much clearer and > concise. So it now reads: /* * Set the timer to its minimum load value to ensure we get an * overflow event right away once we start it. */ omap_dm_timer_set_load_start(omap->dm_timer, true, DM_TIMER_LOAD_MIN); Certainly more concise - thanks. > > In general, it should not be necessary to call these > omap_dm_timer_enable/disable APIs directly. I am not sure what the > history is or if there is a use-case that really requires this. So in > the future may be I should make them static so they cannot be used > directly :-) I've removed the other instance of these calls - in omap_pwm_config. Thanks, NeilBrown
[Thierry: question for you near the end - thanks] On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote: > Hi Neil, > > On 12/12/2012 02:24 AM, NeilBrown wrote: > > > > > > This patch is based on an earlier patch by Grant Erickson > > which provided pwm devices using the 'legacy' interface. > > > > This driver instead uses the new framework interface. > > > > Cc: Grant Erickson <marathon96@gmail.com> > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index ed81720..7df573a 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -85,6 +85,15 @@ config PWM_MXS > > To compile this driver as a module, choose M here: the module > > will be called pwm-mxs. > > > > +config PWM_OMAP > > + tristate "OMAP pwm support" > > + depends on ARCH_OMAP > > We should probably have depends on or selects OMAP_DM_TIMER here too. Sounds sensible - fixed. > > > + help > > + Generic PWM framework driver for OMAP > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-omap > > + > > config PWM_PUV3 > > tristate "PKUnity NetBook-0916 PWM support" > > depends on ARCH_PUV3 > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index acfe482..f5d200d 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o > > obj-$(CONFIG_PWM_MXS) += pwm-mxs.o > > +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o > > obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o > > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o > > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c > > new file mode 100644 > > index 0000000..e3dbce3 > > --- /dev/null > > +++ b/drivers/pwm/pwm-omap.c > > @@ -0,0 +1,318 @@ > > +/* > > + * Copyright (c) 2012 NeilBrown <neilb@suse.de> > > + * Heavily based on earlier code which is: > > + * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com> > > + * > > + * Also based on pwm-samsung.c > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * version 2 as published by the Free Software Foundation. > > + * > > + * Description: > > + * This file is the core OMAP2/3 support for the generic, Linux > > I would drop the OMAP2/3 and just say OMAP here. Potentially this should > work for OMAP1-5. > Done. > > + * PWM driver / controller, using the OMAP's dual-mode timers. > > + * > > + * The 'id' number for the device encodes the number of the dm timer > > + * to use, and the polarity of the output. > > + * lsb is '1' of active-high, and '0' for active low > > + * remaining bit a timer number and need to be shifted down before use. > > + */ > > + > > +#define pr_fmt(fmt) "pwm-omap: " fmt > > + > > +#include <linux/export.h> > > +#include <linux/kernel.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/err.h> > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/pwm.h> > > +#include <linux/module.h> > > + > > +#include <plat/dmtimer.h> > > This is going to be a problem for the single zImage work, because we > cannot include any plat headers in driver code any more. Therefore, > although it is not ideal, one way to handle this is pass function > pointers to the various dmtimer APIs that are needed via the platform > data. Painful I know ... But that doesn't work with devicetree does it? Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something? It only included other things from include/linux, so it should be safe. > > > +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE > > + > > +struct omap_chip { > > + struct platform_device *pdev; > > + > > + struct omap_dm_timer *dm_timer; > > + unsigned int polarity; > > + const char *label; > > + > > + unsigned int duty_ns, period_ns; > > + struct pwm_chip chip; > > +}; > > + > > +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip) > > + > > +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg) > > + > > +/** > > + * pwm_calc_value - determines the counter value for a clock rate and period. > > + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the > > + * counter value for. > > + * @ns: The period, in nanoseconds, to computer the counter value for. > > + * > > + * Returns the PWM counter value for the specified clock rate and period. > > + */ > > +static inline int pwm_calc_value(unsigned long clk_rate, int ns) > > +{ > > + const unsigned long nanoseconds_per_second = 1000000000; > > + int cycles; > > + __u64 c; > > + > > + c = (__u64)clk_rate * ns; > > + do_div(c, nanoseconds_per_second); > > + cycles = c; > > + > > + return DM_TIMER_LOAD_MIN - cycles; > > +} > > + > > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct omap_chip *omap = to_omap_chip(chip); > > + int status = 0; > > + > > + /* Enable the counter--always--before attempting to write its > > + * registers and then set the timer to its minimum load value to > > + * ensure we get an overflow event right away once we start it. > > + */ > > + > > + omap_dm_timer_enable(omap->dm_timer); > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > > + omap_dm_timer_start(omap->dm_timer); > > + omap_dm_timer_disable(omap->dm_timer); > > Why not just use omap_dm_timer_load_start() here instead of the above 4 > APIs? Because I didn't know about it. I do now :-) > > > + > > + return status; > > +} > > + > > +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct omap_chip *omap = to_omap_chip(chip); > > + > > + omap_dm_timer_stop(omap->dm_timer); > > +} > > + > > +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > > + int duty_ns, int period_ns) > > +{ > > + struct omap_chip *omap = to_omap_chip(chip); > > + int status = 0; > > + const bool enable = true; > > + const bool autoreload = true; > > + const bool toggle = true; > > + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE; > > + int load_value, match_value; > > + unsigned long clk_rate; > > + > > + dev_dbg(chip->dev, > > + "duty cycle: %d, period %d\n", > > + duty_ns, period_ns); > > + > > + if (omap->duty_ns == duty_ns && > > + omap->period_ns == period_ns) > > + /* No change - don't cause any transients */ > > + return 0; > > + > > + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer)); > > + > > + /* Calculate the appropriate load and match values based on the > > + * specified period and duty cycle. The load value determines the > > + * cycle time and the match value determines the duty cycle. > > + */ > > + > > + load_value = pwm_calc_value(clk_rate, period_ns); > > + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns); > > + > > + /* We MUST enable yet stop the associated dual-mode timer before > > + * attempting to write its registers. Hopefully it is already > > + * disabled, but call the (idempotent) pwm_disable just in case > > + */ > > + > > + pwm_disable(pwm); > > + > > + omap_dm_timer_enable(omap->dm_timer); > > Do you need to call omap_dm_timer_enable here? _set_load and _set_match > will enable the timer. So this should not be necessary. True. That is what you get for copying someone else's code and not understanding it fully. > > > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); > > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); > > + > > + dev_dbg(chip->dev, > > + "load value: %#08x (%d), " > > + "match value: %#08x (%d)\n", > > + load_value, load_value, > > + match_value, match_value); > > + > > + omap_dm_timer_set_pwm(omap->dm_timer, > > + !omap->polarity, > > + toggle, > > + trigger); > > + > > + /* Set the counter to generate an overflow event immediately. */ > > + > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > > + > > + /* Now that we're done configuring the dual-mode timer, disable it > > + * again. We'll enable and start it later, when requested. > > + */ > > + > > + omap_dm_timer_disable(omap->dm_timer); > > Similarly the disable should not be needed here either. > > > + omap->duty_ns = duty_ns; > > + omap->period_ns = period_ns; > > + > > + return status; > > +} > > + > > + > > +static struct pwm_ops omap_pwm_ops = { > > + .enable = omap_pwm_enable, > > + .disable= omap_pwm_disable, > > + .config = omap_pwm_config, > > + .owner = THIS_MODULE, > > +}; > > + > > +/** > > + * omap_pwm_probe - check for the PWM and bind it to the driver. > > + * @pdev: A pointer to the platform device node associated with the > > + * PWM instance to be probed for driver binding. > > + * > > + * Returns 0 if the PWM instance was successfully bound to the driver; > > + * otherwise, < 0 on error. > > + */ > > +static int __devinit omap_pwm_probe(struct platform_device *pdev) > > I believe that __devinit is no longer required. > > > +{ > > + struct device *dev = &pdev->dev; > > + struct omap_chip *omap; > > + int status = 0; > > + unsigned int id = pdev->id; > > + unsigned int timer = id >> 1; /* lsb is polarity */ > > + > > + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL); > > + > > + if (omap == NULL) { > > + dev_err(dev, "Could not allocate memory.\n"); > > + status = -ENOMEM; > > + goto done; > > + } > > + > > + /* Request the OMAP dual-mode timer that will be bound to and > > + * associated with this generic PWM. > > + */ > > + > > + omap->dm_timer = omap_dm_timer_request_specific(timer); > > I would recommend that you use omap_dm_timer_request_by_cap() (new for > v3.8 so you should be able to use once v3.8-rc1 is out) here to request > a timer that supports the PWM output. The above function will not be > supported when booting with device-tree. I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do this now. Would you object to the patch being submitted with the current call, then an update when I do move on to 3.8? However.... I may be misunderstanding something, but I want a timer to drive a particular output pin - GPIO-57. And I thought that it could only be driver by GPT11. So I need to explicitly request number 11 don't I? > > > + > > + if (omap->dm_timer == NULL) { > > + status = -EPROBE_DEFER; > > + goto err_free; > > + } > > + > > + /* Configure the source for the dual-mode timer backing this > > + * generic PWM device. The clock source will ultimately determine > > + * how small or large the PWM frequency can be. > > + * > > + * At some point, it's probably worth revisiting moving this to > > + * the configure method and choosing either the slow- or > > + * system-clock source as appropriate for the desired PWM period. > > + */ > > + > > + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK); > > + > > + /* Cache away other miscellaneous driver-private data and state > > + * information and add the driver-private data to the platform > > + * device. > > + */ > > + > > + omap->chip.dev = dev; > > + omap->chip.ops = &omap_pwm_ops; > > + omap->chip.base = -1; > > + omap->chip.npwm = 1; > > + omap->polarity = id & 1; > > + > > + status = pwmchip_add(&omap->chip); > > + if (status < 0) { > > + dev_err(dev, "failed to register pwm\n"); > > + omap_dm_timer_free(omap->dm_timer); > > + goto err_free; > > + } > > + > > + platform_set_drvdata(pdev, omap); > > + > > + status = 0; > > + goto done; > > + > > + err_free: > > + kfree(omap); > > + done: > > + return status; > > +} > > + > > +/** > > + * omap_pwm_remove - unbind the specified PWM platform device from the driver. > > + * @pdev: A pointer to the platform device node associated with the > > + * PWM instance to be unbound/removed. > > + * > > + * Returns 0 if the PWM was successfully removed as a platform device; > > + * otherwise, < 0 on error. > > + */ > > +static int __devexit omap_pwm_remove(struct platform_device *pdev) > > I believe that __devexit is no longer required. > > > +{ > > + struct omap_chip *omap = platform_get_drvdata(pdev); > > + int status = 0; > > + > > + status = pwmchip_remove(&omap->chip); > > + if (status < 0) > > + goto done; > > + > > + omap_dm_timer_free(omap->dm_timer); > > Is it guaranteed that the timer will be disabled at this point? Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it might not be disabled. Thierry: should pwm_put do that, or do I need a 'free' function in my chip ops to do that? > > > + > > + kfree(omap); > > + > > + done: > > + return status; > > +} > > + > > +#if CONFIG_PM > > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state) > > +{ > > + struct omap_chip *omap = platform_get_drvdata(pdev); > > + /* No one preserve these values during suspend so reset them > > + * Otherwise driver leaves PWM unconfigured if same values > > + * passed to pwm_config > > + */ > > + omap->period_ns = 0; > > + omap->duty_ns = 0; > > > Hmmm, looks like you are trying to force a reconfiguration after suspend > if the same values are used. Is there an underlying problem here that > you are trying to workaround? I copied that from pwm-samsung.c. The key question is: does a dmtimer preserve all register values over suspend. If so, then I guess we don't need this. If not, we do (because omap_pwm_config short circuits if it thinks the config hasn't changed). Maybe I should test and see - though as my backlight always blanks before suspend that might not be straight forward... > > Please note that I am not familiar with the PWM sub-system to know how > suspend-resume is typically handled and if this is normal or not. > > > + return 0; > > +} > > +#else > > +#define omap_pwm_suspend NULL > > +#endif > > + > > +static struct platform_driver omap_pwm_driver = { > > + .driver.name = "omap-pwm", > > + .driver.owner = THIS_MODULE, > > + .probe = omap_pwm_probe, > > + .remove = __devexit_p(omap_pwm_remove), > > I believe that __devexit_p is no longer required. > > Otherwise it looks good to me. Thanks for sending! And thanks a lot for reading and reviewing! NeilBrown
On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote: > > > + omap_dm_timer_enable(omap->dm_timer); > > > > Do you need to call omap_dm_timer_enable here? _set_load and _set_match > > will enable the timer. So this should not be necessary. > > True. That is what you get for copying someone else's code and not > understanding it fully. However .... omap_dm_timer_write_counter *doesn't* enable the timer, and explicitly checks that it is already runtime-enabled. Does that mean I don't need to call omap_dm_timer_write_counter here? Or does it mean that I do need the enable/disable pair? > > > > > > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); > > > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); > > > + > > > + dev_dbg(chip->dev, > > > + "load value: %#08x (%d), " > > > + "match value: %#08x (%d)\n", > > > + load_value, load_value, > > > + match_value, match_value); > > > + > > > + omap_dm_timer_set_pwm(omap->dm_timer, > > > + !omap->polarity, > > > + toggle, > > > + trigger); > > > + > > > + /* Set the counter to generate an overflow event immediately. */ > > > + > > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > > > + > > > + /* Now that we're done configuring the dual-mode timer, disable it > > > + * again. We'll enable and start it later, when requested. > > > + */ > > > + > > > + omap_dm_timer_disable(omap->dm_timer); > > > > Similarly the disable should not be needed here either. > > Thanks, NeilBrown
On Thu, Dec 13, 2012 at 02:06:35PM +1100, NeilBrown wrote: > > [Thierry: question for you near the end - thanks] > > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote: > > > Hi Neil, > > > > On 12/12/2012 02:24 AM, NeilBrown wrote: [...] > > > +{ > > > + struct omap_chip *omap = platform_get_drvdata(pdev); > > > + int status = 0; > > > + > > > + status = pwmchip_remove(&omap->chip); > > > + if (status < 0) > > > + goto done; > > > + > > > + omap_dm_timer_free(omap->dm_timer); > > > > Is it guaranteed that the timer will be disabled at this point? > > Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it > might not be disabled. > Thierry: should pwm_put do that, or do I need a 'free' function in my chip > ops to do that? To be honest, I haven't decided yet. =) There have been discussions that resulted in a request to run pwm_disable() from pwmchip_remove() on all PWM devices a chip provides. This isn't implemented yet and I'm not sure about all the side-effects. I think for now the best way would be to implement .free() within this driver, or even do an explicit pwm_disable() in the driver's .remove() function to do this. When I've come to a decision I'll refactor all of that in one patch across the whole subsystem. Thierry
On Thu, Dec 13, 2012 at 01:38:28PM +1100, NeilBrown wrote: > On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote: [...] > > > + struct omap_dm_timer *dm_timer; > > > + unsigned int polarity; > > > > The PWM subsystem already has enum pwm_polarity for this. > > > > I'll use that then .... and as there is a pwm_set_polarity() interface, that > probably means that I don't need to configure the polarity via the platform > data? That would be a lot cleaner. I guess the answer to that question is: it depends. If the user can set the polarity (via platform or other means), then yes, you don't have to pass it in here. However there may be users that don't support setting the polarity or there may even be situations where the PWM goes through an additional inverter on the board and therefore doesn't need the polarity inversed after all, even if the user driver requests it. Generally though I think that it is up to the user drivers to take care of this and call pwm_set_polarity() as appropriate, so yes, I don't think you have to explicitly pass it via platform data at all. > > > + if (omap->duty_ns == duty_ns && > > > + omap->period_ns == period_ns) > > > + /* No change - don't cause any transients */ > > > + return 0; > > > > Note to self: this might be a candidate to put in the core. > > might be useful, though the core doesn't currently "know" the current values. Yes, but that can be changed. PWM is still a very young subsystem and I'm trying to be cautious not to add too much cruft to it unless it's really worth it. > > > + omap_dm_timer_set_pwm(omap->dm_timer, > > > + !omap->polarity, > > > + toggle, > > > + trigger); > > > > This doesn't either. Also you should be explicit about the polarity > > parameter, since enum pwm_polarity is an enum and therefore negating it > > isn't very nice (it should work though). > > > > You could solve this by doing something like: > > > > if (omap->polarity == PWM_POLARITY_NORMAL) > > polarity = 1; > > else > > polarity = 0; > > (omap->polarity == PWM_POLARITY_NORMAL) > > would have the same effect. Yes, that should work as well. However I'm not a friend of using such expressions in a function call. But since you'll probably be reworking this anyway because of the pwm_set_polarity() comments from above you might just want to stick the proper value into omap->polarity in your .set_polarity() implementation and not need the extra negation here. > > > +static int __devinit omap_pwm_probe(struct platform_device *pdev) > > > > No more __devinit, please. > > If you say so (having no idea what it did :-) This was used to mark functions depending on whether HOTPLUG was enabled or not. For instance functions marked __devinit could be discarded after the init stage if HOTPLUG was disabled because it would be guaranteed to not be called after the init stage. Recently however HOTPLUG was changed to be always enabled because the gains were very small and most people would get them wrong anyway. > > > +#if CONFIG_PM > > > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state) > > > +{ > > > + struct omap_chip *omap = platform_get_drvdata(pdev); > > > + /* No one preserve these values during suspend so reset them > > > + * Otherwise driver leaves PWM unconfigured if same values > > > + * passed to pwm_config > > > + */ > > > + omap->period_ns = 0; > > > + omap->duty_ns = 0; > > > + > > > + return 0; > > > +} > > > +#else > > > +#define omap_pwm_suspend NULL > > > +#endif > > > > This doesn't look right. You should implement .resume() if you really > > care, in which case the resume callback would have to reconfigure with > > the cached values. In that case maybe you should switch to dev_pm_ops > > and SIMPLE_DEV_PM_OPS() as well. > > > > If you don't, just resetting these values will not make the PWM work > > properly after resume either since it will have to be explicitly > > reconfigured. > > I just copied that from pwm-samsung.c > > I think the point is to avoid the "no transients" short-circuit in > omap_pwm_config if the config is unchanged. > > The assumption is that pwm_disable() will be called before suspend and > pwm_config()/pwm_enable() after resume. So there is no point actually > configuring anything in .resume() - it makes sense to wait until pwm_config() > is called (if ever). But we want to make sure that pwm_config actually does > something. Okay, that makes sense. User drivers should actually be better suited to reset PWM devices to their proper state on resume. > > > +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>"); > > > +MODULE_AUTHOR("NeilBrown <neilb@suse.de>"); > > > > Shouldn't this be "Neil Brown"? I noticed you use the concatenated form > > in the email address as well, so maybe that's on purpose? > > Yes, it is on purpose. With a surname like "Brown", one likes finding ways > to be distinctive :-) Hehe, alright then =). Thierry
On 12/12/2012 09:06 PM, NeilBrown wrote: > > [Thierry: question for you near the end - thanks] > > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote: > >> Hi Neil, >> >> On 12/12/2012 02:24 AM, NeilBrown wrote: >>> >>> >>> This patch is based on an earlier patch by Grant Erickson >>> which provided pwm devices using the 'legacy' interface. >>> >>> This driver instead uses the new framework interface. >>> >>> Cc: Grant Erickson <marathon96@gmail.com> >>> Signed-off-by: NeilBrown <neilb@suse.de> >>> >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >>> index ed81720..7df573a 100644 >>> --- a/drivers/pwm/Kconfig >>> +++ b/drivers/pwm/Kconfig >>> @@ -85,6 +85,15 @@ config PWM_MXS >>> To compile this driver as a module, choose M here: the module >>> will be called pwm-mxs. >>> >>> +config PWM_OMAP >>> + tristate "OMAP pwm support" >>> + depends on ARCH_OMAP >> >> We should probably have depends on or selects OMAP_DM_TIMER here too. > > Sounds sensible - fixed. > >> >>> + help >>> + Generic PWM framework driver for OMAP >>> + >>> + To compile this driver as a module, choose M here: the module >>> + will be called pwm-omap >>> + >>> config PWM_PUV3 >>> tristate "PKUnity NetBook-0916 PWM support" >>> depends on ARCH_PUV3 >>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile >>> index acfe482..f5d200d 100644 >>> --- a/drivers/pwm/Makefile >>> +++ b/drivers/pwm/Makefile >>> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o >>> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o >>> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o >>> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o >>> +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o >>> obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o >>> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o >>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o >>> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c >>> new file mode 100644 >>> index 0000000..e3dbce3 >>> --- /dev/null >>> +++ b/drivers/pwm/pwm-omap.c >>> @@ -0,0 +1,318 @@ >>> +/* >>> + * Copyright (c) 2012 NeilBrown <neilb@suse.de> >>> + * Heavily based on earlier code which is: >>> + * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com> >>> + * >>> + * Also based on pwm-samsung.c >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public License >>> + * version 2 as published by the Free Software Foundation. >>> + * >>> + * Description: >>> + * This file is the core OMAP2/3 support for the generic, Linux >> >> I would drop the OMAP2/3 and just say OMAP here. Potentially this should >> work for OMAP1-5. >> > > Done. > > >>> + * PWM driver / controller, using the OMAP's dual-mode timers. >>> + * >>> + * The 'id' number for the device encodes the number of the dm timer >>> + * to use, and the polarity of the output. >>> + * lsb is '1' of active-high, and '0' for active low >>> + * remaining bit a timer number and need to be shifted down before use. >>> + */ >>> + >>> +#define pr_fmt(fmt) "pwm-omap: " fmt >>> + >>> +#include <linux/export.h> >>> +#include <linux/kernel.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/slab.h> >>> +#include <linux/err.h> >>> +#include <linux/clk.h> >>> +#include <linux/io.h> >>> +#include <linux/pwm.h> >>> +#include <linux/module.h> >>> + >>> +#include <plat/dmtimer.h> >> >> This is going to be a problem for the single zImage work, because we >> cannot include any plat headers in driver code any more. Therefore, >> although it is not ideal, one way to handle this is pass function >> pointers to the various dmtimer APIs that are needed via the platform >> data. Painful I know ... > > But that doesn't work with devicetree does it? Ugh, you are right! This is becoming an increasing problem. > Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something? I can ask Tony if he thinks we could do that. > It only included other things from include/linux, so it should be safe. > >> >>> +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE >>> + >>> +struct omap_chip { >>> + struct platform_device *pdev; >>> + >>> + struct omap_dm_timer *dm_timer; >>> + unsigned int polarity; >>> + const char *label; >>> + >>> + unsigned int duty_ns, period_ns; >>> + struct pwm_chip chip; >>> +}; >>> + >>> +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip) >>> + >>> +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg) >>> + >>> +/** >>> + * pwm_calc_value - determines the counter value for a clock rate and period. >>> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the >>> + * counter value for. >>> + * @ns: The period, in nanoseconds, to computer the counter value for. >>> + * >>> + * Returns the PWM counter value for the specified clock rate and period. >>> + */ >>> +static inline int pwm_calc_value(unsigned long clk_rate, int ns) >>> +{ >>> + const unsigned long nanoseconds_per_second = 1000000000; >>> + int cycles; >>> + __u64 c; >>> + >>> + c = (__u64)clk_rate * ns; >>> + do_div(c, nanoseconds_per_second); >>> + cycles = c; >>> + >>> + return DM_TIMER_LOAD_MIN - cycles; >>> +} >>> + >>> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >>> +{ >>> + struct omap_chip *omap = to_omap_chip(chip); >>> + int status = 0; >>> + >>> + /* Enable the counter--always--before attempting to write its >>> + * registers and then set the timer to its minimum load value to >>> + * ensure we get an overflow event right away once we start it. >>> + */ >>> + >>> + omap_dm_timer_enable(omap->dm_timer); >>> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); >>> + omap_dm_timer_start(omap->dm_timer); >>> + omap_dm_timer_disable(omap->dm_timer); >> >> Why not just use omap_dm_timer_load_start() here instead of the above 4 >> APIs? > > Because I didn't know about it. I do now :-) No problem. >> >>> + >>> + return status; >>> +} >>> + >>> +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) >>> +{ >>> + struct omap_chip *omap = to_omap_chip(chip); >>> + >>> + omap_dm_timer_stop(omap->dm_timer); >>> +} >>> + >>> +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >>> + int duty_ns, int period_ns) >>> +{ >>> + struct omap_chip *omap = to_omap_chip(chip); >>> + int status = 0; >>> + const bool enable = true; >>> + const bool autoreload = true; >>> + const bool toggle = true; >>> + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE; >>> + int load_value, match_value; >>> + unsigned long clk_rate; >>> + >>> + dev_dbg(chip->dev, >>> + "duty cycle: %d, period %d\n", >>> + duty_ns, period_ns); >>> + >>> + if (omap->duty_ns == duty_ns && >>> + omap->period_ns == period_ns) >>> + /* No change - don't cause any transients */ >>> + return 0; >>> + >>> + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer)); >>> + >>> + /* Calculate the appropriate load and match values based on the >>> + * specified period and duty cycle. The load value determines the >>> + * cycle time and the match value determines the duty cycle. >>> + */ >>> + >>> + load_value = pwm_calc_value(clk_rate, period_ns); >>> + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns); >>> + >>> + /* We MUST enable yet stop the associated dual-mode timer before >>> + * attempting to write its registers. Hopefully it is already >>> + * disabled, but call the (idempotent) pwm_disable just in case >>> + */ >>> + >>> + pwm_disable(pwm); >>> + >>> + omap_dm_timer_enable(omap->dm_timer); >> >> Do you need to call omap_dm_timer_enable here? _set_load and _set_match >> will enable the timer. So this should not be necessary. > > True. That is what you get for copying someone else's code and not > understanding it fully. > >> >>> + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); >>> + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); >>> + >>> + dev_dbg(chip->dev, >>> + "load value: %#08x (%d), " >>> + "match value: %#08x (%d)\n", >>> + load_value, load_value, >>> + match_value, match_value); >>> + >>> + omap_dm_timer_set_pwm(omap->dm_timer, >>> + !omap->polarity, >>> + toggle, >>> + trigger); >>> + >>> + /* Set the counter to generate an overflow event immediately. */ >>> + >>> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); >>> + >>> + /* Now that we're done configuring the dual-mode timer, disable it >>> + * again. We'll enable and start it later, when requested. >>> + */ >>> + >>> + omap_dm_timer_disable(omap->dm_timer); >> >> Similarly the disable should not be needed here either. >> >>> + omap->duty_ns = duty_ns; >>> + omap->period_ns = period_ns; >>> + >>> + return status; >>> +} >>> + >>> + >>> +static struct pwm_ops omap_pwm_ops = { >>> + .enable = omap_pwm_enable, >>> + .disable= omap_pwm_disable, >>> + .config = omap_pwm_config, >>> + .owner = THIS_MODULE, >>> +}; >>> + >>> +/** >>> + * omap_pwm_probe - check for the PWM and bind it to the driver. >>> + * @pdev: A pointer to the platform device node associated with the >>> + * PWM instance to be probed for driver binding. >>> + * >>> + * Returns 0 if the PWM instance was successfully bound to the driver; >>> + * otherwise, < 0 on error. >>> + */ >>> +static int __devinit omap_pwm_probe(struct platform_device *pdev) >> >> I believe that __devinit is no longer required. >> >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct omap_chip *omap; >>> + int status = 0; >>> + unsigned int id = pdev->id; >>> + unsigned int timer = id >> 1; /* lsb is polarity */ >>> + >>> + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL); >>> + >>> + if (omap == NULL) { >>> + dev_err(dev, "Could not allocate memory.\n"); >>> + status = -ENOMEM; >>> + goto done; >>> + } >>> + >>> + /* Request the OMAP dual-mode timer that will be bound to and >>> + * associated with this generic PWM. >>> + */ >>> + >>> + omap->dm_timer = omap_dm_timer_request_specific(timer); >> >> I would recommend that you use omap_dm_timer_request_by_cap() (new for >> v3.8 so you should be able to use once v3.8-rc1 is out) here to request >> a timer that supports the PWM output. The above function will not be >> supported when booting with device-tree. > > I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do > this now. > Would you object to the patch being submitted with the current call, then an > update when I do move on to 3.8? I don't have strong objections, but I did not think Thierry liked the use of the id. > However.... I may be misunderstanding something, but I want a timer to drive > a particular output pin - GPIO-57. And I thought that it could only be > driver by GPT11. So I need to explicitly request number 11 don't I? Ugh, good point. Looks like for dev-tree we will need a way to request a timer by IO and this is missing today! > >> >>> + >>> + if (omap->dm_timer == NULL) { >>> + status = -EPROBE_DEFER; >>> + goto err_free; >>> + } >>> + >>> + /* Configure the source for the dual-mode timer backing this >>> + * generic PWM device. The clock source will ultimately determine >>> + * how small or large the PWM frequency can be. >>> + * >>> + * At some point, it's probably worth revisiting moving this to >>> + * the configure method and choosing either the slow- or >>> + * system-clock source as appropriate for the desired PWM period. >>> + */ >>> + >>> + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK); >>> + >>> + /* Cache away other miscellaneous driver-private data and state >>> + * information and add the driver-private data to the platform >>> + * device. >>> + */ >>> + >>> + omap->chip.dev = dev; >>> + omap->chip.ops = &omap_pwm_ops; >>> + omap->chip.base = -1; >>> + omap->chip.npwm = 1; >>> + omap->polarity = id & 1; >>> + >>> + status = pwmchip_add(&omap->chip); >>> + if (status < 0) { >>> + dev_err(dev, "failed to register pwm\n"); >>> + omap_dm_timer_free(omap->dm_timer); >>> + goto err_free; >>> + } >>> + >>> + platform_set_drvdata(pdev, omap); >>> + >>> + status = 0; >>> + goto done; >>> + >>> + err_free: >>> + kfree(omap); >>> + done: >>> + return status; >>> +} >>> + >>> +/** >>> + * omap_pwm_remove - unbind the specified PWM platform device from the driver. >>> + * @pdev: A pointer to the platform device node associated with the >>> + * PWM instance to be unbound/removed. >>> + * >>> + * Returns 0 if the PWM was successfully removed as a platform device; >>> + * otherwise, < 0 on error. >>> + */ >>> +static int __devexit omap_pwm_remove(struct platform_device *pdev) >> >> I believe that __devexit is no longer required. >> >>> +{ >>> + struct omap_chip *omap = platform_get_drvdata(pdev); >>> + int status = 0; >>> + >>> + status = pwmchip_remove(&omap->chip); >>> + if (status < 0) >>> + goto done; >>> + >>> + omap_dm_timer_free(omap->dm_timer); >> >> Is it guaranteed that the timer will be disabled at this point? > > Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it > might not be disabled. > Thierry: should pwm_put do that, or do I need a 'free' function in my chip > ops to do that? > > >> >>> + >>> + kfree(omap); >>> + >>> + done: >>> + return status; >>> +} >>> + >>> +#if CONFIG_PM >>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state) >>> +{ >>> + struct omap_chip *omap = platform_get_drvdata(pdev); >>> + /* No one preserve these values during suspend so reset them >>> + * Otherwise driver leaves PWM unconfigured if same values >>> + * passed to pwm_config >>> + */ >>> + omap->period_ns = 0; >>> + omap->duty_ns = 0; >> >> >> Hmmm, looks like you are trying to force a reconfiguration after suspend >> if the same values are used. Is there an underlying problem here that >> you are trying to workaround? > > I copied that from pwm-samsung.c. > > The key question is: does a dmtimer preserve all register values over suspend. > If so, then I guess we don't need this. > If not, we do (because omap_pwm_config short circuits if it thinks the config > hasn't changed). I gave it a quick test on omap3/4 when just operating the timer as a counter (not driving a pwm output) and suspend/resume works fine. However, it does not work if I enable off mode (via the debugfs). This is not enabled by default and may be I should put that on my to-do list as well. > Maybe I should test and see - though as my backlight always blanks before > suspend that might not be straight forward... Yes it would be great if you can test too. I have been finding that certain features of the timer are not that well tested. >> >> Please note that I am not familiar with the PWM sub-system to know how >> suspend-resume is typically handled and if this is normal or not. >> >>> + return 0; >>> +} >>> +#else >>> +#define omap_pwm_suspend NULL >>> +#endif >>> + >>> +static struct platform_driver omap_pwm_driver = { >>> + .driver.name = "omap-pwm", >>> + .driver.owner = THIS_MODULE, >>> + .probe = omap_pwm_probe, >>> + .remove = __devexit_p(omap_pwm_remove), >> >> I believe that __devexit_p is no longer required. >> >> Otherwise it looks good to me. Thanks for sending! > > And thanks a lot for reading and reviewing! No problem. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Jon Hunter <jon-hunter@ti.com> [121213 09:11]: > On 12/12/2012 09:06 PM, NeilBrown wrote: > > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote: > >> On 12/12/2012 02:24 AM, NeilBrown wrote: > >>> + > >>> +#include <plat/dmtimer.h> > >> > >> This is going to be a problem for the single zImage work, because we > >> cannot include any plat headers in driver code any more. Therefore, > >> although it is not ideal, one way to handle this is pass function > >> pointers to the various dmtimer APIs that are needed via the platform > >> data. Painful I know ... > > > > But that doesn't work with devicetree does it? > > Ugh, you are right! This is becoming an increasing problem. > > > Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something? > > I can ask Tony if he thinks we could do that. Yeah we need to fix this somehow. First we need to limit that header to the minimum and have most of it in a local header file for the clocksource and clockevent. Then let's move the minimal header to include/linux/omap-dmtimer.h until we have something Linux generic available for doing things like this. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* NeilBrown <neilb@suse.de> [121212 19:09]: > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote: > > On 12/12/2012 02:24 AM, NeilBrown wrote: > > > + > > > + /* Request the OMAP dual-mode timer that will be bound to and > > > + * associated with this generic PWM. > > > + */ > > > + > > > + omap->dm_timer = omap_dm_timer_request_specific(timer); > > > > I would recommend that you use omap_dm_timer_request_by_cap() (new for > > v3.8 so you should be able to use once v3.8-rc1 is out) here to request > > a timer that supports the PWM output. The above function will not be > > supported when booting with device-tree. > > I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do > this now. > Would you object to the patch being submitted with the current call, then an > update when I do move on to 3.8? > > However.... I may be misunderstanding something, but I want a timer to drive > a particular output pin - GPIO-57. And I thought that it could only be > driver by GPT11. So I need to explicitly request number 11 don't I? Yes I believe it needs to tied to a specific GPT instance to have access to the pin. We should not export omap_dm_timer_request_specific(), that information should become from platform_data and device tree. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/2012 10:33 PM, NeilBrown wrote: > On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote: > >>>> + omap_dm_timer_enable(omap->dm_timer); >>> >>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match >>> will enable the timer. So this should not be necessary. >> >> True. That is what you get for copying someone else's code and not >> understanding it fully. > > However .... omap_dm_timer_write_counter *doesn't* enable the timer, and > explicitly checks that it is already runtime-enabled. > > Does that mean I don't need to call omap_dm_timer_write_counter here? Or > does it mean that I do need the enable/disable pair? Typically, omap_dm_timer_write_counter() is used to update the counter value while the counter is running and hence is enabled. Looking at the code, some more I now see what they are trying to do. It seems that they are trying to force an overflow to occur as soon as they enable the timer. This will cause the timer to load the count value from the timer load register into the timer counter register. So that does make sense to me. However, this should not be necessary as omap_dm_timer_set_load should do this for you. Therefore, I think that you could accomplish the same thing by doing ... omap_pwm_config --> omap_dm_timer_set_load() --> omap_dm_timer_set_match() --> omap_dm_timer_set_pwm() omap_pwm_enable --> omap_dm_timer_start() If we call _set_load in config then we don't need to call _load_start in the enable, we can just call _start. Can you try this and see if this is working ok? Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index ed81720..7df573a 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -85,6 +85,15 @@ config PWM_MXS To compile this driver as a module, choose M here: the module will be called pwm-mxs. +config PWM_OMAP + tristate "OMAP pwm support" + depends on ARCH_OMAP + help + Generic PWM framework driver for OMAP + + To compile this driver as a module, choose M here: the module + will be called pwm-omap + config PWM_PUV3 tristate "PKUnity NetBook-0916 PWM support" depends on ARCH_PUV3 diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index acfe482..f5d200d 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o obj-$(CONFIG_PWM_PXA) += pwm-pxa.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c new file mode 100644 index 0000000..e3dbce3 --- /dev/null +++ b/drivers/pwm/pwm-omap.c @@ -0,0 +1,318 @@ +/* + * Copyright (c) 2012 NeilBrown <neilb@suse.de> + * Heavily based on earlier code which is: + * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com> + * + * Also based on pwm-samsung.c + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * Description: + * This file is the core OMAP2/3 support for the generic, Linux + * PWM driver / controller, using the OMAP's dual-mode timers. + * + * The 'id' number for the device encodes the number of the dm timer + * to use, and the polarity of the output. + * lsb is '1' of active-high, and '0' for active low + * remaining bit a timer number and need to be shifted down before use. + */ + +#define pr_fmt(fmt) "pwm-omap: " fmt + +#include <linux/export.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/pwm.h> +#include <linux/module.h> + +#include <plat/dmtimer.h> + +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE + +struct omap_chip { + struct platform_device *pdev; + + struct omap_dm_timer *dm_timer; + unsigned int polarity; + const char *label; + + unsigned int duty_ns, period_ns; + struct pwm_chip chip; +}; + +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip) + +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg) + +/** + * pwm_calc_value - determines the counter value for a clock rate and period. + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the + * counter value for. + * @ns: The period, in nanoseconds, to computer the counter value for. + * + * Returns the PWM counter value for the specified clock rate and period. + */ +static inline int pwm_calc_value(unsigned long clk_rate, int ns) +{ + const unsigned long nanoseconds_per_second = 1000000000; + int cycles; + __u64 c; + + c = (__u64)clk_rate * ns; + do_div(c, nanoseconds_per_second); + cycles = c; + + return DM_TIMER_LOAD_MIN - cycles; +} + +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct omap_chip *omap = to_omap_chip(chip); + int status = 0; + + /* Enable the counter--always--before attempting to write its + * registers and then set the timer to its minimum load value to + * ensure we get an overflow event right away once we start it. + */ + + omap_dm_timer_enable(omap->dm_timer); + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); + omap_dm_timer_start(omap->dm_timer); + omap_dm_timer_disable(omap->dm_timer); + + return status; +} + +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct omap_chip *omap = to_omap_chip(chip); + + omap_dm_timer_stop(omap->dm_timer); +} + +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct omap_chip *omap = to_omap_chip(chip); + int status = 0; + const bool enable = true; + const bool autoreload = true; + const bool toggle = true; + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE; + int load_value, match_value; + unsigned long clk_rate; + + dev_dbg(chip->dev, + "duty cycle: %d, period %d\n", + duty_ns, period_ns); + + if (omap->duty_ns == duty_ns && + omap->period_ns == period_ns) + /* No change - don't cause any transients */ + return 0; + + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer)); + + /* Calculate the appropriate load and match values based on the + * specified period and duty cycle. The load value determines the + * cycle time and the match value determines the duty cycle. + */ + + load_value = pwm_calc_value(clk_rate, period_ns); + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns); + + /* We MUST enable yet stop the associated dual-mode timer before + * attempting to write its registers. Hopefully it is already + * disabled, but call the (idempotent) pwm_disable just in case + */ + + pwm_disable(pwm); + + omap_dm_timer_enable(omap->dm_timer); + + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); + + dev_dbg(chip->dev, + "load value: %#08x (%d), " + "match value: %#08x (%d)\n", + load_value, load_value, + match_value, match_value); + + omap_dm_timer_set_pwm(omap->dm_timer, + !omap->polarity, + toggle, + trigger); + + /* Set the counter to generate an overflow event immediately. */ + + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); + + /* Now that we're done configuring the dual-mode timer, disable it + * again. We'll enable and start it later, when requested. + */ + + omap_dm_timer_disable(omap->dm_timer); + omap->duty_ns = duty_ns; + omap->period_ns = period_ns; + + return status; +} + + +static struct pwm_ops omap_pwm_ops = { + .enable = omap_pwm_enable, + .disable= omap_pwm_disable, + .config = omap_pwm_config, + .owner = THIS_MODULE, +}; + +/** + * omap_pwm_probe - check for the PWM and bind it to the driver. + * @pdev: A pointer to the platform device node associated with the + * PWM instance to be probed for driver binding. + * + * Returns 0 if the PWM instance was successfully bound to the driver; + * otherwise, < 0 on error. + */ +static int __devinit omap_pwm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct omap_chip *omap; + int status = 0; + unsigned int id = pdev->id; + unsigned int timer = id >> 1; /* lsb is polarity */ + + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL); + + if (omap == NULL) { + dev_err(dev, "Could not allocate memory.\n"); + status = -ENOMEM; + goto done; + } + + /* Request the OMAP dual-mode timer that will be bound to and + * associated with this generic PWM. + */ + + omap->dm_timer = omap_dm_timer_request_specific(timer); + + if (omap->dm_timer == NULL) { + status = -EPROBE_DEFER; + goto err_free; + } + + /* Configure the source for the dual-mode timer backing this + * generic PWM device. The clock source will ultimately determine + * how small or large the PWM frequency can be. + * + * At some point, it's probably worth revisiting moving this to + * the configure method and choosing either the slow- or + * system-clock source as appropriate for the desired PWM period. + */ + + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK); + + /* Cache away other miscellaneous driver-private data and state + * information and add the driver-private data to the platform + * device. + */ + + omap->chip.dev = dev; + omap->chip.ops = &omap_pwm_ops; + omap->chip.base = -1; + omap->chip.npwm = 1; + omap->polarity = id & 1; + + status = pwmchip_add(&omap->chip); + if (status < 0) { + dev_err(dev, "failed to register pwm\n"); + omap_dm_timer_free(omap->dm_timer); + goto err_free; + } + + platform_set_drvdata(pdev, omap); + + status = 0; + goto done; + + err_free: + kfree(omap); + done: + return status; +} + +/** + * omap_pwm_remove - unbind the specified PWM platform device from the driver. + * @pdev: A pointer to the platform device node associated with the + * PWM instance to be unbound/removed. + * + * Returns 0 if the PWM was successfully removed as a platform device; + * otherwise, < 0 on error. + */ +static int __devexit omap_pwm_remove(struct platform_device *pdev) +{ + struct omap_chip *omap = platform_get_drvdata(pdev); + int status = 0; + + status = pwmchip_remove(&omap->chip); + if (status < 0) + goto done; + + omap_dm_timer_free(omap->dm_timer); + + kfree(omap); + + done: + return status; +} + +#if CONFIG_PM +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct omap_chip *omap = platform_get_drvdata(pdev); + /* No one preserve these values during suspend so reset them + * Otherwise driver leaves PWM unconfigured if same values + * passed to pwm_config + */ + omap->period_ns = 0; + omap->duty_ns = 0; + + return 0; +} +#else +#define omap_pwm_suspend NULL +#endif + +static struct platform_driver omap_pwm_driver = { + .driver.name = "omap-pwm", + .driver.owner = THIS_MODULE, + .probe = omap_pwm_probe, + .remove = __devexit_p(omap_pwm_remove), + .suspend = omap_pwm_suspend, + .resume = NULL, +}; + +static int __init omap_pwm_init(void) +{ + return platform_driver_register(&omap_pwm_driver); +} + +static void __exit omap_pwm_exit(void) +{ + platform_driver_unregister(&omap_pwm_driver); +} + +arch_initcall(omap_pwm_init); +module_exit(omap_pwm_exit); + +MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>"); +MODULE_AUTHOR("NeilBrown <neilb@suse.de>"); +MODULE_LICENSE("GPLv2"); +MODULE_VERSION("2012-12-01");
This patch is based on an earlier patch by Grant Erickson which provided pwm devices using the 'legacy' interface. This driver instead uses the new framework interface. Cc: Grant Erickson <marathon96@gmail.com> Signed-off-by: NeilBrown <neilb@suse.de>