LinuxLists.cc - [PATCH v3 0/2] Add power control for lm90

2013-09-09 10:28:53

Subject: [PATCH v3 0/2] Add power control for lm90

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

This series is v3, previous version patches are:
[v2]: http://www.spinics.net/lists/arm-kernel/msg265373.html
[v1]: http://www.mail-archive.com/[email protected]/msg12034.html

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 (2):
hwmon: (lm90) Add power control
Documentation: dt: hwmon: add OF document for LM90

Documentation/devicetree/bindings/hwmon/lm90.txt | 44 ++++++++++++++++++++++
drivers/hwmon/lm90.c | 25 ++++++++++++
2 files changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/lm90.txt

--
1.7.9.5

2013-09-09 10:29:15

by Wei Ni

[permalink] [raw]

Subject: [PATCH v3 1/2] hwmon: (lm90) Add power control

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 <[email protected]>
---
drivers/hwmon/lm90.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index cdff742..bd313f4 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,23 @@ 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_optional(dev, "vcc");
+ if (!IS_ERR(reg)) {
+ err = regulator_enable(reg);
+ if (err < 0) {
+ dev_err(&client->dev,
+ "Failed to enable regulator: %d\n", err);
+ return err;
+ }
+ msleep(25);
+ } else {
+ if (PTR_ERR(reg) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ }
+
data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
if (!data)
return -ENOMEM;
@@ -1406,6 +1424,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 +1493,9 @@ exit_remove_files:
lm90_remove_files(client, data);
exit_restore:
lm90_restore_conf(client, data);
+ if (!IS_ERR(data->lm90_reg))
+ regulator_disable(data->lm90_reg);
+
return err;
}

@@ -1483,6 +1506,8 @@ static int lm90_remove(struct i2c_client *client)
hwmon_device_unregister(data->hwmon_dev);
lm90_remove_files(client, data);
lm90_restore_conf(client, data);
+ if (!IS_ERR(data->lm90_reg))
+ regulator_disable(data->lm90_reg);

return 0;
}
--
1.7.9.5

2013-09-09 10:29:18

by Wei Ni

[permalink] [raw]

Subject: [PATCH v3 2/2] Documentation: dt: hwmon: add OF document for LM90

