On 05/30/2014 07:06 AM, Viresh Kumar wrote:
On 29 May 2014 15:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all MIPS 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()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
[snip]
diff --git a/arch/mips/ralink/cevt-rt3352.c b/arch/mips/ralink/cevt-rt3352.c index 24bf057..c92b211 100644 --- a/arch/mips/ralink/cevt-rt3352.c +++ b/arch/mips/ralink/cevt-rt3352.c @@ -36,7 +36,7 @@ struct systick_device { int freq_scale; };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt);
static int systick_next_event(unsigned long delta, @@ -76,7 +76,7 @@ static struct systick_device systick = { .rating = 310, .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = systick_next_event,
.set_mode = systick_set_clock_mode,
.set_dev_mode = systick_set_clock_mode, .event_handler = systick_event_handler, },}; @@ -87,7 +87,7 @@ static struct irqaction systick_irqaction = { .dev_id = &systick.dev, };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct systick_device *sdev; @@ -110,10 +110,13 @@ static void systick_set_clock_mode(enum clock_event_mode mode, iowrite32(0, systick.membase + SYSTICK_CONFIG); break;
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME: break;default:return -ENOSYS; }This is another scenario I am confused about. We have a pr_err() already in the default case. Why do we skip adding the pr_err() in the case of MODE_UNUSED and MODE_RESUME and in the default case ? Only then would it be consistent with the earlier scenario right? I am saying something like the below:
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); break;default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);return -ENOSYS; }These modes aren't optional and so we don't return any error from them. And so doing a pr_err wouldn't be right anymore.
In which case why don't you club them with the 'default' case ? default case is meant for unhandled clock modes anyway.
I am fine with the rest of this patch.
Regards Preeti U Murthy