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 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]
\
 
 \ /
  Last update: 2012-12-13 04:21    [W:0.107 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site