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 \ /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: linkBe 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).