Re: [PATCH v3 2/2] regulator: rk808: add dvs support

[Mark Brown]

On Mon, Dec 15, 2014 at 11:07:58AM +0800, Chris Zhong wrote:

> +	sel <<= ffs(rdev->desc->vsel_mask) - 1;
> +	sel |= old_sel & ~rdev->desc->vsel_mask;
> +
> +	ret = regmap_write(rdev->regmap, reg, sel);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value(gpio, !gpio_level);

So, this seems a bit odd.  What we appear to be doing here is
alternating between the two different voltage setting registers which is
all well and good but makes me wonder why we're bothering - it's a bit
more work than just sticking with one.  We do get...

> +	/*
> +	 * dvsok pin would be pull down when dvs1/2 pin changed, and
> +	 * it would be pull up once the voltage regulate complete.
> +	 * No need to wait dvsok signal when voltage falling.
> +	 */

...this but unless the voltage typically ramps much faster than spec
it's never clear to me that we're actually winning by polling the pin
instead of just dead reckoning the time, it's more work for the CPU to
poll the GPIO than to sleep after all.

One thing we can do with hardware like this is to program in a voltage
we're likely to want to switch to quickly and then use the GPIO to get
there.  That can be a bit hard to arrange with the regulator API as it
currently stands since we don't exactly have an interface for it.  

We can just check to see what the two values are current set to before
switching and skip the register write if it's the same (making things
faster since we're typically avoiding an I2C or SPI transaction by doing
that) but that's a bit meh.  We can also try to do things like keep the
top voltage from the voltage ranges we're being given programmed which
for DVS typically ends up doing a reasonable job since governors often
like to jump straight to top speed when things get busy so that's one of
the common cases where we most want to change voltages as quickly as
possible.

Attachment: signature.asc
Description: Digital signature