Re: [PATCH v3] imx: thermal: imx_get_temp might be called before sensor clock is prepared

[Heiner Kallweit <heiner.kallweit@web.de>]

Am 07.11.2014 um 19:18 schrieb Eduardo Valentin:
> 
> Hello Heiner,
> 
> On Mon, Oct 13, 2014 at 10:51:23PM +0200, Heiner Kallweit wrote:
>> imx_get_temp might be called before the sensor clock is 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 <heiner.kallweit@web.de>
>> ---
>> v2: revised error path. Bail out and tidy up properly if we can't
>>     get the clock or fail to enable it
>> v3: don't print error message if getting clock returns EPROBE_DEFER
>> ---
>>  drivers/thermal/imx_thermal.c | 41 +++++++++++++++++++++++++----------------
>>  1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>> index 461bf3d..0e8ef55 100644
>> --- a/drivers/thermal/imx_thermal.c
>> +++ b/drivers/thermal/imx_thermal.c
>> @@ -521,6 +521,30 @@ static int imx_thermal_probe(struct platform_device *pdev)
>>                 return ret;
>>         }
>>
>> +       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
>> +       if (IS_ERR(data->thermal_clk)) {
>> +               ret = PTR_ERR(data->thermal_clk);
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(&pdev->dev,
>> +                               "failed to get thermal clk: %d\n", ret);
>> +               cpufreq_cooling_unregister(data->cdev);
>> +               return ret;
>> +       }
>> +
>> +       /*
>> +        * Thermal sensor needs clk on to get correct value, normally
>> +        * we should enable its clk before taking measurement and disable
>> +        * clk after measurement is done, but if alarm function is enabled,
>> +        * hardware will auto measure the temperature periodically, so we
>> +        * need to keep the clk always on for alarm function.
>> +        */
>> +       ret = clk_prepare_enable(data->thermal_clk);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>> +               cpufreq_cooling_unregister(data->cdev);
>> +               return ret;
>> +       }
>> +
>>         data->tz = thermal_zone_device_register("imx_thermal_zone",
>>                                                 IMX_TRIP_NUM,
>>                                                 BIT(IMX_TRIP_PASSIVE), data,
>> @@ -531,26 +555,11 @@ static int imx_thermal_probe(struct platform_device *pdev)
>>                 ret = PTR_ERR(data->tz);
>>                 dev_err(&pdev->dev,
>>                         "failed to register thermal zone device %d\n", ret);
>> +               clk_disable_unprepare(data->thermal_clk);
>>                 cpufreq_cooling_unregister(data->cdev);
>>                 return ret;
>>         }
>>
>> -       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
>> -       if (IS_ERR(data->thermal_clk)) {
>> -               dev_warn(&pdev->dev, "failed to get thermal clk!\n");
>> -       } else {
>> -               /*
>> -                * Thermal sensor needs clk on to get correct value, normally
>> -                * we should enable its clk before taking measurement and disable
>> -                * clk after measurement is done, but if alarm function is enabled,
>> -                * hardware will auto measure the temperature periodically, so we
>> -                * need to keep the clk always on for alarm function.
>> -                */
>> -               ret = clk_prepare_enable(data->thermal_clk);
>> -               if (ret)
>> -                       dev_warn(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>> -       }
>> -
>>         /* Enable measurements at ~ 10 Hz */
>>         regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
>>         measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
> 
> 
> While here, do you need to move up also the configuration of the
> measurements at ~ 10 Hz? Or would still the first readings be correct
> while at the reset value?
When imx_get_temp is called from thermal_zone_device_register data->mode still is
THERMAL_DEVICE_DISABLED. imx_get_temp therefore will power on the sensor and
trigger a single measurement.
IMHO this is fully ok and I don't think we have to relocate the setup of the
scheduled measurements.
> 
> Cheers,
> 
> Eduardo Valentin
>> --
>> 2.1.2
Rgds, Heiner