LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Sekhar Nori <nsekhar@ti.com>, Kevin Hilman <khilman@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
David Lechner <david@lechnology.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents
Date: Thu, 23 May 2019 15:25:46 +0200 [thread overview]
Message-ID: <ca00f49f-0fa2-1907-feac-ba798dce365b@linaro.org> (raw)
In-Reply-To: <CAMRc=MdQ_GORGaw1szwvBRqKzkZQCZNnEcwkNzmGduEbiDR4Lw@mail.gmail.com>
Hi Bartosz,
On 23/05/2019 14:58, Bartosz Golaszewski wrote:
[ ... ]
>>> +++ b/drivers/clocksource/timer-davinci.c
>>> @@ -0,0 +1,272 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +//
>>> +// TI DaVinci clocksource driver
>>> +//
>>> +// Copyright (C) 2019 Texas Instruments
>>> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> +// (with tiny parts adopted from code by Kevin Hilman <khilman@baylibre.com>)
>>
>> The header format is wrong, it should be:
>>
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> * TI DaVinci clocksource driver
>> *
>> * ...
>> * ...
>> *
>> */
>
> It's not wrong. It looks like it's at the maintainers discretion and
> I've been asked to use both forms by different maintainers. Seems you
> just can't get it right. :) I've changed it in v2 though.
Right, I've been through the documentation but it is still unclear for
me. So let's stick to whatever you want for now.
[ ... ]
>>> +static int
>>> +davinci_clockevent_set_next_event_std(unsigned long cycles,
>>> + struct clock_event_device *dev)
>>> +{
>>> + struct davinci_clockevent *clockevent;
>>> + unsigned int enamode;
>>> +
>>> + clockevent = to_davinci_clockevent(dev);
>>> + enamode = clockevent->enamode_disabled;
>>> +
>>> + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR,
>>> + clockevent->enamode_mask,
>>> + clockevent->enamode_disabled);
>>
>> What is for this function with the DAVINCI_TIMER_REG_TCR parameter?
>
> I'm not sure I understand the question. :(
I meant davinci_clockevent_update is always called with the
DAVINCI_TIMER_REG_TCR parameter.
So it can be changed to:
static void davinci_clockevent_update(struct davinci_clockevent
*clockevent,
unsigned int mask,
unsigned int val)
{
davinci_reg_update(clockevent->base, DAVINCI_TIMER_REG_TCR,
mask, val);
}
Alternatively davinci_clockevent_update can be replaced by a direct call
to davinci_reg_update.
[ ... ]
>>> + clockevent->dev.cpumask = cpumask_of(0);
>>> +
>>> + clockevent->base = base;
>>> + clockevent->tim_off = DAVINCI_TIMER_REG_TIM12;
>>> + clockevent->prd_off = DAVINCI_TIMER_REG_PRD12;
>>> +
>>> + shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12;
>>> + clockevent->enamode_disabled = DAVINCI_TIMER_ENAMODE_DISABLED << shift;
>>> + clockevent->enamode_oneshot = DAVINCI_TIMER_ENAMODE_ONESHOT << shift;
>>> + clockevent->enamode_periodic = DAVINCI_TIMER_ENAMODE_PERIODIC << shift;
>>> + clockevent->enamode_mask = DAVINCI_TIMER_ENAMODE_MASK << shift;
>>
>> I don't see where 'shift' can be different from TIM12 here neither in
>> the second patch for those values. Why create these fields instead of
>> pre-computed macros?
>>
>
> The variable 'shift' here is only to avoid breaking the lines (just a helper).
>
> The shift itself can be different though in the second patch -
> specifically when calling davinci_clocksource_init().
>
> If I were to use predefined values for clockevent, we'd still need
> another set of values for clocksource. I think it's clearer the way it
> is.
Ah yes, I see, it is passed as parameter. Ok, let's keep it this way if
you prefer.
>>> + if (timer_cfg->cmp_off) {
>>> + clockevent->cmp_off = timer_cfg->cmp_off;
>>> + clockevent->dev.set_next_event =
>>> + davinci_clockevent_set_next_event_cmp;
>>> + } else {
>>> + clockevent->dev.set_next_event =
>>> + davinci_clockevent_set_next_event_std;
>>> + }
>>> +
>>> + rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start,
>>> + davinci_timer_irq_timer, IRQF_TIMER,
>>> + "clockevent", clockevent);
>>
>> May be replace "clockevent" by eg. "tim12"?
>>
>
> I don't think this is a good idea. Now if you look at /proc/interrupts
> you can tell immediately what the interrupt is for ("clockevent").
> With "tim12" it's no longer that clear.
Yes, "tim12" can be confusing. However, it is good practice to add a
device name aside with its purpose in case there are several timers
defined on the system. "clockevent" is a kernel internal representation
of a timer, so may be a name like "timer/tim12" or something in the same
spirit would be more adequate.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
next prev parent reply other threads:[~2019-05-23 13:25 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-17 14:47 [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski
2019-04-17 14:47 ` [RFC 1/2] clocksource: davinci-timer: add support for clockevents Bartosz Golaszewski
2019-05-14 19:55 ` Daniel Lezcano
2019-05-23 12:58 ` Bartosz Golaszewski
2019-05-23 13:25 ` Daniel Lezcano [this message]
2019-05-23 15:34 ` Bartosz Golaszewski
2019-04-17 14:47 ` [RFC 2/2] clocksource: timer-davinci: add support for clocksource Bartosz Golaszewski
2019-05-13 7:54 ` [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski
2019-05-13 8:04 ` Daniel Lezcano
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=ca00f49f-0fa2-1907-feac-ba798dce365b@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=bgolaszewski@baylibre.com \
--cc=brgl@bgdev.pl \
--cc=david@lechnology.com \
--cc=khilman@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nsekhar@ti.com \
--cc=tglx@linutronix.de \
--subject='Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents' \
/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
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).