Messages in this thread | | | Date | Wed, 27 Oct 2021 19:10:00 +0530 | From | skakit@codeauro ... | Subject | Re: [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC |
| |
On 2021-10-26 01:16, Stephen Boyd wrote: > Quoting skakit@codeaurora.org (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. >> >> >> + ®_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.
| |