Messages in this thread | | | Subject | Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents | From | Daniel Lezcano <> | Date | Thu, 23 May 2019 15:25:46 +0200 |
| |
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
| |