diff mbox series

[3/6] drivers/thermal/exynos: check IS_ERR(data->clk) consistently

Message ID 20240719120853.1924771-4-m.majewski2@samsung.com (mailing list archive)
State New
Delegated to: Daniel Lezcano
Headers show
Series [1/6] drivers/thermal/exynos: use DEFINE_SIMPLE_DEV_PM_OPS | expand

Commit Message

Mateusz Majewski July 19, 2024, 12:08 p.m. UTC
This will be needed for Exynos 850 support, which does not require this
clock.

It would also be possible to set data->clk to NULL instead, but doing it
like this is consistent with what we do with data->clk_sec.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 58 ++++++++++++++++++----------
 1 file changed, 38 insertions(+), 20 deletions(-)

Comments

Sam Protsenko July 22, 2024, 9:03 p.m. UTC | #1
On Fri, Jul 19, 2024 at 7:10 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> This will be needed for Exynos 850 support, which does not require this
> clock.
>
> It would also be possible to set data->clk to NULL instead, but doing it
> like this is consistent with what we do with data->clk_sec.
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 58 ++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 61606a9b9a00..f0de72a62fd7 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -257,7 +257,8 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>         int ret = 0;
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);

Hmm, it looks like it's better to rework this clock obtaining using
devm_clk_get_optional() API, or even devm_clk_get_optional_prepared()
in this case. It'll simplify its further usage, i.e. it'll be possible
to just call clk_enable() without checking the clock with IS_ERR()
each time. You can check these drivers for pointers on optional API
usage:

  - drivers/i2c/busses/i2c-exynos5.c
  - drivers/watchdog/s3c2410_wdt.c

Also, if it's only optional for Exynos850 (and not optional for other
chips), maybe it would be a good idea to use *_optional_* API only for
Exynos850 case, so that the driver's behavior for those chips stays
unchanged.

Btw, from the downstream kernel code [1] I can see that the only TMU
clock present in Exynos850 is
GOUT_BLK_PERI_UID_BUSIF_TMU_IPCLKPORT_PCLK (which I was able to
confirm in TRM). But it's not enabled in clk-exynos850.c driver right
now. Do you want me by chance to send the patch adding it?

[1] https://gitlab.com/Linaro/96boards/e850-96/kernel/-/blob/android-exynos-5.10-linaro/drivers/soc/samsung/cal-if/s5e3830/cmucal-node.c?ref_type=heads#L1196

>         if (!IS_ERR(data->clk_sec))
>                 clk_enable(data->clk_sec);
>
> @@ -271,7 +272,8 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>
>         if (!IS_ERR(data->clk_sec))
>                 clk_disable(data->clk_sec);
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return ret;
> @@ -295,11 +297,13 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
>         }
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         data->tmu_set_crit_temp(data, temp / MCELSIUS);
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return 0;
> @@ -328,10 +332,12 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
>         struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>         data->tmu_control(pdev, on);
>         data->enabled = on;
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>  }
>
> @@ -648,7 +654,8 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>                 return -EAGAIN;
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         value = data->tmu_read(data);
>         if (value < 0)
> @@ -656,7 +663,8 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>         else
>                 *temp = code_to_temp(data, value) * MCELSIUS;
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return ret;
> @@ -723,9 +731,11 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
>                 goto out;
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>         data->tmu_set_emulation(data, temp);
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>         return 0;
>  out:
> @@ -763,12 +773,14 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
>         thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         /* TODO: take action based on particular interrupt */
>         data->tmu_clear_irqs(data);
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return IRQ_HANDLED;
> @@ -979,7 +991,8 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>         struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         if (low > INT_MIN)
>                 data->tmu_set_low_temp(data, low / MCELSIUS);
> @@ -990,7 +1003,8 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>         else
>                 data->tmu_disable_high(data);
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return 0;
> @@ -1053,10 +1067,12 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>                 }
>         }
>
> -       ret = clk_prepare(data->clk);
> -       if (ret) {
> -               dev_err(dev, "Failed to get clock\n");
> -               goto err_clk_sec;
> +       if (!IS_ERR(data->clk)) {
> +               ret = clk_prepare(data->clk);
> +               if (ret) {
> +                       dev_err(dev, "Failed to get clock\n");
> +                       goto err_clk_sec;
> +               }
>         }
>
>         switch (data->soc) {
> @@ -1113,7 +1129,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>  err_sclk:
>         clk_disable_unprepare(data->sclk);
>  err_clk:
> -       clk_unprepare(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_unprepare(data->clk);
>  err_clk_sec:
>         if (!IS_ERR(data->clk_sec))
>                 clk_unprepare(data->clk_sec);
> @@ -1127,7 +1144,8 @@ static void exynos_tmu_remove(struct platform_device *pdev)
>         exynos_tmu_control(pdev, false);
>
>         clk_disable_unprepare(data->sclk);
> -       clk_unprepare(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_unprepare(data->clk);
>         if (!IS_ERR(data->clk_sec))
>                 clk_unprepare(data->clk_sec);
>  }
> --
> 2.45.1
>
>
Mateusz Majewski July 23, 2024, 2:17 p.m. UTC | #2
> Also, if it's only optional for Exynos850 (and not optional for other
> chips), maybe it would be a good idea to use *_optional_* API only for
> Exynos850 case, so that the driver's behavior for those chips stays
> unchanged.

Probably should just set the clock to NULL in case of 850 then?

> Btw, from the downstream kernel code [1] I can see that the only TMU
> clock present in Exynos850 is
> GOUT_BLK_PERI_UID_BUSIF_TMU_IPCLKPORT_PCLK (which I was able to
> confirm in TRM). But it's not enabled in clk-exynos850.c driver right
> now. Do you want me by chance to send the patch adding it?

Would be very grateful :) If nothing else, it would be useful for
testing.
Sam Protsenko July 23, 2024, 2:51 p.m. UTC | #3
On Tue, Jul 23, 2024 at 9:17 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> > Also, if it's only optional for Exynos850 (and not optional for other
> > chips), maybe it would be a good idea to use *_optional_* API only for
> > Exynos850 case, so that the driver's behavior for those chips stays
> > unchanged.
>
> Probably should just set the clock to NULL in case of 850 then?
>

