| Message ID | 1389121036-3555-4-git-send-email-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Hello. On 07-01-2014 22:57, Geert Uytterhoeven wrote: > From: Geert Uytterhoeven <geert+renesas@linux-m68k.org> > When using DT to instantiate the rcar-thermal device, it prints the > following error: > rcar_thermal e61f0000.thermal: thermal sensor was broken > Explicitly request and enable the thermal clock to fix this. > Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org> > --- > drivers/thermal/rcar_thermal.c | 40 +++++++++++++++++++++++++++++++++------- > 1 file changed, 33 insertions(+), 7 deletions(-) > diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c > index 88f92e1a9944..a5629500723a 100644 > --- a/drivers/thermal/rcar_thermal.c > +++ b/drivers/thermal/rcar_thermal.c [...] > @@ -378,23 +380,38 @@ static int rcar_thermal_probe(struct platform_device *pdev) [...] > + ret = clk_prepare(common->clk); > + if (ret < 0) { > + dev_err(&pdev->dev, "unable to prepare clock\n"); > + return ret; > + } > + > + clk_enable(common->clk); > + Why not just clk_prepare_enable()? [...] > @@ -465,9 +484,13 @@ error_unregister: > rcar_thermal_irq_disable(priv); > } > > +error_unpm: > pm_runtime_put_sync(dev); > pm_runtime_disable(dev); > > + clk_disable(common->clk); > + clk_unprepare(common->clk); > + Why not just clk_disable_unprepare()? > return ret; > } > > @@ -486,6 +509,9 @@ static int rcar_thermal_remove(struct platform_device *pdev) > pm_runtime_put_sync(dev); > pm_runtime_disable(dev); > > + clk_disable(common->clk); > + clk_unprepare(common->clk); > + Likewise. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 07, 2014 at 19:57 +0100, Geert Uytterhoeven wrote: > > @@ -378,23 +380,38 @@ static int rcar_thermal_probe(struct platform_device *pdev) > spin_lock_init(&common->lock); > common->dev = dev; > > + common->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(common->clk)) { > + dev_err(&pdev->dev, "cannot get clock\n"); > + return PTR_ERR(common->clk); > + } > + > + ret = clk_prepare(common->clk); > + if (ret < 0) { > + dev_err(&pdev->dev, "unable to prepare clock\n"); > + return ret; > + } > + > + clk_enable(common->clk); > + > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); clk_enable() can fail, too, so you should check its return value virtually yours Gerhard Sittig
Hi Geert > + common->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(common->clk)) { > + dev_err(&pdev->dev, "cannot get clock\n"); > + return PTR_ERR(common->clk); > + } > + > + ret = clk_prepare(common->clk); > + if (ret < 0) { > + dev_err(&pdev->dev, "unable to prepare clock\n"); > + return ret; > + } > + > + clk_enable(common->clk); > + > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); You can use "dev" instead of "&pdev->dev" :) And this patch seems strange for me. pm_runtime_xxx() is doing same things. If it didn't work, wrong place is not driver, clock side ? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 8, 2014 at 2:08 AM, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote: >> + common->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(common->clk)) { >> + dev_err(&pdev->dev, "cannot get clock\n"); >> + return PTR_ERR(common->clk); >> + } >> + >> + ret = clk_prepare(common->clk); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "unable to prepare clock\n"); >> + return ret; >> + } >> + >> + clk_enable(common->clk); >> + >> pm_runtime_enable(dev); >> pm_runtime_get_sync(dev); > > And this patch seems strange for me. > pm_runtime_xxx() is doing same things. > If it didn't work, wrong place is not driver, clock side ? That's an interesting observation... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/01/14 20:57, Gerhard Sittig wrote: > On Tue, Jan 07, 2014 at 19:57 +0100, Geert Uytterhoeven wrote: >> >> @@ -378,23 +380,38 @@ static int rcar_thermal_probe(struct platform_device *pdev) >> spin_lock_init(&common->lock); >> common->dev = dev; >> >> + common->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(common->clk)) { >> + dev_err(&pdev->dev, "cannot get clock\n"); >> + return PTR_ERR(common->clk); >> + } >> + >> + ret = clk_prepare(common->clk); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "unable to prepare clock\n"); >> + return ret; >> + } >> + >> + clk_enable(common->clk); >> + >> pm_runtime_enable(dev); >> pm_runtime_get_sync(dev); > > clk_enable() can fail, too, so you should check its return value Also, if you enable the pm-runtime then the device clock is actually handled by the common pm-clock framework.
On 07/01/14 18:57, Geert Uytterhoeven wrote: > From: Geert Uytterhoeven <geert+renesas@linux-m68k.org> > > When using DT to instantiate the rcar-thermal device, it prints the > following error: > > rcar_thermal e61f0000.thermal: thermal sensor was broken > > Explicitly request and enable the thermal clock to fix this. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org> > --- > drivers/thermal/rcar_thermal.c | 40 +++++++++++++++++++++++++++++++++------- > 1 file changed, 33 insertions(+), 7 deletions(-) > > diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c > index 88f92e1a9944..a5629500723a 100644 > --- a/drivers/thermal/rcar_thermal.c > +++ b/drivers/thermal/rcar_thermal.c > @@ -17,6 +17,7 @@ > * with this program; if not, write to the Free Software Foundation, Inc., > * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. > */ > +#include <linux/clk.h> > #include <linux/delay.h> > #include <linux/err.h> > #include <linux/irq.h> > @@ -53,6 +54,7 @@ struct rcar_thermal_common { > struct device *dev; > struct list_head head; > spinlock_t lock; > + struct clk *clk; > }; > > struct rcar_thermal_priv { > @@ -378,23 +380,38 @@ static int rcar_thermal_probe(struct platform_device *pdev) > spin_lock_init(&common->lock); > common->dev = dev; > > + common->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(common->clk)) { > + dev_err(&pdev->dev, "cannot get clock\n"); > + return PTR_ERR(common->clk); > + } > + > + ret = clk_prepare(common->clk); > + if (ret < 0) { > + dev_err(&pdev->dev, "unable to prepare clock\n"); > + return ret; > + } > + > + clk_enable(common->clk); > + > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (irq) { > - int ret; > + int ret2; > > /* > * platform has IRQ support. > * Then, drier use common register > */ > > - ret = devm_request_irq(dev, irq->start, rcar_thermal_irq, 0, > - dev_name(dev), common); > - if (ret) { > + ret2 = devm_request_irq(dev, irq->start, rcar_thermal_irq, 0, > + dev_name(dev), common); > + if (ret2) { > dev_err(dev, "irq request failed\n "); > - return ret; > + ret = ret2; > + goto error_unpm; > } I'd suggest not renaming ret2 and just use the original ret.
On Wed, Jan 8, 2014 at 1:23 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: >> @@ -378,23 +380,38 @@ static int rcar_thermal_probe(struct platform_device >> *pdev) >> spin_lock_init(&common->lock); >> common->dev = dev; >> >> + common->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(common->clk)) { >> + dev_err(&pdev->dev, "cannot get clock\n"); >> + return PTR_ERR(common->clk); >> + } >> + >> + ret = clk_prepare(common->clk); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "unable to prepare clock\n"); >> + return ret; >> + } >> + >> + clk_enable(common->clk); >> + >> pm_runtime_enable(dev); >> pm_runtime_get_sync(dev); >> >> irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> if (irq) { >> - int ret; >> + int ret2; >> >> /* >> * platform has IRQ support. >> * Then, drier use common register >> */ >> >> - ret = devm_request_irq(dev, irq->start, rcar_thermal_irq, >> 0, >> - dev_name(dev), common); >> - if (ret) { >> + ret2 = devm_request_irq(dev, irq->start, rcar_thermal_irq, >> 0, >> + dev_name(dev), common); >> + if (ret2) { >> dev_err(dev, "irq request failed\n "); >> - return ret; >> + ret = ret2; >> + goto error_unpm; >> } > > I'd suggest not renaming ret2 and just use the original ret. I did that because the for loop after that block depends on ret still being -ENODEV. Upon closer look, I did break that myself by assigning it the return value of clk_prepare(). So I'll use the original ret, and reset it to -ENODEV before the for loop (unless we manage to fix it in pm_runtime_*()). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Morimoto-san, On Wed, Jan 8, 2014 at 11:23 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Jan 8, 2014 at 2:08 AM, Kuninori Morimoto > <kuninori.morimoto.gx@gmail.com> wrote: >>> + common->clk = devm_clk_get(&pdev->dev, NULL); >>> + if (IS_ERR(common->clk)) { >>> + dev_err(&pdev->dev, "cannot get clock\n"); >>> + return PTR_ERR(common->clk); >>> + } >>> + >>> + ret = clk_prepare(common->clk); >>> + if (ret < 0) { >>> + dev_err(&pdev->dev, "unable to prepare clock\n"); >>> + return ret; >>> + } >>> + >>> + clk_enable(common->clk); >>> + >>> pm_runtime_enable(dev); >>> pm_runtime_get_sync(dev); >> >> And this patch seems strange for me. >> pm_runtime_xxx() is doing same things. >> If it didn't work, wrong place is not driver, clock side ? > > That's an interesting observation... You were right. After applying both of "ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI" and "power: clock_ops.c: fixup clk prepare/unprepare count" from Ben Dooks the issue went away. So my patch can be dropped. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert > >> And this patch seems strange for me. > >> pm_runtime_xxx() is doing same things. > >> If it didn't work, wrong place is not driver, clock side ? (snipO > You were right. After applying both of "ARM: shmobile: compile drivers/sh > for CONFIG_ARCH_SHMOBILE_MULTI" and "power: clock_ops.c: fixup clk > prepare/unprepare count" from Ben Dooks the issue went away. Nice ! Thank you for your help :) Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 13, 2014 at 09:57:08AM +0100, Geert Uytterhoeven wrote: > Hi Morimoto-san, > > On Wed, Jan 8, 2014 at 11:23 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Wed, Jan 8, 2014 at 2:08 AM, Kuninori Morimoto > > <kuninori.morimoto.gx@gmail.com> wrote: > >>> + common->clk = devm_clk_get(&pdev->dev, NULL); > >>> + if (IS_ERR(common->clk)) { > >>> + dev_err(&pdev->dev, "cannot get clock\n"); > >>> + return PTR_ERR(common->clk); > >>> + } > >>> + > >>> + ret = clk_prepare(common->clk); > >>> + if (ret < 0) { > >>> + dev_err(&pdev->dev, "unable to prepare clock\n"); > >>> + return ret; > >>> + } > >>> + > >>> + clk_enable(common->clk); > >>> + > >>> pm_runtime_enable(dev); > >>> pm_runtime_get_sync(dev); > >> > >> And this patch seems strange for me. > >> pm_runtime_xxx() is doing same things. > >> If it didn't work, wrong place is not driver, clock side ? > > > > That's an interesting observation... > > You were right. After applying both of "ARM: shmobile: compile drivers/sh > for CONFIG_ARCH_SHMOBILE_MULTI" and "power: clock_ops.c: fixup clk > prepare/unprepare count" from Ben Dooks the issue went away. There seems to be some lively discussion around Ben's patch. > So my patch can be dropped. I have marked this patch as Rejected accordingly. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c index 88f92e1a9944..a5629500723a 100644 --- a/drivers/thermal/rcar_thermal.c +++ b/drivers/thermal/rcar_thermal.c @@ -17,6 +17,7 @@ * with this program; if not, write to the Free Software Foundation, Inc., * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. */ +#include <linux/clk.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/irq.h> @@ -53,6 +54,7 @@ struct rcar_thermal_common { struct device *dev; struct list_head head; spinlock_t lock; + struct clk *clk; }; struct rcar_thermal_priv { @@ -378,23 +380,38 @@ static int rcar_thermal_probe(struct platform_device *pdev) spin_lock_init(&common->lock); common->dev = dev; + common->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(common->clk)) { + dev_err(&pdev->dev, "cannot get clock\n"); + return PTR_ERR(common->clk); + } + + ret = clk_prepare(common->clk); + if (ret < 0) { + dev_err(&pdev->dev, "unable to prepare clock\n"); + return ret; + } + + clk_enable(common->clk); + pm_runtime_enable(dev); pm_runtime_get_sync(dev); irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (irq) { - int ret; + int ret2; /* * platform has IRQ support. * Then, drier use common register */ - ret = devm_request_irq(dev, irq->start, rcar_thermal_irq, 0, - dev_name(dev), common); - if (ret) { + ret2 = devm_request_irq(dev, irq->start, rcar_thermal_irq, 0, + dev_name(dev), common); + if (ret2) { dev_err(dev, "irq request failed\n "); - return ret; + ret = ret2; + goto error_unpm; } /* @@ -402,8 +419,10 @@ static int rcar_thermal_probe(struct platform_device *pdev) */ res = platform_get_resource(pdev, IORESOURCE_MEM, mres++); common->base = devm_ioremap_resource(dev, res); - if (IS_ERR(common->base)) - return PTR_ERR(common->base); + if (IS_ERR(common->base)) { + ret = PTR_ERR(common->base); + goto error_unpm; + } /* enable temperature comparation */ rcar_thermal_common_write(common, ENR, 0x00030303); @@ -465,9 +484,13 @@ error_unregister: rcar_thermal_irq_disable(priv); } +error_unpm: pm_runtime_put_sync(dev); pm_runtime_disable(dev); + clk_disable(common->clk); + clk_unprepare(common->clk); + return ret; } @@ -486,6 +509,9 @@ static int rcar_thermal_remove(struct platform_device *pdev) pm_runtime_put_sync(dev); pm_runtime_disable(dev); + clk_disable(common->clk); + clk_unprepare(common->clk); + return 0; }