| Message ID | 3aa5c1a740911258698769cbbb8f60ee02178848.1434622147.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Viresh Kumar <viresh.kumar@linaro.org> writes: > @@ -88,26 +88,12 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev) > return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; > } > > -static void > -pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) > +static int pxa_osmr0_shutdown(struct clock_event_device *evt) > { > - switch (mode) { > - case CLOCK_EVT_MODE_ONESHOT: > - timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); > - timer_writel(OSSR_M0, OSSR); > - break; > - > - case CLOCK_EVT_MODE_UNUSED: > - case CLOCK_EVT_MODE_SHUTDOWN: > - /* initializing, released, or preparing for suspend */ > - timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); > - timer_writel(OSSR_M0, OSSR); > - break; > - > - case CLOCK_EVT_MODE_RESUME: > - case CLOCK_EVT_MODE_PERIODIC: > - break; > - } > + /* initializing, released, or preparing for suspend */ > + timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); > + timer_writel(OSSR_M0, OSSR); > + return 0; For consistency, please leave an empty line before that return statement. > @@ -147,13 +133,14 @@ static void pxa_timer_resume(struct clock_event_device *cedev) > #endif > > static struct clock_event_device ckevt_pxa_osmr0 = { > - .name = "osmr0", > - .features = CLOCK_EVT_FEAT_ONESHOT, > - .rating = 200, > - .set_next_event = pxa_osmr0_set_next_event, > - .set_mode = pxa_osmr0_set_mode, > - .suspend = pxa_timer_suspend, > - .resume = pxa_timer_resume, > + .name = "osmr0", > + .features = CLOCK_EVT_FEAT_ONESHOT, > + .rating = 200, > + .set_next_event = pxa_osmr0_set_next_event, > + .set_state_shutdown = pxa_osmr0_shutdown, > + .set_state_oneshot = pxa_osmr0_shutdown, A bit weird to have a "set_state_oneshot" function to point to a function called "X_shutdown". As I don't have a clear idea on what's this new interface for, I'll just hope it's the intended purpose. The code does look equivalent to me anyway. Apart from the cosmetic comment, once it is fixed : Acked-by: Robert Jarzmik <robert.jarzmik@free.fr> Cheers.
Hi Robert, On 04-07-15, 17:42, Robert Jarzmik wrote: > > + /* initializing, released, or preparing for suspend */ > > + timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); > > + timer_writel(OSSR_M0, OSSR); > > + return 0; > For consistency, please leave an empty line before that return statement. Its already applied by Daniel now, and the change is too trivial to request for an update to his tree. Maybe we should leave it as is for now. > > @@ -147,13 +133,14 @@ static void pxa_timer_resume(struct clock_event_device *cedev) > > #endif > > > > static struct clock_event_device ckevt_pxa_osmr0 = { > > - .name = "osmr0", > > - .features = CLOCK_EVT_FEAT_ONESHOT, > > - .rating = 200, > > - .set_next_event = pxa_osmr0_set_next_event, > > - .set_mode = pxa_osmr0_set_mode, > > - .suspend = pxa_timer_suspend, > > - .resume = pxa_timer_resume, > > + .name = "osmr0", > > + .features = CLOCK_EVT_FEAT_ONESHOT, > > + .rating = 200, > > + .set_next_event = pxa_osmr0_set_next_event, > > + .set_state_shutdown = pxa_osmr0_shutdown, > > + .set_state_oneshot = pxa_osmr0_shutdown, > A bit weird to have a "set_state_oneshot" function to point to a function called > "X_shutdown". What's weird (or looks weird) is that we stop the timer when requested to switch to oneshot mode. But that's what we really wanted, because set_next_event is the one that will program the timer in oneshot mode. > As I don't have a clear idea on what's this new interface for, It just provides per-state API's for what's being done in set_mode() earlier. > I'll just hope it's the intended purpose. The code does look equivalent to me > anyway. > > Apart from the cosmetic comment, once it is fixed : > Acked-by: Robert Jarzmik <robert.jarzmik@free.fr> Thanks.
Viresh Kumar <viresh.kumar@linaro.org> writes: > Hi Robert, > > On 04-07-15, 17:42, Robert Jarzmik wrote: >> > + /* initializing, released, or preparing for suspend */ >> > + timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); >> > + timer_writel(OSSR_M0, OSSR); >> > + return 0; >> For consistency, please leave an empty line before that return statement. > > Its already applied by Daniel now, and the change is too trivial to > request for an update to his tree. Maybe we should leave it as is for > now. If it's applied it's indeed too late. >> > + .set_state_shutdown = pxa_osmr0_shutdown, >> > + .set_state_oneshot = pxa_osmr0_shutdown, >> A bit weird to have a "set_state_oneshot" function to point to a function called >> "X_shutdown". > > What's weird (or looks weird) is that we stop the timer when requested > to switch to oneshot mode. But that's what we really wanted, because > set_next_event is the one that will program the timer in oneshot mode. Ok, thanks for the explanation. Cheers.
diff --git a/drivers/clocksource/pxa_timer.c b/drivers/clocksource/pxa_timer.c index d9438af2bbd6..45b6a4999713 100644 --- a/drivers/clocksource/pxa_timer.c +++ b/drivers/clocksource/pxa_timer.c @@ -88,26 +88,12 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; } -static void -pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) +static int pxa_osmr0_shutdown(struct clock_event_device *evt) { - switch (mode) { - case CLOCK_EVT_MODE_ONESHOT: - timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); - timer_writel(OSSR_M0, OSSR); - break; - - case CLOCK_EVT_MODE_UNUSED: - case CLOCK_EVT_MODE_SHUTDOWN: - /* initializing, released, or preparing for suspend */ - timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); - timer_writel(OSSR_M0, OSSR); - break; - - case CLOCK_EVT_MODE_RESUME: - case CLOCK_EVT_MODE_PERIODIC: - break; - } + /* initializing, released, or preparing for suspend */ + timer_writel(timer_readl(OIER) & ~OIER_E0, OIER); + timer_writel(OSSR_M0, OSSR); + return 0; } #ifdef CONFIG_PM @@ -147,13 +133,14 @@ static void pxa_timer_resume(struct clock_event_device *cedev) #endif static struct clock_event_device ckevt_pxa_osmr0 = { - .name = "osmr0", - .features = CLOCK_EVT_FEAT_ONESHOT, - .rating = 200, - .set_next_event = pxa_osmr0_set_next_event, - .set_mode = pxa_osmr0_set_mode, - .suspend = pxa_timer_suspend, - .resume = pxa_timer_resume, + .name = "osmr0", + .features = CLOCK_EVT_FEAT_ONESHOT, + .rating = 200, + .set_next_event = pxa_osmr0_set_next_event, + .set_state_shutdown = pxa_osmr0_shutdown, + .set_state_oneshot = pxa_osmr0_shutdown, + .suspend = pxa_timer_suspend, + .resume = pxa_timer_resume, }; static struct irqaction pxa_ost0_irq = {
Migrate pxa 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. Both oneshot and shutdown modes had exactly same code and so only a single callback is sufficient now, which will be called for both the modes. Cc: Robert Jarzmik <robert.jarzmik@free.fr> Cc: Russell King <linux@arm.linux.org.uk> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/clocksource/pxa_timer.c | 39 +++++++++++++-------------------------- 1 file changed, 13 insertions(+), 26 deletions(-)