Messages in this thread | | | From | "AnilKumar, Chimata" <> | Subject | RE: [PATCH V2 2/2] TPS65217: Add tps65217 regulator driver | Date | Tue, 3 Jan 2012 06:16:18 +0000 |
| |
Hi Mark,
On Mon, Jan 02, 2012 at 18:46:48, Mark Brown wrote: > On Mon, Jan 02, 2012 at 06:18:42PM +0530, AnilKumar Ch wrote: > > This patch adds tps65217 PMIC as a regulator > > Use subject lines appropriate for the subsystem you're submitting > against. > > > +config REGULATOR_TPS65217 > > + tristate "TI TPS65217 Power regulators" > > + depends on (I2C && MFD_TPS65217) > > Should only depend on the MFD, there's no direct I2C usage.
Agree, removed I2C
> > > +static unsigned int tps65217_vsel_to_uv_range0(unsigned int vsel) > > +{ > > + unsigned int uV = 0; > > + > > + if (vsel <= 24) > > + uV = ((vsel * 25000) + 900000); > > + else if (vsel > 24 && vsel <= 52) > > + uV = (((vsel - 24) * 50000) + 1500000); > > + else if (vsel > 52 && vsel <= 56) > > + uV = (((vsel - 52) * 100000) + 2900000); > > No need for the > checks as this is already taken care of by being in > the else clause.
Changed
> > > +static unsigned int tps65217_vsel_to_uv_range2(unsigned int vsel) > > +{ > > + unsigned int uV = 0; > > + > > + if (vsel <= 8) > > + uV = ((vsel * 50000) + 1500000); > > + else if (vsel > 9 && vsel <= 13) > > + uV = (((vsel - 8) * 100000) + 1900000); > > + else if (vsel > 13) > > + uV = (((vsel - 13) * 50000) + 2400000); > > + > > + return uV; > > In all of these functions there ought to be a check to make sure vsel > isn't out of range and return an error if it is.
Added error checks as well.
> > > +static int tps65217_pmic_set_bits(struct tps65217 *tps, unsigned int reg, > > + unsigned int mask, unsigned int val, unsigned int level) > > +{ > > To repeat what I said when reviewing the previous version of the patch > things like this shouldn't be open coded in individual function drivers, > they should be in the MFD driver. There is *nothing* PMIC specific > about register access. > > I've stopped reading at this point. >
Mark sorry about this, password protected writes/update bits are required only for regulator so I put it in regulator driver. I got your point, will be moved to MFD driver.
Thanks for your patience here.
Regards, AnilKumar
| |