[PATCH 0/2] Add power control for lm90

[PATCH 0/2] Add power control for lm90

[Wei Ni]

The device lm90 can be controlled by the vdd rail.
Add function to power on/off the vdd.
Enable the nct1008 on Tegra114 Dalmore board, and set the vdd-regulator.

Wei Ni (2):
  hwmon: (lm90) Add power control
  ARM: dt: t114 dalmore: add dt entry for nct1008

 arch/arm/boot/dts/tegra114-dalmore.dts |   10 +++++-
 drivers/hwmon/lm90.c                   |   52 ++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH 1/2] hwmon: (lm90) Add power control

[Wei Ni]

The device lm90 can be controlled by the vdd rail.
Adding the power control support to power on/off the vdd rail.
And make sure that power is enabled before accessing the device.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index cdff742..eeb0115 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -89,6 +89,8 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 /*
  * Addresses to scan
@@ -179,6 +181,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
 #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
 
+#define POWER_ON_DELAY 20 /*ms*/
+
 /*
  * Driver data (common to all clients)
  */
@@ -302,6 +306,7 @@ static const struct lm90_params lm90_params[] = {
 struct lm90_data {
 	struct device *hwmon_dev;
 	struct mutex update_lock;
+	struct regulator *lm90_reg;
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
 	int kind;
@@ -1391,6 +1396,48 @@ static void lm90_init_client(struct i2c_client *client)
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
 }
 
+static int lm90_power_control(struct i2c_client *client, bool is_enable)
+{
+	struct lm90_data *data = i2c_get_clientdata(client);
+	int ret;
+
+	mutex_lock(&data->update_lock);
+
+	if (!data->lm90_reg) {
+		data->lm90_reg = regulator_get(&client->dev, "vdd");
+		if (IS_ERR_OR_NULL(data->lm90_reg)) {
+			if (PTR_ERR(data->lm90_reg) == -ENODEV)
+				dev_info(&client->dev,
+					 "No regulator found for vdd. Assuming vdd is always powered.");
+			else
+				dev_warn(&client->dev,
+					 "Error [%ld] in getting the regulator handle for vdd.\n",
+					 PTR_ERR(data->lm90_reg));
+			data->lm90_reg = NULL;
+			mutex_unlock(&data->update_lock);
+			return -ENODEV;
+		}
+	}
+	if (is_enable) {
+		ret = regulator_enable(data->lm90_reg);
+		msleep(POWER_ON_DELAY);
+	} else {
+		ret = regulator_disable(data->lm90_reg);
+	}
+
+	if (ret < 0)
+		dev_err(&client->dev,
+			"Error in %s rail vdd, error %d\n",
+			(is_enable) ? "enabling" : "disabling", ret);
+	else
+		dev_info(&client->dev, "success in %s rail vdd\n",
+			 (is_enable) ? "enabling" : "disabling");
+
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1406,6 +1453,10 @@ static int lm90_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 
+	err = lm90_power_control(client, true);
+	if (err < 0)
+		return err;
+
 	/* Set the device type */
 	data->kind = id->driver_data;
 	if (data->kind == adm1032) {
@@ -1483,6 +1534,7 @@ static int lm90_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	lm90_remove_files(client, data);
 	lm90_restore_conf(client, data);
+	lm90_power_control(client, false);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH 1/2] hwmon: (lm90) Add power control

[Guenter Roeck]

On 08/06/2013 11:52 PM, Wei Ni wrote:
> The device lm90 can be controlled by the vdd rail.
> Adding the power control support to power on/off the vdd rail.
> And make sure that power is enabled before accessing the device.
>
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index cdff742..eeb0115 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,8 @@
>   #include <linux/err.h>
>   #include <linux/mutex.h>
>   #include <linux/sysfs.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
>
>   /*
>    * Addresses to scan
> @@ -179,6 +181,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>   #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
>   #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>
> +#define POWER_ON_DELAY 20 /*ms*/
> +
>   /*
>    * Driver data (common to all clients)
>    */
> @@ -302,6 +306,7 @@ static const struct lm90_params lm90_params[] = {
>   struct lm90_data {
>   	struct device *hwmon_dev;
>   	struct mutex update_lock;
> +	struct regulator *lm90_reg;
>   	char valid; /* zero until following fields are valid */
>   	unsigned long last_updated; /* in jiffies */
>   	int kind;
> @@ -1391,6 +1396,48 @@ static void lm90_init_client(struct i2c_client *client)
>   		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>   }
>
> +static int lm90_power_control(struct i2c_client *client, bool is_enable)
> +{
> +	struct lm90_data *data = i2c_get_clientdata(client);
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (!data->lm90_reg) {
> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
> +				dev_info(&client->dev,
> +					 "No regulator found for vdd. Assuming vdd is always powered.");
> +			else
> +				dev_warn(&client->dev,
> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
> +					 PTR_ERR(data->lm90_reg));
> +			data->lm90_reg = NULL;
> +			mutex_unlock(&data->update_lock);
> +			return -ENODEV;

I don't think it is acceptable to have the driver fail on pretty much all PCs.

Also, I dislike that - even if the calling code doesn't fail - the above message would be displayed on unload as well.

In general, the 'unload' flag seems unnecessary. You could just call

	if (data->lm90_reg)
		regulator_disable();

in the remove function. In addition to that, shouldn't you call regulator_put() on exit ?
Also, I am missing error handling in the probe function; if something else fails,
the regulator is neither disabled nor released.

Guenter

> +		}
> +	}
> +	if (is_enable) {
> +		ret = regulator_enable(data->lm90_reg);
> +		msleep(POWER_ON_DELAY);
> +	} else {
> +		ret = regulator_disable(data->lm90_reg);
> +	}
> +
> +	if (ret < 0)
> +		dev_err(&client->dev,
> +			"Error in %s rail vdd, error %d\n",
> +			(is_enable) ? "enabling" : "disabling", ret);
> +	else
> +		dev_info(&client->dev, "success in %s rail vdd\n",
> +			 (is_enable) ? "enabling" : "disabling");
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
>   static int lm90_probe(struct i2c_client *client,
>   		      const struct i2c_device_id *id)
>   {
> @@ -1406,6 +1453,10 @@ static int lm90_probe(struct i2c_client *client,
>   	i2c_set_clientdata(client, data);
>   	mutex_init(&data->update_lock);
>
> +	err = lm90_power_control(client, true);
> +	if (err < 0)
> +		return err;
> +
>   	/* Set the device type */
>   	data->kind = id->driver_data;
>   	if (data->kind == adm1032) {
> @@ -1483,6 +1534,7 @@ static int lm90_remove(struct i2c_client *client)
>   	hwmon_device_unregister(data->hwmon_dev);
>   	lm90_remove_files(client, data);
>   	lm90_restore_conf(client, data);
> +	lm90_power_control(client, false);
>
>   	return 0;
>   }
>

[PATCH 1/2] hwmon: (lm90) Add power control

[Wei Ni]

On 08/07/2013 03:03 PM, Guenter Roeck wrote:
> On 08/06/2013 11:52 PM, Wei Ni wrote:
>> The device lm90 can be controlled by the vdd rail.
>> Adding the power control support to power on/off the vdd rail.
>> And make sure that power is enabled before accessing the device.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index cdff742..eeb0115 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -89,6 +89,8 @@
>>   #include <linux/err.h>
>>   #include <linux/mutex.h>
>>   #include <linux/sysfs.h>
>> +#include <linux/delay.h>
>> +#include <linux/regulator/consumer.h>
>>
>>   /*
>>    * Addresses to scan
>> @@ -179,6 +181,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>>   #define LM90_HAVE_TEMP3		(1 << 6) /* 3rd temperature sensor	*/
>>   #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>>
>> +#define POWER_ON_DELAY 20 /*ms*/
>> +
>>   /*
>>    * Driver data (common to all clients)
>>    */
>> @@ -302,6 +306,7 @@ static const struct lm90_params lm90_params[] = {
>>   struct lm90_data {
>>   	struct device *hwmon_dev;
>>   	struct mutex update_lock;
>> +	struct regulator *lm90_reg;
>>   	char valid; /* zero until following fields are valid */
>>   	unsigned long last_updated; /* in jiffies */
>>   	int kind;
>> @@ -1391,6 +1396,48 @@ static void lm90_init_client(struct i2c_client *client)
>>   		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>   }
>>
>> +static int lm90_power_control(struct i2c_client *client, bool is_enable)
>> +{
>> +	struct lm90_data *data = i2c_get_clientdata(client);
>> +	int ret;
>> +
>> +	mutex_lock(&data->update_lock);
>> +
>> +	if (!data->lm90_reg) {
>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
>> +				dev_info(&client->dev,
>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
>> +			else
>> +				dev_warn(&client->dev,
>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
>> +					 PTR_ERR(data->lm90_reg));
>> +			data->lm90_reg = NULL;
>> +			mutex_unlock(&data->update_lock);
>> +			return -ENODEV;
> 
> I don't think it is acceptable to have the driver fail on pretty much all PCs.

Yes, you are right, I didn't consider it carefully, I will fix it.
I think it's better to move these codes to the probe() directly.

> 
> Also, I dislike that - even if the calling code doesn't fail - the above message would be displayed on unload as well.
> 
> In general, the 'unload' flag seems unnecessary. You could just call
> 
> 	if (data->lm90_reg)
> 		regulator_disable();
> 
> in the remove function. In addition to that, shouldn't you call regulator_put() on exit ?

Oh, sorry, I miss the regulator_put(), I will fix it.

> Also, I am missing error handling in the probe function; if something else fails,
> the regulator is neither disabled nor released.

It looks this patch have many problems, I will fix them.
Thanks for your comments.

> 
> Guenter
> 
>> +		}
>> +	}
>> +	if (is_enable) {
>> +		ret = regulator_enable(data->lm90_reg);
>> +		msleep(POWER_ON_DELAY);
>> +	} else {
>> +		ret = regulator_disable(data->lm90_reg);
>> +	}
>> +
>> +	if (ret < 0)
>> +		dev_err(&client->dev,
>> +			"Error in %s rail vdd, error %d\n",
>> +			(is_enable) ? "enabling" : "disabling", ret);
>> +	else
>> +		dev_info(&client->dev, "success in %s rail vdd\n",
>> +			 (is_enable) ? "enabling" : "disabling");
>> +
>> +	mutex_unlock(&data->update_lock);
>> +
>> +	return ret;
>> +}
>> +
>>   static int lm90_probe(struct i2c_client *client,
>>   		      const struct i2c_device_id *id)
>>   {
>> @@ -1406,6 +1453,10 @@ static int lm90_probe(struct i2c_client *client,
>>   	i2c_set_clientdata(client, data);
>>   	mutex_init(&data->update_lock);
>>
>> +	err = lm90_power_control(client, true);
>> +	if (err < 0)
>> +		return err;
>> +
>>   	/* Set the device type */
>>   	data->kind = id->driver_data;
>>   	if (data->kind == adm1032) {
>> @@ -1483,6 +1534,7 @@ static int lm90_remove(struct i2c_client *client)
>>   	hwmon_device_unregister(data->hwmon_dev);
>>   	lm90_remove_files(client, data);
>>   	lm90_restore_conf(client, data);
>> +	lm90_power_control(client, false);
>>
>>   	return 0;
>>   }
>>
> 

Re: [PATCH 1/2] hwmon: (lm90) Add power control

[Alexander Shiyan]

> The device lm90 can be controlled by the vdd rail.
> Adding the power control support to power on/off the vdd rail.
> And make sure that power is enabled before accessing the device.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
[...]
> +	if (!data->lm90_reg) {
> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
> +				dev_info(&client->dev,
> +					 "No regulator found for vdd. Assuming vdd is always powered.");
> +			else
> +				dev_warn(&client->dev,
> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
> +					 PTR_ERR(data->lm90_reg));
> +			data->lm90_reg = NULL;
> +			mutex_unlock(&data->update_lock);
> +			return -ENODEV;
> +		}
> +	}
> +	if (is_enable) {
> +		ret = regulator_enable(data->lm90_reg);
> +		msleep(POWER_ON_DELAY);

Can this delay be handled directly from regulator?

[...]
---

[PATCH 1/2] hwmon: (lm90) Add power control

[Wei Ni]

On 08/07/2013 03:27 PM, Alexander Shiyan wrote:
>> The device lm90 can be controlled by the vdd rail.
>> Adding the power control support to power on/off the vdd rail.
>> And make sure that power is enabled before accessing the device.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
> [...]
>> +	if (!data->lm90_reg) {
>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
>> +				dev_info(&client->dev,
>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
>> +			else
>> +				dev_warn(&client->dev,
>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
>> +					 PTR_ERR(data->lm90_reg));
>> +			data->lm90_reg = NULL;
>> +			mutex_unlock(&data->update_lock);
>> +			return -ENODEV;
>> +		}
>> +	}
>> +	if (is_enable) {
>> +		ret = regulator_enable(data->lm90_reg);
>> +		msleep(POWER_ON_DELAY);
> 
> Can this delay be handled directly from regulator?

I think it should be handled in the device driver.
Because there have different delay time to wait devices stable.

> 
> [...]
> ---
> 

[PATCH 1/2] hwmon: (lm90) Add power control

[Guenter Roeck]

On 08/07/2013 12:32 AM, Wei Ni wrote:
> On 08/07/2013 03:27 PM, Alexander Shiyan wrote:
>>> The device lm90 can be controlled by the vdd rail.
>>> Adding the power control support to power on/off the vdd rail.
>>> And make sure that power is enabled before accessing the device.
>>>
>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>> ---
>>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> [...]
>>> +	if (!data->lm90_reg) {
>>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
>>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
>>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
>>> +				dev_info(&client->dev,
>>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
>>> +			else
>>> +				dev_warn(&client->dev,
>>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
>>> +					 PTR_ERR(data->lm90_reg));
>>> +			data->lm90_reg = NULL;
>>> +			mutex_unlock(&data->update_lock);
>>> +			return -ENODEV;
>>> +		}
>>> +	}
>>> +	if (is_enable) {
>>> +		ret = regulator_enable(data->lm90_reg);
>>> +		msleep(POWER_ON_DELAY);
>>
>> Can this delay be handled directly from regulator?
>
> I think it should be handled in the device driver.
> Because there have different delay time to wait devices stable.
>

Then why does no other caller of regulator_enable() need this ?
I don't think lm90 is so much different to other users of regulator
functionality.

Besides that, your delay is unconditional, even for static regulators
which are always on. Other callers of regulator_enable() don't need
all that complexity, which I take as sign that it is not needed here either.

Guenter

[PATCH 1/2] hwmon: (lm90) Add power control

[Wei Ni]

On 08/07/2013 03:50 PM, Guenter Roeck wrote:
> On 08/07/2013 12:32 AM, Wei Ni wrote:
>> On 08/07/2013 03:27 PM, Alexander Shiyan wrote:
>>>> The device lm90 can be controlled by the vdd rail.
>>>> Adding the power control support to power on/off the vdd rail.
>>>> And make sure that power is enabled before accessing the device.
>>>>
>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>> ---
>>>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> [...]
>>>> +	if (!data->lm90_reg) {
>>>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
>>>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
>>>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
>>>> +				dev_info(&client->dev,
>>>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
>>>> +			else
>>>> +				dev_warn(&client->dev,
>>>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
>>>> +					 PTR_ERR(data->lm90_reg));
>>>> +			data->lm90_reg = NULL;
>>>> +			mutex_unlock(&data->update_lock);
>>>> +			return -ENODEV;
>>>> +		}
>>>> +	}
>>>> +	if (is_enable) {
>>>> +		ret = regulator_enable(data->lm90_reg);
>>>> +		msleep(POWER_ON_DELAY);
>>>
>>> Can this delay be handled directly from regulator?
>>
>> I think it should be handled in the device driver.
>> Because there have different delay time to wait devices stable.
>>
> 
> Then why does no other caller of regulator_enable() need this ?
> I don't think lm90 is so much different to other users of regulator
> functionality.

May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock
Low Time" is 25ms, so I supposed that it may need about 20ms to stable
after power on.

Anyway, if I remove this delay, the driver also works fine, so I will
remove it in my next patch.

Thanks.
Wei.

> 
> Besides that, your delay is unconditional, even for static regulators
> which are always on. Other callers of regulator_enable() don't need
> all that complexity, which I take as sign that it is not needed here either.
> 
> Guenter
> 

Re: [PATCH 1/2] hwmon: (lm90) Add power control

[Alexander Shiyan]

> On 08/07/2013 03:50 PM, Guenter Roeck wrote:
> > On 08/07/2013 12:32 AM, Wei Ni wrote:
> >> On 08/07/2013 03:27 PM, Alexander Shiyan wrote:
> >>>> The device lm90 can be controlled by the vdd rail.
> >>>> Adding the power control support to power on/off the vdd rail.
> >>>> And make sure that power is enabled before accessing the device.
> >>>>
> >>>> Signed-off-by: Wei Ni <wni@nvidia.com>
> >>>> ---
> >>>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> [...]
> >>>> +	if (!data->lm90_reg) {
> >>>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
> >>>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
> >>>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
> >>>> +				dev_info(&client->dev,
> >>>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
> >>>> +			else
> >>>> +				dev_warn(&client->dev,
> >>>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
> >>>> +					 PTR_ERR(data->lm90_reg));
> >>>> +			data->lm90_reg = NULL;
> >>>> +			mutex_unlock(&data->update_lock);
> >>>> +			return -ENODEV;
> >>>> +		}
> >>>> +	}
> >>>> +	if (is_enable) {
> >>>> +		ret = regulator_enable(data->lm90_reg);
> >>>> +		msleep(POWER_ON_DELAY);
> >>>
> >>> Can this delay be handled directly from regulator?
> >>
> >> I think it should be handled in the device driver.
> >> Because there have different delay time to wait devices stable.
> >>
> > 
> > Then why does no other caller of regulator_enable() need this ?
> > I don't think lm90 is so much different to other users of regulator
> > functionality.
> 
> May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock
> Low Time" is 25ms, so I supposed that it may need about 20ms to stable
> after power on.
> 
> Anyway, if I remove this delay, the driver also works fine, so I will
> remove it in my next patch.

I originally had in mind that regulator API contain own delay option.
E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property.

---

[PATCH 1/2] hwmon: (lm90) Add power control

[Wei Ni]

On 08/07/2013 04:45 PM, Alexander Shiyan wrote:
>> On 08/07/2013 03:50 PM, Guenter Roeck wrote:
>>> On 08/07/2013 12:32 AM, Wei Ni wrote:
>>>> On 08/07/2013 03:27 PM, Alexander Shiyan wrote:
>>>>>> The device lm90 can be controlled by the vdd rail.
>>>>>> Adding the power control support to power on/off the vdd rail.
>>>>>> And make sure that power is enabled before accessing the device.
>>>>>>
>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>> ---
>>>>>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> [...]
>>>>>> +	if (!data->lm90_reg) {
>>>>>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
>>>>>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
>>>>>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
>>>>>> +				dev_info(&client->dev,
>>>>>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
>>>>>> +			else
>>>>>> +				dev_warn(&client->dev,
>>>>>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
>>>>>> +					 PTR_ERR(data->lm90_reg));
>>>>>> +			data->lm90_reg = NULL;
>>>>>> +			mutex_unlock(&data->update_lock);
>>>>>> +			return -ENODEV;
>>>>>> +		}
>>>>>> +	}
>>>>>> +	if (is_enable) {
>>>>>> +		ret = regulator_enable(data->lm90_reg);
>>>>>> +		msleep(POWER_ON_DELAY);
>>>>>
>>>>> Can this delay be handled directly from regulator?
>>>>
>>>> I think it should be handled in the device driver.
>>>> Because there have different delay time to wait devices stable.
>>>>
>>>
>>> Then why does no other caller of regulator_enable() need this ?
>>> I don't think lm90 is so much different to other users of regulator
>>> functionality.
>>
>> May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock
>> Low Time" is 25ms, so I supposed that it may need about 20ms to stable
>> after power on.
>>
>> Anyway, if I remove this delay, the driver also works fine, so I will
>> remove it in my next patch.
> 
> I originally had in mind that regulator API contain own delay option.
> E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property.

As I know the "startup-delay-us" is used for the regulator device, not
the consumer devices.
In this patch, msleep(POWER_ON_DELAY) was used to wait the lm90 stable,
but it seems it's unnecessary now :)

Wei.

> 
> ---
> 

[PATCH 1/2] hwmon: (lm90) Add power control

[Stephen Warren]

On 08/07/2013 03:35 AM, Wei Ni wrote:
> On 08/07/2013 04:45 PM, Alexander Shiyan wrote:
>>> On 08/07/2013 03:50 PM, Guenter Roeck wrote:
>>>> On 08/07/2013 12:32 AM, Wei Ni wrote:
>>>>> On 08/07/2013 03:27 PM, Alexander Shiyan wrote:
>>>>>>> The device lm90 can be controlled by the vdd rail.
>>>>>>> Adding the power control support to power on/off the vdd rail.
>>>>>>> And make sure that power is enabled before accessing the device.
>>>>>>>
>>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>>> ---
>>>>>>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> [...]
>>>>>>> +	if (!data->lm90_reg) {
>>>>>>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
>>>>>>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
>>>>>>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
>>>>>>> +				dev_info(&client->dev,
>>>>>>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
>>>>>>> +			else
>>>>>>> +				dev_warn(&client->dev,
>>>>>>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
>>>>>>> +					 PTR_ERR(data->lm90_reg));
>>>>>>> +			data->lm90_reg = NULL;
>>>>>>> +			mutex_unlock(&data->update_lock);
>>>>>>> +			return -ENODEV;
>>>>>>> +		}
>>>>>>> +	}
>>>>>>> +	if (is_enable) {
>>>>>>> +		ret = regulator_enable(data->lm90_reg);
>>>>>>> +		msleep(POWER_ON_DELAY);
>>>>>>
>>>>>> Can this delay be handled directly from regulator?
>>>>>
>>>>> I think it should be handled in the device driver.
>>>>> Because there have different delay time to wait devices stable.
>>>>>
>>>>
>>>> Then why does no other caller of regulator_enable() need this ?
>>>> I don't think lm90 is so much different to other users of regulator
>>>> functionality.
>>>
>>> May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock
>>> Low Time" is 25ms, so I supposed that it may need about 20ms to stable
>>> after power on.
>>>
>>> Anyway, if I remove this delay, the driver also works fine, so I will
>>> remove it in my next patch.
>>
>> I originally had in mind that regulator API contain own delay option.
>> E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property.
> 
> As I know the "startup-delay-us" is used for the regulator device, not
> the consumer devices.

Yes, the regulator should encoded its own startup delay. Each individual
device should handle its own requirements for delay after power is stable.

> In this patch, msleep(POWER_ON_DELAY) was used to wait the lm90 stable,
> but it seems it's unnecessary now :)

No, the driver needs to handle this properly. If the datasheet says a
delay is needed, it is.

It's probably working because in your tests the supply just happens to
be on already.

[PATCH 1/2] hwmon: (lm90) Add power control

[Guenter Roeck]

On Wed, Aug 07, 2013 at 10:06:30AM -0600, Stephen Warren wrote:
> On 08/07/2013 03:35 AM, Wei Ni wrote:
> > On 08/07/2013 04:45 PM, Alexander Shiyan wrote:
> >>> On 08/07/2013 03:50 PM, Guenter Roeck wrote:
> >>>> On 08/07/2013 12:32 AM, Wei Ni wrote:
> >>>>> On 08/07/2013 03:27 PM, Alexander Shiyan wrote:
> >>>>>>> The device lm90 can be controlled by the vdd rail.
> >>>>>>> Adding the power control support to power on/off the vdd rail.
> >>>>>>> And make sure that power is enabled before accessing the device.
> >>>>>>>
> >>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
> >>>>>>> ---
> >>>>>>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>> [...]
> >>>>>>> +	if (!data->lm90_reg) {
> >>>>>>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
> >>>>>>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
> >>>>>>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
> >>>>>>> +				dev_info(&client->dev,
> >>>>>>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
> >>>>>>> +			else
> >>>>>>> +				dev_warn(&client->dev,
> >>>>>>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
> >>>>>>> +					 PTR_ERR(data->lm90_reg));
> >>>>>>> +			data->lm90_reg = NULL;
> >>>>>>> +			mutex_unlock(&data->update_lock);
> >>>>>>> +			return -ENODEV;
> >>>>>>> +		}
> >>>>>>> +	}
> >>>>>>> +	if (is_enable) {
> >>>>>>> +		ret = regulator_enable(data->lm90_reg);
> >>>>>>> +		msleep(POWER_ON_DELAY);
> >>>>>>
> >>>>>> Can this delay be handled directly from regulator?
> >>>>>
> >>>>> I think it should be handled in the device driver.
> >>>>> Because there have different delay time to wait devices stable.
> >>>>>
> >>>>
> >>>> Then why does no other caller of regulator_enable() need this ?
> >>>> I don't think lm90 is so much different to other users of regulator
> >>>> functionality.
> >>>
> >>> May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock
> >>> Low Time" is 25ms, so I supposed that it may need about 20ms to stable
> >>> after power on.
> >>>
> >>> Anyway, if I remove this delay, the driver also works fine, so I will
> >>> remove it in my next patch.
> >>
> >> I originally had in mind that regulator API contain own delay option.
> >> E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property.
> > 
> > As I know the "startup-delay-us" is used for the regulator device, not
> > the consumer devices.
> 
> Yes, the regulator should encoded its own startup delay. Each individual
> device should handle its own requirements for delay after power is stable.
> 
> > In this patch, msleep(POWER_ON_DELAY) was used to wait the lm90 stable,
> > but it seems it's unnecessary now :)
> 
> No, the driver needs to handle this properly. If the datasheet says a
> delay is needed, it is.
> 
Yes but, if at all, only if it is known that the supply has just been turned on.
Imposing the delay on every user of the driver is unacceptable.

Guenter

[PATCH 1/2] hwmon: (lm90) Add power control

[Guenter Roeck]

On 08/07/2013 12:27 AM, Alexander Shiyan wrote:
>> The device lm90 can be controlled by the vdd rail.
>> Adding the power control support to power on/off the vdd rail.
>> And make sure that power is enabled before accessing the device.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>   drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
> [...]
>> +	if (!data->lm90_reg) {
>> +		data->lm90_reg = regulator_get(&client->dev, "vdd");
>> +		if (IS_ERR_OR_NULL(data->lm90_reg)) {
>> +			if (PTR_ERR(data->lm90_reg) == -ENODEV)
>> +				dev_info(&client->dev,
>> +					 "No regulator found for vdd. Assuming vdd is always powered.");
>> +			else
>> +				dev_warn(&client->dev,
>> +					 "Error [%ld] in getting the regulator handle for vdd.\n",
>> +					 PTR_ERR(data->lm90_reg));
>> +			data->lm90_reg = NULL;
>> +			mutex_unlock(&data->update_lock);
>> +			return -ENODEV;
>> +		}
>> +	}
>> +	if (is_enable) {
>> +		ret = regulator_enable(data->lm90_reg);
>> +		msleep(POWER_ON_DELAY);
>
> Can this delay be handled directly from regulator?
>

Good question. Browsing through other regulator_enable calls,
I don't see a similar delay anywhere else, so one should assume
that this is not necessary.

Guenter

[PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008

[Wei Ni]

Enable thermal sensor nct1008 for t114 dalmore.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 arch/arm/boot/dts/tegra114-dalmore.dts |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
index b5a42f0..9d4d2b2 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -738,6 +738,14 @@
 			realtek,ldo1-en-gpios =
 				<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
 		};
+
+		nct1008 {
+			compatible = "onnn,nct1008";
+			reg = <0x4c>;
+			vdd-supply = <&palmas_ldo6_reg>;
+			interrupt-parent = <&gpio>;
+			interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
+		};
 	};
 
 	i2c at 7000d000 {
@@ -945,7 +953,7 @@
 						regulator-max-microvolt = <1800000>;
 					};
 
-					ldo6 {
+					palmas_ldo6_reg: ldo6 {
 						regulator-name = "vdd-sensor-2v85";
 						regulator-min-microvolt = <2850000>;
 						regulator-max-microvolt = <2850000>;
-- 
1.7.9.5

[PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008

[Stephen Warren]

On 08/07/2013 12:52 AM, Wei Ni wrote:
> Enable thermal sensor nct1008 for t114 dalmore.

Wei, I assume this patch doesn't depend on any of the other LM90-related
patches you've sent; I can simply apply it right away?

Is the LM90 DT binding fully documented somewhere, including the
vdd-supply property?

[PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008

[Wei Ni]

On 08/08/2013 12:03 AM, Stephen Warren wrote:
> On 08/07/2013 12:52 AM, Wei Ni wrote:
>> Enable thermal sensor nct1008 for t114 dalmore.
> 
> Wei, I assume this patch doesn't depend on any of the other LM90-related
> patches you've sent; I can simply apply it right away?

In my [PATCH 1/2], I add codes to get the vdd regulator and enable it,
so if without it, the lm90 can't work properly on Dalmore, although it
can be loaded.
I think it's better to wait my fist patch applied.

> 
> Is the LM90 DT binding fully documented somewhere, including the
> vdd-supply property?

No LM90 DT binding document yet, I will add it in my next version.

>