Ah, you are right, there is not much sense in doing both. I guess the
canonical way to do that (please check the drivers I referenced) -- is
not to check the chip, but just run devm_clk_get_optional(), which
sets the clock to NULL in case it's missing in dts. Less code this
way. And while at it, maybe consider reducing the code even more by
using devm_clk_get_optional_prepared().

> > Btw, from the downstream kernel code [1] I can see that the only TMU
> > clock present in Exynos850 is
> > GOUT_BLK_PERI_UID_BUSIF_TMU_IPCLKPORT_PCLK (which I was able to
> > confirm in TRM). But it's not enabled in clk-exynos850.c driver right
> > now. Do you want me by chance to send the patch adding it?
>
> Would be very grateful :) If nothing else, it would be useful for
> testing.

Cool, will try to do that soon!
diff mbox series

Patch

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 61606a9b9a00..f0de72a62fd7 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -257,7 +257,8 @@  static int exynos_tmu_initialize(struct platform_device *pdev)
 	int ret = 0;
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 	if (!IS_ERR(data->clk_sec))
 		clk_enable(data->clk_sec);
 
@@ -271,7 +272,8 @@  static int exynos_tmu_initialize(struct platform_device *pdev)
 
 	if (!IS_ERR(data->clk_sec))
 		clk_disable(data->clk_sec);
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return ret;
@@ -295,11 +297,13 @@  static int exynos_thermal_zone_configure(struct platform_device *pdev)
 	}
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	data->tmu_set_crit_temp(data, temp / MCELSIUS);
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return 0;
@@ -328,10 +332,12 @@  static void exynos_tmu_control(struct platform_device *pdev, bool on)
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 	data->tmu_control(pdev, on);
 	data->enabled = on;
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 }
 
@@ -648,7 +654,8 @@  static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 		return -EAGAIN;
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	value = data->tmu_read(data);
 	if (value < 0)
@@ -656,7 +663,8 @@  static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 	else
 		*temp = code_to_temp(data, value) * MCELSIUS;
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return ret;
@@ -723,9 +731,11 @@  static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
 		goto out;
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 	data->tmu_set_emulation(data, temp);
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 	return 0;
 out:
@@ -763,12 +773,14 @@  static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
 	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	/* TODO: take action based on particular interrupt */
 	data->tmu_clear_irqs(data);
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return IRQ_HANDLED;
@@ -979,7 +991,8 @@  static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 	struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	if (low > INT_MIN)
 		data->tmu_set_low_temp(data, low / MCELSIUS);
@@ -990,7 +1003,8 @@  static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 	else
 		data->tmu_disable_high(data);
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return 0;
@@ -1053,10 +1067,12 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = clk_prepare(data->clk);
-	if (ret) {
-		dev_err(dev, "Failed to get clock\n");
-		goto err_clk_sec;
+	if (!IS_ERR(data->clk)) {
+		ret = clk_prepare(data->clk);
+		if (ret) {
+			dev_err(dev, "Failed to get clock\n");
+			goto err_clk_sec;
+		}
 	}
 
 	switch (data->soc) {
@@ -1113,7 +1129,8 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 err_sclk:
 	clk_disable_unprepare(data->sclk);
 err_clk:
-	clk_unprepare(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_unprepare(data->clk);
 err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
@@ -1127,7 +1144,8 @@  static void exynos_tmu_remove(struct platform_device *pdev)
 	exynos_tmu_control(pdev, false);
 
 	clk_disable_unprepare(data->sclk);
-	clk_unprepare(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_unprepare(data->clk);
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
 }