Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
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>
In-Reply-To: <Yp+gS6r5Kpi33Ags@sirena.org.uk>
From:   ChiYuan Huang <u0084500@gmail.com>
Date:   Wed, 8 Jun 2022 11:15:56 +0800
Message-ID: <CADiBU38+0vp3Dv6i7uYzCwR431PKBr-HNQnY0Qe7fvvRYGEJmw@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=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:
>
> This looks mostly good, a few things though:
>
> > +static void rt5120_fillin_regulator_desc(struct regulator_desc *desc, =
int rid)
> > +{
> > +     static const char * const name[] =3D { "buck1", "buck2", "buck3",=
 "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.
>
> > +static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid,
> > +                           struct of_regulator_match *match)
> > +{
> > +     struct regulator_desc *desc =3D priv->rdesc + rid;
> > +     struct regulator_init_data *init_data =3D match->init_data;
> > +
> > +     if (!init_data || rid =3D=3D RT5120_REGULATOR_BUCK1)
> > +             return 0;
> > +
> > +     if (init_data->constraints.min_uV !=3D init_data->constraints.max=
_uV) {
> > +             dev_err(priv->dev, "Variable voltage for fixed regulator\=
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 drivers,
> if there's validation needed let the core do it.  The same probably
> applies to providing a voltage range for a fixed regulator though that's
> not modifying everything so not such a problem.
Please check the above explanation about each power rails.
>
> > +static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv)
> > +{
> > +     struct device *dev =3D priv->dev->parent;
> > +     struct device_node *reg_node;
> > +     int i, ret;
> > +
> > +     for (i =3D 0; i < RT5120_MAX_REGULATOR; i++) {
> > +             rt5120_fillin_regulator_desc(priv->rdesc + i, i);
> > +
> > +             rt5120_regu_match[i].desc =3D priv->rdesc + i;
> > +     }
>
> Like I said above just make the list of regulators static data and loop
> through registering them.
Ditto
>
> > +
> > +     reg_node =3D of_get_child_by_name(dev->of_node, "regulators");
> > +     if (!reg_node) {
> > +             dev_err(priv->dev, "Couldn't find 'regulators' node\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     ret =3D of_regulator_match(priv->dev, reg_node, rt5120_regu_match=
,
> > +                              ARRAY_SIZE(rt5120_regu_match));
> > +
> > +     of_node_put(reg_node);
> > +
> > +     if (ret < 0) {
> > +             dev_err(priv->dev,
> > +                     "Error parsing regulator init data (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     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_cb\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
>
> > +static int rt5120_device_property_init(struct rt5120_priv *priv)
> > +{
> > +     struct device *dev =3D priv->dev->parent;
> > +     bool prot_enable;
> > +     unsigned int prot_enable_val =3D 0;
> > +
> > +     /* Assign UV/OV HW protection behavior */
> > +     prot_enable =3D device_property_read_bool(dev,
> > +                                     "richtek,enable-undervolt-hiccup"=
);
> > +     if (prot_enable)
> > +             prot_enable_val |=3D RT5120_UVHICCUP_MASK;
>
> Use the DT APIs to parse DT - since ACPI has a very strong idea of how
> power management works which is fundamentally incompatible with with the
> DT model we should be writing code in a way that minimises the risk that
> we'll end up trying to parse DT properties out of ACPI systems and
> creating confusion as DT and ACPI software tries to run on the same
> system.
Ok, I'll replace it by DT API 'of_property_read_bool'.