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

On 08/07/2013 03:35 AM, Wei Ni wrote:

On 08/07/2013 04:45 PM, Alexander Shiyan wrote:

quoted

quoted

On 08/07/2013 03:50 PM, Guenter Roeck wrote:

quoted

On 08/07/2013 12:32 AM, Wei Ni wrote:

quoted

On 08/07/2013 03:27 PM, Alexander Shiyan wrote:

quoted

quoted

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 <redacted>
---
  drivers/hwmon/lm90.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++

[...]

quoted

+	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.