Re: [PATCH RFC 2/2] regulator: qcom_smd: Disable unused regulators

[Konrad Dybcio]

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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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