Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1654581161-12349-1-git-send-email-u0084500@gmail.com>
 <1654581161-12349-4-git-send-email-u0084500@gmail.com> <Yp+gS6r5Kpi33Ags@sirena.org.uk>
 <CADiBU38+0vp3Dv6i7uYzCwR431PKBr-HNQnY0Qe7fvvRYGEJmw@mail.gmail.com> <YqB19O/HYvEAxdiM@sirena.org.uk>
In-Reply-To: <YqB19O/HYvEAxdiM@sirena.org.uk>
From:   ChiYuan Huang <u0084500@gmail.com>
Date:   Thu, 9 Jun 2022 14:35:07 +0800
Message-ID: <CADiBU390XRXZ2yx5CT2NxhN3aROHXcxs7w2d-xhB6+EYn+uTfA@mail.gmail.com>
Subject: Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
To:     Mark Brown <broonie@kernel.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Lee Jones <lee.jones@linaro.org>, dmitry.torokhov@gmail.com,
        Liam Girdwood <lgirdwood@gmail.com>,
        cy_huang <cy_huang@richtek.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        linux-input@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Mark Brown <broonie@kernel.org> =E6=96=BC 2022=E5=B9=B46=E6=9C=888=E6=97=A5=
 =E9=80=B1=E4=B8=89 =E4=B8=8B=E5=8D=886:12=E5=AF=AB=E9=81=93=EF=BC=9A
>
> On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> > Mark Brown <broonie@kernel.org> =E6=96=BC 2022=E5=B9=B46=E6=9C=888=E6=
=97=A5 =E9=80=B1=E4=B8=89 =E4=B8=8A=E5=8D=883:00=E5=AF=AB=E9=81=93=EF=BC=9A
> > > On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote:
>
> > > > +     static const char * const name[] =3D { "buck1", "buck2", "buc=
k3", "buck4",
> > > > +                                          "ldo", "exten" };
> > > > +     static const char * const sname[] =3D { "vin1", "vin2", "vin3=
", "vin4",
> > > > +                                           "vinldo", NULL };
>
> > > It would be easier and clearer to just make this a static table like
> > > other drivers do, there's no need to generate anything dynamically as
> > > far as I can see.
>
> > My excuse. let me explain it.
> > buck1 voltage range from 600mV to 1393.75mV.
> > buck2~4/ldo/exten is the fixed regulator.
> > buck3 and buck4 is fixed by the IC efuse default.
> > buck2 and ldo is fixed by the external resistor chosen.
> > exten is designed to connected to the external power.
>
> > That's why I cannot directly declared it as the static regulator_desc.
>
> So buck 2-4 need some dynamic handling then but the rest can be static -
> that would be a lot clearer.  You could also have a template for the
> ones with some dynamic values and just override the few fields that need
> it.
>
Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling.

> > > > +     if (init_data->constraints.min_uV !=3D init_data->constraints=
.max_uV) {
> > > > +             dev_err(priv->dev, "Variable voltage for fixed regula=
tor\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     desc->fixed_uV =3D init_data->constraints.min_uV;
> > > > +     init_data->constraints.apply_uV =3D 0;
>
> > > Drivers should never override constraints passed in by machine driver=
s,
> > > if there's validation needed let the core do it.  The same probably
> > > applies to providing a voltage range for a fixed regulator though tha=
t's
> > > not modifying everything so not such a problem.
>
> > Please check the above explanation about each power rails.
>
> I'm not sure what you're referencing here?
>
Sorry. Let me explain it.
You mean 'of_parse_cb' must not override constraint.
But if the regulator is fixed and dynamic, after
'of_get_regulation_constraint', apply_uV will be true.
The is referring to 'fixed.c'

> > > > +     for (i =3D 0; i < RT5120_MAX_REGULATOR; i++) {
> > > > +             ret =3D rt5120_of_parse_cb(priv, i, rt5120_regu_match=
 + i);
> > > > +             if (ret) {
> > > > +                     dev_err(priv->dev, "Failed in [%d] of_passe_c=
b\n", i);
> > > > +                     return ret;
> > > > +             }
> > > > +     }
> > >
> > > This is all open coding stuff that's in the core - just provde an
> > > of_parse_cb() operation and let the core take care of calling it.
>
> > Ditto
>
> Or here.
If I put 'of_parce_cb' to make core handling it, the input parameter
'init_data' is declared as const.
I cannot override the 'apply_uV'.
Right?