On 09/10/2013 01:54 PM, Guenter Roeck wrote:
> On 09/09/2013 10:39 PM, Wei Ni wrote:
>> On 09/10/2013 12:50 PM, Guenter Roeck wrote:
>>> On 09/09/2013 09:05 PM, Wei Ni wrote:
>>>> On 09/10/2013 04:39 AM, Mark Brown wrote:
>>>>> * PGP Signed by an unknown key
>>>>>
>>>>> On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote:
>>>>>> On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote:
>>>>>
>>>>>>> It does, though it gets complicated trying to use it for a case like
>>>>>>> this since you can't really tell if the regulator was powered on
>>>>>>> immediately before the device got probed by another device on the bus.
>>>>>
>>>>>> Why not ? Just keep a timestamp.
>>>>>
>>>>> The support is a callback on state changes; we could keep a timestamp
>>>>> but there's still going to be race conditions around bootloaders. It's
>>>>> doable though.
>>>>>
>>>>>>>> On a higher level, I wonder if such functionality should be added in the i2c
>>>>>>>> subsystem and not in i2c client drivers. Has anyone thought about this ?
>>>>>
>>>>>>> I'm not sure what the subsystem would do for such delays? It's fairly
>>>>>>> common for things that need this to also want to do things like
>>>>>>> manipulate GPIOs as part of the power on sequence so the applicability
>>>>>>> is relatively limited, plus it's not even I2C specific, the same applies
>>>>>>> to other buses so it ought to be a driver core thing.
>>>>>
>>>>>> Possibly. I just thought about i2c since it also takes care of basic
>>>>>> devicetree bindings. Something along the line of
>>>>>> if devicetree bindings for this device declare one or more
>>>>>> regulators, enable those regulators before calling the driver
>>>>>> probe function.
>>>>>
>>>>> That's definitely a driver core thing, not I2C - there's nothing
>>>>> specific to I2C in there at all, needing power is pretty generic. I
>>>>> have considered this before, something along the lines of what we have
>>>>> for pinctrl, but unfortunately the generic case isn't quite generic
>>>>> enough to make it easy. It'd need to be an explicit list of regulators
>>>>> (partly just to make it opt in and avoid breaking things) and you'd want
>>>>> to have a way of handling the different suspend/resume behaviour that
>>>>> devices want. There's a few patterns there.
>>>>>
>>>>> It's definitely something I think about from time to time and it would
>>>>> be useful to factor things out, the issue is getting a good enough model
>>>>> of what's going on.
>>>>>
>>>>>>> There was some work on a generic helper for power on sequences but it
>>>>>>> stalled since it wasn't accepted for the original purpose (LCD panel
>>>>>>> power ons IIRC).
>>>>>
>>>>>> Too bad. I think it could be kept quite simple, though, by handling it
>>>>>> through the regulator subsystem as suggested above. A generic binding
>>>>>> for a per-regulator and per-device poweron delay should solve that
>>>>>> and possibly even make it transparent to the actual driver code.
>>>>>
>>>>> Lots of things have a GPIO for reset too, and some want clocks too. For
>>>>> maximum usefulness this should be cross subsystem. I suspect the reset
>>>>> controller API may be able to handle some of it.
>>>>>
>>>>> The regulator power on delays are already handled transparently, by the
>>>>> time regulator_enable() returns the ramp should be finished.
>>>>
>>>> I think the regulator should encoded its own startup delay. Each
>>>> individual device should handle its own requirements for delay after
>>>> power is stable.
>>>> The regulator_enable() will handle the delays for the regulator device.
>>>> And adding the msleep(25) is for lm90 device. If without delay,
>>>> sometimes the device can't work properly. If read lm90 register
>>>> immediately after enabling regulator, the reading may be failed.
>>>> I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max
>>>> of "SMBus Clock Low Time" is 25ms, so I supposed that it may need about
>>>> 25ms to stable after power on.
>>>>
>>>
>>> Problem is that you are always waiting, even if the same regulator was
>>> turned on already, and even if it is a dummy regulator.
>>>
>>> Imagine every driver doing that. Booting would take forever, just because of
>>> unnecessary delays all over the place. There has to be a better solution
>>> which does not include a mandatory and potentially unnecessary wait time
>>> in the driver. At a previous company we had a design with literally dozens
>>> of those chip. You really want to force such a boot delay on every user ?
>>>
>>> But essentially you don't even know if it is needed; you are just guessing.
>>> That is not an acceptable reason to add such a delay, mandatory or not.
>> I think the device need time to wait stable after power on, but it's
>> difficult to get an exact delay value, and this delay may also relate
>> with platform design, so how about to add a optional property in the DT
>> node, such as "power-on-delay-ms" ?
>>
>
> Possibly, but that still doesn't solve the problem that you are going
> to wait even if the regulator was already turned on. Simple example:
> A system with two sensors, both of which share the same regulator.
> Each of them will require a delay after turning on power,
> but only if it was just turned on and not if it was already active.

Yes, but as Mark said, the regulator subsystem still can't know if the
regulator was powered on.
Do you have any suggestions?

>
> Guenter
>

2013-09-10 10:10:55

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] hwmon: (lm90) Add power control

On Mon, Sep 09, 2013 at 10:13:56PM -0600, Stephen Warren wrote:
> On 09/09/2013 09:53 PM, Guenter Roeck wrote:

> > Earlier comments suggest that this is not the intended use case for
> > regulator_get_optional().

That's right.

> Isn't the issue only whether the optional aspect of the regulator is
> implemented by:

> a) regulator_get_optional() returning failure, then the driver having to
> check for that and either using or not-using the regulator.

> b) regulator_get_optional() returning a dummy regulator automatically
> when none is specified in DT or the regulator lookup table, and hence
> the driver can always call regulator_enable/disable on the returned value.

No. There are a couple of issues here. One is that we don't want to
litter all drivers with conditional code to check if they actually got
the regulator and so on, that's just pointless make work on the part of
consumers. The other is that just ignoring errors is generally terrible
practice which we don't want to encourage - ignoring the specific case
where nothing is provided and the system has control of that is one
thing but just ignoring any error is another.

Attachments:

(No filename) (1.12 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-09-10 10:14:27

by Wei Ni

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] hwmon: (lm90) Add power control

On 09/11/2013 02:52 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Sep 10, 2013 at 12:37:47PM -0600, Stephen Warren wrote:
>
>> OK, so for the DT binding we should make vcc-supply a required
>> property, yet the driver will still work OK if that property just
>> happens to be missing (or e.g. when instantiated from a board file,
>> and there's no regulator).
>
> Yup. That way we've got both the binding and code trying to make things
> work, hopefully that'll maximise robustness.

Ok, it looks like regulator_get will handle all things, looking forward
to your patches :)
Then I think my changes will be simple, just something like:
+ reg = devm_regulator_get(dev, "vcc");
+ if (!IS_ERR(reg)) {
+ err = regulator_enable(reg);
+ if (err < 0)
+ return err;
+ } else {
+ return PTR_ERR(reg);
+ }

Wei.

>
> * Unknown Key
> * 0x7EA229BD
>