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

[Lukasz Majewski]

On Wed, 14 Aug 2013 11:57:06 -0400 Tom Rini <trini at ti.com> wrote:
> 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. 

As fair as I remember, there is no such assumption. The pmic driver
allocates each pmic object separately (which can be distinguished by
unique name - also many instances of the same devices are possible).
Each power device is treated in the same way (described by struct
pmic), no matter if it is a battery, charger, PMIC or MUIC.

Then each reference is done via struct pmic *p pointer. The charger is
not needed to use the generic pmic_reg_write().


The tps65217_reg_read() method is used at board/ti/am335x/board.c -
[PATCH v2 6/6] am335x_evm: am33xx_spl_board_init function and scale
core frequency 

It is a similar use to pmic_init_max8997(void) defined
at board/samsung/trats/trats.c


> We need both in the same
> binary (since we decide at run-time if it's one of the boards with
> 65910 or 65217).

The pmic core can register both devices, then with OF decide to which
one refer with e.g. p->name field.

> 
> > 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).

We have spent some time with Stefano to provide correct read/write for
the following:

- 1,2,3 bytes transfers
- little + big endian data format support
- support for SPI and I2C.

This is already implemented at pmic_reg_write().

> 
> [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.

At least we shall give it a try.

> 
> [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.
> 

Ok, thanks.

I'm aware, that current pmic framework has some shortcomings, but I
also believe that it can be developed to serve as a unified power
management framework for u-boot users.

Regards,
Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130814/512ff1aa/attachment.pgp>