* [PATCH 0/2] Add power control for lm90
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q
Cc: linux-0h96xk9xTtrk1uMJSBkQmQ, MLongnecker-DDmLM1+adcrQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 0/2] Add power control for lm90
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali, swarren
Cc: linux, MLongnecker, linux-arm-kernel, lm-sensors, linux-kernel,
linux-tegra, 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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 0/2] Add power control for lm90
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [lm-sensors] [PATCH 0/2] Add power control for lm90
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q
Cc: linux-0h96xk9xTtrk1uMJSBkQmQ, MLongnecker-DDmLM1+adcrQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, 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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 6:52 ` Wei Ni
(?)
(?)
@ 2013-08-07 6:52 ` Wei Ni
-1 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali, swarren
Cc: linux, MLongnecker, linux-arm-kernel, lm-sensors, linux-kernel,
linux-tegra, 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali, swarren
Cc: linux, MLongnecker, linux-arm-kernel, lm-sensors, linux-kernel,
linux-tegra, 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali, swarren
Cc: linux, MLongnecker, linux-arm-kernel, lm-sensors, linux-kernel,
linux-tegra, 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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
2013-08-07 6:52 ` Wei Ni
(?)
(?)
@ 2013-08-07 6:52 ` Wei Ni
-1 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali, swarren
Cc: linux, MLongnecker, linux-arm-kernel, lm-sensors, linux-kernel,
linux-tegra, 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali, swarren
Cc: linux, MLongnecker, linux-arm-kernel, lm-sensors, linux-kernel,
linux-tegra, 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [lm-sensors] [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-07 6:52 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 6:52 UTC (permalink / raw)
To: khali, swarren
Cc: linux, MLongnecker, linux-arm-kernel, lm-sensors, linux-kernel,
linux-tegra, 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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 6:52 ` Wei Ni
(?)
(?)
@ 2013-08-07 7:03 ` Guenter Roeck
-1 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:03 UTC (permalink / raw)
To: Wei Ni
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q,
MLongnecker-DDmLM1+adcrQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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;
> }
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:03 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:03 UTC (permalink / raw)
To: Wei Ni
Cc: khali, swarren, MLongnecker, linux-arm-kernel, lm-sensors,
linux-kernel, linux-tegra
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;
> }
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:03 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:03 UTC (permalink / raw)
To: linux-arm-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:03 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:03 UTC (permalink / raw)
To: Wei Ni
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q,
MLongnecker-DDmLM1+adcrQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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;
> }
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 7:03 ` Guenter Roeck
(?)
(?)
@ 2013-08-07 7:15 ` Wei Ni
-1 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:15 UTC (permalink / raw)
To: Guenter Roeck
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q,
Matthew Longnecker,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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;
>> }
>>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:15 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:15 UTC (permalink / raw)
To: Guenter Roeck
Cc: khali, swarren, Matthew Longnecker, linux-arm-kernel, lm-sensors,
linux-kernel, linux-tegra
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;
>> }
>>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:15 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:15 UTC (permalink / raw)
To: linux-arm-kernel
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;
>> }
>>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:15 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:15 UTC (permalink / raw)
To: Guenter Roeck
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q,
Matthew Longnecker,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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;
>> }
>>
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 6:52 ` Wei Ni
(?)
(?)
@ 2013-08-07 7:27 ` Alexander Shiyan
-1 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 7:27 UTC (permalink / raw)
To: Wei Ni
Cc: swarren, linux-kernel, lm-sensors, linux-tegra, MLongnecker,
linux-arm-kernel, khali, linux
> 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?
[...]
---
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:27 ` Alexander Shiyan
0 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 7:27 UTC (permalink / raw)
To: Wei Ni
Cc: khali, swarren, linux-kernel, lm-sensors, MLongnecker, linux,
linux-tegra, linux-arm-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1165 bytes --]
> 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?
[...]
---
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:27 ` Alexander Shiyan
0 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 7:27 UTC (permalink / raw)
To: linux-arm-kernel
> 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?
[...]
---
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add po
@ 2013-08-07 7:27 ` Alexander Shiyan
0 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 7:27 UTC (permalink / raw)
To: Wei Ni
Cc: swarren, linux-kernel, lm-sensors, linux-tegra, MLongnecker,
linux-arm-kernel, khali, linux
> 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?
[...]
---
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 7:27 ` Alexander Shiyan
(?)
(?)
@ 2013-08-07 7:32 ` Wei Ni
-1 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:32 UTC (permalink / raw)
To: Alexander Shiyan
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Matthew Longnecker,
linux-0h96xk9xTtrk1uMJSBkQmQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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.
>
> [...]
> ---
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:32 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:32 UTC (permalink / raw)
To: Alexander Shiyan
Cc: khali, swarren, linux-kernel, lm-sensors, Matthew Longnecker,
linux, linux-tegra, linux-arm-kernel
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.
>
> [...]
> ---
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:32 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:32 UTC (permalink / raw)
To: linux-arm-kernel
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.
>
> [...]
> ---
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:32 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 7:32 UTC (permalink / raw)
To: Alexander Shiyan
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Matthew Longnecker,
linux-0h96xk9xTtrk1uMJSBkQmQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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.
>
> [...]
> ---
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 7:27 ` Alexander Shiyan
(?)
@ 2013-08-07 7:45 ` Guenter Roeck
-1 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:45 UTC (permalink / raw)
To: Alexander Shiyan
Cc: Wei Ni, khali, swarren, linux-kernel, lm-sensors, MLongnecker,
linux-tegra, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:45 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:45 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:45 UTC (permalink / raw)
To: Alexander Shiyan
Cc: Wei Ni, khali, swarren, linux-kernel, lm-sensors, MLongnecker,
linux-tegra, linux-arm-kernel
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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 7:32 ` Wei Ni
(?)
(?)
@ 2013-08-07 7:50 ` Guenter Roeck
-1 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:50 UTC (permalink / raw)
To: Wei Ni
Cc: Alexander Shiyan, khali-PUYAD+kWke1g9hUCZPvPmw,
swarren-3lzwWm7+Weoh9ZMKESR00Q,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Matthew Longnecker,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:50 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:50 UTC (permalink / raw)
To: Wei Ni
Cc: Alexander Shiyan, khali, swarren, linux-kernel, lm-sensors,
Matthew Longnecker, linux-tegra, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:50 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:50 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 7:50 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 7:50 UTC (permalink / raw)
To: Wei Ni
Cc: Alexander Shiyan, khali-PUYAD+kWke1g9hUCZPvPmw,
swarren-3lzwWm7+Weoh9ZMKESR00Q,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Matthew Longnecker,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 7:50 ` Guenter Roeck
(?)
(?)
@ 2013-08-07 8:07 ` Wei Ni
-1 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 8:07 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alexander Shiyan, khali, swarren, linux-kernel, lm-sensors,
Matthew Longnecker, linux-tegra, linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 8:07 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 8:07 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alexander Shiyan, khali, swarren, linux-kernel, lm-sensors,
Matthew Longnecker, linux-tegra, linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 8:07 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 8:07 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 8:07 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 8:07 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alexander Shiyan, khali, swarren, linux-kernel, lm-sensors,
Matthew Longnecker, linux-tegra, linux-arm-kernel
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
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 8:07 ` Wei Ni
(?)
(?)
@ 2013-08-07 8:45 ` Alexander Shiyan
-1 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 8:45 UTC (permalink / raw)
To: Wei Ni
Cc: Guenter Roeck, swarren@wwwdotorg.org,
linux-kernel@vger.kernel.org, lm-sensors@lm-sensors.org,
linux-tegra@vger.kernel.org, Matthew Longnecker,
khali@linux-fr.org, linux-arm-kernel@lists.infradead.org
> 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.
---
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 8:45 ` Alexander Shiyan
0 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 8:45 UTC (permalink / raw)
To: Wei Ni
Cc: Guenter Roeck, swarren, linux-kernel, lm-sensors, linux-tegra,
Matthew Longnecker, khali, linux-arm-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 2164 bytes --]
> 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.
---
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 8:45 ` Alexander Shiyan
0 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 8:45 UTC (permalink / raw)
To: linux-arm-kernel
> 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.
---
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add po
@ 2013-08-07 8:45 ` Alexander Shiyan
0 siblings, 0 replies; 63+ messages in thread
From: Alexander Shiyan @ 2013-08-07 8:45 UTC (permalink / raw)
To: Wei Ni
Cc: Guenter Roeck, swarren@wwwdotorg.org,
linux-kernel@vger.kernel.org, lm-sensors@lm-sensors.org,
linux-tegra@vger.kernel.org, Matthew Longnecker,
khali@linux-fr.org, linux-arm-kernel@lists.infradead.org
> 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.
---
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 8:45 ` Alexander Shiyan
(?)
(?)
@ 2013-08-07 9:35 ` Wei Ni
-1 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 9:35 UTC (permalink / raw)
To: Alexander Shiyan
Cc: Guenter Roeck, swarren-3lzwWm7+Weoh9ZMKESR00Q,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Matthew Longnecker,
khali-PUYAD+kWke1g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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.
>
> ---
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 9:35 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 9:35 UTC (permalink / raw)
To: Alexander Shiyan
Cc: Guenter Roeck, swarren, linux-kernel, lm-sensors, linux-tegra,
Matthew Longnecker, khali, linux-arm-kernel
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.
>
> ---
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 9:35 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 9:35 UTC (permalink / raw)
To: linux-arm-kernel
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.
>
> ---
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 9:35 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-07 9:35 UTC (permalink / raw)
To: Alexander Shiyan
Cc: Guenter Roeck, swarren-3lzwWm7+Weoh9ZMKESR00Q,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Matthew Longnecker,
khali-PUYAD+kWke1g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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.
>
> ---
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
2013-08-07 6:52 ` Wei Ni
(?)
(?)
@ 2013-08-07 16:03 ` Stephen Warren
-1 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:03 UTC (permalink / raw)
To: Wei Ni
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, linux-0h96xk9xTtrk1uMJSBkQmQ,
MLongnecker-DDmLM1+adcrQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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?
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-07 16:03 ` Stephen Warren
0 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:03 UTC (permalink / raw)
To: Wei Ni
Cc: khali, linux, MLongnecker, linux-arm-kernel, lm-sensors,
linux-kernel, linux-tegra
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?
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-07 16:03 ` Stephen Warren
0 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:03 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-07 16:03 ` Stephen Warren
0 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:03 UTC (permalink / raw)
To: Wei Ni
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, linux-0h96xk9xTtrk1uMJSBkQmQ,
MLongnecker-DDmLM1+adcrQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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?
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 9:35 ` Wei Ni
(?)
(?)
@ 2013-08-07 16:06 ` Stephen Warren
-1 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:06 UTC (permalink / raw)
To: Wei Ni
Cc: Alexander Shiyan, Guenter Roeck,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Matthew Longnecker,
khali-PUYAD+kWke1g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 16:06 ` Stephen Warren
0 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:06 UTC (permalink / raw)
To: Wei Ni
Cc: Alexander Shiyan, Guenter Roeck, linux-kernel, lm-sensors,
linux-tegra, Matthew Longnecker, khali, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 16:06 ` Stephen Warren
0 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:06 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 16:06 ` Stephen Warren
0 siblings, 0 replies; 63+ messages in thread
From: Stephen Warren @ 2013-08-07 16:06 UTC (permalink / raw)
To: Wei Ni
Cc: Alexander Shiyan, Guenter Roeck,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Matthew Longnecker,
khali-PUYAD+kWke1g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
2013-08-07 16:06 ` Stephen Warren
(?)
(?)
@ 2013-08-07 16:44 ` Guenter Roeck
-1 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 16:44 UTC (permalink / raw)
To: Stephen Warren
Cc: Wei Ni, Alexander Shiyan, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Matthew Longnecker,
khali-PUYAD+kWke1g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 16:44 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 16:44 UTC (permalink / raw)
To: Stephen Warren
Cc: Wei Ni, Alexander Shiyan, linux-kernel, lm-sensors, linux-tegra,
Matthew Longnecker, khali, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 16:44 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 16:44 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add power control
@ 2013-08-07 16:44 ` Guenter Roeck
0 siblings, 0 replies; 63+ messages in thread
From: Guenter Roeck @ 2013-08-07 16:44 UTC (permalink / raw)
To: Stephen Warren
Cc: Wei Ni, Alexander Shiyan, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Matthew Longnecker,
khali-PUYAD+kWke1g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
2013-08-07 16:03 ` Stephen Warren
(?)
(?)
@ 2013-08-08 2:36 ` Wei Ni
-1 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-08 2:36 UTC (permalink / raw)
To: Stephen Warren
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, linux-0h96xk9xTtrk1uMJSBkQmQ,
Matthew Longnecker,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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.
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-08 2:36 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-08 2:36 UTC (permalink / raw)
To: Stephen Warren
Cc: khali, linux, Matthew Longnecker, linux-arm-kernel, lm-sensors,
linux-kernel, linux-tegra
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.
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-08 2:36 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-08 2:36 UTC (permalink / raw)
To: linux-arm-kernel
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.
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [lm-sensors] [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008
@ 2013-08-08 2:36 ` Wei Ni
0 siblings, 0 replies; 63+ messages in thread
From: Wei Ni @ 2013-08-08 2:36 UTC (permalink / raw)
To: Stephen Warren
Cc: khali-PUYAD+kWke1g9hUCZPvPmw, linux-0h96xk9xTtrk1uMJSBkQmQ,
Matthew Longnecker,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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.
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 63+ messages in thread
end of thread, other threads:[~2013-08-08 2:36 UTC | newest]
Thread overview: 63+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-07 6:52 [PATCH 0/2] Add power control for lm90 Wei Ni
2013-08-07 6:52 ` [lm-sensors] " Wei Ni
2013-08-07 6:52 ` Wei Ni
2013-08-07 6:52 ` Wei Ni
2013-08-07 6:52 ` [PATCH 1/2] hwmon: (lm90) Add power control Wei Ni
2013-08-07 6:52 ` [lm-sensors] " Wei Ni
2013-08-07 6:52 ` Wei Ni
2013-08-07 6:52 ` Wei Ni
[not found] ` <1375858358-15070-2-git-send-email-wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-08-07 7:03 ` Guenter Roeck
2013-08-07 7:03 ` [lm-sensors] " Guenter Roeck
2013-08-07 7:03 ` Guenter Roeck
2013-08-07 7:03 ` Guenter Roeck
[not found] ` <5201F138.3080906-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2013-08-07 7:15 ` Wei Ni
2013-08-07 7:15 ` [lm-sensors] " Wei Ni
2013-08-07 7:15 ` Wei Ni
2013-08-07 7:15 ` Wei Ni
2013-08-07 7:27 ` Alexander Shiyan
2013-08-07 7:27 ` [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add po Alexander Shiyan
2013-08-07 7:27 ` [PATCH 1/2] hwmon: (lm90) Add power control Alexander Shiyan
2013-08-07 7:27 ` Alexander Shiyan
[not found] ` <1375860442.896960598-syZRKAW8O9ZsdVUOrk1QfQ@public.gmane.org>
2013-08-07 7:32 ` Wei Ni
2013-08-07 7:32 ` [lm-sensors] " Wei Ni
2013-08-07 7:32 ` Wei Ni
2013-08-07 7:32 ` Wei Ni
[not found] ` <5201F811.9050602-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-08-07 7:50 ` Guenter Roeck
2013-08-07 7:50 ` [lm-sensors] " Guenter Roeck
2013-08-07 7:50 ` Guenter Roeck
2013-08-07 7:50 ` Guenter Roeck
2013-08-07 8:07 ` Wei Ni
2013-08-07 8:07 ` [lm-sensors] " Wei Ni
2013-08-07 8:07 ` Wei Ni
2013-08-07 8:07 ` Wei Ni
[not found] ` <52020047.1080705-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-08-07 8:45 ` Alexander Shiyan
2013-08-07 8:45 ` [lm-sensors] [PATCH 1/2] hwmon: (lm90) Add po Alexander Shiyan
2013-08-07 8:45 ` [PATCH 1/2] hwmon: (lm90) Add power control Alexander Shiyan
2013-08-07 8:45 ` Alexander Shiyan
[not found] ` <1375865105.562600640-y1D/hCXJdYBsdVUOrk1QfQ@public.gmane.org>
2013-08-07 9:35 ` Wei Ni
2013-08-07 9:35 ` [lm-sensors] " Wei Ni
2013-08-07 9:35 ` Wei Ni
2013-08-07 9:35 ` Wei Ni
[not found] ` <520214C9.1010200-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-08-07 16:06 ` Stephen Warren
2013-08-07 16:06 ` [lm-sensors] " Stephen Warren
2013-08-07 16:06 ` Stephen Warren
2013-08-07 16:06 ` Stephen Warren
[not found] ` <52027086.1090608-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-08-07 16:44 ` Guenter Roeck
2013-08-07 16:44 ` [lm-sensors] " Guenter Roeck
2013-08-07 16:44 ` Guenter Roeck
2013-08-07 16:44 ` Guenter Roeck
2013-08-07 7:45 ` Guenter Roeck
2013-08-07 7:45 ` [lm-sensors] " Guenter Roeck
2013-08-07 7:45 ` Guenter Roeck
2013-08-07 6:52 ` [PATCH 2/2] ARM: dt: t114 dalmore: add dt entry for nct1008 Wei Ni
2013-08-07 6:52 ` [lm-sensors] " Wei Ni
2013-08-07 6:52 ` Wei Ni
2013-08-07 6:52 ` Wei Ni
[not found] ` <1375858358-15070-3-git-send-email-wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2013-08-07 16:03 ` Stephen Warren
2013-08-07 16:03 ` [lm-sensors] " Stephen Warren
2013-08-07 16:03 ` Stephen Warren
2013-08-07 16:03 ` Stephen Warren
[not found] ` <52026FEE.9070003-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-08-08 2:36 ` Wei Ni
2013-08-08 2:36 ` [lm-sensors] " Wei Ni
2013-08-08 2:36 ` Wei Ni
2013-08-08 2:36 ` Wei Ni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.