[PATCH v4 0/3] Add power control for lm90

[PATCH v4 0/3] Add power control for lm90

[Wei Ni]

The device lm90 can be controlled by the vcc rail.
Add function to power on/off the vcc.

This series must base on Mark's new dummy regulator support.

This series is v4, previous version patches are:
[v3]: http://www.spinics.net/lists/lm-sensors/msg39503.html
[v2]: http://www.spinics.net/lists/arm-kernel/msg265373.html
[v1]: http://www.mail-archive.com/linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg12034.html

Changes from v3:
1. change to use devm_regulator_get, as per discussion in previous patches.
2. just return errors when get error from regulator_get(), Mark's new dummy
regulator can cope with errors.
3. add vendor prefix for GMT
3. list manufacturer in the binding doc.

Changes from v2:
1. use devm_regulator_get_optional(), as suggested by Mark.
2. remove the lm90_power_control(), enable the regulator in the probe()
directly, so the codes are more clear, as suggested by Alexander and Guenter.
3. change the binding documentation, as per Stephen suggestion.
4. couple of changes, as per previous review.

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
  of: add vendor prefix for GMT
  Documentation: dt: hwmon: add OF document for LM90

 Documentation/devicetree/bindings/hwmon/lm90.txt   |   44 ++++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 drivers/hwmon/lm90.c                               |   20 +++++++++
 3 files changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/lm90.txt

-- 
1.7.9.5

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

[Wei Ni]

The device lm90 can be controlled by the vcc rail.
Adding the regulator support to power on/off the vcc rail.
Enable the "vcc" regulator before accessing the device.

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index cdff742..f3f66d5 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/regulator/consumer.h>
+#include <linux/delay.h>
 
 /*
  * Addresses to scan
@@ -302,6 +304,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;
@@ -1397,8 +1400,20 @@ static int lm90_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct i2c_adapter *adapter = to_i2c_adapter(dev->parent);
 	struct lm90_data *data;
+	struct regulator *reg;
 	int err;
 
+	reg = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	err = regulator_enable(reg);
+	if (err < 0) {
+		dev_err(&client->dev,
+			"Failed to enable regulator: %d\n", err);
+		return err;
+	}
+
 	data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -1406,6 +1421,8 @@ static int lm90_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 
+	data->lm90_reg = reg;
+
 	/* Set the device type */
 	data->kind = id->driver_data;
 	if (data->kind == adm1032) {
@@ -1473,6 +1490,8 @@ exit_remove_files:
 	lm90_remove_files(client, data);
 exit_restore:
 	lm90_restore_conf(client, data);
+	regulator_disable(data->lm90_reg);
+
 	return err;
 }
 
@@ -1483,6 +1502,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);
+	regulator_disable(data->lm90_reg);
 
 	return 0;
 }
-- 
1.7.9.5

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]

On Thu, Sep 12, 2013 at 07:24:19PM +0800, Wei Ni wrote:

> +#include <linux/delay.h>

You don't need this any more, otherwise

Reviewed-by: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Wei Ni]

On 09/12/2013 07:51 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Sep 12, 2013 at 07:24:19PM +0800, Wei Ni wrote:
> 
>> +#include <linux/delay.h>
> 
> You don't need this any more, otherwise

Oh, yes, I will remove it.

> 
> Reviewed-by: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> * Unknown Key
> * 0x7EA229BD
> 

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

[Guenter Roeck]

On 09/12/2013 04:24 AM, Wei Ni wrote:
> The device lm90 can be controlled by the vcc rail.
> Adding the regulator support to power on/off the vcc rail.
> Enable the "vcc" regulator before accessing the device.
>
> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I still believe that you should update all .dts files referencing this driver
to match the new requirement.

Guenter

> ---
>   drivers/hwmon/lm90.c |   20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index cdff742..f3f66d5 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/regulator/consumer.h>
> +#include <linux/delay.h>
>
>   /*
>    * Addresses to scan
> @@ -302,6 +304,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;
> @@ -1397,8 +1400,20 @@ static int lm90_probe(struct i2c_client *client,
>   	struct device *dev = &client->dev;
>   	struct i2c_adapter *adapter = to_i2c_adapter(dev->parent);
>   	struct lm90_data *data;
> +	struct regulator *reg;
>   	int err;
>
> +	reg = devm_regulator_get(dev, "vcc");
> +	if (IS_ERR(reg))
> +		return PTR_ERR(reg);
> +
> +	err = regulator_enable(reg);
> +	if (err < 0) {
> +		dev_err(&client->dev,
> +			"Failed to enable regulator: %d\n", err);
> +		return err;
> +	}
> +
>   	data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
> @@ -1406,6 +1421,8 @@ static int lm90_probe(struct i2c_client *client,
>   	i2c_set_clientdata(client, data);
>   	mutex_init(&data->update_lock);
>
> +	data->lm90_reg = reg;
> +
>   	/* Set the device type */
>   	data->kind = id->driver_data;
>   	if (data->kind == adm1032) {
> @@ -1473,6 +1490,8 @@ exit_remove_files:
>   	lm90_remove_files(client, data);
>   exit_restore:
>   	lm90_restore_conf(client, data);
> +	regulator_disable(data->lm90_reg);
> +
>   	return err;
>   }
>
> @@ -1483,6 +1502,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);
> +	regulator_disable(data->lm90_reg);
>
>   	return 0;
>   }
>

