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.