Hi Thomas,
On 23 January 2015 at 17:27, Thomas Gleixner <t...@linutronix.de> wrote:
> 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);
Because setting to all these existing modes isn't allowed
to fail currently, shouldn't we make return type of all the new
callbacks as 'void' and return 0 from this routine ? That way, these
callbacks would stay consistent with the set_mode() callback..
>
> 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.
Now that we are looking forward to providing individual callbacks
per feature, I wanted to propose few more solutions to it, though I am
quite sure you would have already considered and rejected them.
I remember from your earlier mail that you didn't wanted to add
ONESHOT_STOPPED as a feature (as it isn't a feature really)
and that restricts us from doing this:
static int __clockevents_set_mode(dev, mode)
{
if ((mode == CLOCK_EVT_MODE_ONESHOT_STOPPED) &&
!(dev->features & CLOCK_EVT_FEAT_ONESHOT_STOPPED))
return -ENOSYS;
dev->set_mode(mode, dev);
return 0;
}
But what about providing a separate callback only for ONESHOT_STOPPED
mode and use set_mode() for everything else ?
static int __clockevents_set_mode(dev, mode)
{
if (mode == CLOCK_EVT_MODE_ONESHOT_STOPPED) {
if (dev->stop_onshot)
return dev->stop_onshot(dev);
else
return -ENOSYS;
}
dev->set_mode(mode, dev);
return 0;
}
Thanks for looking into this thread again..
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
