Re: [PATCH v4 2/4] MAX8997/8966 PMIC Regulator Driver Initial Release

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

On Tue, Mar 08, 2011 at 05:37:21PM +0900, MyungJoo Ham wrote:
> This patch supports PMIC/Regulator part of MAX8997/MAX8966 MFD.
> In this initial release, selecting voltages or current-limit
> and switching on/off the regulators are supported.

This looks pretty good, it's probably OK to apply as-is and clean up
some more stuff later on.

> +static int max8997_list_voltage_charger_cv(struct regulator_dev *rdev,
> +		unsigned int selector)
> +{
> +	int rid = max8997_get_rid(rdev);
> +
> +	if (rid == MAX8997_CHARGER_CV) {

A little nicer to do if != then error out (saves indentation for the
entire rest of the function.

> +/*
> + * Assess the damage on the voltage setting of BUCK1,2,5 by the change.
> + *
> + * When GPIO-DVS mode is used for multiple bucks, changing the voltage value
> + * of one of the bucks may affect that of another buck, which is the side
> + * effect of the change (set_voltage). This function examines the GPIO-DVS
> + * configurations and checks whether such side-effect exists.
> + */

I'm not 100% sure I understand exactly what the mechanism for side
effects to occur is here (the code is a little abstruse, I'd expect it
to go through and set a flag if it finds a side effect would occur).
However, one thing that occurs to me is that you're not using the fact
that the regulator API passes in voltage ranges in the checks - you're
checking for any change, but it's possible that a side effect could
occur and still be in the range that was requested which should ideally
be allowed.  For example, if the side effect would be to change from
1.1V to 1.2V but the requested range was 1.1-1.3V then while there is
a side effect all the requirements that drivers provided are still being
met so we don't need to worry.

This sort of side effect is something we shouild probably add to the
core, other hardware has the possibility of having similar gang control
of the regulators although the mechanism here seems unusually complex.
I don't think we need to do that for this driver though.