| Message ID | fe0d96a4671d94c60f7b252d1c038cd62ed71162.1434622147.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 18, 2015 at 04:24:37PM +0530, Viresh Kumar wrote: > Migrate sun4i driver to the new 'set-state' interface provided by > clockevents core, the earlier 'set-mode' interface is marked obsolete > now. > > This also enables us to implement callbacks for new states of clockevent > devices, for example: ONESHOT_STOPPED. > > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/clocksource/sun4i_timer.c | 41 +++++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 19 deletions(-) > > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c > index 1928a8912584..6f3719d73390 100644 > --- a/drivers/clocksource/sun4i_timer.c > +++ b/drivers/clocksource/sun4i_timer.c > @@ -81,25 +81,25 @@ static void sun4i_clkevt_time_start(u8 timer, bool periodic) > timer_base + TIMER_CTL_REG(timer)); > } > > -static void sun4i_clkevt_mode(enum clock_event_mode mode, > - struct clock_event_device *clk) > +static int sun4i_clkevt_shutdown(struct clock_event_device *evt) > { > - switch (mode) { > - case CLOCK_EVT_MODE_PERIODIC: > - sun4i_clkevt_time_stop(0); > - sun4i_clkevt_time_setup(0, ticks_per_jiffy); > - sun4i_clkevt_time_start(0, true); > - break; > - case CLOCK_EVT_MODE_ONESHOT: > - sun4i_clkevt_time_stop(0); > - sun4i_clkevt_time_start(0, false); > - break; > - case CLOCK_EVT_MODE_UNUSED: > - case CLOCK_EVT_MODE_SHUTDOWN: > - default: > - sun4i_clkevt_time_stop(0); > - break; > - } > + sun4i_clkevt_time_stop(0); > + return 0; > +} > + > +static int sun4i_clkevt_set_oneshot(struct clock_event_device *evt) > +{ > + sun4i_clkevt_time_stop(0); > + sun4i_clkevt_time_start(0, false); > + return 0; > +} > + > +static int sun4i_clkevt_set_periodic(struct clock_event_device *evt) > +{ > + sun4i_clkevt_time_stop(0); > + sun4i_clkevt_time_setup(0, ticks_per_jiffy); > + sun4i_clkevt_time_start(0, true); > + return 0; > } > > static int sun4i_clkevt_next_event(unsigned long evt, > @@ -116,7 +116,10 @@ static struct clock_event_device sun4i_clockevent = { > .name = "sun4i_tick", > .rating = 350, > .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, > - .set_mode = sun4i_clkevt_mode, > + .set_state_shutdown = sun4i_clkevt_shutdown, > + .set_state_periodic = sun4i_clkevt_set_periodic, > + .set_state_oneshot = sun4i_clkevt_set_oneshot, > + .tick_resume = sun4i_clkevt_shutdown, I'm not exactly sure of the context here, but I wouldn't expect a callback called tick_resume to stop a timer. Is this expected? Maxime
On 18-06-15, 14:01, Maxime Ripard wrote: > On Thu, Jun 18, 2015 at 04:24:37PM +0530, Viresh Kumar wrote: > > +static int sun4i_clkevt_shutdown(struct clock_event_device *evt) > > { > > - switch (mode) { > > - case CLOCK_EVT_MODE_PERIODIC: > > - sun4i_clkevt_time_stop(0); > > - sun4i_clkevt_time_setup(0, ticks_per_jiffy); > > - sun4i_clkevt_time_start(0, true); > > - break; > > - case CLOCK_EVT_MODE_ONESHOT: > > - sun4i_clkevt_time_stop(0); > > - sun4i_clkevt_time_start(0, false); > > - break; > > - case CLOCK_EVT_MODE_UNUSED: > > - case CLOCK_EVT_MODE_SHUTDOWN: > > - default: > > - sun4i_clkevt_time_stop(0); > > - break; Because sun4i_clkevt_time_stop() is getting called in default case, it is getting called for tick-resume mode already with the old set_mode interface. > > + .tick_resume = sun4i_clkevt_shutdown, > > I'm not exactly sure of the context here, but I wouldn't expect a > callback called tick_resume to stop a timer. Is this expected? And so this patch carried the same logic here. At suspend: clockevents core calls ->set_state_shutdown() and at resume it calls ->tick_resume() followed by setting to the proper mode, i.e. periodic or oneshot. Many driver authors didn't knew about these details and so did shutdown in resume path as well. For me, you might not even need a tick_resume() at all, as your driver would have already shutted down on suspend and is just required to be put to the right mode again. But, I didn't wanted to change the way things behaved until now. I can add another patch to get things fixed separately if you want me to.
On Thu, Jun 18, 2015 at 05:53:36PM +0530, Viresh Kumar wrote: > On 18-06-15, 14:01, Maxime Ripard wrote: > > On Thu, Jun 18, 2015 at 04:24:37PM +0530, Viresh Kumar wrote: > > > +static int sun4i_clkevt_shutdown(struct clock_event_device *evt) > > > { > > > - switch (mode) { > > > - case CLOCK_EVT_MODE_PERIODIC: > > > - sun4i_clkevt_time_stop(0); > > > - sun4i_clkevt_time_setup(0, ticks_per_jiffy); > > > - sun4i_clkevt_time_start(0, true); > > > - break; > > > - case CLOCK_EVT_MODE_ONESHOT: > > > - sun4i_clkevt_time_stop(0); > > > - sun4i_clkevt_time_start(0, false); > > > - break; > > > - case CLOCK_EVT_MODE_UNUSED: > > > - case CLOCK_EVT_MODE_SHUTDOWN: > > > - default: > > > - sun4i_clkevt_time_stop(0); > > > - break; > > Because sun4i_clkevt_time_stop() is getting called in default case, it > is getting called for tick-resume mode already with the old set_mode > interface. > > > > + .tick_resume = sun4i_clkevt_shutdown, > > > > I'm not exactly sure of the context here, but I wouldn't expect a > > callback called tick_resume to stop a timer. Is this expected? > > And so this patch carried the same logic here. > > At suspend: clockevents core calls ->set_state_shutdown() and at > resume it calls ->tick_resume() followed by setting to the proper > mode, i.e. periodic or oneshot. > > Many driver authors didn't knew about these details and so did > shutdown in resume path as well. > > For me, you might not even need a tick_resume() at all, as your driver > would have already shutted down on suspend and is just required to be > put to the right mode again. Ok, thanks for the explanation. It looks good for both this patch and the sun5i one. You can add my Acked-by. > But, I didn't wanted to change the way things behaved until now. I can > add another patch to get things fixed separately if you want me to. If you have some spare time, it would be great :) Thanks! Maxime
diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c index 1928a8912584..6f3719d73390 100644 --- a/drivers/clocksource/sun4i_timer.c +++ b/drivers/clocksource/sun4i_timer.c @@ -81,25 +81,25 @@ static void sun4i_clkevt_time_start(u8 timer, bool periodic) timer_base + TIMER_CTL_REG(timer)); } -static void sun4i_clkevt_mode(enum clock_event_mode mode, - struct clock_event_device *clk) +static int sun4i_clkevt_shutdown(struct clock_event_device *evt) { - switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - sun4i_clkevt_time_stop(0); - sun4i_clkevt_time_setup(0, ticks_per_jiffy); - sun4i_clkevt_time_start(0, true); - break; - case CLOCK_EVT_MODE_ONESHOT: - sun4i_clkevt_time_stop(0); - sun4i_clkevt_time_start(0, false); - break; - case CLOCK_EVT_MODE_UNUSED: - case CLOCK_EVT_MODE_SHUTDOWN: - default: - sun4i_clkevt_time_stop(0); - break; - } + sun4i_clkevt_time_stop(0); + return 0; +} + +static int sun4i_clkevt_set_oneshot(struct clock_event_device *evt) +{ + sun4i_clkevt_time_stop(0); + sun4i_clkevt_time_start(0, false); + return 0; +} + +static int sun4i_clkevt_set_periodic(struct clock_event_device *evt) +{ + sun4i_clkevt_time_stop(0); + sun4i_clkevt_time_setup(0, ticks_per_jiffy); + sun4i_clkevt_time_start(0, true); + return 0; } static int sun4i_clkevt_next_event(unsigned long evt, @@ -116,7 +116,10 @@ static struct clock_event_device sun4i_clockevent = { .name = "sun4i_tick", .rating = 350, .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = sun4i_clkevt_mode, + .set_state_shutdown = sun4i_clkevt_shutdown, + .set_state_periodic = sun4i_clkevt_set_periodic, + .set_state_oneshot = sun4i_clkevt_set_oneshot, + .tick_resume = sun4i_clkevt_shutdown, .set_next_event = sun4i_clkevt_next_event, };
Migrate sun4i driver to the new 'set-state' interface provided by clockevents core, the earlier 'set-mode' interface is marked obsolete now. This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED. Cc: Maxime Ripard <maxime.ripard@free-electrons.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/clocksource/sun4i_timer.c | 41 +++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 19 deletions(-)