Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support

[ChiYuan Huang]

Mark Brown <broonie@xxxxxxxxxx> 於 2022年6月8日 週三 下午6:12寫道：
>
> On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> > Mark Brown <broonie@xxxxxxxxxx> 於 2022年6月8日 週三 上午3:00寫道：
> > > On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote:
>
> > > > +     static const char * const name[] = { "buck1", "buck2", "buck3", "buck4",
> > > > +                                          "ldo", "exten" };
> > > > +     static const char * const sname[] = { "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 != init_data->constraints.max_uV) {
> > > > +             dev_err(priv->dev, "Variable voltage for fixed regulator\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     desc->fixed_uV = init_data->constraints.min_uV;
> > > > +     init_data->constraints.apply_uV = 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.
>
> 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 = 0; i < RT5120_MAX_REGULATOR; i++) {
> > > > +             ret = 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
>
> 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?