[UPSTREAM - RFC] pcf50633_use_regulator_api.patch

[Mark Brown]

On Wed, Oct 08, 2008 at 07:38:45PM +0530, Balaji Rao wrote:

The regulator bits of this look pretty good.  A few comments below...

> @@ -0,0 +1,259 @@
> +/*
> + * Regulator driver for pcf50633
> + */
> +
> +#include <linux/regulator/driver.h>
> +#include <linux/pcf50633.h>

Should probably push that into linux/mfd.

> +#define PCF50633_REGULATOR(_name, _id) \
> +	{				\
> +		.name = _name, .id = _id, .ops = &pcf50633_reg_ops, \
> +		.irq = 1, .type = REGULATOR_VOLTAGE, \
> +		.owner = THIS_MODULE, \
> +	}

The IRQ ought to be passed in as board/platform data rather than being
hard coded.  Though I can't currently see any actual use of it currently
so it'd be even easier to just drop it for now?

> +static int pcf50633_regulator_set_voltage(struct regulator_dev *rdev,
> +			int min_uV, int max_uV)
> +{

...

> +	millivolts = min_uV / 1000;
> +
> +	regnr = regulator_registers[regulator_id];
> +
> +	switch (regulator_id) {
> +	case PCF50633_REGULATOR_AUTO:
> +		volt_bits = auto_voltage_bits(millivolts);
> +		break;

There's no check to see if the voltage being requested is actually
supported by the regulator - if the regulator can't satisfy the request
it should return -EINVAL.

> +/* 
> + * Initialize and register the regulator with the core.
> + */
> +
> +int pcf50633_regulator_init(struct pcf50633_data *pcf, int irq)
> +{
> +	int i;
> +
> +	printk("pcf50633 regulator init\n");
> +
> +	for (i = 0; i < ARRAY_SIZE(pcf50633_regulators); i++) {
> +		pcf50633_regulators[i].irq = irq;
> +		regulator_register(&pcf50633_regulators[i], pcf);
> +	}
> +
> +	return 0;
> +}

This has changed in Liam's current for-linus branch.  See

	git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6