Re: [PATCH V1] TPS62360: Add tps62360 regulator driver

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

On Thu, Jan 05, 2012 at 07:18:25PM +0530, Laxman Dewangan wrote:

Please fix your mailer to word wrap in less than 80 columns.  I've
reflowed your text for legibility.

> > > +	/* Configure the output discharge path */
> > > +	st = tps62360_reg_modify_bits(tps, REG_RAMPCTRL, BIT(2), BIT(2));
> > > +	if (st < 0)
> > > +		dev_err(tps->dev, "%s() fails in updating reg %d\n",
> > > +			__func__, REG_RAMPCTRL);
> > > +}

> > I rather suspect that if this is worth doing it's also worth doing over
> > suspend...

> The discharge path should be only enabled when the output voltage will
> go to 0.  If it stay in non-zero voltage and enabling discharge path
> will consume more power.  In suspend, it is not necessarily that
> voltage output will be 0 and hence it should not be enabled. In
> shutdown all power goes off from pmu and hence it is worth to enable
> discharge path for quick fall of output voltage.

Why is shutdown particularly special in this regard, and surely the
hardware is capable of automatically disabling the discharge while the
regulator is enabled?

Though frankly it seems rather broken if the hardware doesn't
automatically remove the clamp when 

> > > + * @vsel: Select the voltage id register.

> > What's this?

> There is 4 sets of voltage configuration register for voltage output.
> The power on reset values for this registers are different and
> selection of the voltage configuration register is done by 2 pins of
> the pmu chip which is board dependent.  Based on board connection, it
> need to be program the desired voltage configuration Register to
> achieve the desired output.

Ah, you've misunderstood this functionality.  The reason regulators have
these pin based register selections is that they'll be connected to
GPIOs so that we can change voltages quickly without having to take the
cost of I2C I/O.

For all practical purposes it's reasonable to assume that if vsel is
fixed to anything rather than being connected to GPIOs it'll be
connected to ground.

The other thing is that the naming is really unclear.