Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Mark Brown]

On Mon, Jun 08, 2020 at 08:16:44PM -0700, Saravana Kannan wrote:
> On Mon, Jun 1, 2020 at 10:23 AM Mark Brown <broonie@xxxxxxxxxx> wrote:

> > This is I think the first time anyone has suggested that this is likely
> > to be an issue - the previous concerns have all been about screens
> > glitching.

> Looks like we got at least one concrete example now in [1].

That's the Exynos VDD_INT/CPU issue.  I'm not clear that this is
entirely covered TBH, AIUI it needs a coupler all the time so it's not
just a case of waiting for the consumers.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

> But it doesn't really ignore consumer requests though. In response to
> all your comments above, I haven't done a good job of explaining the
> issues and the solution. I'll try to redo that part again when I send
> v3 and hopefully I can do better.

Yes, I think a lot of this is about clarity of explanation.

> > My concern is that introducing extra delays is likely to make things
> > more fragile and complex.  As far as I can see this is just making
> > things even worse by adding spurious dependencies and delaying things
> > further.

> I wouldn't call it delaying any requests. This is just an additional
> constraint like any other consumer. This definitely makes things more
> stable in cases where all the devices probe and even in cases where
> some of those devices might never probe (example I gave in [2]).

Since your current implementation restricts things until essentially the
entire system is running this is going to affect consumers that don't
share their regulator.  Part of the reason I am so against that idea is
that when it is very important that a driver be able to change the
voltage and have that actually take effect usually the hardware will be
built such that that regulator isn't shared so shareability issues don't
apply, we have regulator_get_exclusive() for such situations though
it's wound up not as widely used as it could be for a bunch of reasons.
Things like MMC where we have to conform to a hardware spec that
includes lowering as well as raising voltages will have issues with
this.

> > At best this is working around a problem that has been
> > introduced by the decision to do everything at far too broad a level,
> > it feels like it's defeating the point of trying to track dependencies
> > at all.  The whole performance seems completely redundant if we're
> > trying to aggregate things so aggressively, we end up needing pretty
> > much every device in the system to come up before we can do anything.

> This is definitely not true on real Android phones. Keep in mind all
> of this only applies to resources actually left on by the bootloader.
> So, that drastically cuts down what all devices are affected by this.
> So, if a PMIC has 5 regulators and only 1 is left on by the
> bootloader, then this patch series would be a NOP for the 4
> regulators.

This is your systems - it is not that unusual for systems to come up
with most if not all of the regulators up in order to keep things
simpler, it's generally easier during bringup and people don't always
see any particular reason to modify the bootloader to change things (and
may not be easily able to customize what the PMIC does itself even if
they wanted to do things at that level).  Such systems will be very much
impacted by this change.

Attachment: signature.asc
Description: PGP signature