On Tue, 9 Dec 2014, Kevin Hilman wrote:
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index 55449909f114..f7614041240e 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -105,7 +105,16 @@ void clockevents_set_mode(struct clock_event_device *dev,
> enum clock_event_mode mode)
> {
> if (dev->mode != mode) {
> - dev->set_mode(mode, dev);
> + if (dev->set_dev_mode) {
> + int ret = dev->set_dev_mode(mode, dev);
> +
> + /* Currently available modes shouldn't fail */
> + WARN_ONCE(ret, "Requested mode: %d, error: %d\n",
> + mode, ret);
> + } else {
> + dev->set_mode(mode, dev);
> + }
> +
> dev->mode = mode;
>
> /*
> @@ -448,8 +457,14 @@ int __clockevents_update_freq(struct clock_event_device
> *dev, u32 freq)
> if (dev->mode == CLOCK_EVT_MODE_ONESHOT)
> return clockevents_program_event(dev, dev->next_event, false);
>
> - if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
> - dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
> + if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
> + /* Shouldn't fail while switching to PERIODIC MODE */
> + if (dev->set_dev_mode)
> + WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC,
> + dev));
> + else
> + dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
Please split that out into
static int __clockevents_set_mode(dev, mode)
{
int ret = 0;
if (dev->set_dev_mode) {
ret = set_dev_mode(mode, dev);
WARN_ON_ONCE(ret);
} else {
dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
}
return ret;
}
Now looking at the conversion patch, it looks like we add lots of
case X:
case Y:
return 0;
to avoid the
default:
return -ENOSYS;
After staring at that I'm less convinced about this approach. So lets
think about something different:
static int __clockevents_set_mode(dev, mode)
{
/* Transition mode */
if (dev->set_mode) {
if (mode > CLOCK_EVT_MODE_RESUME)
return -ENOSYS;
dev->set_mode(mode, dev);
return 0;
}
if (dev->features & CLOCK_EVT_FEAT_DUMMY)
return 0;
switch (mode) {
/*
* This is an internal state, which is guaranteed to
* go from SHUTDOWN to UNUSED. No driver interaction
* required.
*/
case CLOCK_EVT_MODE_UNUSED:
return 0;
case CLOCK_EVT_MODE_SHUTDOWN:
return dev->shutdown(dev);
case CLOCK_EVT_MODE_PERIODIC:
/* Core internal bug */
if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC))
return -ENOSYS;
return dev->setup_periodic(dev);
case CLOCK_EVT_MODE_ONESHOT:
/* Core internal bug */
if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
return -ENOSYS;
return dev->setup_oneshot(dev);
case CLOCK_EVT_MODE_RESUME:
return dev->setup_resume(dev);
default:
return -ENOSYS;
}
And do sanity checking on registration time:
static int ce_check(dev)
{
if (dev->set_mode)
return 0;
if (dev->features & CLOCK_EVT_FEAT_DUMMY)
return 0;
if (!dev->shutdown)
return -EMORON;
if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) &&
!dev->setup_periodic)
return -EMORON;
if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) &&
!dev->setup_oneshot)
return -EMORON;
/* Optional callbacks */
if (!dev->setup_resume)
dev->setup_resume = setup_noop();
return 0;
}
That gives us a clear distinction of required and optional callbacks,
which will make error handling and recovery simpler as well.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
