imx: thermal: imx_get_temp might be called before the sensor clock ist prepared

imx_get_temp might be called before the sensor clock ist prepared thus resulting in a timeout of the first attempt to read temp:
thermal thermal_zone0: failed to read out thermal zone 0 Happened to me on a Utilite Standard with IMX6 Dual SoC.

Reason is that in imx_thermal_probe thermal_zone_device_register is called before the sensor clock is prepared. thermal_zone_device_register however calls thermal_zone_device_update which eventually calls imx_get_temp.

Fix this by preparing the clock before calling
thermal_zone_device_register.

Signed-off-by Heiner Kallweit <[email protected]>

drivers/thermal/imx_thermal.c +0 -0

@@ -551,6 +537,20 @@ static int imx_thermal_probe(struct platform_device *pdev)
                        dev_warn(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
        }
 

drivers/thermal/imx_thermal.c +4 -1

+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
        /* Enable measurements at ~ 10 Hz */
        regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
        measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
--

2.1.2

Hi Heiner,

Am Montag, den 06.10.2014, 20:37 +0200 schrieb Heiner Kallweit:

Quoted 10 lines

imx_get_temp might be called before the sensor clock ist prepared thus resulting in a timeout of the first attempt to read temp:
thermal thermal_zone0: failed to read out thermal zone 0 Happened to me on a Utilite Standard with IMX6 Dual SoC.

Reason is that in imx_thermal_probe thermal_zone_device_register is called before the sensor clock is prepared. thermal_zone_device_register however calls thermal_zone_device_update which eventually calls imx_get_temp.

Quoted 4 lines

Fix this by preparing the clock before calling
thermal_zone_device_register.

Signed-off-by Heiner Kallweit <[email protected]>

Looks good to me, just an issue with the error path - see below.

[...]

Quoted 4 lines

drivers/thermal/imx_thermal.c +0 -0

@@ -551,6 +537,20 @@ static int imx_thermal_probe(struct platform_device *pdev)
                        dev_warn(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
        }
 

drivers/thermal/imx_thermal.c static int imx_thermal_probe(struct platform_device *pdev) · line 549

Quoted 10 lines

drivers/thermal/imx_thermal.c +10 -0

+       data->tz = thermal_zone_device_register("imx_thermal_zone",
+                                               IMX_TRIP_NUM,
+                                               BIT(IMX_TRIP_PASSIVE), data,
+                                               &imx_tz_ops, NULL,
+                                               IMX_PASSIVE_DELAY,
+                                               IMX_POLLING_DELAY);
+       if (IS_ERR(data->tz)) {
+               ret = PTR_ERR(data->tz);
+               dev_err(&pdev->dev,
+                       "failed to register thermal zone device %d\n", ret);

If the thermal_clk was enabled before, clk_disable_unprepare() should be called on it here.

Quoted 9 lines

drivers/thermal/imx_thermal.c +4 -1

+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
        /* Enable measurements at ~ 10 Hz */
        regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
        measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
--

2.1.2

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Thanks for the hint, Philipp.
After thinking a little bit more about it I'm not sure about one thing:

If we can't get the clock or fail to enable it: Currently the code just throws a warning and proceeds with trying to register the thermal device.
Does this make sense or shall we bail out instead (like in case of other error conditions in imx_thermal_probe)? I'd tend to bail out ..

Hi Heiner,

Quoted 7 lines · Heiner Kallweit

Thanks for the hint, Philipp.
After thinking a little bit more about it I'm not sure about one thing:

If we can't get the clock or fail to enable it: Currently the code just throws a warning and proceeds with trying to register the thermal device.
Does this make sense or shall we bail out instead (like in case of other error conditions in imx_thermal_probe)? I'd tend to bail out ..

Agree with you. We should do a 'return ret' if clk_prepare_enable() fails. Thanks