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

login
register
mail settings
Submitter Tom Rini
Date Aug. 14, 2013, 2:17 p.m.
Message ID <1376489839-18046-2-git-send-email-trini@ti.com>
Download mbox | patch
Permalink /patch/267135/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Tom Rini - Aug. 14, 2013, 2:17 p.m.
From: Greg Guyotte <gguyotte@ti.com>

Add a driver for the TPS65217 PMIC that is found in the Beaglebone
family of boards.

Signed-off-by: Greg Guyotte <gguyotte@ti.com>
[trini: Split and rework Greg's changes into new drivers/power
framework]
Signed-off-by: Tom Rini <trini@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@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
Ɓukasz Majewski - Aug. 14, 2013, 3:08 p.m.
Hi Tom, Greg

> From: Greg Guyotte <gguyotte@ti.com>
> 
> Add a driver for the TPS65217 PMIC that is found in the Beaglebone
> family of boards.
> 
> Signed-off-by: Greg Guyotte <gguyotte@ti.com>
> [trini: Split and rework Greg's changes into new drivers/power
> framework]
> Signed-off-by: Tom Rini <trini@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@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

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?

> +}
> +
> +/**
> + *  tps65217_reg_write() - Generic function that can write a
> TPS65217 PMIC
> + *			   register or bit field regardless of
> protection
> + *			   level.
> + *
> + *  @prot_level:	   Register password protection.  Use
> + *			   TPS65217_PROT_LEVEL_NONE,
> + *			   TPS65217_PROT_LEVEL_1 or
> TPS65217_PROT_LEVEL_2
> + *  @dest_reg:		   Register address to write.
> + *  @dest_val:		   Value to write.
> + *  @mask:		   Bit mask (8 bits) to be applied.
> Function will only
> + *			   change bits that are set in the bit
> mask.
> + *
> + *  @return:		   0 for success, not 0 on failure, as
> per the i2c API
> + */
> +int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar
> dest_val,

The same as above.

