lkml.org 
[lkml]   [2012]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] OMAP: add pwm driver using dmtimers.
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-12-13 04:21    [W:0.160 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site