* [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
* [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 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
* [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 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
* [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 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
* [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
[parent not found: <1375858358-15070-2-git-send-email-wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* 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: [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
* [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: [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
[parent not found: <5201F138.3080906-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>]
* 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: [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
* [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: [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
* 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: [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 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: [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
[parent not found: <1375860442.896960598-syZRKAW8O9ZsdVUOrk1QfQ@public.gmane.org>]
* 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: [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
* [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: [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
[parent not found: <5201F811.9050602-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* 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: [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
* [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: [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
* 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: [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
* [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: [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
[parent not found: <52020047.1080705-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* 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: [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 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: [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
[parent not found: <1375865105.562600640-y1D/hCXJdYBsdVUOrk1QfQ@public.gmane.org>]
* 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: [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
* [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: [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
[parent not found: <520214C9.1010200-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* 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: [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
* [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: [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
[parent not found: <52027086.1090608-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]
* 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: [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
* [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: [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
* 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
* 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
* [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
* [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
* [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
* [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
* [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
[parent not found: <1375858358-15070-3-git-send-email-wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* 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: [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
* [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: [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
[parent not found: <52026FEE.9070003-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]
* 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: [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
* [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: [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
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.