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

[Mark Brown]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/