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

[Guenter Roeck]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/