Re: [RFC] regulator: add driver for MAX8660/8661

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Tue, Sep 22, 2009 at 03:18:19PM +0200, Wolfram Sang wrote:

Overall this looks pretty good - some comments...

>  Documentation/power/regulator/max8660.txt |   32 ++

Hrm, if we're going to do this we should do it consistently for all the
drivers.  I think I prefer documentation embedded in the source TBH but
only a little bit.

> +This chip is a bit nasty because it is a write-only device. Thus, the driver
> +uses shadow registers to keep track of its values. The main problem appears to
> +be the initialization: When Linux boots up, we cannot know if the chip is in
> +the default state or not, so we would have to pass such information in
> +platform_data. As this adds a bit of complexity to the driver, this is left
> +out for now until it is really needed.

The AB3100 regulator has a somewhat similar problem in that it appears
to pretty much need some very device specific setup to be done since it
expects software to do a lot of the bootstrapping.  Your plan of passing
in platform data and just blatting the device configuration does seem
reasonable if there's stuff like that.

> +Note that disabling V3 or V4 has no effect if pin EN34 is driven high (pin and
> +register are ORed, see datasheet).

Might be worth exposing this for control via GPIO in a future version of
the driver.

> +- Make use of the forced PWM modes?

regulator_set_mode() - should be fairly straightforward.

> +- ARD?

I'm not sure what you mean by this?

> +	struct regulator_dev *rdev[0];

I'm not a big fan of the 0 length array - just [] ought to do?

> +static int max8660_dcdc_enable(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	struct max8660 *max8660 = rdev_get_drvdata(rdev);
> +	u8 val = (rdev_get_id(rdev) == MAX8660_V3) ? 1 : 4;
> +	ret = max8660_write(max8660, MAX8660_OVER1, 0xff, val);
> +	val = (rdev_get_id(rdev) == MAX8660_V3) ? 0x03 : 0x30;
> +	return (ret != 0) ? :
> +		max8660_write(max8660, MAX8660_VCC1, ~val, val & 0x11);

Some comments here as to why you're also updating VCC1 would be helpful
here, it's a bit obscure at the minute.

> +		switch (pdata->subdevs[i].id) {
> +		case MAX8660_V3:
> +			if (boot_on)
> +				max8660->shadow_regs[MAX8660_OVER1] |= 1;
> +			break;

Might be worth a comment explaining why you're doing this - I believe
you need this to be done first so that set_voltage() doesn't power
things off but it's not immediately obvious from the code.