Re: [PATCH V3 20/21] thermal: exynos: Support for TMU regulator definedat device tree

[Eduardo Valentin]

On 09-05-2013 22:28, amit daniel kachhap wrote:
> Hi Eduardo,
> 
> On Thu, May 9, 2013 at 8:14 PM, Eduardo Valentin
> <eduardo.valentin@xxxxxx> wrote:
>> On 07-05-2013 09:01, Amit Daniel Kachhap wrote:
>>> TMU probe function now checks for a device tree defined regulator.
>>> For compatibility reasons it is allowed to probe driver even without
>>> this regulator defined.
>>>
>>> Signed-off-by: Lukasz Majewski <l.majewski@xxxxxxxxxxx>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx>
>>> ---
>>>  .../devicetree/bindings/thermal/exynos-thermal.txt |    4 ++++
>>>  drivers/thermal/samsung/exynos_tmu.c               |   19 +++++++++++++++++++
>>>  2 files changed, 23 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>> index 970eeba..ff62f7a 100644
>>> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>> @@ -14,6 +14,9 @@
>>>  - interrupts : Should contain interrupt for thermal system
>>>  - clocks : The main clock for TMU device
>>>  - clock-names : Thermal system clock name
>>> +- vtmu-supply: This entry is optional and provides the regulator node supplying
>>> +             voltage to TMU. If needed this entry can be placed inside
>>> +             board/platform specific dts file.
>>>
>>>  Example 1):
>>>
>>> @@ -25,6 +28,7 @@ Example 1):
>>>               clocks = <&clock 383>;
>>>               clock-names = "tmu_apbif";
>>>               status = "disabled";
>>> +             vtmu-supply = <&tmu_regulator_node>;
>>>       };
>>>
>>>  Example 2):
>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>> index 72446c9..b7c609a 100644
>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>> @@ -32,6 +32,7 @@
>>>  #include <linux/of_address.h>
>>>  #include <linux/of_irq.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/workqueue.h>
>>>  #include "exynos_thermal_common.h"
>>> @@ -52,6 +53,7 @@
>>>   * @clk: pointer to the clock structure.
>>>   * @temp_error1: fused value of the first point trim.
>>>   * @temp_error2: fused value of the second point trim.
>>> + * @regulator: pointer to the TMU regulator structure.
>>>   * @reg_conf: pointer to structure to register with core thermal.
>>>   */
>>>  struct exynos_tmu_data {
>>> @@ -65,6 +67,7 @@ struct exynos_tmu_data {
>>>       struct mutex lock;
>>>       struct clk *clk;
>>>       u8 temp_error1, temp_error2;
>>> +     struct regulator *regulator;
>>>       struct thermal_sensor_conf *reg_conf;
>>>  };
>>>
>>> @@ -501,10 +504,23 @@ static int exynos_map_dt_data(struct platform_device *pdev)
>>>       struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>>       struct exynos_tmu_platform_data *pdata = data->pdata;
>>>       struct resource res;
>>> +     int ret;
>>>
>>>       if (!data)
>>>               return -ENODEV;
>>>
>>> +     /* Try enabling the regulator if found */
>>> +     data->regulator = devm_regulator_get(&pdev->dev, "vtmu");
>>> +     if (!IS_ERR(data->regulator)) {
>>> +             ret = regulator_enable(data->regulator);
>>> +             if (ret) {
>>> +                     dev_err(&pdev->dev, "failed to enable vtmu\n");
>>> +                     return ret;
>>> +             }
>>> +     } else {
>>> +             dev_info(&pdev->dev, "Regulator node (vtmu) not found\n");
>>
>> Now that you have a bitfield for your features, shouldnt this become a
>> check? If the SoC requires the regulator, then it has to return a valid
>> regulator (regulator_get). Meaning, if your SoC version requires this
>> feature and the regulator_get returns an error, you must treat as an
>> error an not continue gracefuly.
> 
> Earlier I also thought of using bit feature for this but then the
> regulator source usually depends upon the board design so each soc may
> have several boards. So regulator information is not part of SOC data.
> Since this information is there is in DT only so I left this part for
> the DT to handle.
> 


Hmmm.. well, that is actually arguable. Take from driver perspective. If
a regulator is required for a device to work you have to make it a
requirement and not rely on whatever state the system has booted.

From previous discussions, I understood on of your chip versions
actually require a regulator to be activated in order to get the sensors
properly working. Is this understanding correct?

> Thanks,
> Amit Daniel
>>
>>> +     }
>>> +
>>>       data->id = of_alias_get_id(pdev->dev.of_node, "tmuctrl");
>>>       if (data->id < 0)
>>>               data->id = 0;
>>> @@ -669,6 +685,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
>>>
>>>       clk_unprepare(data->clk);
>>>
>>> +     if (!IS_ERR(data->regulator))
>>> +             regulator_disable(data->regulator);
>>> +
>>>       platform_set_drvdata(pdev, NULL);
>>>
>>>       return 0;
>>>
>>
>>
> 
>

Attachment: signature.asc
Description: OpenPGP digital signature