On Wed, 12 Dec 2012 10:20:34 -0600 Jon Hunter <jon-hun...@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
signature.asc
Description: PGP signature
