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: <52bf96e4-f8eb-4f3b-ae2e-3c33d314b09c@linaro.org>
Date:   Mon, 9 Oct 2023 23:00:07 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC 2/2] regulator: qcom_smd: Disable unused regulators
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-2-5d682493d555@kernkonzept.com>
 <9b7744bf-d42c-4922-85e8-ca4777f313a7@linaro.org>
 <ZSRhTIqYX7hQ0_lc@gerhold.net>
Content-Language: en-US
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <ZSRhTIqYX7hQ0_lc@gerhold.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/9/23 22:23, Stephan Gerhold wrote:
> On Fri, Oct 06, 2023 at 11:15:40PM +0200, Konrad Dybcio wrote:
>> On 4.10.2023 16:17, Stephan Gerhold wrote:
>>> The RPM firmware on Qualcomm platforms does not provide a way to check
>>> if a regulator is on during boot using the SMD interface. If the
>>> regulators are already on during boot and Linux does not make use of
>>> them they will currently stay enabled forever. The regulator core does
>>> not know these regulators are on and cannot clean them up together with
>>> the other unused regulators.
>>>
>>> Fix this by setting the initial enable state to -EINVAL similar to
>>> qcom-rpmh-regulator.c. The regulator core will then also explicitly
>>> disable all unused regulators with unknown status.
>>>
>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
>>> ---
>>> NOTE: This has a slight potential of breaking boards that rely on having
>>> unused regulators permanently enabled (without regulator-always-on).
>>> However, this is always a mistake in the device tree so it's probably
>>> better to risk some breakage now, add the missing regulators and avoid
>>> this problem for all future boards.
>>> ---
>>>   drivers/regulator/qcom_smd-regulator.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>> index f53ada076252..0bbfba2e17ff 100644
>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>> @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
>>>   		reqlen++;
>>>   	}
>>>   
>>> -	if (vreg->uv_updated && vreg->is_enabled) {
>>> +	if (vreg->uv_updated && vreg->is_enabled > 0) {
>> At a quick glance, are there any states for this value, other
>> than 0 and 1? This is not the regulator_ops->is_enabled, but
>> qcom_rpm_reg->is_enabled.
>>
> 
> Yes, I initially assign vreg->is_enabled = -EINVAL (for use with PATCH
> 1/2). It's in the part of the patch that you trimmed in your reply. :D
> 
> Thanks,
> Stephan
Oh, right ^^

Konrad