On 11 June 2014 05:29, Kevin Hilman khilman@linaro.org wrote:
Viresh Kumar viresh.kumar@linaro.org writes:
Two new APIs tick_stop_event() and tick_restart_event() are also implemented.
but they aren't used here, so it's better to implment them (and explain/justify them) where they are used.
Okay.
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index eaf5b0d..9348da1 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -107,8 +107,18 @@ void clockevents_set_mode(struct clock_event_device *dev, if (dev->mode != 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);
if (ret) {/** Supported Modes shouldn't fail and only* ONESTOP_STOPPED is optional*/This patch needs to be very clear about what "optional" means. Based on the current code, optional seems to mean that can fail and we dont' do anything about it...
Yes, comment surely needs some improvement but does it look wrong codewise as well ?
We are returning early (without updating dev->mode) if ONESHOT isn't implemented by driver as it returned -ENOSYS. Otherwise we are always doing a WARN() (even if ONESHOT failed).
If the code looks fine, what about this update to above comment
/* * Switching to any mode *must not* fail. * * Only ONESTOP_STOPPED is a optional and drivers should * return -ENOSYS if they don't implement it. */
if (mode == CLOCK_EVT_MODE_ONESHOT_STOPPED &&ret == -ENOSYS)return;elseWARN_ONCE(1, "Requested mode: %d, error: %d\n",mode, ret);}... However, since we know that it's very likely that unsupported modes will leave the timers disabled, there needs to be some some error recovery here. e.g. it should be checking what mode the clockevent device is in before attempting to change mode, and putting it back into that mode upon failure.
You meant this only for the if block above, right? As only ONESHOT_STOPPED is optional.
Yeah, we should try to revert to ONESHOT (we must be in that mode earlier) mode here.
Also, I suggest that you test this error recovery approach for the platform you're testing on *before* the final patch which adds ONESHOT_STOPPED support to the clockevent device, and then document in the cover letter how the error path was tested, and on which platform(s).
Will do that.