[U-Boot] [PATCH 2/6] drivers/power/pmic: Add tps65217 driver

[Tom Rini]

On Tue, Jul 23, 2013 at 01:34:15PM -0500, Dan Murphy wrote:
> On 07/19/2013 02:00 PM, Tom Rini wrote:
> > From: Greg Guyotte <gguyotte at ti.com>
> >
> > Add a driver for the TPS65217 PMIC that is found in the Beaglebone
> > family of boards.
> >
> > Signed-off-by: Greg Guyotte <gguyotte at ti.com>
> > [trini: Split and rework Greg's changes into new drivers/power
> > framework]
> > Signed-off-by: Tom Rini <trini at ti.com>
> > ---
> >  drivers/power/pmic/Makefile        |    1 +
> >  drivers/power/pmic/pmic_tps65217.c |  108 ++++++++++++++++++++++++++++++++++++
> >  include/power/tps65217.h           |   92 ++++++++++++++++++++++++++++++
> >  3 files changed, 201 insertions(+)
> >  create mode 100644 drivers/power/pmic/pmic_tps65217.c
> >  create mode 100644 include/power/tps65217.h
> >
> > diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
> > index 14d426f..473cb80 100644
> > --- a/drivers/power/pmic/Makefile
> > +++ b/drivers/power/pmic/Makefile
> > @@ -29,6 +29,7 @@ COBJS-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
> >  COBJS-$(CONFIG_POWER_MAX8997) += pmic_max8997.o
> >  COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o
> >  COBJS-$(CONFIG_POWER_MAX77686) += pmic_max77686.o
> > +COBJS-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o
> >  
> >  COBJS	:= $(COBJS-y)
> >  SRCS	:= $(COBJS:.o=.c)
> > diff --git a/drivers/power/pmic/pmic_tps65217.c b/drivers/power/pmic/pmic_tps65217.c
> > new file mode 100644
> > index 0000000..c84bbcd
> > --- /dev/null
> > +++ b/drivers/power/pmic/pmic_tps65217.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + * (C) Copyright 2011-2013
> Curious if this is the first time in why does it have a 2011 copyright?

Because the code was written in 2011 (and has been whacked around a few
times every year.

[snip]
> > +/**
> > + * tps65217_reg_read() - Generic function that can read a TPS65217 register
> > + * @src_reg:	  Source register address
> > + * @src_val:	  Address of destination variable
> No return defined here in the brief

Fixed.

> > + */
> > +uchar tps65217_reg_read(uchar src_reg, uchar *src_val)
> > +{
> > +	if (i2c_read(TPS65217_CHIP_PM, src_reg, 1, src_val, 1))
> > +		return 1;
> This may be nit picky but generally in error cases we return negative.
> Also why not return an error from errno?

Because we're following i2c which is 0 or not 0, updated to use ret =
i2c_read(...); if (ret) return ret here and throughout.

> Also why an uchar when you are returning an int?

Fixed.

[snip]
> > +int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar dest_val,
> is prot_level a uchar or int?

It's 0/1/2.  I don't have a strong preference on if we type this out as
an int or uchar.

> Also would it not be better to have an interface that will check for
> mask and do the read and just have a dedicated write function?

I don't see the benefit, especially given the usage we have of just
updating certain bitfields at a time.

[snip]
> > +int tps65217_voltage_update(uchar dc_cntrl_reg, uchar volt_sel)
> No header for the interface

Fixed.

> > +{
> > +	if ((dc_cntrl_reg != DEFDCDC1) && (dc_cntrl_reg != DEFDCDC2) &&
> > +	    (dc_cntrl_reg != DEFDCDC3))
> What do these magic numbers mean?  Are these HEX numbers or a string?

OK, it took me a minute to understand your question here.  These are
defines to register names, matching the TRM for the part.  The register
names are however annoyingly and easily confused as hex values.

> > +#define PROT_LEVEL_NONE			0x00
> Are these registers or a mask now?
> > +#define PROT_LEVEL_1			0x01
> > +#define PROT_LEVEL_2			0x02

These are values as to what level of protection the chip has the
register under.

> > +uchar tps65217_reg_read(uchar src_reg, uchar *src_val);
> > +int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar dest_val,
> > +		       uchar mask);
> > +int tps65217_voltage_update(uchar dc_cntrl_reg, uchar volt_sel);
> Are these interfaces supposed to be accessed by an outside object?
> 
> Typically there should be no direct register access from other objects.

We can evaluate if there's consolidation to be done here once other
boards go and adapt MPU clock frequency scaling.  What registers need to
be whacked on what board are going to decide if we can hide more
details, or not.

-- 
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/20130725/2ece9786/attachment.pgp>