Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <1a0b7d17-8f5e-4372-830a-da0a92f711ea@linaro.org>
Date:   Tue, 10 Oct 2023 14:14:46 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC 1/2] regulator: core: Disable unused regulators with
 unknown status
Content-Language: en-US
To:     Stephan Gerhold <stephan@gerhold.net>
Cc:     Stephan Gerhold <stephan.gerhold@kernkonzept.com>,
        Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
References: <20231004-reg-smd-unused-v1-0-5d682493d555@kernkonzept.com>
 <20231004-reg-smd-unused-v1-1-5d682493d555@kernkonzept.com>
 <9b78cab5-d72f-469a-816d-6b3f86aecada@linaro.org>
 <ZSRg1H772gMTl-d3@gerhold.net>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <ZSRg1H772gMTl-d3@gerhold.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/9/23 22:21, Stephan Gerhold wrote:
> On Fri, Oct 06, 2023 at 11:11:48PM +0200, Konrad Dybcio wrote:
>> On 4.10.2023 16:17, Stephan Gerhold wrote:
>>> Some regulator drivers do not provide a way to check if the regulator is
>>> currently enabled or not. That does not necessarily mean that the
>>> regulator is always-on. For example, the regulators managed by the RPM
>>> firmware on Qualcomm platforms can be either on or off during boot but
>>> the initial state is not known. To sync the state the regulator should
>>> get either explicitly enabled or explicitly disabled.
>>>
>>> Enabling all regulators unconditionally is not safe, because we might
>>> not know which voltages are safe. The devices supplied by those
>>> regulators might also require a special power-up sequence where the
>>> regulators are turned on in a certain order or with specific delay.
>>>
>>> Disabling all unused regulators is safer. If the regulator is already
>>> off it will just stay that way. If the regulator is on, disabling it
>>> explicitly allows the firmware to turn it off for reduced power
>>> consumption.
>>>
>>> The regulator core already has functionality for disabling unused
>>> regulators. However, at the moment it assumes that all regulators where
>>> the .is_enabled() callback fails are actually off. There is no way to
>>> return a special value for the "unknown" state to explicitly ask for
>>> disabling those regulators.
>>>
>>> Some drivers (e.g. qcom-rpmh-regulator.c) return -EINVAL for the case
>>> where the initial status is unknown. Use that return code to assume the
>>> initial status is unknown and try to explicitly disable the regulator
>>> in that case.
>>>
>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
>>> ---
>>> 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. :)
>>
>> -EOPNOTSUPP for "doesn't support getting is_enabled state"?
>>
> 
> The way it is implemented right now the Qualcomm SMD RPM regulator does
> actually support getting the .is_enabled() state. It is only unable to
> determine the initial state during boot. Once the regulator has been
> enabled by some consumer for the first time the .is_enabled() callback
> starts returning the expected results.
> 
> Typically -EOPNOTSUPP is used when the driver callback (or similar) is
> not implemented at all. I'm not sure if using -EOPNOTSUPP for the
> "temporarily unable to determine state" purpose would be misleading.
I'd say EOPNOTSUPP is fair here because calling is_enabled in that 
context is not supported, but I guess it's up to Mark.

Konrad