Hi Simon,
> Hi Lukasz,
>
> On 27 March 2014 11:59, Lukasz Majewski <l.majew...@samsung.com>
> wrote: Hi Simon,
>
> > From: Tom Wai-Hong Tam <waih...@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 <waih...@chromium.org>
> > Signed-off-by: Simon Glass <s...@chromium.org>
> > Signed-off-by: Hatim Ali <hatim...@samsung.com>
> > Signed-off-by: Katie Roberts-Hoffman <kati...@chromium.org>
> > Signed-off-by: Rong Chang <rongch...@chromium.org>
> > Signed-off-by: Sean Paul <seanp...@chromium.org>
> > Signed-off-by: Vincent Palatin <vpala...@chromium.org>
> > Signed-off-by: Aaron Durbin <adur...@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
>
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
