Message-ID: <20200605155903.GI5413@sirena.org.uk>
Date:   Fri, 5 Jun 2020 16:59:03 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Marek Szyprowski <m.szyprowski@...sung.com>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Dmitry Osipenko <digetx@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>, peron.clem@...il.com,
        Nishanth Menon <nm@...com>, Stephen Boyd <sboyd@...nel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Rafael Wysocki <rjw@...ysocki.net>,
        linux-samsung-soc@...r.kernel.org,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Saravana Kannan <saravanak@...gle.com>
Subject: Re: [PATCH] regulator: do not balance 'boot-on' coupled regulators
 without constraints

On Fri, Jun 05, 2020 at 03:37:32PM +0200, Marek Szyprowski wrote:
> On 05.06.2020 12:20, Mark Brown wrote:

> > No, this is not what boot-on means at all.  It is there for cases where
> > we can't read the enable status from the hardware.  Trying to infer
> > *anything* about the runtime behaviour from it being present or absent
> > is very badly broken.

> Okay, what about the 'always-on' property? I don't think that we need 
> another property for annotating this behavior, as in my opinion this is 

No, that's just as disconnected from the need - we may as well do it
based on the regulator name being an odd number of characters.

> just an implementation issue on the Linux kernel and regulator 
> framework. Alternatively I can drop the property check, but then it 
> won't be possible to have a regulator without a consumer, which follows 
> the other one (although we still don't have a real use case for it).

> If you don't like this idea at all, I will try to move this logic to the 
> custom coupler again, although it would mean some code copying.

I think that's better TBH.

> > Saravana (CCed) was working on some patches which tried to deal with
> > some stuff around this for enables using the sync_state() callback.
> > Unfortunately there's quite a few problems with the current approach
> > (the biggest one from my point of view being that it's implemented so
> > that it requires every single consumer of every device on the PMIC to
> > come up but there's others at more of an implementation level).

> I'm not sure if we really need such complex solution for this...

So I think that the specific approach there is overly heavyweight and
restrictive but I do see the general use case here for something per
regulator providing we can avoid breaking anything that does actually
need to change the regulator state (eg, raising the voltage for
cpufreq).  Previously to the past week I'd only really heard about it
causing problems in the context of displays left on by the bootloader
glitching during boot but this is a concrete use case and we already
have the infrastructure to track dependencies at the device model level
if we use it well.  

OTOH if you have a coupler already that needs to be doing stuff all the
time at runtime it may be easier to just put this in the coupler,
especially I think in this case where the lack of the devfreq driver
wouldn't mean that the hardware being controlled wasn't being used at
all.  The coupler would end up backstopping a missing cpufreq or devfreq
driver.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists