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

From: Doug Anderson <dianders@chromium.org>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: zhang.chunyan@linaro.org, Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	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:02:26 -0700	[thread overview]
Message-ID: <CAD=FV=W7M8mwQqnPyU9vsK5VAdqqJdQdyxcoe9FRRGTY8zjnFw@mail.gmail.com> (raw)
In-Reply-To: <20190917154021.14693-2-m.felsch@pengutronix.de>

Hi,

On Tue, Sep 17, 2019 at 8:40 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Since commit 1fc12b05895e ("regulator: core: Avoid propagating to
> supplies when possible") regulators marked with boot-on can't be
> disabled anymore because the commit handles always-on and boot-on
> regulators the same way.
>
> Now commit 05f224ca6693 ("regulator: core: Clean enabling always-on
> regulators + their supplies") changed the regulator_resolve_supply()
> logic a bit by using 'use_count'. So we can't just skip the
> 'use_count++' during set_machine_constraints(). The easiest way I found
> is to correct the 'use_count' just before returning the rdev device
> during regulator_register().
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/regulator/core.c | 5 +++++
>  1 file changed, 5 insertions(+)

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:

 * @always_on: Set if the regulator should never be disabled.
 * @boot_on: Set if the regulator is enabled when the system is initially
 *           started.  If the regulator is not enabled by the hardware or
 *           bootloader then it will be enabled when the constraints are
 *           applied.

What I would take from that is that "boot_on" means that we expect
that the firmware probably turned this regulator on but if it didn't
then the kernel should turn it on (and presumably leave it on?).  That
would mean that your patch is incorrect, I think?

...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.

===

NOTE: I'm curious why you even have the "regulator-boot-on" attribute
in your case to begin with.  Why not leave it off?

-Doug