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

[Ulf Hansson]

On Tue, 9 Jun 2020 at 12:51, Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> 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.
>

eMMC is not only about voltage levels, but also about enable/disable
of the regulator(s).

More precisely, one needs to follow the steps specified in the eMMC
spec, when disabling the regulator(s).

In other words, the mmc host driver needs to be probed (consumer of
the regulator), before the regulator(s) can be disabled.

Kind regards
Uffe