[PATCH 0/3] thermal: rcar_thermal: Update how temperature is read

[PATCH 0/3] thermal: rcar_thermal: Update how temperature is read

[Niklas Söderlund]

Hi,

This series simplifies how the temperature is read, actually notifies a 
zone on temperature changes and removes the need for locking in some 
code paths. The change is made possible by a quiet old commit [1] which 
made reading the ctemp value from hardware every time it's needed 
mandatory.

1. a1ade5653804b8eb ("thermal: rcar: check every rcar_thermal_update_temp() return value")

Niklas Söderlund (3):
  thermal: rcar_thermal: Always update thermal zone on interrupt
  thermal: rcar_thermal: Do not store ctemp in rcar_thermal_priv
  thermal: rcar_thermal: Remove lock in rcar_thermal_get_current_temp()

 drivers/thermal/rcar_thermal.c | 47 ++++++++++------------------------
 1 file changed, 13 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH 1/3] thermal: rcar_thermal: Always update thermal zone on interrupt

[Niklas Söderlund]

Since commit a1ade5653804b8eb ("thermal: rcar: check every
rcar_thermal_update_temp() return value") the temperature is always read
in rcar_thermal_get_current_temp() so comparing it before and after
enabling interrupts have little effect. Remove the check and always
update the thermal zone when we get an interrupt that the temperature
have changed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_thermal.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 8f1aafa2044e5ba7..f379eb5f8b9ecd14 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -387,28 +387,17 @@ static void _rcar_thermal_irq_ctrl(struct rcar_thermal_priv *priv, int enable)
 static void rcar_thermal_work(struct work_struct *work)
 {
 	struct rcar_thermal_priv *priv;
-	int cctemp, nctemp;
 	int ret;
 
 	priv = container_of(work, struct rcar_thermal_priv, work.work);
 
-	ret = rcar_thermal_get_current_temp(priv, &cctemp);
-	if (ret < 0)
-		return;
-
 	ret = rcar_thermal_update_temp(priv);
 	if (ret < 0)
 		return;
 
 	rcar_thermal_irq_enable(priv);
 
-	ret = rcar_thermal_get_current_temp(priv, &nctemp);
-	if (ret < 0)
-		return;
-
-	if (nctemp != cctemp)
-		thermal_zone_device_update(priv->zone,
-					   THERMAL_EVENT_UNSPECIFIED);
+	thermal_zone_device_update(priv->zone, THERMAL_EVENT_UNSPECIFIED);
 }
 
 static u32 rcar_thermal_had_changed(struct rcar_thermal_priv *priv, u32 status)
-- 
2.25.1

[PATCH 2/3] thermal: rcar_thermal: Do not store ctemp in rcar_thermal_priv

[Niklas Söderlund]

There is no need to cache the ctemp value in the private data structure
as it's always prefetched before it's used. Remove it from the structure
and have rcar_thermal_update_temp return the value instead of storing
it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_thermal.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index f379eb5f8b9ecd14..953dc28d1dd1d499 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -95,7 +95,6 @@ struct rcar_thermal_priv {
 	struct mutex lock;
 	struct list_head list;
 	int id;
-	u32 ctemp;
 };
 
 #define rcar_thermal_for_each_priv(pos, common)	\
@@ -201,7 +200,6 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
 	struct device *dev = rcar_priv_to_dev(priv);
 	int i;
 	u32 ctemp, old, new;
-	int ret = -EINVAL;
 
 	mutex_lock(&priv->lock);
 
@@ -247,32 +245,28 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
 						   ((ctemp - 1) << 0)));
 	}
 
-	dev_dbg(dev, "thermal%d  %d -> %d\n", priv->id, priv->ctemp, ctemp);
-
-	priv->ctemp = ctemp;
-	ret = 0;
 err_out_unlock:
 	mutex_unlock(&priv->lock);
