Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Tue, 24 Oct 2023 10:57:38 +0200
From:   Stephan Gerhold <stephan.gerhold@kernkonzept.com>
To:     Mark Brown <broonie@kernel.org>
Cc:     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,
        Stephan Gerhold <stephan@gerhold.net>
Subject: Re: [PATCH RFC 1/2] regulator: core: Disable unused regulators with
 unknown status
Message-ID: <ZTeHAqL5QB2w33RN@kernkonzept.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <80307316-f55e-4540-9c5f-655844c3b3f4@sirena.org.uk>
Precedence: bulk

On Mon, Oct 23, 2023 at 01:09:11PM +0100, Mark Brown wrote:
> On Wed, Oct 04, 2023 at 04:17:17PM +0200, Stephan Gerhold wrote:
> 
> > Instead of -EINVAL we could also use a different return code to indicate
> > the initial status is unknown. Or maybe there is some other option that
> > would be easier? This is working for me but I'm sending it as RFC to get
> > more feedback. :)
> 
> The more normal thing here would be -EBUSY I think - -EINVAL kind of
> indicates that the operation will never work while in reality it could
> possibly work in future.  Though for the RPMH it's not really the case
> that it ever supports readback, what it does is have it's own reference
> counting in the driver.  Rather than doing this we should probably have
> logic in the core which sees that the driver has a write operation but
> no read operation and implements appropriate behaviour.

Yep, I agree that it would be nicer to handle this case in the core,
rather than duplicating the logic in all the RPM-related drivers.

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?

The second challenge I see on a quick look is that both
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.

Thanks,
-- 
Stephan Gerhold <stephan.gerhold@kernkonzept.com>
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth