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

[Satya Priya Kakitapalli (Temp)]

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