Messages in this thread | | | Date | Thu, 13 Dec 2012 13:45:34 +1100 | From | NeilBrown <> | Subject | Re: [PATCH] OMAP: add pwm driver using dmtimers. |
| |
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 [unhandled content-type:application/pgp-signature] | |