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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121213134534.0cd32544@notabene.brown>
Date:	Thu, 13 Dec 2012 13:45:34 +1100
From:	NeilBrown <neilb@...e.de>
To:	Jon Hunter <jon-hunter@...com>
Cc:	Thierry Reding <thierry.reding@...onic-design.de>,
	Grant Erickson <marathon96@...il.com>,
	<linux-omap@...r.kernel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] OMAP: add pwm driver using dmtimers.

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

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)