Re: [PATCH V3 3/8] clockevents: mips: migrate to ->set_dev_mode()

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/kernel/cevt-bcm1480.c b/arch/mips/kernel/cevt-bcm1480.c index 7976457..007fdb1 100644 --- a/arch/mips/kernel/cevt-bcm1480.c +++ b/arch/mips/kernel/cevt-bcm1480.c @@ -40,7 +40,7 @@

• The general purpose timer ticks at 1MHz independent if
• the rest of the system

*/ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -63,10 +63,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;

• case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */

Why is this comment removed?

• case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME:

• ;
  

• break;
  

• default:

• return -ENOSYS;
  

  }
• return 0;

}

static int sibyte_next_event(unsigned long delta, struct clock_event_device *cd) @@ -130,7 +133,7 @@ void sb1480_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = sibyte_next_event;

• cd->set_mode = sibyte_set_mode;

• cd->set_dev_mode = sibyte_set_mode; clockevents_register_device(cd);

  bcm1480_mask_irq(cpu, irq);

diff --git a/arch/mips/kernel/cevt-ds1287.c b/arch/mips/kernel/cevt-ds1287.c index ff1f01b..f86642a 100644 --- a/arch/mips/kernel/cevt-ds1287.c +++ b/arch/mips/kernel/cevt-ds1287.c @@ -59,7 +59,7 @@ static int ds1287_set_next_event(unsigned long delta, return -EINVAL; }

-static void ds1287_set_mode(enum clock_event_mode mode, +static int ds1287_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u8 val; @@ -72,14 +72,20 @@ static void ds1287_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: val |= RTC_PIE; break;

• default:

• case CLOCK_EVT_MODE_UNUSED:

• case CLOCK_EVT_MODE_SHUTDOWN:

• case CLOCK_EVT_MODE_RESUME: val &= ~RTC_PIE; break;

• default:

• spin_unlock(&rtc_lock);
  

• return -ENOSYS;
  

  }

  CMOS_WRITE(val, RTC_REG_B);

What about this ^^ in default case ? In default case, they would fall through and execute this instruction. After this patch, they won't. Would this be ok?

spin_unlock(&rtc_lock);

• return 0;

}

[snip]

diff --git a/arch/mips/kernel/cevt-sb1250.c b/arch/mips/kernel/cevt-sb1250.c index 5ea6d6b..f7cf430 100644 --- a/arch/mips/kernel/cevt-sb1250.c +++ b/arch/mips/kernel/cevt-sb1250.c @@ -38,7 +38,7 @@

• The general purpose timer ticks at 1MHz independent if
• the rest of the system

*/ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -61,10 +61,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;

• case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */

Same here.

• case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME:

• ;
  

• break;
  

• default:

• return -ENOSYS;
  

  }
• return 0;

}

[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; }

• return 0;

}

static void __init ralink_systick_init(struct device_node *np) diff --git a/arch/mips/sgi-ip27/ip27-timer.c b/arch/mips/sgi-ip27/ip27-timer.c index 1d97eab..5d5e90c 100644 --- a/arch/mips/sgi-ip27/ip27-timer.c +++ b/arch/mips/sgi-ip27/ip27-timer.c @@ -63,10 +63,20 @@ static int rt_next_event(unsigned long delta, struct clock_event_device *evt) return LOCAL_HUB_L(PI_RT_COUNT) >= cnt ? -ETIME : 0; }

-static void rt_set_mode(enum clock_event_mode mode, +static int rt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {

• switch (mode) {
• case CLOCK_EVT_MODE_ONESHOT:
• case CLOCK_EVT_MODE_UNUSED:
• case CLOCK_EVT_MODE_SHUTDOWN:
• case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */

• break;
  

• default:

• return -ENOSYS;
  

• }
• return 0;

}

unsigned int rt_timer_irq; @@ -123,7 +133,7 @@ void hub_rt_clock_event_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = rt_next_event;

• cd->set_mode = rt_set_mode;

• cd->set_dev_mode = rt_set_mode; clockevents_register_device(cd);

}

diff --git a/arch/mips/sni/time.c b/arch/mips/sni/time.c index cf8ec56..ac666e5 100644 --- a/arch/mips/sni/time.c +++ b/arch/mips/sni/time.c @@ -14,7 +14,7 @@ #define SNI_COUNTER2_DIV 64 #define SNI_COUNTER0_DIV ((SNI_CLOCK_TICK_RATE / SNI_COUNTER2_DIV) / HZ)

-static void a20r_set_mode(enum clock_event_mode mode, +static int a20r_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -34,13 +34,14 @@ static void a20r_set_mode(enum clock_event_mode mode, wmb();

break;

• case CLOCK_EVT_MODE_ONESHOT:

Why is this line deleted?