Re: [PATCH V3 6/8] clockevents: drivers/: 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 drivers in 'drivers/clocksource' 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.

[snip]

diff --git a/drivers/clocksource/qcom-timer.c b/drivers/clocksource/qcom-timer.c index e807acf..f72ef19 100644 --- a/drivers/clocksource/qcom-timer.c +++ b/drivers/clocksource/qcom-timer.c @@ -75,7 +75,7 @@ static int msm_timer_set_next_event(unsigned long cycles, return 0; }

-static void msm_timer_set_mode(enum clock_event_mode mode, +static int msm_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 ctrl; @@ -85,16 +85,17 @@ static void msm_timer_set_mode(enum clock_event_mode mode,

switch (mode) { case CLOCK_EVT_MODE_RESUME:

• case CLOCK_EVT_MODE_PERIODIC:

• break;
  

Sorry about repeatedly pointing this. I may be missing something. But why are the above two lines removed?

case CLOCK_EVT_MODE_ONESHOT: /* Timer is enabled in set_next_event */ break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: break;

• default:

• return -ENOSYS;
  

  } writel_relaxed(ctrl, event_base + TIMER_ENABLE);

Would it be fine to skip this ^^ in the default case?

• return 0;

}

[snip]

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c index 0451e62..8aa35c1 100644 --- a/drivers/clocksource/time-armada-370-xp.c +++ b/drivers/clocksource/time-armada-370-xp.c @@ -120,12 +120,12 @@ armada_370_xp_clkevt_next_event(unsigned long delta, return 0; }

-static void +static int armada_370_xp_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) {

• if (mode == CLOCK_EVT_MODE_PERIODIC) {

• switch (mode) {
• case CLOCK_EVT_MODE_PERIODIC: /*
  • Setup timer to fire at 1/HZ intervals.
  */

@@ -136,7 +136,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * Enable timer. */ local_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | enable_mask);

• } else {

• break;
  

• case CLOCK_EVT_MODE_ONESHOT:
• case CLOCK_EVT_MODE_UNUSED:
• case CLOCK_EVT_MODE_SHUTDOWN:
• case CLOCK_EVT_MODE_RESUME: /*
  • Disable timer.
  */

@@ -146,7 +150,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * ACK pending timer interrupt. */ writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);

This line in the right place right? I mean do you need it in default case also? I am unable to make out.

• break;
  

• default:

• return -ENOSYS;
  

  }
• return 0;

}

static int armada_370_xp_clkevt_irq; @@ -184,7 +192,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt) evt->shift = 32, evt->rating = 300, evt->set_next_event = armada_370_xp_clkevt_next_event,

• evt->set_mode = armada_370_xp_clkevt_mode,

• evt->set_dev_mode = armada_370_xp_clkevt_mode, evt->irq = armada_370_xp_clkevt_irq; evt->cpumask = cpumask_of(cpu);

@@ -196,7 +204,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt)

static void armada_370_xp_timer_stop(struct clock_event_device *evt) {

• evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);

• evt->set_dev_mode(CLOCK_EVT_MODE_UNUSED, evt); disable_percpu_irq(evt->irq);

[snip]

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c index 1098ed3..17492ed 100644 --- a/drivers/clocksource/vt8500_timer.c +++ b/drivers/clocksource/vt8500_timer.c @@ -88,12 +88,11 @@ static int vt8500_timer_set_next_event(unsigned long cycles, return 0; }

-static void vt8500_timer_set_mode(enum clock_event_mode mode, +static int vt8500_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { case CLOCK_EVT_MODE_RESUME:

• case CLOCK_EVT_MODE_PERIODIC: break;

Is this deletion right?

case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: @@ -102,7 +101,10 @@ static void vt8500_timer_set_mode(enum clock_event_mode mode, regbase + TIMER_CTRL_VAL); writel(0, regbase + TIMER_IER_VAL); break;

• default:

• return -ENOSYS;
  

  }
• return 0;

}

static struct clock_event_device clockevent = { @@ -110,7 +112,7 @@ static struct clock_event_device clockevent = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = vt8500_timer_set_next_event,

• .set_mode = vt8500_timer_set_mode,

• .set_dev_mode = vt8500_timer_set_mode,

};

static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)

Regards Preeti U Murthy