Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown]

On Wed, 12 Dec 2012 10:20:34 -0600 Jon Hunter <jon-hunter@xxxxxx> 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

Attachment: signature.asc
Description: PGP signature