From: Magnus Damm <magnus.damm@gmail.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org, horms@verge.net.au, arnd@arndb.de,
linux-sh@vger.kernel.org, johnstul@us.ibm.com, rjw@sisk.pl,
lethal@linux-sh.org, gregkh@linuxfoundation.org, olof@lixom.net
Subject: Re: [PATCH] clocksource: em_sti: Emma Mobile STI driver
Date: Wed, 9 May 2012 02:06:32 +0900 [thread overview]
Message-ID: <CANqRtoRr5z2GLOfaFnRCFcsQaAaVMvkaDh2=yrhQTvNmAQN-Pg@mail.gmail.com> (raw)
In-Reply-To: <alpine.LFD.2.02.1205072054130.6271@ionos>
Hi Thomas,
On Tue, May 8, 2012 at 4:10 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 3 May 2012, Magnus Damm wrote:
>> +/* private flags */
>> +#define FLAG_CLOCKEVENT (1 << 0)
>> +#define FLAG_CLOCKSOURCE (1 << 1)
>> +
>> +static int em_sti_start(struct em_sti_priv *p, unsigned long flag)
>> +{
>> + int ret = 0;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&p->lock, flags);
>> +
>> + if (!(p->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
>> + ret = em_sti_enable(p);
>
> That's confusing. You seem to enable both CLOCKEVENT and CLOCKSOURCE
> independent of "flag" value.
Hm, I believe the idea is to check if it has been enabled already...
>> +
>> + if (!ret)
>> + p->flags |= flag;
>
> And then just or "flag" ??????
... but it certainly is overly complicated. I'll rework it into
something that is easier to digest. =)
>> + spin_unlock_irqrestore(&p->lock, flags);
>> +
>> + return ret;
>> +}
>> +
>> +static void em_sti_stop(struct em_sti_priv *p, unsigned long flag)
>> +{
>> + unsigned long flags;
>> + unsigned long f;
>> +
>> + spin_lock_irqsave(&p->lock, flags);
>> +
>> + f = p->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE);
>> + p->flags &= ~flag;
>> +
>> + if (f && !(p->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
>> + em_sti_disable(p);
>
> Huch? If the caller wants to disable the clockevent, you stop the
> clocksource as well because em_sti_disable() stops the clock.
>
> /me is confused.
I believe the idea is that if the timer is currently enabled (f) and
if we are the last user thenthen stop.
A regular usage counter probably makes much more sense!
>> +static void em_sti_clock_event_start(struct em_sti_priv *p)
>> +{
>> + struct clock_event_device *ced = &p->ced;
>> +
>> + em_sti_start(p, FLAG_CLOCKEVENT);
>> +
>> + /* TODO: calculate good shift from rate and counter bit width */
>> +
>> + ced->shift = 32;
>> + ced->mult = div_sc(p->rate, NSEC_PER_SEC, ced->shift);
>
> IIRC, we have a generic function to do that :)
Ok, thanks for pointing that out. Need to look it up!
Will update and post a V2. Thanks for your help!
Cheers,
/ magnus
prev parent reply other threads:[~2012-05-08 17:06 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-03 12:56 [PATCH] clocksource: em_sti: Emma Mobile STI driver Magnus Damm
2012-05-07 5:20 ` Simon Horman
2012-05-07 19:10 ` Thomas Gleixner
2012-05-08 17:06 ` Magnus Damm [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANqRtoRr5z2GLOfaFnRCFcsQaAaVMvkaDh2=yrhQTvNmAQN-Pg@mail.gmail.com' \
--to=magnus.damm@gmail.com \
--cc=arnd@arndb.de \
--cc=gregkh@linuxfoundation.org \
--cc=horms@verge.net.au \
--cc=johnstul@us.ibm.com \
--cc=lethal@linux-sh.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=olof@lixom.net \
--cc=rjw@sisk.pl \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).