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

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3311 bytes --]

Hi Mark,

> >  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.

Fine with me; I think the chance of being read is bigger if such comments are
embedded in the source.

> > +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.

See mail to Liam.

> > +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.

Hmm, I have the impression that EN34 is usually either driven high or low
constantly. I'd also vote for just adding the GPIO-possibility when it is
needed.

> > +- Make use of the forced PWM modes?
> 
> regulator_set_mode() - should be fairly straightforward.

I just checked the details again; you can't save power with switching to PWM. It
is mainly intended for low-noise systems.

> > +- ARD?
> 
> I'm not sure what you mean by this?

Me neither :) That's a function of this chip we don't use.

> > +	struct regulator_dev *rdev[0];
> 
> I'm not a big fan of the 0 length array - just [] ought to do?

OK.

> 
> > +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.

ACK. Will describe.

> > +		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.

There is a comment:

	/* First loop sets up shadow registers to prevent glitches */

I agree it is suboptimally placed and could be more informative.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]