[3/4] thermal: rcar: Add missing clock handling
diff mbox

Message ID 1389121036-3555-4-git-send-email-geert@linux-m68k.org
State Rejected
Delegated to: Zhang Rui
Headers show

Commit Message

Geert Uytterhoeven Jan. 7, 2014, 6:57 p.m. UTC
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(-)

Comments

Sergei Shtylyov Jan. 7, 2014, 8:04 p.m. UTC | #1
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gerhard Sittig Jan. 7, 2014, 8:57 p.m. UTC | #2
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
Kuninori Morimoto Jan. 8, 2014, 1:08 a.m. UTC | #3
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Jan. 8, 2014, 10:23 a.m. UTC | #4
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Dooks Jan. 8, 2014, 12:20 p.m. UTC | #5
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.
Ben Dooks Jan. 8, 2014, 12:23 p.m. UTC | #6
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.
Geert Uytterhoeven Jan. 8, 2014, 12:58 p.m. UTC | #7
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Jan. 13, 2014, 8:57 a.m. UTC | #8
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kuninori Morimoto Jan. 14, 2014, 12:20 a.m. UTC | #9
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman Jan. 14, 2014, 1:27 a.m. UTC | #10
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 }