Re: [RFC PATCH 1/3] arm: plat-omap: dmtimer: Add clock source from DT
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(>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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 1/3] arm: plat-omap: dmtimer: Add clock source from DT
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 LindgrenSigned-off-by: Neil Armstrong --- 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; + + if (IS_ERR(timer->fclk)) + return -EINVAL; + + parent = clk_get(>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) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/3] arm: plat-omap: dmtimer: Add clock source from DT
Hi, Neil Armstrongwrites: > 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 > Signed-off-by: Neil Armstrong > --- > 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(>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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- balbi signature.asc Description: PGP signature