> +		       uchar mask)
> +{
> +	uchar read_val;
> +	uchar xor_reg;
> +	int ret;
> +
> +	/*
> +	 * If we are affecting only a bit field, read dest_reg and
> apply the
> +	 * mask
> +	 */
> +	if (mask != TPS65217_MASK_ALL_BITS) {
> +		ret = i2c_read(TPS65217_CHIP_PM, dest_reg, 1,
> &read_val, 1);
> +		if (ret)
> +			return ret;
> +		read_val &= (~mask);
> +		read_val |= (dest_val & mask);
> +		dest_val = read_val;
> +	}
> +
> +	if (prot_level > 0) {
> +		xor_reg = dest_reg ^ TPS65217_PASSWORD_UNLOCK;
> +		ret = i2c_write(TPS65217_CHIP_PM, TPS65217_PASSWORD,
> 1,
> +				&xor_reg, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = i2c_write(TPS65217_CHIP_PM, dest_reg, 1, &dest_val, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (prot_level == TPS65217_PROT_LEVEL_2) {
> +		ret = i2c_write(TPS65217_CHIP_PM, TPS65217_PASSWORD,
> 1,
> +				&xor_reg, 1);
> +		if (ret)
> +			return ret;
> +
> +		ret = i2c_write(TPS65217_CHIP_PM, dest_reg, 1,
> &dest_val, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * 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?

> +{
> +	if ((dc_cntrl_reg != TPS65217_DEFDCDC1) &&
> +	    (dc_cntrl_reg != TPS65217_DEFDCDC2) &&
> +	    (dc_cntrl_reg != TPS65217_DEFDCDC3))
> +		return 1;
> +
> +	/* set voltage level */
> +	if (tps65217_reg_write(TPS65217_PROT_LEVEL_2, dc_cntrl_reg,
> volt_sel,
> +			       TPS65217_MASK_ALL_BITS))
> +		return 1;
> +
> +	/* set GO bit to initiate voltage transition */
> +	if (tps65217_reg_write(TPS65217_PROT_LEVEL_2,
> TPS65217_DEFSLEW,
> +			       TPS65217_DCDC_GO, TPS65217_DCDC_GO))
> +		return 1;
> +
> +	return 0;
> +}
> diff --git a/include/power/tps65217.h b/include/power/tps65217.h
> new file mode 100644
> index 0000000..f4c7a2b
> --- /dev/null
> +++ b/include/power/tps65217.h
> @@ -0,0 +1,79 @@
> +/*
> + * (C) Copyright 2011-2013
> + * Texas Instruments, <www.ti.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + *
> + * For more details, please see the TRM at
> http://www.ti.com/product/tps65217a
> + */
> +
> +#ifndef __POWER_TPS65217_H__
> +#define __POWER_TPS65217_H__
> +
> +/* I2C chip address */
> +#define TPS65217_CHIP_PM			0x24
> +
> +/* Registers */
> +#define TPS65217_CHIPID				0x00
> +#define TPS65217_POWER_PATH			0x01
> +#define TPS65217_INTERRUPT			0x02
> +#define TPS65217_CHGCONFIG0			0x03
> +#define TPS65217_CHGCONFIG1			0x04
> +#define TPS65217_CHGCONFIG2			0x05
> +#define TPS65217_CHGCONFIG3			0x06
> +#define TPS65217_WLEDCTRL1			0x07
> +#define TPS65217_WLEDCTRL2			0x08
> +#define TPS65217_MUXCTRL			0x09
> +#define TPS65217_STATUS				0x0A
> +#define TPS65217_PASSWORD			0x0B
> +#define TPS65217_PGOOD				0x0C
> +#define TPS65217_DEFPG				0x0D
> +#define TPS65217_DEFDCDC1			0x0E
> +#define TPS65217_DEFDCDC2			0x0F
> +#define TPS65217_DEFDCDC3			0x10
> +#define TPS65217_DEFSLEW			0x11
> +#define TPS65217_DEFLDO1			0x12
> +#define TPS65217_DEFLDO2			0x13
> +#define TPS65217_DEFLS1				0x14
> +#define TPS65217_DEFLS2				0x15
> +#define TPS65217_ENABLE				0x16
> +#define TPS65217_DEFUVLO			0x18
> +#define TPS65217_SEQ1				0x19
> +#define TPS65217_SEQ2				0x1A
> +#define TPS65217_SEQ3				0x1B
> +#define TPS65217_SEQ4				0x1C
> +#define TPS65217_SEQ5				0x1D
> +#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
}

> +#define TPS65217_PROT_LEVEL_NONE		0x00
> +#define TPS65217_PROT_LEVEL_1			0x01
> +#define TPS65217_PROT_LEVEL_2			0x02
> +
> +#define TPS65217_PASSWORD_LOCK_FOR_WRITE	0x00
> +#define TPS65217_PASSWORD_UNLOCK		0x7D
> +
> +#define TPS65217_DCDC_GO			0x80
> +
> +#define TPS65217_MASK_ALL_BITS			0xFF
> +
> +#define TPS65217_USB_INPUT_CUR_LIMIT_MASK	0x03
> +#define TPS65217_USB_INPUT_CUR_LIMIT_100MA	0x00
> +#define TPS65217_USB_INPUT_CUR_LIMIT_500MA	0x01
> +#define TPS65217_USB_INPUT_CUR_LIMIT_1300MA	0x02
> +#define TPS65217_USB_INPUT_CUR_LIMIT_1800MA	0x03
> +
> +#define TPS65217_DCDC_VOLT_SEL_1275MV		0x0F
> +#define TPS65217_DCDC_VOLT_SEL_1325MV		0x11
> +
> +#define TPS65217_LDO_MASK			0x1F
> +#define TPS65217_LDO_VOLTAGE_OUT_1_8		0x06
> +#define TPS65217_LDO_VOLTAGE_OUT_3_3		0x1F
> +
> +#define TPS65217_PWR_SRC_USB_BITMASK		0x4
> +#define TPS65217_PWR_SRC_AC_BITMASK		0x8
> +
> +int 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);
> +#endif	/* __POWER_TPS65217_H__ */
Tom Rini - Aug. 14, 2013, 3:57 p.m.
On Wed, Aug 14, 2013 at 05:08:12PM +0200, Lukasz Majewski wrote:
> Hi Tom, Greg
> 
> > From: Greg Guyotte <gguyotte@ti.com>
> > 
> > Add a driver for the TPS65217 PMIC that is found in the Beaglebone
> > family of boards.
> > 
> > Signed-off-by: Greg Guyotte <gguyotte@ti.com>
> > [trini: Split and rework Greg's changes into new drivers/power
> > framework]
> > Signed-off-by: Tom Rini <trini@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@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.
Lukasz Majewski - Aug. 14, 2013, 8:25 p.m.
On Wed, 14 Aug 2013 11:57:06 -0400 Tom Rini <trini@ti.com> wrote:
> On Wed, Aug 14, 2013 at 05:08:12PM +0200, Lukasz Majewski wrote:
> > Hi Tom, Greg
> > 
> > > From: Greg Guyotte <gguyotte@ti.com>
> > > 
> > > Add a driver for the TPS65217 PMIC that is found in the Beaglebone
> > > family of boards.
> > > 
> > > Signed-off-by: Greg Guyotte <gguyotte@ti.com>
> > > [trini: Split and rework Greg's changes into new drivers/power
> > > framework]
> > > Signed-off-by: Tom Rini <trini@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@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
Tom Rini - Aug. 28, 2013, 1:16 p.m.
On Wed, Aug 14, 2013 at 10:25:43PM +0200, Lukasz Majewski wrote:
> On Wed, 14 Aug 2013 11:57:06 -0400 Tom Rini <trini@ti.com> wrote:
> > On Wed, Aug 14, 2013 at 05:08:12PM +0200, Lukasz Majewski wrote:
> > > Hi Tom, Greg
> > >
> > > > From: Greg Guyotte <gguyotte@ti.com>
> > > >
> > > > Add a driver for the TPS65217 PMIC that is found in the Beaglebone
> > > > family of boards.
> > > >
> > > > Signed-off-by: Greg Guyotte <gguyotte@ti.com>
> > > > [trini: Split and rework Greg's changes into new drivers/power
> > > > framework]
> > > > Signed-off-by: Tom Rini <trini@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@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.

Getting back to this thread again, sorry, drivers/power/pmic/pmic_max*
each has 'pmic_init' as a function meaning that only one each may be
built at a time.

> 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

In concept, yes, except we might have either a tps65910 or a tps65217
and we won't know which until run-time, so we need to build in both.

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

Except for the function problem above, yes :)

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

Right, but it won't buy us anything when we have to wrap our call around
that with calls to do the password logic.  That's actually the "hard"
part of these writes.

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

If we make pmic_reg_write be per-chip or so, yes, we could make use of a
general "do something" function.

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

Right, but we need to think about it a bit more and the first step is
getting some non-Maxim chips in the area so people see them.  Then we
can adapt everyone as a follow-up.
Lukasz Majewski - Aug. 28, 2013, 9:24 p.m.
Hi Tom,

> On Wed, Aug 14, 2013 at 10:25:43PM +0200, Lukasz Majewski wrote:
> > On Wed, 14 Aug 2013 11:57:06 -0400 Tom Rini <trini@ti.com> wrote:
> > > On Wed, Aug 14, 2013 at 05:08:12PM +0200, Lukasz Majewski wrote:
> > > > Hi Tom, Greg
> > > >
> > > > > From: Greg Guyotte <gguyotte@ti.com>
> > > > >
> > > > > Add a driver for the TPS65217 PMIC that is found in the
> > > > > Beaglebone family of boards.
> > > > >
> > > > > Signed-off-by: Greg Guyotte <gguyotte@ti.com>
> > > > > [trini: Split and rework Greg's changes into new drivers/power
> > > > > framework]
> > > > > Signed-off-by: Tom Rini <trini@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@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.
> 
> Getting back to this thread again, sorry, drivers/power/pmic/pmic_max*
> each has 'pmic_init' as a function meaning that only one each may be
> built at a time.

Good point.... :/

> 
> > 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
> 
> In concept, yes, except we might have either a tps65910 or a tps65217
> and we won't know which until run-time, so we need to build in both.
> 
> > > 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.
> 
> Except for the function problem above, yes :)

I think that {pmic}_init function shall be moved from per device file
to ./drivers/power/power_core.c

Only then we can provide support for many "pmics" compiled in with run
time initialization and per name (e.g. "MAX8998_PMIC0",
"MAX8998_PMIC1", "MAX8998_PMICn" ....) selection.

This shall facilitate usage of one u-boot binary supporting many
boards/chip versions.

> 
> > > > 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().
> 
> Right, but it won't buy us anything when we have to wrap our call
> around that with calls to do the password logic.  That's actually the
> "hard" part of these writes.

I must check what the "password logic" with TI chips mean.

> 
> > > [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.
> 
> If we make pmic_reg_write be per-chip or so, yes, we could make use
> of a general "do something" function.

One of possible solutions. Good idea.

> 
> > > [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.
> 
> Right, but we need to think about it a bit more and the first step is
> getting some non-Maxim chips in the area so people see them.  Then we
> can adapt everyone as a follow-up.

As a starting point - we need to support compiled in pmic (or any
other) devices of the same kind (like many pmics) as you pointed out
with TI chips.

Then we can adjust the pmic_reg_write definition

What do you think?
> 

Regards,
Lukasz Majewski
Tom Rini - Aug. 28, 2013, 9:44 p.m.
On Wed, Aug 28, 2013 at 11:24:47PM +0200, Lukasz Majewski wrote:
> Hi Tom,
> 
> > On Wed, Aug 14, 2013 at 10:25:43PM +0200, Lukasz Majewski wrote:
> > > On Wed, 14 Aug 2013 11:57:06 -0400 Tom Rini <trini@ti.com> wrote:
> > > > On Wed, Aug 14, 2013 at 05:08:12PM +0200, Lukasz Majewski wrote:
> > > > > Hi Tom, Greg
> > > > >
> > > > > > From: Greg Guyotte <gguyotte@ti.com>
> > > > > >
> > > > > > Add a driver for the TPS65217 PMIC that is found in the
> > > > > > Beaglebone family of boards.
> > > > > >
> > > > > > Signed-off-by: Greg Guyotte <gguyotte@ti.com>
> > > > > > [trini: Split and rework Greg's changes into new drivers/power
> > > > > > framework]
> > > > > > Signed-off-by: Tom Rini <trini@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@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.
> > 
> > Getting back to this thread again, sorry, drivers/power/pmic/pmic_max*
> > each has 'pmic_init' as a function meaning that only one each may be
> > built at a time.
> 
> Good point.... :/
> 
> > 
> > > 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
> > 
> > In concept, yes, except we might have either a tps65910 or a tps65217
> > and we won't know which until run-time, so we need to build in both.
> > 
> > > > 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.
> > 
> > Except for the function problem above, yes :)
> 
> I think that {pmic}_init function shall be moved from per device file
> to ./drivers/power/power_core.c
> 
> Only then we can provide support for many "pmics" compiled in with run
> time initialization and per name (e.g. "MAX8998_PMIC0",
> "MAX8998_PMIC1", "MAX8998_PMICn" ....) selection.
> 
> This shall facilitate usage of one u-boot binary supporting many
> boards/chip versions.

Right, sounds workable.

> > > > > 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().
> > 
> > Right, but it won't buy us anything when we have to wrap our call
> > around that with calls to do the password logic.  That's actually the
> > "hard" part of these writes.
> 
> I must check what the "password logic" with TI chips mean.
> 
> > 
> > > > [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.
> > 
> > If we make pmic_reg_write be per-chip or so, yes, we could make use
> > of a general "do something" function.
> 
> One of possible solutions. Good idea.
> 
> > 
> > > > [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.
> > 
> > Right, but we need to think about it a bit more and the first step is
> > getting some non-Maxim chips in the area so people see them.  Then we
> > can adapt everyone as a follow-up.
> 
> As a starting point - we need to support compiled in pmic (or any
> other) devices of the same kind (like many pmics) as you pointed out
> with TI chips.
> 
> Then we can adjust the pmic_reg_write definition
> 
> What do you think?

I think we're getting there, and it will be easier with other PMICs in
tree ;)

Patch

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);
+}
+
+/**
+ *  tps65217_reg_write() - Generic function that can write a TPS65217 PMIC
+ *			   register or bit field regardless of protection
+ *			   level.
+ *
+ *  @prot_level:	   Register password protection.  Use
+ *			   TPS65217_PROT_LEVEL_NONE,
+ *			   TPS65217_PROT_LEVEL_1 or TPS65217_PROT_LEVEL_2
+ *  @dest_reg:		   Register address to write.
+ *  @dest_val:		   Value to write.
+ *  @mask:		   Bit mask (8 bits) to be applied.  Function will only
+ *			   change bits that are set in the bit mask.
+ *
+ *  @return:		   0 for success, not 0 on failure, as per the i2c API
+ */
+int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar dest_val,
+		       uchar mask)
+{
+	uchar read_val;
+	uchar xor_reg;
+	int ret;
+
+	/*
+	 * If we are affecting only a bit field, read dest_reg and apply the
+	 * mask
+	 */
+	if (mask != TPS65217_MASK_ALL_BITS) {
+		ret = i2c_read(TPS65217_CHIP_PM, dest_reg, 1, &read_val, 1);
+		if (ret)
+			return ret;
+		read_val &= (~mask);
+		read_val |= (dest_val & mask);
+		dest_val = read_val;
+	}
+
+	if (prot_level > 0) {
+		xor_reg = dest_reg ^ TPS65217_PASSWORD_UNLOCK;
+		ret = i2c_write(TPS65217_CHIP_PM, TPS65217_PASSWORD, 1,
+				&xor_reg, 1);
+		if (ret)
+			return ret;
+	}
+
+	ret = i2c_write(TPS65217_CHIP_PM, dest_reg, 1, &dest_val, 1);
+	if (ret)
+		return ret;
+
+	if (prot_level == TPS65217_PROT_LEVEL_2) {
+		ret = i2c_write(TPS65217_CHIP_PM, TPS65217_PASSWORD, 1,
+				&xor_reg, 1);
+		if (ret)
+			return ret;
+
+		ret = i2c_write(TPS65217_CHIP_PM, dest_reg, 1, &dest_val, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * 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)
+{
+	if ((dc_cntrl_reg != TPS65217_DEFDCDC1) &&
+	    (dc_cntrl_reg != TPS65217_DEFDCDC2) &&
+	    (dc_cntrl_reg != TPS65217_DEFDCDC3))
+		return 1;
+
+	/* set voltage level */
+	if (tps65217_reg_write(TPS65217_PROT_LEVEL_2, dc_cntrl_reg, volt_sel,
+			       TPS65217_MASK_ALL_BITS))
+		return 1;
+
+	/* set GO bit to initiate voltage transition */
+	if (tps65217_reg_write(TPS65217_PROT_LEVEL_2, TPS65217_DEFSLEW,
+			       TPS65217_DCDC_GO, TPS65217_DCDC_GO))
+		return 1;
+
+	return 0;
+}
diff --git a/include/power/tps65217.h b/include/power/tps65217.h
new file mode 100644
index 0000000..f4c7a2b
--- /dev/null
+++ b/include/power/tps65217.h
@@ -0,0 +1,79 @@ 
+/*
+ * (C) Copyright 2011-2013
+ * Texas Instruments, <www.ti.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ *
+ * For more details, please see the TRM at http://www.ti.com/product/tps65217a
+ */
+
+#ifndef __POWER_TPS65217_H__
+#define __POWER_TPS65217_H__
+
+/* I2C chip address */
+#define TPS65217_CHIP_PM			0x24
+
+/* Registers */
+#define TPS65217_CHIPID				0x00
+#define TPS65217_POWER_PATH			0x01
+#define TPS65217_INTERRUPT			0x02
+#define TPS65217_CHGCONFIG0			0x03
+#define TPS65217_CHGCONFIG1			0x04
+#define TPS65217_CHGCONFIG2			0x05
+#define TPS65217_CHGCONFIG3			0x06
+#define TPS65217_WLEDCTRL1			0x07
+#define TPS65217_WLEDCTRL2			0x08
+#define TPS65217_MUXCTRL			0x09
+#define TPS65217_STATUS				0x0A
+#define TPS65217_PASSWORD			0x0B
+#define TPS65217_PGOOD				0x0C
+#define TPS65217_DEFPG				0x0D
+#define TPS65217_DEFDCDC1			0x0E
+#define TPS65217_DEFDCDC2			0x0F
+#define TPS65217_DEFDCDC3			0x10
+#define TPS65217_DEFSLEW			0x11
+#define TPS65217_DEFLDO1			0x12
+#define TPS65217_DEFLDO2			0x13
+#define TPS65217_DEFLS1				0x14
+#define TPS65217_DEFLS2				0x15
+#define TPS65217_ENABLE				0x16
+#define TPS65217_DEFUVLO			0x18
+#define TPS65217_SEQ1				0x19
+#define TPS65217_SEQ2				0x1A
+#define TPS65217_SEQ3				0x1B
+#define TPS65217_SEQ4				0x1C
+#define TPS65217_SEQ5				0x1D
+#define TPS65217_SEQ6				0x1E
+
+#define TPS65217_PROT_LEVEL_NONE		0x00
+#define TPS65217_PROT_LEVEL_1			0x01
+#define TPS65217_PROT_LEVEL_2			0x02
+
+#define TPS65217_PASSWORD_LOCK_FOR_WRITE	0x00
+#define TPS65217_PASSWORD_UNLOCK		0x7D
+
+#define TPS65217_DCDC_GO			0x80
+
+#define TPS65217_MASK_ALL_BITS			0xFF
+
+#define TPS65217_USB_INPUT_CUR_LIMIT_MASK	0x03
+#define TPS65217_USB_INPUT_CUR_LIMIT_100MA	0x00
+#define TPS65217_USB_INPUT_CUR_LIMIT_500MA	0x01
+#define TPS65217_USB_INPUT_CUR_LIMIT_1300MA	0x02
+#define TPS65217_USB_INPUT_CUR_LIMIT_1800MA	0x03
+
+#define TPS65217_DCDC_VOLT_SEL_1275MV		0x0F
+#define TPS65217_DCDC_VOLT_SEL_1325MV		0x11
+
+#define TPS65217_LDO_MASK			0x1F
+#define TPS65217_LDO_VOLTAGE_OUT_1_8		0x06
+#define TPS65217_LDO_VOLTAGE_OUT_3_3		0x1F
+
+#define TPS65217_PWR_SRC_USB_BITMASK		0x4
+#define TPS65217_PWR_SRC_AC_BITMASK		0x8
+
+int 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);
+#endif	/* __POWER_TPS65217_H__ */