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