Viresh Kumar viresh.kumar@linaro.org writes:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all ARM specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err() for unsupported modes. These statements are removed now as proper error handling with a WARN_ON() is done at clockevents core.
Also it is made sure that drivers don't disable events for unsupported modes.
Why wouldn't you want that? Most drivers are leaving events disabled for unsupported modes currently. Doing this seems to complicate things, and needlessly change/complicate current behavior, as well as make this patch more difficult to read/review.
In any case, I've given a quick skim over some of the driver changes below and found several (possible) issues.
This will be tricky to get right on all platforms, so IMO the changes should be minimal.
[...]
diff --git a/arch/arm/mach-clps711x/common.c b/arch/arm/mach-clps711x/common.c index aee81fa..fa7c090 100644 --- a/arch/arm/mach-clps711x/common.c +++ b/arch/arm/mach-clps711x/common.c @@ -69,30 +69,30 @@ static u64 notrace clps711x_sched_clock_read(void) return ~readw_relaxed(CLPS711X_VIRT_BASE + TC1D); } -static void clps711x_clockevent_set_mode(enum clock_event_mode mode, +static int clps711x_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {
- disable_irq(IRQ_TC2OI);
You probably noticed this, but this step is now skipped for _PERIODIC. Since the IRQ is immediately re-enabled, it's probably not a problem, but it *is* a functional change that should be noted.
switch (mode) { case CLOCK_EVT_MODE_PERIODIC: enable_irq(IRQ_TC2OI); break;
- case CLOCK_EVT_MODE_ONESHOT:
case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME:/* Not supported */
/* Left event sources disabled, no more interrupts appear */ break;disable_irq(IRQ_TC2OI);
- default:
}return -ENOSYS;
- return 0;
}
[...]
diff --git a/arch/arm/mach-cns3xxx/core.c b/arch/arm/mach-cns3xxx/core.c index 2ae28a6..9746501 100644 --- a/arch/arm/mach-cns3xxx/core.c +++ b/arch/arm/mach-cns3xxx/core.c @@ -113,7 +113,7 @@ void cns3xxx_power_off(void) */ static void __iomem *cns3xxx_tmr1; -static void cns3xxx_timer_set_mode(enum clock_event_mode mode, +static int cns3xxx_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *clk) { unsigned long ctrl = readl(cns3xxx_tmr1 + TIMER1_2_CONTROL_OFFSET); @@ -132,11 +132,15 @@ static void cns3xxx_timer_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN:
- default:
- case CLOCK_EVT_MODE_RESUME: ctrl = 0;
break;
- default:
}return -ENOSYS;
writel(ctrl, cns3xxx_tmr1 + TIMER1_2_CONTROL_OFFSET);
Hmm, looks like the timer is left disabled for the unsupported modes, which is the opposite of what's done elsewhere.
- return 0;
}
[...]
diff --git a/arch/arm/mach-omap1/timer32k.c b/arch/arm/mach-omap1/timer32k.c index 107e7ab..c589402 100644 --- a/arch/arm/mach-omap1/timer32k.c +++ b/arch/arm/mach-omap1/timer32k.c @@ -119,11 +119,9 @@ static int omap_32k_timer_set_next_event(unsigned long delta, return 0; } -static void omap_32k_timer_set_mode(enum clock_event_mode mode, +static int omap_32k_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {
- omap_32k_timer_stop();
- switch (mode) { case CLOCK_EVT_MODE_PERIODIC: omap_32k_timer_start(OMAP_32K_TIMER_TICK_PERIOD);
Note that this is also a functional change because the timer doesn't get stopped before it is restarted in periodic mode. I can't remember if that's safe on OMAP1 (I think not), but it in any case, a functional change like should not be made without more thorough testing (IMO, this kind of functional change should not be part of this patch.)
@@ -131,17 +129,20 @@ static void omap_32k_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN:
case CLOCK_EVT_MODE_RESUME:break;
break;omap_32k_timer_stop();
- default:
}return -ENOSYS;
- return 0;
}
[...]
diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c index fca174e..a4a8789 100644 --- a/arch/arm/mach-pxa/time.c +++ b/arch/arm/mach-pxa/time.c @@ -67,7 +67,7 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; } -static void +static int pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -84,9 +84,11 @@ pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) break; case CLOCK_EVT_MODE_RESUME:
- case CLOCK_EVT_MODE_PERIODIC:
Hmm, you're changing periodic mode from supported to unsupported. Why?
I see some others like this later also.
OK, I give up for now. I've found enough issues to convince myself that this needs some closer attention, and possibly a rethink about how to handle unsupported modes.
Kevin