Messages in this thread | | | Date | Mon, 09 Sep 2013 22:54:38 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH v3 1/2] hwmon: (lm90) Add power control |
| |
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.
Guenter
| |