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

[Jon Hunter <jon-hunter@ti.com>]

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.

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 :-)

Cheers
Jon