Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

Satya Priya Kakitapalli (Temp)
2022-02-11T10:01:01Z

On 2/9/2022 7:48 PM, Mark Brown wrote:
> On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote:
>
>> +static int pm8008_regulator_of_parse(struct device_node *node,
>> + const struct regulator_desc *desc,
>> + struct regulator_config *config)
>> +{
>> + struct pm8008_regulator *pm8008_reg = config->driver_data;
>> + int rc;
>> + unsigned int reg;
>> +
>> + /* get slew rate */
>> + rc = regmap_bulk_read(pm8008_reg->regmap,
>> + LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
>> + if (rc < 0) {
>> + dev_err(pm8008_reg->dev,
>> + "%s: failed to read step rate configuration rc=%d\n",
>> + pm8008_reg->rdesc.name, rc);
>> + return rc;
>> + }
>> + reg &= STEP_RATE_MASK;
>> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
>> +
>> + return 0;
> This is not doing any parsing of any DT properties at all, it is just
> reading a default value back from the hardware. This shouldn't be in
> the of_parse() callback, it should be done on probe() or something so
> that if someone adds ACPI support or whatever there's no surprise
> breakage, and so that we've got this configured even if there's no DT
> bindings for the specific regulator.

Okay, I'll move it to probe.

>
>> +}
>> +
>> +static int pm8008_regulator_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + int id = pdev->id % PM8008_NUM_LDOS;
>> + struct regulator_dev *rdev;
>> + struct pm8008_regulator *pm8008_reg;
>> + struct regmap *regmap;
>> + struct regulator_config reg_config = {};
>> + int rc;
>> +
>> + dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
>> +
>> + regmap = dev_get_regmap(dev->parent, NULL);
>> + if (!regmap) {
>> + dev_err(dev, "parent regmap is missing\n");
>> + return -EINVAL;
>> + }
>> +
>> + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
>> + if (!pm8008_reg)
>> + return -ENOMEM;
>> +
>> + pm8008_reg->regmap = regmap;
>> + pm8008_reg->dev = dev;
>> + pm8008_reg->base = reg_data[id].base;
>> +
>> + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
>> + pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
>> + pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
>> + pm8008_reg->rdesc.name = reg_data[id].name;
>> + pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
>> + pm8008_reg->rdesc.of_match = reg_data[id].name;
>> + pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
>> + pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
>> + pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
>> + pm8008_reg->rdesc.n_voltages
>> + = ((reg_data[id].max_uv - reg_data[id].min_uv)
>> + / pm8008_reg->rdesc.uV_step) + 1;
>> + pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range;
>> + pm8008_reg->rdesc.n_linear_ranges = 1;
>> + pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
>> + pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
>> + pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv;
>> +
>> + reg_config.dev = dev->parent;
>> + reg_config.driver_data = pm8008_reg;
>> +
>> + rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, &reg_config);
>> + if (IS_ERR(rdev)) {
>> + rc = PTR_ERR(rdev);
>> + dev_err(dev, "%s: failed to register regulator rc=%d\n",
>> + reg_data[id].name, rc);
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver pm8008_regulator_driver = {
>> + .driver = {
>> + .name = "qcom,pm8008-regulators",
>> + },
>> + .probe = pm8008_regulator_probe,
>> +};
>> +
>> +module_platform_driver(pm8008_regulator_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.7.4
>>