Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Wed, 25 Oct 2023 21:51:51 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Mark Brown <broonie@kernel.org>
Cc:     Stephan Gerhold <stephan.gerhold@kernkonzept.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH RFC 1/2] regulator: core: Disable unused regulators with
 unknown status
Message-ID: <ZTlx13fBddvf4n0h@gerhold.net>
References: <20231004-reg-smd-unused-v1-0-5d682493d555@kernkonzept.com>
 <20231004-reg-smd-unused-v1-1-5d682493d555@kernkonzept.com>
 <80307316-f55e-4540-9c5f-655844c3b3f4@sirena.org.uk>
 <ZTeHAqL5QB2w33RN@kernkonzept.com>
 <802e7f15-029c-4eb6-b0d8-53d16f7da37a@sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <802e7f15-029c-4eb6-b0d8-53d16f7da37a@sirena.org.uk>
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, Oct 25, 2023 at 06:49:47PM +0100, Mark Brown wrote:
> On Tue, Oct 24, 2023 at 10:57:38AM +0200, Stephan Gerhold wrote:
> 
> > I think it does not change much for this patch, though. Even when
> > implemented in the core we still need to represent this situation
> > somehow for regulator_is_enabled(). Simply returning 0 (disabled) or
> > 1 (enabled) would be wrong. Do you think returning -EBUSY would be
> > appropriate for that?
> 
> In these cases where we simply can't read the expectation is that we'll
> always be using the logical state - one way of thinking about it is that
> the operation is mostly a bootstrapping helper to figure out what the
> initial state is.  A quick survey of users suggest they'll pretty much
> all be buggy if we start returning errors, and I frankly even if all the
> current users were fixed I'd expect that to continue to be a common
> error.  I suppose that the effect of ignoring the possibility of error
> is like the current behaviour though.
> 

regulator_is_enabled() already returns error codes in various cases,
e.g. regulator_is_enabled_regmap() returns the original error code from
the regmap_read() call if that fails. So if users ignore that and
interpret the value as logical one they either don't care (which is
probably fine in some cases?) or already use it wrong. Or am I missing
something?

> > qcom_smd-regulator.c and qcom-rpmh-regulator.c use their reference
> > counter internally in other function (e.g. to decide if a voltage change
> > should be sent, see "vreg->enabled" checks). I think we would also need
> > to add some rdev_is_enabled() function that would expose the core
> > reference counter to the driver?
>
> > Tracking the enable state in the driver (the way it is right now) is not
> > that much code, so I'm not entirely sure if we might actually end up
> > with more code/complexity when moving this to the core.
>
> We have to do the reference count in the core anyway since it's a
> reference count not just a simple on/off so it doesn't really cost us
> anything to make it available to drivers.

I assume you're referring to "use_count" as the reference counter?

On a closer look I think it cannot be used as-is for my purpose:

 1. With "regulator-boot-on", set_machine_constraints() explicitly
    enables the regulator, but doesn't increase the use_count.
    In that case we should return true in ->is_enabled(). I'm not sure
    how we would know, just based on use_count = 0.

 2. To cleanup unused regulators that may or may not be enabled we need
    to know if the regulator was ever explicitly enabled/disabled before.
    It's pointless to send a disable request for a regulator that we
    already disabled explicitly before (after a enable -> disable cycle).
    use_count just tells us if there is currently a user, but not if
    there was one before.

I think I would literally need to move the existing "enabled" field from
the RPM regulator drivers to the core and manage it similarly there
based on ->enable() and ->disable() calls. Which would be a (slight)
overhead for all regulators rather than being isolated for the few RPM
regulator drivers.

Thanks,
Stephan