[U-Boot] [PATCH 02/10] power: Add support for TPS65090 PMU chip.

[Lukasz Majewski <l.majewski@samsung.com>]

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 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 <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,
> > &reg))
> > + ? ? ? ? ? ? ? ? ? ? ? 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, &reg);
> > + ? ? ? 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