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.
This series is v2, previous version patches:
[v1]: http://www.mail-archive.com/[email protected]/msg12034.html
Changes from v1:
1. if get regulator failed, we should continue to run probe function,
not return fail.
2. call regulator_put() in error handler and remove function.
3. add LM90 DT binding document.
Wei Ni (3):
hwmon: (lm90) Add power control
ARM: dt: t114 dalmore: add dt entry for nct1008
Documentation: dt: hwmon: add OF document for lm90
Documentation/devicetree/bindings/hwmon/lm90.txt | 20 +++++++++
arch/arm/boot/dts/tegra114-dalmore.dts | 10 ++++-
drivers/hwmon/lm90.c | 49 ++++++++++++++++++++++
3 files changed, 78 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/lm90.txt
--
1.7.9.5
Enable thermal sensor nct1008 for t114 dalmore.
Signed-off-by: Wei Ni <[email protected]>
---
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
Add OF document for lm90 in Documentation/devicetree/.
Signed-off-by: Wei Ni <[email protected]>
---
Documentation/devicetree/bindings/hwmon/lm90.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/lm90.txt
diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
new file mode 100644
index 0000000..8024f11
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
@@ -0,0 +1,20 @@
+* LM90 series thermometer.
+
+Required node properties:
+- compatible: manufacture and chip name,
+ which are listed in the Documentation/hwmon/lm90
+- reg: I2C bus address of the device
+
+Optional properties:
+- vdd-supply: vdd regulator for the supply voltage. If this is not set,
+ assuming vdd is always powered.
+- interrupts: lm90 can support interrupt mode, you can set interrupt here.
+
+Example lm90 node:
+
+nct1008 {
+ compatible = "onnn,nct1008";
+ reg = <0x4c>;
+ vdd-supply = <&palmas_ldo6_reg>;
+ interrupt-parent = <&gpio>;
+ interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
--
1.7.9.5
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 <[email protected]>
---
drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index cdff742..306a348 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -89,6 +89,7 @@
#include <linux/err.h>
#include <linux/mutex.h>
#include <linux/sysfs.h>
+#include <linux/regulator/consumer.h>
/*
* Addresses to scan
@@ -302,6 +303,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 +1393,32 @@ static void lm90_init_client(struct i2c_client *client)
i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
}
+static void lm90_power_control(struct i2c_client *client, bool is_enable)
+{
+ struct lm90_data *data = i2c_get_clientdata(client);
+ int ret;
+
+ if (!data->lm90_reg)
+ return;
+
+ mutex_lock(&data->update_lock);
+
+ if (is_enable)
+ ret = regulator_enable(data->lm90_reg);
+ 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);
+}
+
static int lm90_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
i2c_set_clientdata(client, data);
mutex_init(&data->update_lock);
+ 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;
+ }
+
+ lm90_power_control(client, true);
+
/* Set the device type */
data->kind = id->driver_data;
if (data->kind == adm1032) {
@@ -1473,6 +1515,10 @@ exit_remove_files:
lm90_remove_files(client, data);
exit_restore:
lm90_restore_conf(client, data);
+ lm90_power_control(client, false);
+ if (data->lm90_reg)
+ regulator_put(data->lm90_reg);
+
return err;
}
@@ -1483,6 +1529,9 @@ 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);
+ if (data->lm90_reg)
+ regulator_put(data->lm90_reg);
return 0;
}
--
1.7.9.5
> 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 <[email protected]>
> ---
> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
[...]
> +static void lm90_power_control(struct i2c_client *client, bool is_enable)
> +{
> + struct lm90_data *data = i2c_get_clientdata(client);
> + int ret;
> +
> + if (!data->lm90_reg)
> + return;
> +
> + mutex_lock(&data->update_lock);
> +
> + if (is_enable)
> + ret = regulator_enable(data->lm90_reg);
> + 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");
dev_dbg() ?
> +
> + mutex_unlock(&data->update_lock);
> +}
> +
> static int lm90_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
> i2c_set_clientdata(client, data);
> mutex_init(&data->update_lock);
>
> + data->lm90_reg = regulator_get(&client->dev, "vdd");
> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
What about deferred probe?
if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER)
return -EPROBE_DEFER;
> + if (PTR_ERR(data->lm90_reg) == -ENODEV)
> + dev_info(&client->dev,
> + "No regulator found for vdd. Assuming vdd is always powered.");
On my opinion it is unnecessary message.
> + else
> + dev_warn(&client->dev,
> + "Error [%ld] in getting the regulator handle for vdd.\n",
> + PTR_ERR(data->lm90_reg));
Ditto.
> + data->lm90_reg = NULL;
You can just use !IS_ERR(data->lm90_reg) macro in the future,
rather than set this to NULL.
[...]
---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 08/07/2013 11:56 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 <[email protected]>
> ---
> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index cdff742..306a348 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,7 @@
> #include <linux/err.h>
> #include <linux/mutex.h>
> #include <linux/sysfs.h>
> +#include <linux/regulator/consumer.h>
>
> /*
> * Addresses to scan
> @@ -302,6 +303,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 +1393,32 @@ static void lm90_init_client(struct i2c_client *client)
> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> }
>
> +static void lm90_power_control(struct i2c_client *client, bool is_enable)
> +{
> + struct lm90_data *data = i2c_get_clientdata(client);
> + int ret;
> +
> + if (!data->lm90_reg)
> + return;
> +
> + mutex_lock(&data->update_lock);
> +
This is only called during probe and remove, so the mutex is unnecessary.
> + if (is_enable)
> + ret = regulator_enable(data->lm90_reg);
> + 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");
> +
which reduces the function to (pretty much unnecessary) messages and an if statement
which you only need because you have the function.
You should just call regulator_enable in probe and regulator_disable in remove.
Guenter
> + mutex_unlock(&data->update_lock);
> +}
> +
> static int lm90_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
> i2c_set_clientdata(client, data);
> mutex_init(&data->update_lock);
>
> + data->lm90_reg = regulator_get(&client->dev, "vdd");
You should use devm_regulator_get(). Then you also don't need the call to regulator_put().
> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
The function never returns NULL except if the regulator subsystem is not configured,
so IS_ERR() is more appropriate.
If the regulator subsystem is not configured, you especially don't need or want
to pollute the log with an error message.
> + 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));
I consider the messages unnecessary and confusing. You are polluting the log
of pretty much every PC user who has one of the supported chips in the system,
and of everyone else not using regulators for this chip.
> + data->lm90_reg = NULL;
As pointed out, this is unnecessary, and you should handle -EPROBE_DEFER correctly.
> + }
> +
> + lm90_power_control(client, true);
> +
> /* Set the device type */
> data->kind = id->driver_data;
> if (data->kind == adm1032) {
> @@ -1473,6 +1515,10 @@ exit_remove_files:
> lm90_remove_files(client, data);
> exit_restore:
> lm90_restore_conf(client, data);
> + lm90_power_control(client, false);
> + if (data->lm90_reg)
> + regulator_put(data->lm90_reg);
> +
> return err;
> }
>
> @@ -1483,6 +1529,9 @@ 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);
> + if (data->lm90_reg)
> + regulator_put(data->lm90_reg);
>
> return 0;
> }
>
On 08/08/2013 03:13 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 <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> [...]
>> +static void lm90_power_control(struct i2c_client *client, bool is_enable)
>> +{
>> + struct lm90_data *data = i2c_get_clientdata(client);
>> + int ret;
>> +
>> + if (!data->lm90_reg)
>> + return;
>> +
>> + mutex_lock(&data->update_lock);
>> +
>> + if (is_enable)
>> + ret = regulator_enable(data->lm90_reg);
>> + 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");
>
> dev_dbg() ?
As Guenter said, I will remove these messages, and remove this function,
the code will be more clean.
>
>> +
>> + mutex_unlock(&data->update_lock);
>> +}
>> +
>> static int lm90_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
>> i2c_set_clientdata(client, data);
>> mutex_init(&data->update_lock);
>>
>> + data->lm90_reg = regulator_get(&client->dev, "vdd");
>> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
>
> What about deferred probe?
> if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER)
> return -EPROBE_DEFER;
Oh, yes, I should add it.
>
>> + if (PTR_ERR(data->lm90_reg) == -ENODEV)
>> + dev_info(&client->dev,
>> + "No regulator found for vdd. Assuming vdd is always powered.");
>
> On my opinion it is unnecessary message.
>
>> + else
>> + dev_warn(&client->dev,
>> + "Error [%ld] in getting the regulator handle for vdd.\n",
>> + PTR_ERR(data->lm90_reg));
>
> Ditto.
I will remove these log messages.
>
>> + data->lm90_reg = NULL;
>
> You can just use !IS_ERR(data->lm90_reg) macro in the future,
> rather than set this to NULL.
Yes, it's better, I will do it.
>
> [...]
>
> ---
>
On 08/08/2013 04:42 PM, Guenter Roeck wrote:
> On 08/07/2013 11:56 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 <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index cdff742..306a348 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -89,6 +89,7 @@
>> #include <linux/err.h>
>> #include <linux/mutex.h>
>> #include <linux/sysfs.h>
>> +#include <linux/regulator/consumer.h>
>>
>> /*
>> * Addresses to scan
>> @@ -302,6 +303,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 +1393,32 @@ static void lm90_init_client(struct i2c_client *client)
>> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>> }
>>
>> +static void lm90_power_control(struct i2c_client *client, bool is_enable)
>> +{
>> + struct lm90_data *data = i2c_get_clientdata(client);
>> + int ret;
>> +
>> + if (!data->lm90_reg)
>> + return;
>> +
>> + mutex_lock(&data->update_lock);
>> +
>
> This is only called during probe and remove, so the mutex is unnecessary.
I considered that we may call this function in suspend/resume routine,
so I add this mutex.
But as you said, currently we doesn't have these routine yet, the mutex
is unnecessary, so I will remove it.
>
>> + if (is_enable)
>> + ret = regulator_enable(data->lm90_reg);
>> + 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");
>> +
> which reduces the function to (pretty much unnecessary) messages and an if statement
> which you only need because you have the function.
>
> You should just call regulator_enable in probe and regulator_disable in remove.
Ok, I will remove these messages and this function.
>
> Guenter
>
>> + mutex_unlock(&data->update_lock);
>> +}
>> +
>> static int lm90_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
>> i2c_set_clientdata(client, data);
>> mutex_init(&data->update_lock);
>>
>> + data->lm90_reg = regulator_get(&client->dev, "vdd");
>
> You should use devm_regulator_get(). Then you also don't need the call to regulator_put().
Oh, yes, you are right, I will do it.
>
>> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
>
> The function never returns NULL except if the regulator subsystem is not configured,
> so IS_ERR() is more appropriate.
>
> If the regulator subsystem is not configured, you especially don't need or want
> to pollute the log with an error message.
>
>> + 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));
>
> I consider the messages unnecessary and confusing. You are polluting the log
> of pretty much every PC user who has one of the supported chips in the system,
> and of everyone else not using regulators for this chip.
Ok, I will remove these codes.
So I will write something like:
if (!IS_ERR(data->lm90_reg)) {
ret = regulator_enable(data->lm90_reg);
if (ret < 0) {
dev_err();
return ret;
}
} else {
if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER)
return -EPRPBE_DEFER;
data->lm90_reg = !!IS_ERR(data->lm90_reg);
}
>
>> + data->lm90_reg = NULL;
>
> As pointed out, this is unnecessary, and you should handle -EPROBE_DEFER correctly.
I think get_regulator() will return error values, not only
-EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other
error values.
>
>> + }
>> +
>> + lm90_power_control(client, true);
>> +
>> /* Set the device type */
>> data->kind = id->driver_data;
>> if (data->kind == adm1032) {
>> @@ -1473,6 +1515,10 @@ exit_remove_files:
>> lm90_remove_files(client, data);
>> exit_restore:
>> lm90_restore_conf(client, data);
>> + lm90_power_control(client, false);
>> + if (data->lm90_reg)
>> + regulator_put(data->lm90_reg);
>> +
>> return err;
>> }
>>
>> @@ -1483,6 +1529,9 @@ 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);
>> + if (data->lm90_reg)
>> + regulator_put(data->lm90_reg);
>>
>> return 0;
>> }
>>
>
On 08/08/2013 05:57 PM, Alexander Shiyan wrote:
>> On 08/08/2013 04:42 PM, Guenter Roeck wrote:
>>> On 08/07/2013 11:56 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 <[email protected]>
>>>> ---
>>>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> [...]
>>> I consider the messages unnecessary and confusing. You are polluting the log
>>> of pretty much every PC user who has one of the supported chips in the system,
>>> and of everyone else not using regulators for this chip.
>>
>> Ok, I will remove these codes.
>> So I will write something like:
>> if (!IS_ERR(data->lm90_reg)) {
>> ret = regulator_enable(data->lm90_reg);
>> if (ret < 0) {
>> dev_err();
>> return ret;
>> }
>> } else {
>> if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER)
>> return -EPRPBE_DEFER;
>>
>> data->lm90_reg = !!IS_ERR(data->lm90_reg);
>
> No. You do not need this line.
>
> Just use in remove():
> if (!IS_ERR(data->lm90_reg))
> regulator_disable(data->lm90_reg);
Oh, I see, thank you very much.
>
> ---
>
> On 08/08/2013 04:42 PM, Guenter Roeck wrote:
> > On 08/07/2013 11:56 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 <[email protected]>
> >> ---
> >> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
[...]
> > I consider the messages unnecessary and confusing. You are polluting the log
> > of pretty much every PC user who has one of the supported chips in the system,
> > and of everyone else not using regulators for this chip.
>
> Ok, I will remove these codes.
> So I will write something like:
> if (!IS_ERR(data->lm90_reg)) {
> ret = regulator_enable(data->lm90_reg);
> if (ret < 0) {
> dev_err();
> return ret;
> }
> } else {
> if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER)
> return -EPRPBE_DEFER;
>
> data->lm90_reg = !!IS_ERR(data->lm90_reg);
No. You do not need this line.
Just use in remove():
if (!IS_ERR(data->lm90_reg))
regulator_disable(data->lm90_reg);
---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 08/08/2013 05:57 PM, Alexander Shiyan wrote:
>> On 08/08/2013 04:42 PM, Guenter Roeck wrote:
>>> On 08/07/2013 11:56 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 <[email protected]>
>>>> ---
>>>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> [...]
>>> I consider the messages unnecessary and confusing. You are polluting the log
>>> of pretty much every PC user who has one of the supported chips in the system,
>>> and of everyone else not using regulators for this chip.
>>
>> Ok, I will remove these codes.
>> So I will write something like:
>> if (!IS_ERR(data->lm90_reg)) {
>> ret = regulator_enable(data->lm90_reg);
>> if (ret < 0) {
>> dev_err();
>> return ret;
>> }
>> } else {
>> if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER)
>> return -EPRPBE_DEFER;
>>
>> data->lm90_reg = !!IS_ERR(data->lm90_reg);
BTW, since it may return EPROBE_DEFER, these codes should be put in the
beginning of the probe() function, should before allocate lm90_data.
>
> No. You do not need this line.
>
> Just use in remove():
> if (!IS_ERR(data->lm90_reg))
> regulator_disable(data->lm90_reg);
>
> ---
>
On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote:
> + mutex_lock(&data->update_lock);
> +
> + if (is_enable)
> + ret = regulator_enable(data->lm90_reg);
> + 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);
Two things here. One is that it's not clear what this lokc is
protecting since the only thing in the locked region is the regulator
operation and that is thread safe. The other thing is that I'm not
seeing anthing that ensures that enables and disables are matched -
regulators are reference counted so two enables need two disables.
> + data->lm90_reg = regulator_get(&client->dev, "vdd");
> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
NULL is a valid regulator, use IS_ERR().
> + 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));
You shouldn't just be ignoring errors here, though there are deployment
difficulties with making sure a stub regulator is provided. These
should be getting easier after the next merge window, the stubs will be
being tweaked slightly to have an "assume it's there" option even when
regulators are used. Especially in cases with device tree you should be
paying attention to -EPROBE_DEFER, that will accurately reflect if a
regulator is present but not loaded yet.
That said if you *are* going to do this you should request the
regulator using devm_regulator_get_optional(), this is intended to
support things that don't need regulators (though that's not the case
here).
On 08/08/2013 02:47 AM, Wei Ni wrote:
> On 08/08/2013 04:42 PM, Guenter Roeck wrote:
>> On 08/07/2013 11:56 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 <[email protected]>
>>> ---
>>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>> index cdff742..306a348 100644
>>> --- a/drivers/hwmon/lm90.c
>>> +++ b/drivers/hwmon/lm90.c
>>> @@ -89,6 +89,7 @@
>>> #include <linux/err.h>
>>> #include <linux/mutex.h>
>>> #include <linux/sysfs.h>
>>> +#include <linux/regulator/consumer.h>
>>>
>>> /*
>>> * Addresses to scan
>>> @@ -302,6 +303,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 +1393,32 @@ static void lm90_init_client(struct i2c_client *client)
>>> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>> }
>>>
>>> +static void lm90_power_control(struct i2c_client *client, bool is_enable)
>>> +{
>>> + struct lm90_data *data = i2c_get_clientdata(client);
>>> + int ret;
>>> +
>>> + if (!data->lm90_reg)
>>> + return;
>>> +
>>> + mutex_lock(&data->update_lock);
>>> +
>>
>> This is only called during probe and remove, so the mutex is unnecessary.
>
> I considered that we may call this function in suspend/resume routine,
> so I add this mutex.
> But as you said, currently we doesn't have these routine yet, the mutex
> is unnecessary, so I will remove it.
>
In that case, you can call
mutex_lock()
regulator_enable() / regulator_disable()
mutex_unlock()
directly in those functions. Again no need for the additional function.
>>
>>> + if (is_enable)
>>> + ret = regulator_enable(data->lm90_reg);
>>> + 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");
>>> +
>> which reduces the function to (pretty much unnecessary) messages and an if statement
>> which you only need because you have the function.
>>
>> You should just call regulator_enable in probe and regulator_disable in remove.
>
> Ok, I will remove these messages and this function.
>
>>
>> Guenter
>>
>>> + mutex_unlock(&data->update_lock);
>>> +}
>>> +
>>> static int lm90_probe(struct i2c_client *client,
>>> const struct i2c_device_id *id)
>>> {
>>> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
>>> i2c_set_clientdata(client, data);
>>> mutex_init(&data->update_lock);
>>>
>>> + data->lm90_reg = regulator_get(&client->dev, "vdd");
>>
>> You should use devm_regulator_get(). Then you also don't need the call to regulator_put().
>
> Oh, yes, you are right, I will do it.
>
>>
>>> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
>>
>> The function never returns NULL except if the regulator subsystem is not configured,
>> so IS_ERR() is more appropriate.
>>
>> If the regulator subsystem is not configured, you especially don't need or want
>> to pollute the log with an error message.
>>
>>> + 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));
>>
>> I consider the messages unnecessary and confusing. You are polluting the log
>> of pretty much every PC user who has one of the supported chips in the system,
>> and of everyone else not using regulators for this chip.
>
> Ok, I will remove these codes.
> So I will write something like:
> if (!IS_ERR(data->lm90_reg)) {
> ret = regulator_enable(data->lm90_reg);
> if (ret < 0) {
> dev_err();
> return ret;
> }
> } else {
Handle the error in the if case.
> if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER)
> return -EPRPBE_DEFER;
>
> data->lm90_reg = !!IS_ERR(data->lm90_reg);
You know that IS_ERR is true here. Unless I am missing something, this would assign "1" to lm90_reg.
> }
>
>>
>>> + data->lm90_reg = NULL;
>>
>> As pointed out, this is unnecessary, and you should handle -EPROBE_DEFER correctly.
>
> I think get_regulator() will return error values, not only
> -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other
> error values.
>
Matter of opinion if you want to check for IS_ERR or NULL later on.
>>
>>> + }
>>> +
>>> + lm90_power_control(client, true);
>>> +
>>> /* Set the device type */
>>> data->kind = id->driver_data;
>>> if (data->kind == adm1032) {
>>> @@ -1473,6 +1515,10 @@ exit_remove_files:
>>> lm90_remove_files(client, data);
>>> exit_restore:
>>> lm90_restore_conf(client, data);
>>> + lm90_power_control(client, false);
>>> + if (data->lm90_reg)
>>> + regulator_put(data->lm90_reg);
>>> +
>>> return err;
>>> }
>>>
>>> @@ -1483,6 +1529,9 @@ 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);
>>> + if (data->lm90_reg)
>>> + regulator_put(data->lm90_reg);
>>>
>>> return 0;
>>> }
>>>
>>
>
>
>
On 08/08/2013 04:01 AM, Mark Brown wrote:
> On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote:
>
>> + mutex_lock(&data->update_lock);
>> +
>> + if (is_enable)
>> + ret = regulator_enable(data->lm90_reg);
>> + 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);
>
> Two things here. One is that it's not clear what this lokc is
> protecting since the only thing in the locked region is the regulator
> operation and that is thread safe. The other thing is that I'm not
> seeing anthing that ensures that enables and disables are matched -
> regulators are reference counted so two enables need two disables.
>
>> + data->lm90_reg = regulator_get(&client->dev, "vdd");
>> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
>
> NULL is a valid regulator, use IS_ERR().
>
>> + 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));
>
> You shouldn't just be ignoring errors here, though there are deployment
> difficulties with making sure a stub regulator is provided. These
> should be getting easier after the next merge window, the stubs will be
> being tweaked slightly to have an "assume it's there" option even when
> regulators are used. Especially in cases with device tree you should be
> paying attention to -EPROBE_DEFER, that will accurately reflect if a
> regulator is present but not loaded yet.
>
> That said if you *are* going to do this you should request the
> regulator using devm_regulator_get_optional(), this is intended to
> support things that don't need regulators (though that's not the case
> here).
>
The lm90 driver works perfectly fine without regulator.
Guenter
On Thu, Aug 08, 2013 at 04:25:41AM -0700, Guenter Roeck wrote:
> On 08/08/2013 04:01 AM, Mark Brown wrote:
> >That said if you *are* going to do this you should request the
> >regulator using devm_regulator_get_optional(), this is intended to
> >support things that don't need regulators (though that's not the case
> >here).
> The lm90 driver works perfectly fine without regulator.
I'd be most surprised if the device worked without power, if the driver
fails to get and enable a regulator for it then that's not great
(especially if it does so silently).
Note that the regulator API is written to stub itself out if not
enabled, the code should be able to assume that it's there.
On 08/08/2013 06:08 AM, Mark Brown wrote:
> On Thu, Aug 08, 2013 at 04:25:41AM -0700, Guenter Roeck wrote:
>> On 08/08/2013 04:01 AM, Mark Brown wrote:
>
>>> That said if you *are* going to do this you should request the
>>> regulator using devm_regulator_get_optional(), this is intended to
>>> support things that don't need regulators (though that's not the case
>>> here).
>
>> The lm90 driver works perfectly fine without regulator.
>
> I'd be most surprised if the device worked without power, if the driver
> fails to get and enable a regulator for it then that's not great
> (especially if it does so silently).
>
Correct, but it appears that the driver magically worked for a long time
without it.
Is it guaranteed that the driver keeps working for all cases where
regulator support is enabled in the kernel, and where it used to work
so far without mandating the existence of this specific regulator ?
My main concern is having to deal with complaints that the driver stopped
working for no good reason.
In this context, is it common practice to name such regulators "vdd"
or similar ? What if there are multiple LM90 or compatible chips
in a system, connected to different power rails ? Who determines
if the regulator is supposed to be named "vdd" or "vcc" or anything
else, and to which power rails it is actually connected ? How can
and does one guarantee that "vdd" is the correct regulator to use
for all systems ? What if some other driver requests "vdd", but the chip
it supports happens to be connected to a different power rail ?
Sorry for my ignorance in that matter. I did browse through Documentation/power,
but did not find a clear answer.
> Note that the regulator API is written to stub itself out if not
> enabled, the code should be able to assume that it's there.
>
I am aware of that; this is why the driver should not dump an error message
if the function returns NULL and bail out, as it did originally.
Guenter
On Thu, Aug 08, 2013 at 08:21:48AM -0700, Guenter Roeck wrote:
> On 08/08/2013 06:08 AM, Mark Brown wrote:
> >I'd be most surprised if the device worked without power, if the driver
> >fails to get and enable a regulator for it then that's not great
> >(especially if it does so silently).
> Correct, but it appears that the driver magically worked for a long time
> without it.
Sure, it'll work in systems that have always on regulators.
> Is it guaranteed that the driver keeps working for all cases where
> regulator support is enabled in the kernel, and where it used to work
> so far without mandating the existence of this specific regulator ?
> My main concern is having to deal with complaints that the driver stopped
> working for no good reason.
Sure, that's the transition issues I mentioned - the regulator API does
have stubbing facilities which should cover things and it's very easy to
define stub regulators if you need to. Like I say I expect this to be a
lot easier after the next merge window as another way of doing stubs is
being added which should make this even easier by avoiding disrupting
drivers that do genuinely want to check for absent supplies and handle
that better.
> In this context, is it common practice to name such regulators "vdd"
> or similar ? What if there are multiple LM90 or compatible chips
> in a system, connected to different power rails ? Who determines
> if the regulator is supposed to be named "vdd" or "vcc" or anything
> else, and to which power rails it is actually connected ? How can
> and does one guarantee that "vdd" is the correct regulator to use
> for all systems ? What if some other driver requests "vdd", but the chip
> it supports happens to be connected to a different power rail ?
That's not what the name means - they are nothing to do with the board.
The names requested by a driver are defined with regard to the device
and should be the names used by the chip itself as defined in the
datasheet. A board that uses regulators then maps these onto specific
regulators in the system, the driver doesn't need to know anything about
this process.
On 08/08/2013 02:42 AM, Guenter Roeck wrote:
> On 08/07/2013 11:56 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.
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> static int lm90_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
>> i2c_set_clientdata(client, data);
>> mutex_init(&data->update_lock);
>>
>> + data->lm90_reg = regulator_get(&client->dev, "vdd");
>
> You should use devm_regulator_get(). Then you also don't need the call
> to regulator_put().
>
>> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
>
> The function never returns NULL except if the regulator subsystem is not
> configured,
> so IS_ERR() is more appropriate.
>
> If the regulator subsystem is not configured, you especially don't need
> or want
> to pollute the log with an error message.
DT parsing errors should be reported. However, if there's nothing to
parse, it's not an error.
So, I think this should report an error message *if* there is a DT
property that defines the regulator to use. If there's no property, just
use no regulator. If there is a property, it had better be possible to
parse it.
On 08/08/2013 05:23 AM, Guenter Roeck wrote:
> On 08/08/2013 02:47 AM, Wei Ni wrote:
...
>> I think get_regulator() will return error values, not only
>> -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other
>> error values.
>>
> Matter of opinion if you want to check for IS_ERR or NULL later on.
No, if regulator returns either:
* Something valid
* Someting IS_ERR()
... then everywhere has to check the value using IS_ERR().
If regulator returns either:
* Something valid
* Someting NULL
... then everywhere has to check the value against NULL.
checking against both IS_ERR() and NULL shouldn't ever happen, and
likewise IS_ERR_OR_NULL() is deprecated.
On 08/08/2013 12:56 AM, Wei Ni wrote:
> Enable thermal sensor nct1008 for t114 dalmore.
It'd be best to remove this patch from the series; send one series to
the LM90 maintainer with just the driver and DT binding changes, and
another series to the Tegra maintainer with the *.dts changes. You can
tell me not to apply the *.dts changes until the driver/binding changes
are accepted (or just wait until they are before sending the patch).
On 08/08/2013 12:56 AM, Wei Ni wrote:
> Add OF document for lm90 in Documentation/devicetree/.
> diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
> +Required node properties:
> +- compatible: manufacture and chip name,
> + which are listed in the Documentation/hwmon/lm90
You need to specify the exact values here. DT binding documentations
should be OS-agnostic, and hence can't depend on Linux-specific
documentation or other details.
> +Optional properties:
> +- vdd-supply: vdd regulator for the supply voltage. If this is not set,
s/set/present/.
> +- interrupts: lm90 can support interrupt mode, you can set interrupt here.
You might want to mention that a single entry ("specifier") is expected
in the list.
On Thu, Aug 08, 2013 at 11:33:13AM -0600, Stephen Warren wrote:
> On 08/08/2013 05:23 AM, Guenter Roeck wrote:
> > On 08/08/2013 02:47 AM, Wei Ni wrote:
> ...
> >> I think get_regulator() will return error values, not only
> >> -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other
> >> error values.
> >>
> > Matter of opinion if you want to check for IS_ERR or NULL later on.
>
> No, if regulator returns either:
>
> * Something valid
> * Someting IS_ERR()
>
> ... then everywhere has to check the value using IS_ERR().
>
> If regulator returns either:
>
> * Something valid
> * Someting NULL
>
> ... then everywhere has to check the value against NULL.
>
Other drivers calling get_regulator() don't check against NULL,
so it should not be needed here either.
Guenter
> checking against both IS_ERR() and NULL shouldn't ever happen, and
> likewise IS_ERR_OR_NULL() is deprecated.
>
On Thu, Aug 08, 2013 at 11:30:30AM -0600, Stephen Warren wrote:
> On 08/08/2013 02:42 AM, Guenter Roeck wrote:
> > On 08/07/2013 11:56 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.
>
> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>
> >> static int lm90_probe(struct i2c_client *client,
> >> const struct i2c_device_id *id)
> >> {
> >> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client,
> >> i2c_set_clientdata(client, data);
> >> mutex_init(&data->update_lock);
> >>
> >> + data->lm90_reg = regulator_get(&client->dev, "vdd");
> >
> > You should use devm_regulator_get(). Then you also don't need the call
> > to regulator_put().
> >
> >> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
> >
> > The function never returns NULL except if the regulator subsystem is not
> > configured,
> > so IS_ERR() is more appropriate.
> >
> > If the regulator subsystem is not configured, you especially don't need
> > or want
> > to pollute the log with an error message.
>
> DT parsing errors should be reported. However, if there's nothing to
> parse, it's not an error.
>
> So, I think this should report an error message *if* there is a DT
> property that defines the regulator to use. If there's no property, just
> use no regulator. If there is a property, it had better be possible to
> parse it.
>
Agreed.
Guenter
On 08/08/2013 11:59 AM, Guenter Roeck wrote:
> On Thu, Aug 08, 2013 at 11:33:13AM -0600, Stephen Warren wrote:
>> On 08/08/2013 05:23 AM, Guenter Roeck wrote:
>>> On 08/08/2013 02:47 AM, Wei Ni wrote:
>> ...
>>>> I think get_regulator() will return error values, not only
>>>> -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other
>>>> error values.
>>>>
>>> Matter of opinion if you want to check for IS_ERR or NULL later on.
>>
>> No, if regulator returns either:
>>
>> * Something valid
>> * Someting IS_ERR()
>>
>> ... then everywhere has to check the value using IS_ERR().
>>
>> If regulator returns either:
>>
>> * Something valid
>> * Someting NULL
>>
>> ... then everywhere has to check the value against NULL.
>>
> Other drivers calling get_regulator() don't check against NULL,
> so it should not be needed here either.
Right I should have mentioned that I believe regulator falls into the
first valid-or-IS_ERR case, and not the second valid-or-NULL case.
On Thu, Aug 08, 2013 at 11:30:30AM -0600, Stephen Warren wrote:
> On 08/08/2013 02:42 AM, Guenter Roeck wrote:
> > If the regulator subsystem is not configured, you especially don't need
> > or want
> > to pollute the log with an error message.
> DT parsing errors should be reported. However, if there's nothing to
> parse, it's not an error.
> So, I think this should report an error message *if* there is a DT
> property that defines the regulator to use. If there's no property, just
> use no regulator. If there is a property, it had better be possible to
> parse it.
On a DT system you should get this behaviour simply by paying attention
to the error code from the regualtor API - the core should complain
loudly if the DT is incomprehensible.
On Thu, Aug 08, 2013 at 06:15:54PM +0100, Mark Brown wrote:
> On Thu, Aug 08, 2013 at 08:21:48AM -0700, Guenter Roeck wrote:
> > On 08/08/2013 06:08 AM, Mark Brown wrote:
>
> > >I'd be most surprised if the device worked without power, if the driver
> > >fails to get and enable a regulator for it then that's not great
> > >(especially if it does so silently).
>
> > Correct, but it appears that the driver magically worked for a long time
> > without it.
>
> Sure, it'll work in systems that have always on regulators.
>
> > Is it guaranteed that the driver keeps working for all cases where
> > regulator support is enabled in the kernel, and where it used to work
> > so far without mandating the existence of this specific regulator ?
> > My main concern is having to deal with complaints that the driver stopped
> > working for no good reason.
>
> Sure, that's the transition issues I mentioned - the regulator API does
> have stubbing facilities which should cover things and it's very easy to
> define stub regulators if you need to. Like I say I expect this to be a
> lot easier after the next merge window as another way of doing stubs is
> being added which should make this even easier by avoiding disrupting
> drivers that do genuinely want to check for absent supplies and handle
> that better.
>
We will need to make sure that all dts files using any of the compatible chips
are updated accordingly. There are several entries in various dts files for
adm1032, adt7461, lm90, and nct1008.
> > In this context, is it common practice to name such regulators "vdd"
> > or similar ? What if there are multiple LM90 or compatible chips
> > in a system, connected to different power rails ? Who determines
> > if the regulator is supposed to be named "vdd" or "vcc" or anything
> > else, and to which power rails it is actually connected ? How can
> > and does one guarantee that "vdd" is the correct regulator to use
> > for all systems ? What if some other driver requests "vdd", but the chip
> > it supports happens to be connected to a different power rail ?
>
> That's not what the name means - they are nothing to do with the board.
Ok, makes sense.
> The names requested by a driver are defined with regard to the device
> and should be the names used by the chip itself as defined in the
9 votes for vdd, 11 votes for vcc, one undecided (no datasheet available).
Guess one is as good as the other ;-).
Guenter
> datasheet. A board that uses regulators then maps these onto specific
> regulators in the system, the driver doesn't need to know anything about
> this process.
Hello.
On 08/08/2013 10:56 AM, Wei Ni wrote:
> Enable thermal sensor nct1008 for t114 dalmore.
> Signed-off-by: Wei Ni <[email protected]>
> ---
> 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 {
ePAPR [1] says: "the name of a node should be somewhat generic, reflecting
the function of the device and not its precise programming model". So I
suggest "thermal"
> + compatible = "onnn,nct1008";
> + reg = <0x4c>;
> + vdd-supply = <&palmas_ldo6_reg>;
> + interrupt-parent = <&gpio>;
> + interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
> + };
> };
[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf
WBR, Sergei
On 08/08/2013 02:36 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 08/08/2013 10:56 AM, Wei Ni wrote:
>
>> Enable thermal sensor nct1008 for t114 dalmore.
>
>> Signed-off-by: Wei Ni <[email protected]>
>> ---
>> 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 {
>
> ePAPR [1] says: "the name of a node should be somewhat generic,
> reflecting the function of the device and not its precise programming
> model". So I suggest "thermal"
True, although there's quite some precedent for node-names being the
chip name for external chips in existing DTs. If we change this node
name, I'd like to see a patch that makes all the other "nct1008" nodes
match the new name...
On Thu, Aug 08, 2013 at 01:00:26PM -0700, Guenter Roeck wrote:
> On Thu, Aug 08, 2013 at 06:15:54PM +0100, Mark Brown wrote:
> > Sure, that's the transition issues I mentioned - the regulator API does
> > have stubbing facilities which should cover things and it's very easy to
> > define stub regulators if you need to. Like I say I expect this to be a
> > lot easier after the next merge window as another way of doing stubs is
> > being added which should make this even easier by avoiding disrupting
> > drivers that do genuinely want to check for absent supplies and handle
> > that better.
> We will need to make sure that all dts files using any of the compatible chips
> are updated accordingly. There are several entries in various dts files for
> adm1032, adt7461, lm90, and nct1008.
Yes, and probably also board files as well. Or either just accept
bisection trouble for now or wait till the better stubbing is in there -
that will mean that for DT systems the core will just assume the supply
is really there and not fail requests if it's not in the DT.
> > The names requested by a driver are defined with regard to the device
> > and should be the names used by the chip itself as defined in the
> 9 votes for vdd, 11 votes for vcc, one undecided (no datasheet available).
> Guess one is as good as the other ;-).
What I've suggested before is to use the name from the part for which
the driver is named. Assuming the vendor doesn't randomly change their
datasheet (but that causes problems for hardware engineers so tends to
be avoided).
On 08/08/2013 02:18 PM, Mark Brown wrote:
> On Thu, Aug 08, 2013 at 01:00:26PM -0700, Guenter Roeck wrote:
>> On Thu, Aug 08, 2013 at 06:15:54PM +0100, Mark Brown wrote:
>
>>> Sure, that's the transition issues I mentioned - the regulator API does
>>> have stubbing facilities which should cover things and it's very easy to
>>> define stub regulators if you need to. Like I say I expect this to be a
>>> lot easier after the next merge window as another way of doing stubs is
>>> being added which should make this even easier by avoiding disrupting
>>> drivers that do genuinely want to check for absent supplies and handle
>>> that better.
>
>> We will need to make sure that all dts files using any of the compatible chips
>> are updated accordingly. There are several entries in various dts files for
>> adm1032, adt7461, lm90, and nct1008.
>
> Yes, and probably also board files as well. Or either just accept
> bisection trouble for now or wait till the better stubbing is in there -
Ah, that is exactly the trouble I wanted to avoid.
> that will mean that for DT systems the core will just assume the supply
> is really there and not fail requests if it's not in the DT.
>
>>> The names requested by a driver are defined with regard to the device
>>> and should be the names used by the chip itself as defined in the
>
>> 9 votes for vdd, 11 votes for vcc, one undecided (no datasheet available).
>> Guess one is as good as the other ;-).
>
> What I've suggested before is to use the name from the part for which
> the driver is named. Assuming the vendor doesn't randomly change their
> datasheet (but that causes problems for hardware engineers so tends to
> be avoided).
>
Makes sense.
Guenter
On 08/08/2013 01:40 PM, Stephen Warren wrote:
> On 08/08/2013 02:36 PM, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 08/08/2013 10:56 AM, Wei Ni wrote:
>>
>>> Enable thermal sensor nct1008 for t114 dalmore.
>>
>>> Signed-off-by: Wei Ni <[email protected]>
>>> ---
>>> 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 {
>>
>> ePAPR [1] says: "the name of a node should be somewhat generic,
>> reflecting the function of the device and not its precise programming
>> model". So I suggest "thermal"
>
> True, although there's quite some precedent for node-names being the
> chip name for external chips in existing DTs. If we change this node
> name, I'd like to see a patch that makes all the other "nct1008" nodes
> match the new name...
>
On the other side, one should not use a bad example as an argument or excuse
to make the same mistake again (though I keep hearing it all the time ... ).
I for my part tend to use something like temp-sensor or temp-sensor@1c.
Advantage of that kind of node name is that it auto-describes the node.
Guenter
Hi all.
Instead of adding the support of regulators in each device, let's think about
whether it is possible to create a global regulator for any device on the I2C bus.
I see it like this:
We add an extra field in the i2c_board_info structure "power_name" and handle
it in the i2c_device_{probe/remove} functions.
The question remains how to maintain such regulator in the PM functions,
but we can discuss it.
Changes like this, will remove existing regulators from drivers and allows to
use power regulator for any I2C device.
If such idea is good, such changes could be made to other devices (SPI, w1, etc.).
Thanks.
---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 08/09/2013 01:35 AM, Stephen Warren wrote:
> On 08/08/2013 12:56 AM, Wei Ni wrote:
>> Enable thermal sensor nct1008 for t114 dalmore.
>
> It'd be best to remove this patch from the series; send one series to
> the LM90 maintainer with just the driver and DT binding changes, and
> another series to the Tegra maintainer with the *.dts changes. You can
> tell me not to apply the *.dts changes until the driver/binding changes
> are accepted (or just wait until they are before sending the patch).
Ok, I will do it.
>
On 08/09/2013 01:37 AM, Stephen Warren wrote:
> On 08/08/2013 12:56 AM, Wei Ni wrote:
>> Add OF document for lm90 in Documentation/devicetree/.
>
>> diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
>
>> +Required node properties:
>> +- compatible: manufacture and chip name,
>> + which are listed in the Documentation/hwmon/lm90
>
> You need to specify the exact values here. DT binding documentations
> should be OS-agnostic, and hence can't depend on Linux-specific
> documentation or other details.
Ok, I will list it.
>
>> +Optional properties:
>> +- vdd-supply: vdd regulator for the supply voltage. If this is not set,
>
> s/set/present/.
Oh, yes, I will change it.
>
>> +- interrupts: lm90 can support interrupt mode, you can set interrupt here.
>
> You might want to mention that a single entry ("specifier") is expected
> in the list.
Sorry, I didn't get your point, could you explain more?
>
On 08/09/2013 05:33 AM, Guenter Roeck wrote:
> On 08/08/2013 01:40 PM, Stephen Warren wrote:
>> On 08/08/2013 02:36 PM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 08/08/2013 10:56 AM, Wei Ni wrote:
>>>
>>>> Enable thermal sensor nct1008 for t114 dalmore.
>>>
>>>> Signed-off-by: Wei Ni <[email protected]>
>>>> ---
>>>> 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 {
>>>
>>> ePAPR [1] says: "the name of a node should be somewhat generic,
>>> reflecting the function of the device and not its precise programming
>>> model". So I suggest "thermal"
>>
>> True, although there's quite some precedent for node-names being the
>> chip name for external chips in existing DTs. If we change this node
>> name, I'd like to see a patch that makes all the other "nct1008" nodes
>> match the new name...
>>
>
> On the other side, one should not use a bad example as an argument or excuse
> to make the same mistake again (though I keep hearing it all the time ... ).
> I for my part tend to use something like temp-sensor or temp-sensor@1c.
> Advantage of that kind of node name is that it auto-describes the node.
Ok, so I will set the node name as "temp-sensor"
And I will send out patches to change all other "nct1008" nodes.
>
> Guenter
>
On 08/08/2013 07:01 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote:
>
>> + mutex_lock(&data->update_lock);
>> +
>> + if (is_enable)
>> + ret = regulator_enable(data->lm90_reg);
>> + 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);
>
> Two things here. One is that it's not clear what this lokc is
> protecting since the only thing in the locked region is the regulator
> operation and that is thread safe. The other thing is that I'm not
> seeing anthing that ensures that enables and disables are matched -
> regulators are reference counted so two enables need two disables.
>
>> + data->lm90_reg = regulator_get(&client->dev, "vdd");
>> + if (IS_ERR_OR_NULL(data->lm90_reg)) {
>
> NULL is a valid regulator, use IS_ERR().
>
>> + 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));
>
> You shouldn't just be ignoring errors here, though there are deployment
> difficulties with making sure a stub regulator is provided. These
> should be getting easier after the next merge window, the stubs will be
> being tweaked slightly to have an "assume it's there" option even when
Oh, really, could you show me the patch, I wish to take a look :)
Wei.
> regulators are used. Especially in cases with device tree you should be
> paying attention to -EPROBE_DEFER, that will accurately reflect if a
> regulator is present but not loaded yet.
>
> That said if you *are* going to do this you should request the
> regulator using devm_regulator_get_optional(), this is intended to
> support things that don't need regulators (though that's not the case
> here).
>
> * Unknown Key
> * 0x7EA229BD
>
On Fri, Aug 09, 2013 at 09:57:00AM +0400, Alexander Shiyan wrote:
> Instead of adding the support of regulators in each device, let's think about
> whether it is possible to create a global regulator for any device on the I2C bus.
> I see it like this:
> We add an extra field in the i2c_board_info structure "power_name" and handle
> it in the i2c_device_{probe/remove} functions.
This would need to be an array of supplies, relatively few devices need
only a single power supply. This is also not something that should be
handled in I2C, power is not something that's uniquely needed by devices
on an I2C bus.
> The question remains how to maintain such regulator in the PM functions,
> but we can discuss it.
Well, there's some basic stuff for this for clocks already - a similar
pattern should work. You need to use runtime PM to hook in the power
management and it's not going to work for everything (especially things
that can be wake sources) but there's some value in trying to factor out
the basic use case.
> On Fri, Aug 09, 2013 at 09:57:00AM +0400, Alexander Shiyan wrote:
>
> > Instead of adding the support of regulators in each device, let's think about
> > whether it is possible to create a global regulator for any device on the I2C bus.
>
> > I see it like this:
> > We add an extra field in the i2c_board_info structure "power_name" and handle
> > it in the i2c_device_{probe/remove} functions.
>
> This would need to be an array of supplies, relatively few devices need
> only a single power supply. This is also not something that should be
> handled in I2C, power is not something that's uniquely needed by devices
> on an I2C bus.
Additional regulators can be handled in the driver, or "parent"-scheme can
be used. Is not it?
---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Fri, Aug 09, 2013 at 03:23:31PM +0800, Wei Ni wrote:
> On 08/08/2013 07:01 PM, Mark Brown wrote:
> > You shouldn't just be ignoring errors here, though there are deployment
> > difficulties with making sure a stub regulator is provided. These
> > should be getting easier after the next merge window, the stubs will be
> > being tweaked slightly to have an "assume it's there" option even when
> Oh, really, could you show me the patch, I wish to take a look :)
No patch quite yet - the basic idea is that for plain regulator_get()
you'll only ever get -EPROBE_DEFER as an error, not -ENODEV. If the
regulator is missing in the DT we assume it's there and provide a dummy
if the DT doesn't hook it up. If the consumer genuinely wants to see if
a supply might not be wired up it should use regulator_get_optional()
which is in -next at the minute.
On Fri, Aug 09, 2013 at 02:50:11PM +0400, Alexander Shiyan wrote:
> > This would need to be an array of supplies, relatively few devices need
> > only a single power supply. This is also not something that should be
> > handled in I2C, power is not something that's uniquely needed by devices
> > on an I2C bus.
> Additional regulators can be handled in the driver, or "parent"-scheme can
> be used. Is not it?
Given how common it is to have multiple supplies it seems sensible to
just handle that in the core - it's common to have a digital supply and
an analogue supply for example. The regulator framework already has
APIs for working with multiple regulators in a single call so it's not
going to be much more effort on the framework side.
On 08/09/2013 12:10 AM, Wei Ni wrote:
> On 08/09/2013 01:37 AM, Stephen Warren wrote:
>> On 08/08/2013 12:56 AM, Wei Ni wrote:
>>> Add OF document for lm90 in Documentation/devicetree/.
>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
>>> +- interrupts: lm90 can support interrupt mode, you can set interrupt here.
>>
>> You might want to mention that a single entry ("specifier") is expected
>> in the list.
>
> Sorry, I didn't get your point, could you explain more?
In general, a device may have more than one interrupt output. Hence in
general, the interrupts property is a list of 1 or more interrupt
specifiers. In this case, that list should be a single entry. The
binding should state this. how about:
----------
Optional properties:
interrupts: Contains a single interrupt specifier which describes the
LM90 XXXX output. See interrupt-controller/interrupts.txt for the format.
----------
(where you'd replace XXXX with the pin/signal name from the datasheet).