| Message ID | 5620ED30.2050507@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Neil Armstrong <narmstrong@baylibre.com> writes: > Add a function which sets the timer source from the clocks > binding on dm_timer_prepare call. > > In case the clocks property is not valid, it falls back to > the set_source() with 32_KHZ clock as default. > > Suggested-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > arch/arm/plat-omap/dmtimer.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c > index 8ca94d3..5e8aece 100644 > --- a/arch/arm/plat-omap/dmtimer.c > +++ b/arch/arm/plat-omap/dmtimer.c > @@ -137,9 +137,33 @@ static int omap_dm_timer_reset(struct omap_dm_timer *timer) > return 0; > } > > +static int omap_dm_timer_of_set_source(struct omap_dm_timer *timer) > +{ > + int ret; > + struct clk *parent; > + > + if (unlikely(!timer)) > + return -EINVAL; IMO, let this crash. If this happens we have bigger problems. > + if (IS_ERR(timer->fclk)) > + return -EINVAL; We bail out if we can't get fclk, this check is unnecessary. > + parent = clk_get(&timer->pdev->dev, NULL); why NULL ? You could use something more descriptive, but no strong feelings. > + if (IS_ERR(parent)) > + return -ENODEV; > + > + ret = clk_set_parent(timer->fclk, parent); > + if (ret < 0) > + pr_err("%s: failed to set parent\n", __func__); > + > + clk_put(parent); > + > + return ret; > +} > + > static int omap_dm_timer_prepare(struct omap_dm_timer *timer) > { > - int rc; > + int rc, ret; doesn't seem like you need this extra 'ret' variable. Just use 'rc'. > @@ -166,7 +190,11 @@ static int omap_dm_timer_prepare(struct omap_dm_timer *timer) > __omap_dm_timer_enable_posted(timer); > omap_dm_timer_disable(timer); > > - return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ); > + ret = omap_dm_timer_of_set_source(timer); > + if (ret < 0 && ret == -ENODEV) this < 0 check is pointless if you're going to check for equality to -ENODEV > + return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ); > + > + return ret; > } > > static inline u32 omap_dm_timer_reserved_systimer(int id) > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Hi, On 10/16/2015 05:40 PM, Felipe Balbi wrote: >> +static int omap_dm_timer_of_set_source(struct omap_dm_timer *timer) >> +{ >> + int ret; >> + struct clk *parent; >> + >> + if (unlikely(!timer)) >> + return -EINVAL; > > IMO, let this crash. If this happens we have bigger problems. > >> + if (IS_ERR(timer->fclk)) >> + return -EINVAL; > > We bail out if we can't get fclk, this check is unnecessary. Sure, I will remove the checks. This was also breaking OMAP1, fclk must be checked against NULL here to detect OMAP1 and return -ENODEV; > >> + parent = clk_get(&timer->pdev->dev, NULL); > > why NULL ? You could use something more descriptive, but no strong > feelings. Actually, I just wanted the "default" clock for this device, NULL will select the first OF clock found. If we specify a name, we should also specify it in the DT as clock-names, is it desired ? If you feel I should name it somehow... I don't want it to conflict all the HWMOD clk namings actually. > >> + if (IS_ERR(parent)) >> + return -ENODEV; >> + >> + ret = clk_set_parent(timer->fclk, parent); >> + if (ret < 0) >> + pr_err("%s: failed to set parent\n", __func__); >> + >> + clk_put(parent); >> + >> + return ret; >> +} >> + >> static int omap_dm_timer_prepare(struct omap_dm_timer *timer) >> { >> - int rc; >> + int rc, ret; > > doesn't seem like you need this extra 'ret' variable. Just use 'rc'. > >> @@ -166,7 +190,11 @@ static int omap_dm_timer_prepare(struct omap_dm_timer *timer) >> __omap_dm_timer_enable_posted(timer); >> omap_dm_timer_disable(timer); >> >> - return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ); >> + ret = omap_dm_timer_of_set_source(timer); >> + if (ret < 0 && ret == -ENODEV) > > this < 0 check is pointless if you're going to check for equality to -ENODEV Sure, I was certainly tired, this was a so stupid mistake... Neil
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c index 8ca94d3..5e8aece 100644 --- a/arch/arm/plat-omap/dmtimer.c +++ b/arch/arm/plat-omap/dmtimer.c @@ -137,9 +137,33 @@ static int omap_dm_timer_reset(struct omap_dm_timer *timer) return 0; } +static int omap_dm_timer_of_set_source(struct omap_dm_timer *timer) +{ + int ret; + struct clk *parent; + + if (unlikely(!timer)) + return -EINVAL; + + if (IS_ERR(timer->fclk)) + return -EINVAL; + + parent = clk_get(&timer->pdev->dev, NULL); + if (IS_ERR(parent)) + return -ENODEV; + + ret = clk_set_parent(timer->fclk, parent); + if (ret < 0) + pr_err("%s: failed to set parent\n", __func__); + + clk_put(parent); + + return ret; +} + static int omap_dm_timer_prepare(struct omap_dm_timer *timer) { - int rc; + int rc, ret; /* * FIXME: OMAP1 devices do not use the clock framework for dmtimers so @@ -166,7 +190,11 @@ static int omap_dm_timer_prepare(struct omap_dm_timer *timer) __omap_dm_timer_enable_posted(timer); omap_dm_timer_disable(timer); - return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ); + ret = omap_dm_timer_of_set_source(timer); + if (ret < 0 && ret == -ENODEV) + return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ); + + return ret; } static inline u32 omap_dm_timer_reserved_systimer(int id)
Add a function which sets the timer source from the clocks binding on dm_timer_prepare call. In case the clocks property is not valid, it falls back to the set_source() with 32_KHZ clock as default. Suggested-by: Tony Lindgren <tony@atomide.com> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- arch/arm/plat-omap/dmtimer.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)