Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3484 bytes --]

On Sun, Feb 05, 2017 at 08:30:33PM -0800, Dmitry Torokhov wrote:
> On Sun, Feb 05, 2017 at 05:07:37PM +0100, Mark Brown wrote:

> > The tlv320aic32x4 driver isn't a particularly well written driver in
> > this regard in the first place - I don't recall the details but I
> > strongly suspect that the driver is an example of abuse of the optional
> > API and that of the supplies possibly only ldoin is actually optional.
> > I would expect that this should look *much* more like sgtl5000, or

> OK, I think the optional support in regulator_bulk_get will help
> sgtl5000 as well, as you would not have to go through contortions of
> requesting the optional regulator first and then figuring out if it
> should be included or excluded from the bulk supply list.

It does make things better, though I'm not sure that simply open coding
the optional regulators wouldn't be just as good.  I'm not 100% sure
that this is what the driver actually should be doing - there were lots
of problems with that driver figuring out what on earth it was supposed
to do as there were some older revisions of the chip with very serious
errata which used to be handled in the driver but broken the production
versions.  There's a degree of "it works, don't touch it" going on.

> > I'd be a lot happier if I were seeing examples where this helped a lot
> > and the original code looked good, I've not really been seeing those
> > though.

> If you can point me at a few examples where original code looks good I
> can see how it can be converter to bulk with optionals support.

The main one is the MMC core (which is what this was written for).

> >  A lot of the examples of use of optional regulators that I see
> > (including both those in drivers/input FWIW) don't look like they are
> > for supplies that should be optional.

> Fair enough. The regulators there are not optional, rather we are trying
> to avoid delays (i.e. doing msleep(N)) if regulators are not descried by
> the firmware (incomplete DT in OF system, or ACPI system). Will you
> accept:

> bool regulator_is_dummy(struct regulator *r)
> {
> 	return r == dummy_regulator_rdev;
> }

This is obviously a total failure of abstraction.

> or maybe even better

> bool regulator_is_always_on(struct regulator *r)
> {
> 	return r->always_on;
> }

> Then we can switch to straightforward (non-optional) regulator_get() in
> input and still avoid waiting when we don't actually control regulators.

This is also an abstraction failure, though it is better, and doesn't
quite do what you want since if the regulator is shared you may still
end up delaying when you don't need to.  What the drivers should be
doing is registering a notifier which they use to check when the
regulator was last enabled and using that to tell them if they need to
delay (with bonus points for adjusting the delay if we're part way
through the needed time).

It's probably worth having a helper which registers notifiers and
provides query functions that wraps this up, updates a timestamp for the
last enable and has a helper drivers can use to check if it was within a
given period, this is probably going to be a useful enough pattern for
that.  Drivers can then just enable and ask if how long ago the last
physical enable happened.

Depending on how severe the delays are and if anything else needs doing
(like resetting registers) a simpler thing can be to use a notifier to
determine if the power has been removed and reinitialize if that's
happened.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]