| Message ID | 1377766145-6678-2-git-send-email-p.wilczek@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
Dear Piotr Wilczek, On 29/08/13 17:49, Piotr Wilczek wrote: > Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/power/battery/Makefile | 1 + > drivers/power/battery/bat_trats2.c | 65 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > create mode 100644 drivers/power/battery/bat_trats2.c > bat_trats2.c is almost same with bat_trat.c I think, it can be reuse bat_trat.c Do you have special reason to add new file? Thanks, Minkyu Kang.
Dear Minkyu Kang, > -----Original Message----- > From: Minkyu Kang [mailto:mk7.kang@samsung.com] > Sent: Friday, August 30, 2013 6:39 AM > To: Piotr Wilczek > Cc: u-boot@lists.denx.de; Kyungmin Park; Lukasz Majewski > Subject: Re: [PATCH V4 1/3] power:battery: add battery support for > Trats2 board > > Dear Piotr Wilczek, > > On 29/08/13 17:49, Piotr Wilczek wrote: > > Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > --- > > drivers/power/battery/Makefile | 1 + > > drivers/power/battery/bat_trats2.c | 65 > ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 66 insertions(+) > > create mode 100644 drivers/power/battery/bat_trats2.c > > > > bat_trats2.c is almost same with bat_trat.c I think, it can be reuse > bat_trat.c Do you have special reason to add new file? If several boards would use that file, any change to it will affect all these boards. Also why it should be named 'bat_trats' then? Other reason is that I don't want to block command line while charging battery as in 'bat_trats'. Anyway it's not that important. I will modify it the way you prefer. Best regards, Piotr Wilczek > > Thanks, > Minkyu Kang.
Dear Minkyu Kang, > -----Original Message----- > From: Piotr Wilczek [mailto:p.wilczek@samsung.com] > Sent: Friday, August 30, 2013 8:01 AM > To: 'Minkyu Kang' > Cc: 'u-boot@lists.denx.de'; 'Kyungmin Park'; Lukasz Majewski > Subject: RE: [PATCH V4 1/3] power:battery: add battery support for > Trats2 board > > Dear Minkyu Kang, > > > -----Original Message----- > > From: Minkyu Kang [mailto:mk7.kang@samsung.com] > > Sent: Friday, August 30, 2013 6:39 AM > > To: Piotr Wilczek > > Cc: u-boot@lists.denx.de; Kyungmin Park; Lukasz Majewski > > Subject: Re: [PATCH V4 1/3] power:battery: add battery support for > > Trats2 board > > > > Dear Piotr Wilczek, > > > > On 29/08/13 17:49, Piotr Wilczek wrote: > > > Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > > --- > > > drivers/power/battery/Makefile | 1 + > > > drivers/power/battery/bat_trats2.c | 65 > > ++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 66 insertions(+) > > > create mode 100644 drivers/power/battery/bat_trats2.c > > > > > > > bat_trats2.c is almost same with bat_trat.c I think, it can be reuse > > bat_trat.c Do you have special reason to add new file? > > If several boards would use that file, any change to it will affect all > these boards. Also why it should be named 'bat_trats' then? > > Other reason is that I don't want to block command line while charging > battery as in 'bat_trats'. Is this reason good enough to add new file or should I post a new version of this patch? > > Anyway it's not that important. I will modify it the way you prefer. > > Best regards, > Piotr Wilczek > > > > > Thanks, > > Minkyu Kang. Best regards, Piotr Wilczek
Dear Piotr Wilczek, On 30/08/13 15:00, Piotr Wilczek wrote: > Dear Minkyu Kang, > >> -----Original Message----- >> From: Minkyu Kang [mailto:mk7.kang@samsung.com] >> Sent: Friday, August 30, 2013 6:39 AM >> To: Piotr Wilczek >> Cc: u-boot@lists.denx.de; Kyungmin Park; Lukasz Majewski >> Subject: Re: [PATCH V4 1/3] power:battery: add battery support for >> Trats2 board >> >> Dear Piotr Wilczek, >> >> On 29/08/13 17:49, Piotr Wilczek wrote: >>> Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>> --- >>> drivers/power/battery/Makefile | 1 + >>> drivers/power/battery/bat_trats2.c | 65 >> ++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 66 insertions(+) >>> create mode 100644 drivers/power/battery/bat_trats2.c >>> >> >> bat_trats2.c is almost same with bat_trat.c I think, it can be reuse >> bat_trat.c Do you have special reason to add new file? > > If several boards would use that file, any change to it will affect all > these boards. Also why it should be named 'bat_trats' then? OK. I understood what you said. But, I don't understand why this file (or directory - battery) is needed. It is not a driver, it just settings for specific board. If so, why don't you move to board file instead? Do we need to make new files for every boards? Lukasz, how you think? > > Other reason is that I don't want to block command line while charging > battery as in 'bat_trats'. > > Anyway it's not that important. I will modify it the way you prefer. > > Best regards, > Piotr Wilczek > >> >> Thanks, >> Minkyu Kang. > > > Thanks, Minkyu Kang.
Hi Minkyu, > Dear Piotr Wilczek, > > On 30/08/13 15:00, Piotr Wilczek wrote: > > Dear Minkyu Kang, > > > >> -----Original Message----- > >> From: Minkyu Kang [mailto:mk7.kang@samsung.com] > >> Sent: Friday, August 30, 2013 6:39 AM > >> To: Piotr Wilczek > >> Cc: u-boot@lists.denx.de; Kyungmin Park; Lukasz Majewski > >> Subject: Re: [PATCH V4 1/3] power:battery: add battery support for > >> Trats2 board > >> > >> Dear Piotr Wilczek, > >> > >> On 29/08/13 17:49, Piotr Wilczek wrote: > >>> Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >>> --- > >>> drivers/power/battery/Makefile | 1 + > >>> drivers/power/battery/bat_trats2.c | 65 > >> ++++++++++++++++++++++++++++++++++++ > >>> 2 files changed, 66 insertions(+) > >>> create mode 100644 drivers/power/battery/bat_trats2.c > >>> > >> > >> bat_trats2.c is almost same with bat_trat.c I think, it can be > >> reuse bat_trat.c Do you have special reason to add new file? > > > > If several boards would use that file, any change to it will affect > > all these boards. Also why it should be named 'bat_trats' then? > > OK. I understood what you said. > > But, I don't understand why this file (or directory - battery) is > needed. It is not a driver, In the pmic framework the battery is treated in the same way as MUIC, PMIC, FG. This is the reason for separate directory. > it just settings for specific board. > If so, why don't you move to board file instead? I would like to avoid code duplication. > Do we need to make new files for every boards? The problem here is with the way we are handling charging. Trats uses the "busy loop" approach. In the TRATS2 the busy loop is omitted, and only charging is enabled. Those are two different approaches for handling charging (this may also depend on PMIC capabilities). > > Lukasz, > how you think? For the PMIC itself - it needs to be rewritten to be prepared for multi board support for u-boot. It doesn't support it now (as Tom pointed out recently). Also - as shown with PMIC batteries - different charging "profiles" are needed. The bat_trats.c, bat_trats2.c [*] would be renamed to bat_profile1.c and bat_profile2.c. Also some common code from [*] would be extracted. Now it seems, that acceptance of Trats2 board depends on the shortcoming in the PMIC framework. My proposition - accept the Trats2 code (since it works and is tested). The battery code is going to be cleaned up when we finish and post PMIC framework rework. I will post request for PMIC v3 requirements soon. > > > > > Other reason is that I don't want to block command line while > > charging battery as in 'bat_trats'. > > > > Anyway it's not that important. I will modify it the way you prefer. > > > > Best regards, > > Piotr Wilczek > > > >> > >> Thanks, > >> Minkyu Kang. > > > > > > > > Thanks, > Minkyu Kang.
On 11 September 2013 19:48, Lukasz Majewski <l.majewski@samsung.com> wrote: > Hi Minkyu, > > > Dear Piotr Wilczek, > > > > On 30/08/13 15:00, Piotr Wilczek wrote: > > > Dear Minkyu Kang, > > > > > >> -----Original Message----- > > >> From: Minkyu Kang [mailto:mk7.kang@samsung.com] > > >> Sent: Friday, August 30, 2013 6:39 AM > > >> To: Piotr Wilczek > > >> Cc: u-boot@lists.denx.de; Kyungmin Park; Lukasz Majewski > > >> Subject: Re: [PATCH V4 1/3] power:battery: add battery support for > > >> Trats2 board > > >> > > >> Dear Piotr Wilczek, > > >> > > >> On 29/08/13 17:49, Piotr Wilczek wrote: > > >>> Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > > >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > >>> --- > > >>> drivers/power/battery/Makefile | 1 + > > >>> drivers/power/battery/bat_trats2.c | 65 > > >> ++++++++++++++++++++++++++++++++++++ > > >>> 2 files changed, 66 insertions(+) > > >>> create mode 100644 drivers/power/battery/bat_trats2.c > > >>> > > >> > > >> bat_trats2.c is almost same with bat_trat.c I think, it can be > > >> reuse bat_trat.c Do you have special reason to add new file? > > > > > > If several boards would use that file, any change to it will affect > > > all these boards. Also why it should be named 'bat_trats' then? > > > > OK. I understood what you said. > > > > But, I don't understand why this file (or directory - battery) is > > needed. It is not a driver, > > In the pmic framework the battery is treated in the same way as MUIC, > PMIC, FG. This is the reason for separate directory. > > > it just settings for specific board. > > If so, why don't you move to board file instead? > > > I would like to avoid code duplication. > > > > Do we need to make new files for every boards? > > The problem here is with the way we are handling charging. Trats uses > the "busy loop" approach. > In the TRATS2 the busy loop is omitted, and only charging is enabled. > > Those are two different approaches for handling charging (this may also > depend on PMIC capabilities). > > > > > > Lukasz, > > how you think? > > For the PMIC itself - it needs to be rewritten to be prepared for multi > board support for u-boot. It doesn't support it now (as Tom pointed > out recently). > > Also - as shown with PMIC batteries - different charging "profiles" are > needed. > > The bat_trats.c, bat_trats2.c [*] would be renamed to bat_profile1.c and > bat_profile2.c. Also some common code from [*] would be extracted. > > Now it seems, that acceptance of Trats2 board depends on the > shortcoming in the PMIC framework. > > My proposition - accept the Trats2 code (since it works and is tested). > > The battery code is going to be cleaned up when we finish and post PMIC > framework rework. > > I will post request for PMIC v3 requirements soon. > OK. > > > > > > > > > Other reason is that I don't want to block command line while > > > charging battery as in 'bat_trats'. > > > > > > Anyway it's not that important. I will modify it the way you prefer. > > > > > > Best regards, > > > Piotr Wilczek > > > > > >> > > >> Thanks, > > >> Minkyu Kang. > > > > > > > > > > > > > Thanks, > > Minkyu Kang. > > > -- > 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 > Thanks, Minkyu Kang.
diff --git a/drivers/power/battery/Makefile b/drivers/power/battery/Makefile index 637d1ff..4bf315d 100644 --- a/drivers/power/battery/Makefile +++ b/drivers/power/battery/Makefile @@ -10,6 +10,7 @@ include $(TOPDIR)/config.mk LIB := $(obj)libbattery.o COBJS-$(CONFIG_POWER_BATTERY_TRATS) += bat_trats.o +COBJS-$(CONFIG_POWER_BATTERY_TRATS2) += bat_trats2.o COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/power/battery/bat_trats2.c b/drivers/power/battery/bat_trats2.c new file mode 100644 index 0000000..f264832 --- /dev/null +++ b/drivers/power/battery/bat_trats2.c @@ -0,0 +1,65 @@ +/* + * Copyright (C) 2013 Samsung Electronics + * Piotr Wilczek <p.wilczek@samsung.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <power/pmic.h> +#include <power/battery.h> +#include <power/max8997_pmic.h> +#include <errno.h> + +static struct battery battery_trats; + +static int power_battery_charge(struct pmic *bat) +{ + struct power_battery *p_bat = bat->pbat; + + if (bat->chrg->chrg_state(p_bat->chrg, CHARGER_ENABLE, 450)) + return -1; + + return 0; +} + +static int power_battery_init_trats2(struct pmic *bat_, + struct pmic *fg_, + struct pmic *chrg_, + struct pmic *muic_) +{ + bat_->pbat->fg = fg_; + bat_->pbat->chrg = chrg_; + bat_->pbat->muic = muic_; + + bat_->fg = fg_->fg; + bat_->chrg = chrg_->chrg; + bat_->chrg->chrg_type = muic_->chrg->chrg_type; + return 0; +} + +static struct power_battery power_bat_trats2 = { + .bat = &battery_trats, + .battery_init = power_battery_init_trats2, + .battery_charge = power_battery_charge, +}; + +int power_bat_init(unsigned char bus) +{ + static const char name[] = "BAT_TRATS2"; + struct pmic *p = pmic_alloc(); + + if (!p) { + printf("%s: POWER allocation error!\n", __func__); + return -ENOMEM; + } + + debug("Board BAT init\n"); + + p->interface = PMIC_NONE; + p->name = name; + p->bus = bus; + + p->pbat = &power_bat_trats2; + return 0; +}