Hi Simon,
Hi Lukasz,
On 27 March 2014 11:59, Lukasz Majewski l.majewski@samsung.com wrote: Hi Simon,
From: Tom Wai-Hong Tam waihong@chromium.org
This adds driver support for the TPS65090 PMU. Support includes hooking into the pmic infrastructure so that the pmic commands can be used on the console. The TPS65090 supports the following functionality:
- fet enable/disable/querying
- getting and setting of charge state
Even though it is connected to the pmic infrastructure it does not hook into the pmic charging charging infrastructure.
Can I ask what was the problem with adding support for "pmic bat [state|charge]" command on this PMIC?
Was the framework unfriendly or there was no need to do that?
It's not great. I spent a bit of time trying again. It think the issues are:
- no device tree support
- no comments in the pmic.h file so it's hard do know what everything
is for
You are right here - the lack of DT support is the main problem here.
As Tom Rini pointed out - it is problematic in this framework to support more than one instance of the same PMIC device.
I must confess that I've overlooked this use case.
- the battery charging command should really be common, not specific
to each driver
I agree. We can try to discuss one solution suitable for Exynos4 and 5.
I did actually create a battery system in the Chromium source tree a while back, but never sent it upstream, partly because the pmic stuff was there and I was not sure how to get it into that framework.
I think maybe if someone can comment the pmic.h file then I could have another try. But it would be a separate series to this one, which is focussed on the LCD, not the battery.
For a quick reference please consider Trats and Trats2. If you need any more help, then please write an e-mail to me.
Signed-off-by: Tom Wai-Hong Tam waihong@chromium.org Signed-off-by: Simon Glass sjg@chromium.org Signed-off-by: Hatim Ali hatim.rv@samsung.com Signed-off-by: Katie Roberts-Hoffman katierh@chromium.org Signed-off-by: Rong Chang rongchang@chromium.org Signed-off-by: Sean Paul seanpaul@chromium.org Signed-off-by: Vincent Palatin vpalatin@chromium.org Signed-off-by: Aaron Durbin adurbin@chromium.org
doc/device-tree-bindings/power/tps65090.txt | 21 ++ drivers/power/pmic/Makefile | 1 + drivers/power/pmic/pmic_tps65090.c | 296 ++++++++++++++++++++++++++++ include/fdtdec.h | 1 + include/power/tps65090_pmic.h | 83 ++++++++ lib/fdtdec.c | 1 + 6 files changed, 403 insertions(+) create mode 100644 doc/device-tree-bindings/power/tps65090.txt create mode 100644 drivers/power/pmic/pmic_tps65090.c create mode 100644 include/power/tps65090_pmic.h
diff --git a/doc/device-tree-bindings/power/tps65090.txt b/doc/device-tree-bindings/power/tps65090.txt new file mode 100644 index 0000000..6a8a884 --- /dev/null +++ b/doc/device-tree-bindings/power/tps65090.txt @@ -0,0 +1,21 @@ +TPSchrome binding +=================
+The device tree node which describes the operation of the Texas Instrument +TPS65090 power regulator chip is as follows:
+Required properties : +- compatible : "ti,tps65090" +- reg : slave address on the i2c bus
+The tps65090 node should appear as a subnode of the i2c bus that connects it. + +Example +=======
- i2c@12ca0000 {
- power-regulator@48 {
- compatible = "ti,tps65090"
- reg = <0x48>;
- };
- };
Those bindings look very different from the one at Linux kernel for this device. Therefore I assume that there was justification to not stick to the linux kernel DT format.
Yes they pre-date the kernel. But now that the kernel has support I will pull those in. For the parts we use it is the same.
That would be great, thanks.
diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile index 0b45ffa..7ed55e6 100644 --- a/drivers/power/pmic/Makefile +++ b/drivers/power/pmic/Makefile @@ -9,5 +9,6 @@ obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o obj-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o obj-$(CONFIG_POWER_MAX77686) += pmic_max77686.o +obj-$(CONFIG_POWER_TPS65090) += pmic_tps65090.o obj-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o obj-$(CONFIG_POWER_TPS65910) += pmic_tps65910.o diff --git a/drivers/power/pmic/pmic_tps65090.c b/drivers/power/pmic/pmic_tps65090.c new file mode 100644 index 0000000..ef9f911 --- /dev/null +++ b/drivers/power/pmic/pmic_tps65090.c @@ -0,0 +1,296 @@ +/*
- Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
- Use of this source code is governed by a BSD-style license that
can be
- found in the LICENSE file.
- Alternatively, this software may be distributed under the terms
of the
- GNU General Public License ("GPL") version 2 as published by the
Free
- Software Foundation.
Would it be possible to tune this license description to be similar to SPDX-License-Identifier: GPL-2.0+|BSD
Yes, will do.
- */
+#include <common.h> +#include <errno.h> +#include <fdtdec.h> +#include <i2c.h> +#include <power/pmic.h> +#include <power/tps65090_pmic.h>
+DECLARE_GLOBAL_DATA_PTR;
+#define TPS65090_NAME "TPS65090_PMIC"
+/* TPS65090 register addresses */ +enum {
- REG_CG_CTRL0 = 4,
- REG_CG_STATUS1 = 0xa,
- REG_FET1_CTRL = 0x0f,
- REG_FET2_CTRL,
- REG_FET3_CTRL,
- REG_FET4_CTRL,
- REG_FET5_CTRL,
- REG_FET6_CTRL,
- REG_FET7_CTRL,
- TPS65090_NUM_REGS,
+};
+enum {
- CG_CTRL0_ENC_MASK = 0x01,
- MAX_FET_NUM = 7,
- MAX_CTRL_READ_TRIES = 5,
- /* TPS65090 FET_CTRL register values */
- FET_CTRL_TOFET = 1 << 7, /* Timeout, startup,
overload */
- FET_CTRL_PGFET = 1 << 4, /* Power good for FET
status */
- FET_CTRL_WAIT = 3 << 2, /* Overcurrent timeout
max */
- FET_CTRL_ADENFET = 1 << 1, /* Enable output auto
discharge */
- FET_CTRL_ENFET = 1 << 0, /* Enable FET */
+};
+/**
- Checks for a valid FET number
- @param fet_id FET number to check
- @return 0 if ok, -1 if FET value is out of range
- */
+static int tps65090_check_fet(unsigned int fet_id) +{
- if (fet_id == 0 || fet_id > MAX_FET_NUM) {
- debug("parameter fet_id is out of range, %u not in 1
~ %u\n",
- fet_id, MAX_FET_NUM);
- return -1;
In the recent code (like trats/trats2, bugs fixed at existing power infrastructure) I'm encouraging people to use error descriptions from <errno.h>, not the -1/-2 values. Can you fix this globally in this patch?
Yes good idea.
- }
- return 0;
+}
+/**
- Set the power state for a FET
- @param pmic pmic structure for the tps65090
- @param fet_id Fet number to set (1..MAX_FET_NUM)
- @param set 1 to power on FET, 0 to power off
- @return FET_ERR_COMMS if we got a comms error, FET_ERR_NOT_READY
if the
- FET failed to change state. If all is ok, returns 0.
- */
+static int tps65090_fet_set(struct pmic *pmic, int fet_id, int set)
^^^^ maybe bool
+{
- int retry;
- u32 reg, value;
- value = FET_CTRL_ADENFET | FET_CTRL_WAIT;
- if (set)
- value |= FET_CTRL_ENFET;
- if (pmic_reg_write(pmic, REG_FET1_CTRL + fet_id - 1, value))
- return FET_ERR_COMMS;
- /* Try reading until we get a result */
- for (retry = 0; retry < MAX_CTRL_READ_TRIES; retry++) {
- if (pmic_reg_read(pmic, REG_FET1_CTRL + fet_id - 1,
®))
- return FET_ERR_COMMS;
- /* Check that the fet went into the expected state
*/
- if (!!(reg & FET_CTRL_PGFET) == set)
- return 0;
- /* If we got a timeout, there is no point in waiting
longer */
- if (reg & FET_CTRL_TOFET)
- break;
- mdelay(1);
- }
- debug("FET %d: Power good should have set to %d but
reg=%#02x\n",
- fet_id, set, reg);
- return FET_ERR_NOT_READY;
+}
+int tps65090_fet_enable(unsigned int fet_id) +{
- int loops;
- ulong start;
- struct pmic *pmic;
- int ret = 0;
- if (tps65090_check_fet(fet_id))
- return -1;
As I've described above - maybe -ENODEV?
- pmic = pmic_get(TPS65090_NAME);
- if (pmic == NULL)
It seems like in the code I tend to use: if (!pmic)
OK
- return -1;
- start = get_timer(0);
- for (loops = 0;; loops++) {
- ret = tps65090_fet_set(pmic, fet_id, 1);
- if (!ret)
- break;
- if (get_timer(start) > 100)
- break;
- /* Turn it off and try again until we time out */
- tps65090_fet_set(pmic, fet_id, 0);
- }
- if (ret) {
- debug("%s: FET%d failed to power on: time=%lums,
loops=%d\n",
- __func__, fet_id, get_timer(start), loops);
- } else if (loops) {
- debug("%s: FET%d powered on after %lums,
loops=%d\n",
- __func__, fet_id, get_timer(start), loops);
- }
Here the parenthesis can be omitted.
OK
- /*
- * Unfortunately, there are some conditions where the power
- * good bit will be 0, but the fet still comes up. One such
- * case occurs with the lcd backlight. We'll just return 0
here
- * and assume that the fet will eventually come up.
- */
- if (ret == FET_ERR_NOT_READY)
- ret = 0;
- return ret;
+}
+int tps65090_fet_disable(unsigned int fet_id) +{
- int ret;
- struct pmic *pmic;
- if (tps65090_check_fet(fet_id))
- return -1;
- pmic = pmic_get(TPS65090_NAME);
- if (pmic == NULL)
- return -1;
- ret = tps65090_fet_set(pmic, fet_id, 0);
- return ret;
+}
+int tps65090_fet_is_enabled(unsigned int fet_id) +{
- u32 reg;
- int ret;
- struct pmic *pmic;
- if (tps65090_check_fet(fet_id))
- return -1;
- pmic = pmic_get(TPS65090_NAME);
- if (pmic == NULL)
- return -1;
- ret = pmic_reg_read(pmic, REG_FET1_CTRL + fet_id - 1, ®);
- if (ret) {
- debug("fail to read FET%u_CTRL register over I2C",
fet_id);
- return -2;
- }
- return reg & FET_CTRL_ENFET;
+}
+int tps65090_get_charging(void) +{
- u32 val;
- int ret;
- struct pmic *pmic;
- pmic = pmic_get(TPS65090_NAME);
- if (pmic == NULL)
- return -1;
- ret = pmic_reg_read(pmic, REG_CG_CTRL0, &val);
- if (ret)
- return ret;
- return val & CG_CTRL0_ENC_MASK ? 1 : 0;
+}
+int tps65090_set_charge_enable(int enable)
This can be easily added to be used at "pmic bat charge" command.
Please look into the ./drivers/power/mfd/pmic_max77693.c
Not easily. As mentioned above this is quite a bit of work. For a later series I think.
I'm looking forward for questions :-).
Regards, Simon