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

[Guenter Roeck <linux@roeck-us.net>]

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.

Guenter