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:5.526 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site