Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage

From: Doug Anderson <dianders@chromium.org>
To: Mark Brown <broonie@kernel.org>
Cc: Marco Felsch <m.felsch@pengutronix.de>,
	zhang.chunyan@linaro.org, Liam Girdwood <lgirdwood@gmail.com>,
	ckeepax@opensource.cirrus.com,
	LKML <linux-kernel@vger.kernel.org>,
	Sascha Hauer <kernel@pengutronix.de>
Subject: Re: [PATCH 1/3] regulator: core: fix boot-on regulators use_count usage
Date: Mon, 23 Sep 2019 11:36:11 -0700	[thread overview]
Message-ID: <CAD=FV=WVGj8xzKFFxsjpeuqtVzSvv22cHmWBRJtTbH00eC=E9w@mail.gmail.com> (raw)
In-Reply-To: <20190923181431.GU2036@sirena.org.uk>

Hi,

On Mon, Sep 23, 2019 at 11:14 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Sep 23, 2019 at 11:02:26AM -0700, Doug Anderson wrote:
>
> > I will freely admit my ignorance here, but I've always been slightly
> > confused by the "always-on" vs. "boot-on" distinction...
>
> > The bindings say:
>
> >   regulator-always-on:
> >     description: boolean, regulator should never be disabled
>
> >   regulator-boot-on:
> >     description: bootloader/firmware enabled regulator
>
> > For 'boot-on' that's a bit ambiguous about what it means.  The
> > constraints have a bit more details:
>
> Boot on means that it's powered on when the kernel starts, it's
> for regulators that we can't read back the status of.

1. Would it be valid to say that it's always incorrect to set this
property if there is a way to read the status back from the regulator?

2. Would this be a valid description of how the property is expected to behave
a) At early boot this regulator will be turned on if it wasn't already on.
b) If no clients are found for this regulator after everything has
loaded, this regulator will be automatically disabled.

If so then I don't _think_ #2b is happening, but I haven't confirmed.

> > ...but then that begs the question of why we have two attributes?
> > Maybe this has already been discussed before and someone can point me
> > to a previous discussion?  We should probably make it more clear in
> > the bindings and/or the constraints.
>
> boot-on just refers to the status at boot, we can still turn
> those regulators off later on if we want to.

How, exactly?  As of my commit 5451781dadf8 ("regulator: core: Only
count load for enabled consumers") if you do:

  r = regulator_get(...)
  regulator_disable(r)

...then you'll get "Underflow of regulator enable count".  In other
words, if a given regulator client disables more times than it enables
then you will get an error.  Since there is no client that did the
initial "boot" enable then there's no way to do the disable unless it
happens automatically (as per 2b above).

...or do you mean that people could call regulator_force_disable()?
Couldn't they also do that with an always-on regulator?

-Doug