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[] = { "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.
> +static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid,
> + struct of_regulator_match *match)
> +{
> + struct regulator_desc *desc = priv->rdesc + rid;
> + struct regulator_init_data *init_data = match->init_data;
> +
> + if (!init_data || rid == RT5120_REGULATOR_BUCK1)
> + return 0;
> +
> + 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.
> +static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv)
> +{
> + struct device *dev = priv->dev->parent;
> + struct device_node *reg_node;
> + int i, ret;
> +
> + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> + rt5120_fillin_regulator_desc(priv->rdesc + i, i);
> +
> + rt5120_regu_match[i].desc = priv->rdesc + i;
> + }
Like I said above just make the list of regulators static data and loop
through registering them.
> +
> + reg_node = 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 = 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 = 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.
> +static int rt5120_device_property_init(struct rt5120_priv *priv)
> +{
> + struct device *dev = priv->dev->parent;
> + bool prot_enable;
> + unsigned int prot_enable_val = 0;
> +
> + /* Assign UV/OV HW protection behavior */
> + prot_enable = device_property_read_bool(dev,
> + "richtek,enable-undervolt-hiccup");
> + if (prot_enable)
> + prot_enable_val |= 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.
Mark Brown <[email protected]> 於 2022年6月8日 週三 上午3:00寫道：
>
> 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[] = { "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.
>
> > +static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid,
> > + struct of_regulator_match *match)
> > +{
> > + struct regulator_desc *desc = priv->rdesc + rid;
> > + struct regulator_init_data *init_data = match->init_data;
> > +
> > + if (!init_data || rid == RT5120_REGULATOR_BUCK1)
> > + return 0;
> > +
> > + 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.
>
> > +static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv)
> > +{
> > + struct device *dev = priv->dev->parent;
> > + struct device_node *reg_node;
> > + int i, ret;
> > +
> > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> > + rt5120_fillin_regulator_desc(priv->rdesc + i, i);
> > +
> > + rt5120_regu_match[i].desc = priv->rdesc + i;
> > + }
>
> Like I said above just make the list of regulators static data and loop
> through registering them.
Ditto
>
> > +
> > + reg_node = 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 = 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 = 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
>
> > +static int rt5120_device_property_init(struct rt5120_priv *priv)
> > +{
> > + struct device *dev = priv->dev->parent;
> > + bool prot_enable;
> > + unsigned int prot_enable_val = 0;
> > +
> > + /* Assign UV/OV HW protection behavior */
> > + prot_enable = device_property_read_bool(dev,
> > + "richtek,enable-undervolt-hiccup");
> > + if (prot_enable)
> > + prot_enable_val |= 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'.
On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> Mark Brown <[email protected]> 於 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.
> > > + 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?
> > > + 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.
Mark Brown <[email protected]> 於 2022年6月8日 週三 下午6:12寫道：
>
> On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> > Mark Brown <[email protected]> 於 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?
On Thu, Jun 09, 2022 at 02:35:07PM +0800, ChiYuan Huang wrote:
> Mark Brown <[email protected]> 於 2022年6月8日 週三 下午6:12寫道：
> > On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> > > 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.
Why do the others need it?
> > > > 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'
fixed.c is a special case due to legacy issues and being generic, for
normal fixed voltage regulators in a device where we know what they're
fixed to they can just have their voltage hard coded in the driver. If
there's issues with the machine providing invalid or nonsensical
constraints the driver should just let the core deal with them.
> > > > 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?
Yes, that's by design.
Mark Brown <[email protected]> 於 2022年6月10日 週五 下午7:03寫道：
>
> On Thu, Jun 09, 2022 at 02:35:07PM +0800, ChiYuan Huang wrote:
> > Mark Brown <[email protected]> 於 2022年6月8日 週三 下午6:12寫道：
> > > On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
>
> > > > 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.
>
> Why do the others need it?
>
Sometimes, for this kind of general purpose PMIC, it need to provide
the flexibility.
Cause buck2 and ldo can already be fixed by the external resistor,
buck3 and buck4 seems to be fixed by IC default.
So there may be the same part number and use the postfix to be
different like as 5120'A'/5120'B', etc...
And use it to define the voltage for the different IC default for
buck3 and buck4, and exten behavior.
That's due to the different application use the different power on
sequence and default voltages.l
> > > > > 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'
>
> fixed.c is a special case due to legacy issues and being generic, for
> normal fixed voltage regulators in a device where we know what they're
> fixed to they can just have their voltage hard coded in the driver. If
> there's issues with the machine providing invalid or nonsensical
> constraints the driver should just let the core deal with them.
>
> > > > > 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?
>
> Yes, that's by design.
I have traced the code for 'of_get_regulator_init_data' and
'set_machine_constraints' in regulator register.
If I cannot overwrite apply_uV variable, it will cause the
regulator_register return -EINVAL.
Is the below flow that you suggested?
1. of_parse_cb to check min_uV and max_uV, and fill in the fixed_uV in
regulator_desc
2. provide the duummy set/get voltage to make set_machine_constraints
not return '-EINVAL'.
On Mon, Jun 13, 2022 at 09:49:31AM +0800, ChiYuan Huang wrote:
> Mark Brown <[email protected]> 於 2022年6月10日 週五 下午7:03寫道：
> > > Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling.
> > Why do the others need it?
> Sometimes, for this kind of general purpose PMIC, it need to provide
> the flexibility.
> Cause buck2 and ldo can already be fixed by the external resistor,
> buck3 and buck4 seems to be fixed by IC default.
> So there may be the same part number and use the postfix to be
> different like as 5120'A'/5120'B', etc...
> And use it to define the voltage for the different IC default for
> buck3 and buck4, and exten behavior.
> That's due to the different application use the different power on
> sequence and default voltages.l
Variants should have separate compatibles, and if the code is doing
something fixed then it shouldn't make any difference if that fixed
thing is written in code or as a data table.
> > > 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?
> > Yes, that's by design.
> I have traced the code for 'of_get_regulator_init_data' and
> 'set_machine_constraints' in regulator register.
> If I cannot overwrite apply_uV variable, it will cause the
> regulator_register return -EINVAL.
We have a very large number of fixed voltage regulators in use on
various systems which manage perfectly fine without this. If the
constraints are trying to set something invalid then they're what
should be fixed.
> Is the below flow that you suggested?
> 1. of_parse_cb to check min_uV and max_uV, and fill in the fixed_uV in
> regulator_desc
No, the device should just know what voltage the fixed voltage
regulators in the device have.
> 2. provide the duummy set/get voltage to make set_machine_constraints
> not return '-EINVAL'.
No, people just shouldn't be trying to set the voltage for fixed voltage
regulators. If the regulators are configurable in hardware then provide
DT properties for whatever is configured (eg, resistors).