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

[Tom Rini]

On Wed, Aug 14, 2013 at 05:08:12PM +0200, Lukasz Majewski wrote:
> Hi Tom, Greg
> 
> > 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>
> > 
> > ---
> > Changes in v2:
> > - Address Dan's comments
> > - Change to SPDX license tag
> > - Add TRM link in the header
> > 
> > Signed-off-by: Tom Rini <trini at ti.com>
> > ---
> >  drivers/power/pmic/Makefile        |    1 +
> >  drivers/power/pmic/pmic_tps65217.c |  109
> > ++++++++++++++++++++++++++++++++++++
> > include/power/tps65217.h           |   79 ++++++++++++++++++++++++++
> > 3 files changed, 189 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 f054470..ac2b625 100644
> > --- a/drivers/power/pmic/Makefile
> > +++ b/drivers/power/pmic/Makefile
> > @@ -13,6 +13,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..36e9024
> > --- /dev/null
> > +++ b/drivers/power/pmic/pmic_tps65217.c
> > @@ -0,0 +1,109 @@
> > +/*
> > + * (C) Copyright 2011-2013
> > + * Texas Instruments, <www.ti.com>
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <i2c.h>
> > +#include <power/tps65217.h>
> > +
> > +/**
> > + * tps65217_reg_read() - Generic function that can read a TPS65217
> > register
> > + * @src_reg:		 Source register address
> > + * @src_val:		 Address of destination variable
> > + * @return:		 0 for success, not 0 on failure.
> > + */
> > +int tps65217_reg_read(uchar src_reg, uchar *src_val)
> > +{
> > +	return i2c_read(TPS65217_CHIP_PM, src_reg, 1, src_val, 1);
> 
> Would it be possible to comply with pmic driver model?
> It can be found at ./drivers/power/power_core.c

At the high level, not yet.  We don't have battery support (but fixing
that to be optional in the core wouldn't be hard) but the general pmic
code assumes one pmic charger per binary.  We need both in the same
binary (since we decide at run-time if it's one of the boards with 65910
or 65217).

> Moreover the generic function for reading/writing data to/from pmic is
> already defined at ./drivers/power/power_{i2c|spi}.c 
> 
> Maybe it would be possible to use/modify the already available code?

Without the MAX family datasheets handy, I'm not sure how exactly the
tx_num stuff maps to the password concept the TI parts have.  Skimming
the kernel mfd drivers implies to me that logic ends up being per-chip
(or at least vendor).

[snip]
> > +/**
> > + * tps65217_voltage_update() - Function to change a voltage level,
> > as this
> > + *			       is a multi-step process.
> > + * @dc_cntrl_reg:	       DC voltage control register to
> > change.
> > + * @volt_sel:		       New value for the voltage
> > register
> > + * @return:		       0 for success, not 0 on failure.
> > + */
> > +int tps65217_voltage_update(uchar dc_cntrl_reg, uchar volt_sel)
> 
> Maybe pmic_set_output() method from ./drivers/power/power_core.c can be
> reused?

I'm not sure.

[snip]
> > +#define TPS65217_SEQ6				0x1E
> 
> Shouldn't the above registers be defined as enum?
> 
> For example at ./include/power/max8997_pmic.h
> /* MAX 8997 registers */
> enum {
> 	MAX8997_REG_PMIC_ID0	= 0x00,
> 	MAX8997_REG_PMIC_ID1	= 0x01,
> 	MAX8997_REG_INTSRC	= 0x02,
> 	....
> 	PMIC_NUM_OF_REGS

I assume it's a style thing I've overlooked, so sure, not a problem in
general.

-- 
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/20130814/8f9dff65/attachment.pgp>