On Fri, May 23, 2014 at 08:25:30PM +0530, Viresh Kumar wrote:
On 23 May 2014 19:24, Frederic Weisbecker fweisbec@gmail.com wrote:
Also probably the error handling should be done in this patchset?
What kind of error handling ? Do we need to return the error code? Thomas rather asked us to do error handling here only and I am not sure what else we can do here apart from a WARN.
So clockevents_set_mode() returns void so for now I don't know. But __clockevents_update_freq() can return errors.
I tried that initially while you weren't cc'd and this is what I received.
On 21 May 2014 23:29, Kevin Hilman khilman@linaro.org wrote:
Viresh Kumar viresh.kumar@linaro.org writes:
int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) {
int ret = 0;
clockevents_config(dev, 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) {
if (dev->set_dev_mode) {
ret = dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, dev);
WARN_ON(ret);
} else {
dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
}
}
return 0;
return ret;
}
First, this change isn't mentioned in the changelog. But more importantly (as Daniel has mentioned), returning an error here has consequences.
Specifically, why would the caller of this function care that the specific clkevt mode was not implmented? As Thomas mentioned in his reviews, the clockevent code should handle the failures itself, not just pass them up and expect other layers to handle them.
Also, if PERIODIC mode can't fail today, why would we expect it to fail in the changed version?
Hmm, ok lets just keep the WARN and see then :)