| 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 |
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 > >
> 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.
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 --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); }
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(-)