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