-	return ret;
+
+	return ctemp ? ctemp : -EINVAL;
 }
 
 static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
 					 int *temp)
 {
-	int tmp;
-	int ret;
+	int ctemp, tmp;
 
-	ret = rcar_thermal_update_temp(priv);
-	if (ret < 0)
-		return ret;
+	ctemp = rcar_thermal_update_temp(priv);
+	if (ctemp < 0)
+		return ctemp;
 
 	mutex_lock(&priv->lock);
 	if (priv->chip->ctemp_bands == 1)
-		tmp = MCELSIUS((priv->ctemp * 5) - 65);
-	else if (priv->ctemp < 24)
-		tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
+		tmp = MCELSIUS((ctemp * 5) - 65);
+	else if (ctemp < 24)
+		tmp = MCELSIUS(((ctemp * 55) - 720) / 10);
 	else
-		tmp = MCELSIUS((priv->ctemp * 5) - 60);
+		tmp = MCELSIUS((ctemp * 5) - 60);
 	mutex_unlock(&priv->lock);
 
 	/* Guaranteed operating range is -45C to 125C. */
-- 
2.25.1

[PATCH 3/3] thermal: rcar_thermal: Remove lock in rcar_thermal_get_current_temp()

[Niklas Söderlund]

With the ctemp value returned instead of cached in the private data
structure their is no need to take the lock when translating ctemp into
a temperature.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_thermal.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 953dc28d1dd1d499..8bdcb449bd280a18 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -254,24 +254,20 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
 static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
 					 int *temp)
 {
-	int ctemp, tmp;
+	int ctemp;
 
 	ctemp = rcar_thermal_update_temp(priv);
 	if (ctemp < 0)
 		return ctemp;
 
-	mutex_lock(&priv->lock);
+	/* Guaranteed operating range is -45C to 125C. */
+
 	if (priv->chip->ctemp_bands == 1)
-		tmp = MCELSIUS((ctemp * 5) - 65);
+		*temp = MCELSIUS((ctemp * 5) - 65);
 	else if (ctemp < 24)
-		tmp = MCELSIUS(((ctemp * 55) - 720) / 10);
+		*temp = MCELSIUS(((ctemp * 55) - 720) / 10);
 	else
-		tmp = MCELSIUS((ctemp * 5) - 60);
-	mutex_unlock(&priv->lock);
-
-	/* Guaranteed operating range is -45C to 125C. */
-
-	*temp = tmp;
+		*temp = MCELSIUS((ctemp * 5) - 60);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 1/3] thermal: rcar_thermal: Always update thermal zone on interrupt

[Geert Uytterhoeven]

On Tue, Mar 10, 2020 at 6:02 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Since commit a1ade5653804b8eb ("thermal: rcar: check every
> rcar_thermal_update_temp() return value") the temperature is always read
> in rcar_thermal_get_current_temp() so comparing it before and after
> enabling interrupts have little effect. Remove the check and always
> update the thermal zone when we get an interrupt that the temperature
> have changed.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH 2/3] thermal: rcar_thermal: Do not store ctemp in rcar_thermal_priv

[Geert Uytterhoeven]

Hi Niklas,

On Tue, Mar 10, 2020 at 6:02 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> There is no need to cache the ctemp value in the private data structure
> as it's always prefetched before it's used. Remove it from the structure
> and have rcar_thermal_update_temp return the value instead of storing
> it.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

One suggestion below...

> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -201,7 +200,6 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>         struct device *dev = rcar_priv_to_dev(priv);
>         int i;
>         u32 ctemp, old, new;
> -       int ret = -EINVAL;

It might make sense to change ctemp to int, and preinitialize it to
-EINVAL...

>
>         mutex_lock(&priv->lock);
>
> @@ -247,32 +245,28 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>                                                    ((ctemp - 1) << 0)));
>         }
>
> -       dev_dbg(dev, "thermal%d  %d -> %d\n", priv->id, priv->ctemp, ctemp);
> -
> -       priv->ctemp = ctemp;
> -       ret = 0;
>  err_out_unlock:
>         mutex_unlock(&priv->lock);
> -       return ret;
> +
> +       return ctemp ? ctemp : -EINVAL;

... so you can just return ctemp here.

>  }

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

Re: [PATCH 3/3] thermal: rcar_thermal: Remove lock in rcar_thermal_get_current_temp()

[Geert Uytterhoeven]

On Tue, Mar 10, 2020 at 6:02 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> With the ctemp value returned instead of cached in the private data
> structure their is no need to take the lock when translating ctemp into
> a temperature.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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