[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

Re: [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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>   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;
>   }
>

Re: [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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   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?

[...]
---

Re: [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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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.

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

Re: [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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>   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

Re: [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.

---

Re: [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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>> ---
>>>>>>   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.

> 
> ---
> 

Re: [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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>>> ---
>>>>>>>   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.

Re: [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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>>>>>> ---
> >>>>>>>   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

Re: [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@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

Re: [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?

Re: [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.

>