Re: [PATCH 3/3] regulator: aat2870: Add AAT2870 regulator driver

Mark Brown
2011-07-01T16:39:53Z

On Wed, Jun 29, 2011 at 11:06:52PM +0900, Jin Park wrote:

> +config REGULATOR_AAT2870
> + bool "AnalogicTech AAT2870 Regulators"

Why bool?

> +static int aat2870_ldo_set_voltage(struct regulator_dev *rdev,
> + int min_uV, int max_uV, unsigned *selector)
> +{
> + struct aat2870_regulator *ri = rdev_get_drvdata(rdev);
> + struct aat2870_data *aat2870 = dev_get_drvdata(ri->pdev->dev.parent);
> + u8 val;
> + int uV;
> + int i;
> +
> + if ((min_uV < ri->min_uV) || (max_uV > ri->max_uV)) {
> + dev_err(&rdev->dev,
> + "Invalid voltage, min %duV(>=%duV), max %duV(<=%duV)\n",
> + min_uV, ri->min_uV, max_uV, ri->max_uV);
> + return -EDOM;
> + }
> +
> + for (i = 0; i < ri->desc.n_voltages; i++) {
> + uV = ri->voltages[i];
> + if ((min_uV <= uV) && (uV <= max_uV)) {
> + val = (i << ri->voltage_shift) & ri->voltage_mask;
> + break;
> + }
> + }

This should be a set_voltage_sel(), the core will do all the above for
you and the driver only needs to worry about the register writes. Note
also that the first test isn't well formed - so long as we can select a
voltage between min_uV and max_uV we're fine even if one or both of
those limits is outside the valid range.

> +static int aat2870_ldo_get_voltage(struct regulator_dev *rdev)
> +{
> + struct aat2870_regulator *ri = rdev_get_drvdata(rdev);
> + struct aat2870_data *aat2870 = dev_get_drvdata(ri->pdev->dev.parent);
> + u8 val;
> + int ret;
> +
> + ret = aat2870->read(aat2870, ri->voltage_addr, &val);
> + if (ret)
> + return ret;
> +
> + val = (val & ri->voltage_mask) >> ri->voltage_shift;
> + if (val >= ri->desc.n_voltages)
> + return -EIO;
> +
> + return ri->voltages[val];

Again, get_voltage_sel().

> +static int aat2870_regulator_probe(struct platform_device *pdev)
> +{
> + struct regulator_init_data *init_data = pdev->dev.platform_data;
> + struct aat2870_regulator *ri;
> + struct regulator_dev *rdev;
> +
> + if (!init_data) {
> + dev_err(&pdev->dev, "No regulator init data\n");
> + return -ENXIO;
> + }

Let the core worry about this.