Re: [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC

From: skakit
Date: Wed Oct 27 2021 - 09:40:20 EST

On 2021-10-26 01:16, Stephen Boyd wrote:
Quoting skakit@xxxxxxxxxxxxxx (2021-10-22 05:28:34)
On 2021-10-06 00:05, Stephen Boyd wrote:
> Quoting Satya Priya (2021-09-30 21:00:58)
>> diff --git a/drivers/regulator/qcom-pm8008-regulator.c
>> b/drivers/regulator/qcom-pm8008-regulator.c
>> new file mode 100644
>> index 0000000..5dacaa4
>> --- /dev/null
>> +++ b/drivers/regulator/qcom-pm8008-regulator.c
>> @@ -0,0 +1,320 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
>> +
>> +#include <linux/delay.h>
[...]
>> +
>> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
>> +{
>> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
>> + u8 vset_raw[2];
>> + int rc;
>> +
>> + rc = pm8008_read(pm8008_reg->regmap,
>> + LDO_VSET_LB_REG(pm8008_reg->base),
>> + vset_raw, 2);
>
> Can this be an __le16 mV?
>

Below is the diff after changing as per your suggestion, Please correct
me if wrong.

- u8 vset_raw[2];
+ __le16 mV;
int rc;

- rc = pm8008_read(pm8008_reg->regmap,
- LDO_VSET_LB_REG(pm8008_reg->base),
- vset_raw, 2);
+ rc = regmap_bulk_read(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base), &mV, 2);
if (rc < 0) {
dev_err(pm8008_reg->dev, "failed to read regulator
voltage rc=%d\n", rc);
return rc;
}

- return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
+ return le16_to_cpu(mV) * 1000;

Looks good. Does mV need to be casted when passed to regmap_bulk_read()?


Below is the diff:

- int rc = 0, mv;
- u8 vset_raw[2];
+ int rc, mv;
+ u16 vset_raw;
[...]
- vset_raw[0] = mv & 0xff;
- vset_raw[1] = (mv & 0xff00) >> 8;
- rc = pm8008_write(pm8008_reg->regmap,
LDO_VSET_LB_REG(pm8008_reg->base),
- vset_raw, 2);
+ vset_raw = cpu_to_le16(mv);
+
+ rc = regmap_bulk_write(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base), &vset_raw,
+ sizeof(vset_raw));


Ok, thanks

>> + dev_err(dev, "%s: failed to get regulator data\n",
>> name);
>> + return -ENODATA;
>> + }
>> +
>> + init_data->constraints.input_uV =
>> init_data->constraints.max_uV;
>> + reg_config.dev = dev;
>> + reg_config.init_data = init_data;
>> + reg_config.driver_data = pm8008_reg;
>> + reg_config.of_node = reg_node;
>> +
>> + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
>> + pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
>> + pm8008_reg->rdesc.name = init_data->constraints.name;
>> + pm8008_reg->rdesc.supply_name = reg_data[i].supply_name;
>> + pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
>> + pm8008_reg->rdesc.min_uV = reg_data[i].min_uv;
>> + pm8008_reg->rdesc.n_voltages
>> + = ((reg_data[i].max_uv - reg_data[i].min_uv)
>> + / pm8008_reg->rdesc.uV_step) + 1;
>> +
>> + pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(base);
>> + pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
>> + pm8008_reg->rdesc.min_dropout_uV = reg_data[i].min_dropout_uv;
>> + of_property_read_u32(reg_node, "qcom,min-dropout-voltage",
>> + &pm8008_reg->rdesc.min_dropout_uV);
>
> Why do we allow DT to override this? Isn't it a property of the
> hardware
> that doesn't change? So the driver can hardcode the knowledge about the
> dropout.
>

The headroom values change with targets. We are adding some default
headroom values in the driver and later overwriting them with the actual
values specified in the DT.

What do you mean by "targets"? Is that the SoC the PMIC is paired with?

Yes I meant the SoC/board on which the pmic is present.

I'd prefer it be a standard regulator property instead of qcom specific
if it actually needs to be different based on different devices.


Ok, I'll drop the qcom prefix.


>> +
>> + pm8008_reg->rdev = devm_regulator_register(dev,
>> &pm8008_reg->rdesc,
>
> Is this assignment ever used? Seems like it would be better to merely
>
> return PTR_ERR_OR_ZERO(devm_regulator_register(dev, ...));
>

Okay.

>> + &reg_config);
>> + if (IS_ERR(pm8008_reg->rdev)) {
>> + rc = PTR_ERR(pm8008_reg->rdev);
>> + dev_err(dev, "%s: failed to register regulator
>> rc=%d\n",
>> + pm8008_reg->rdesc.name, rc);
>> + return rc;
>> + }
>> +
>> + dev_dbg(dev, "%s regulator registered\n", name);
>> +
>> + return 0;
>> +}
>> +
>> +static int pm8008_parse_regulator(struct regmap *regmap, struct
>> device *dev)
>> +{
>> + int rc = 0;
>
> Drop initialization.
>

Okay.

>> + const char *name;
>> + struct device_node *child;
>> + struct pm8008_regulator *pm8008_reg;
>> +
>> + /* parse each subnode and register regulator for regulator
>> child */
>> + for_each_available_child_of_node(dev->of_node, child) {
>> + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg),
>> GFP_KERNEL);
>> +
>> + pm8008_reg->regmap = regmap;
>> + pm8008_reg->of_node = child;
>> + pm8008_reg->dev = dev;
>> +
>> + rc = of_property_read_string(child, "regulator-name",
>> &name);
>> + if (rc)
>> + continue;
>> +
>> + rc = pm8008_register_ldo(pm8008_reg, name);
>
> Can we use the of_parse_cb similar to qcom_spmi-regulator.c?
>

Are you suggesting to remove the pm8008_register_ldo API and add its
contents in probe itself and then use of_parse_cb callback like in
qcom_spmi-regulator.c?

Yes


Okay.


Do we have any advantage using that here? Also I am not exactly sure
what all contents to put in that. Seems like we can put the step rate
and min-dropout-voltage configurations in there.

Right. The regulator code is setup to do "DT parsing stuff" for each
regulator node already, so you don't need to duplicate that logic in
this driver. That's the main goal, consolidate regulator matching and
iteration into the core. Maybe Mark has more info.

Okay.