[PATCH v4 2/3] of: add vendor prefix for GMT

[Wei Ni]

Adding Global Mixed-mode Technology Inc. to the list
of devicetree vendor prefixes.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 2956800..634c35f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -28,6 +28,7 @@ est	ESTeem Wireless Modems
 fsl	Freescale Semiconductor
 GEFanuc	GE Fanuc Intelligent Platforms Embedded Systems, Inc.
 gef	GE Fanuc Intelligent Platforms Embedded Systems, Inc.
+gmt	Global Mixed-mode Technology Inc.
 hisilicon	Hisilicon Limited.
 hp	Hewlett Packard
 ibm	International Business Machines (IBM)
-- 
1.7.9.5

Re: [PATCH v4 2/3] of: add vendor prefix for GMT

[Stephen Warren]

On 09/12/2013 05:24 AM, Wei Ni wrote:
> Adding Global Mixed-mode Technology Inc. to the list
> of devicetree vendor prefixes.

> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt

> +gmt	Global Mixed-mode Technology Inc.

There's some opportunity for confusion here re: the GMT timezone, but
GMT seems to be what the company abbreviates itself to on their website,
so I guess this is reasonable.

Acked-by: Stephen Warren <swarren@nvidia.com>

[PATCH v4 3/3] Documentation: dt: hwmon: add OF document for LM90

[Wei Ni]

Add OF document for LM90 in Documentation/devicetree/.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 Documentation/devicetree/bindings/hwmon/lm90.txt |   44 ++++++++++++++++++++++
 1 file changed, 44 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..6888498
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
@@ -0,0 +1,44 @@
+* LM90 series thermometer.
+
+Required node properties:
+- compatible: manufacturer and chip name, one of
+		"adi,adm1032"
+		"adi,adt7461"
+		"adi,adt7461a"
+		"gmt,g781"
+		"national,lm90"
+		"national,lm86"
+		"national,lm89"
+		"national,lm99"
+		"dallas,max6646"
+		"dallas,max6647"
+		"dallas,max6649"
+		"dallas,max6657"
+		"dallas,max6658"
+		"dallas,max6659"
+		"dallas,max6680"
+		"dallas,max6681"
+		"dallas,max6695"
+		"dallas,max6696"
+		"onnn,nct1008"
+		"winbond,w83l771"
+		"nxp,sa56004"
+
+- reg: I2C bus address of the device
+
+Optional properties:
+- vcc-supply: vcc regulator for the supply voltage.
+              If this is not set, assuming vcc is always powered.
+- interrupts: Contains a single interrupt specifier which describes the
+              LM90 "-ALERT" pin output.
+              See interrupt-controller/interrupts.txt for the format.
+
+Example LM90 node:
+
+temp-sensor {
+	compatible = "onnn,nct1008";
+	reg = <0x4c>;
+	vcc-supply = <&palmas_ldo6_reg>;
+	interrupt-parent = <&gpio>;
+	interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
+}
-- 
1.7.9.5

Re: [PATCH v4 3/3] Documentation: dt: hwmon: add OF document for LM90

[Stephen Warren]

On 09/12/2013 05:24 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

> +Optional properties:
> +- vcc-supply: vcc regulator for the supply voltage.
> +              If this is not set, assuming vcc is always powered.

I thought that was going to be a "Required Property".

Re: [PATCH v4 3/3] Documentation: dt: hwmon: add OF document for LM90

[Wei Ni]

On 09/12/2013 11:00 PM, Stephen Warren wrote:
> On 09/12/2013 05:24 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
> 
>> +Optional properties:
>> +- vcc-supply: vcc regulator for the supply voltage.
>> +              If this is not set, assuming vcc is always powered.
> 
> I thought that was going to be a "Required Property".

There may have many platforms without regulator, they always power on
this device, so how can they set this "vcc-supply "property if we set it
as "Required Property".
Set to dummy-regulator? but it seems the regulator framework can't
handle it yet.

Wei.

> 

Re: [PATCH v4 3/3] Documentation: dt: hwmon: add OF document for LM90

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Mon, Sep 16, 2013 at 12:28:43PM +0800, Wei Ni wrote:
> On 09/12/2013 11:00 PM, Stephen Warren wrote:

> >> +- vcc-supply: vcc regulator for the supply voltage.
> >> +              If this is not set, assuming vcc is always powered.

> > I thought that was going to be a "Required Property".

> There may have many platforms without regulator, they always power on
> this device, so how can they set this "vcc-supply "property if we set it
> as "Required Property".
> Set to dummy-regulator? but it seems the regulator framework can't
> handle it yet.

To repeat once more: these platforms will physically have a regulator,
the device won't function without power and unfortunately the device
tree bindings only have optional and required properties.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]