Messages in this thread | | | Date | Wed, 23 Sep 2009 17:15:38 +0200 | From | Wolfram Sang <> | Subject | Re: [RFC] regulator: add driver for MAX8660/8661 |
| |
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/ | [unhandled content-type:application/pgp-signature] | |