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 <tr...@ti.com> wrote:
> > > > On Wed, Aug 14, 2013 at 05:08:12PM +0200, Lukasz Majewski wrote:
> > > > > Hi Tom, Greg
> > > > >
> > > > > > From: Greg Guyotte <gguyo...@ti.com>
> > > > > >
> > > > > > Add a driver for the TPS65217 PMIC that is found in the
> > > > > > Beaglebone family of boards.
> > > > > >
> > > > > > Signed-off-by: Greg Guyotte <gguyo...@ti.com>
> > > > > > [trini: Split and rework Greg's changes into new drivers/power
> > > > > > framework]
> > > > > > Signed-off-by: Tom Rini <tr...@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 <tr...@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 ;)
--
Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
