[U-Boot] [PATCH 3/6] drivers/power/pmic: Add tps65910 driver

[Tom Rini <trini@ti.com>]

On Thu, Jul 25, 2013 at 12:41:46PM -0500, Dan Murphy wrote:
> On 07/19/2013 02:00 PM, Tom Rini wrote:
> > From: "Philip, Avinash" <avinashphilip@ti.com>
> >
> > Add a driver for the TPS65910 PMIC that is found in the AM335x GP EVM,
> > AM335x EVM SK and others.
> 
> Can we add a link to the technical manual?
> http://www.ti.com/product/tps65910

Yup, in the code.

[snip]
> > +/*
> > + * voltage switching for MPU frequency switching.
> > + * @module = mpu - 0, core - 1
> > + * @vddx_op_vol_sel = vdd voltage to set
> 
> No return identified here

Fixed.

> > + */
> > +int tps65910_voltage_update(unsigned int module, unsigned char vddx_op_vol_sel)
> > +{
> > +	uchar buf[4];
> 
> If we only read and write one byte at a time why is this an array of 4?

Fixed.

> > +	unsigned int reg_offset;
> > +
> > +	if (module == MPU)
> > +		reg_offset = TPS65910_VDD1_OP_REG;
> > +	else
> > +		reg_offset = TPS65910_VDD2_OP_REG;
> 
> This seems very specific to the implementation.
>
> Can VDD2 ever be used for the MPU?  Or are there constraints on the
> VDD2 SMPS that will not allow that?

Can?  Probably.  Will someone deviate from the reference design?
Probably not.

> > +
> > +	/* Select VDDx OP   */
> > +	if (i2c_read(TPS65910_CTRL_I2C_ADDR, reg_offset, 1, buf, 1))
> > +		return 1;
> 
> I am going to assume that you changed this as well like you did for
> the TPS65217?

Correct.

> > +int tps65910_voltage_update(unsigned int module, unsigned char vddx_op_vol_sel);
> 
> Nitpick - Don't the interface definitions go at the end of the file?

Sure.

[snip]
> Is this not a family of parts?
> 
> http://www.ti.com/product/tps65910
> 
> TPS65910, TPS65910A, TPS65910A3, TPS659101, TPS659102, TPS659103
> TPS659104, TPS659105, TPS659106, TPS659107, TPS659108, TPS659109
> 
> Do you think we could rename the definitions and the file to TPS65910x?

Fixed the other two minor things, and yes, but if we follow the kernel
example here (note that we didn't copy code) it's still just tps65910
and tps65217 (tps65217 has a/b/c variants).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130726/82ca76c7/attachment.pgp>