From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukasz Majewski Date: Mon, 31 Mar 2014 16:33:18 +0200 Subject: [U-Boot] [PATCH 02/10] power: Add support for TPS65090 PMU chip. In-Reply-To: References: <1395856590-21917-1-git-send-email-sjg@chromium.org> <1395856590-21917-3-git-send-email-sjg@chromium.org> <20140327185904.3cc16073@amdc2363> Message-ID: <20140331163318.3be62823@amdc2363> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, > Hi Lukasz, > > On 27 March 2014 11:59, Lukasz Majewski > wrote: Hi Simon, > > > From: Tom Wai-Hong Tam > > > > 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 > > Signed-off-by: Simon Glass > > Signed-off-by: Hatim Ali > > Signed-off-by: Katie Roberts-Hoffman > > Signed-off-by: Rong Chang > > Signed-off-by: Sean Paul > > Signed-off-by: Vincent Palatin > > Signed-off-by: Aaron Durbin > > --- > > > > ?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 at 12ca0000 { > > + ? ? ? ? ? ? ? power-regulator at 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +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 > , 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 > -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group