Message ID | 1347637215-4830-9-git-send-email-l.majewski@samsung.com |
---|---|
State | Superseded |
Delegated to: | Minkyu Kang |
Headers | show |
On 14/09/2012 17:40, Lukasz Majewski wrote: > Support for MUIC (Micro USB Integrated Circuit) built into the MAX8997 > power management device. > > The MUIC device will work with redesigned PMIC framework. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Stefano Babic <sbabic@denx.de> > --- Hi Lukasz, > drivers/misc/Makefile | 1 + > drivers/misc/muic_max8997.c | 80 ++++++++++++++++++++++++++++++++++++++++++ > include/power/max8997_muic.h | 61 ++++++++++++++++++++++++++++++++ > include/power/power_chrg.h | 1 + > 4 files changed, 143 insertions(+), 0 deletions(-) > create mode 100644 drivers/misc/muic_max8997.c > create mode 100644 include/power/max8997_muic.h > I see now with this patch, but we have a mix between drivers/misc and drivers/power. I think this was done because the power directory was introduced later, but really all pmics under drivers/misc should be moved into drivers/power. Maybe belong this one also to drivers/power ? You added several "add-ons" for MAX8997. Maybe it is clearer if everything goes into a separate directory, such as power/max8997/ > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 271463c..f08a800 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -41,6 +41,7 @@ COBJS-$(CONFIG_PMIC_I2C) += pmic_i2c.o > COBJS-$(CONFIG_PMIC_SPI) += pmic_spi.o > COBJS-$(CONFIG_PMIC_MAX8998) += pmic_max8998.o > COBJS-$(CONFIG_PMIC_MAX8997) += pmic_max8997.o > +COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o > > COBJS := $(COBJS-y) > SRCS := $(COBJS:.o=.c) > diff --git a/drivers/misc/muic_max8997.c b/drivers/misc/muic_max8997.c > new file mode 100644 > index 0000000..d332c09 > --- /dev/null > +++ b/drivers/misc/muic_max8997.c > @@ -0,0 +1,80 @@ > +/* > + * Copyright (C) 2012 Samsung Electronics > + * Lukasz Majewski <l.majewski@samsung.com> > + * > + * See file CREDITS for list of people who contributed to this > + * project. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > + * MA 02111-1307 USA > + */ > + > +#include <common.h> > +#include <power/pmic.h> > +#include <power/power_chrg.h> > +#include <power/max8997_muic.h> > +#include <i2c.h> > + > +int power_muic_init(unsigned int bus) You add a different function to initialize it, but this is a version opf pmic_init(). Why cannot we consider this one as all other pmics, avoiding to introduce new and undocumented functions ? If I do not misunderstand, you use the different functions to select the pmic. In other words, calling power_update_battery() in the trats code selects automatically the fulel-gauge, because this code implements this function. But this is against the goal of your patches, that is to add multi instances of the pmics (and maybe a second instance of fuel_gauge..). I think this should be solved moving this function to the general API and calling it via pointer. For example, something like: p = pmic_get("MAX_8997); p->read(...); p = pmic_get("MAX17042_FG"); p->power_update_battery(..); What do you think ? Best regards, Stefano Babic
Hi Stefano, > On 14/09/2012 17:40, Lukasz Majewski wrote: > > Support for MUIC (Micro USB Integrated Circuit) built into the > > MAX8997 power management device. > > > > The MUIC device will work with redesigned PMIC framework. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Cc: Stefano Babic <sbabic@denx.de> > > --- > > Hi Lukasz, > > > drivers/misc/Makefile | 1 + > > drivers/misc/muic_max8997.c | 80 > > ++++++++++++++++++++++++++++++++++++++++++ > > include/power/max8997_muic.h | 61 > > ++++++++++++++++++++++++++++++++ include/power/power_chrg.h | > > 1 + 4 files changed, 143 insertions(+), 0 deletions(-) create mode > > 100644 drivers/misc/muic_max8997.c create mode 100644 > > include/power/max8997_muic.h > > > > I see now with this patch, but we have a mix between drivers/misc and > drivers/power. I think this was done because the power directory was > introduced later, but really all pmics under drivers/misc should be > moved into drivers/power. Yes, I agree about that. PMIC (or in the future POWER) related code shall be moved to drivers/power/{subdir_per_PMIC_device} Please also keep in mind, that I plan to rename the "pmic" command/framework to "power" to better reflect its purpose. > > Maybe belong this one also to drivers/power ? > > You added several "add-ons" for MAX8997. Maybe it is clearer if > everything goes into a separate directory, such as power/max8997/ Very good idea. I've already proposed a little "cleanup" with putting all power related includes to ./include/power/ directory. > > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > index 271463c..f08a800 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -41,6 +41,7 @@ COBJS-$(CONFIG_PMIC_I2C) += pmic_i2c.o > > COBJS-$(CONFIG_PMIC_SPI) += pmic_spi.o > > COBJS-$(CONFIG_PMIC_MAX8998) += pmic_max8998.o > > COBJS-$(CONFIG_PMIC_MAX8997) += pmic_max8997.o > > +COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o > > > > COBJS := $(COBJS-y) > > SRCS := $(COBJS:.o=.c) > > diff --git a/drivers/misc/muic_max8997.c > > b/drivers/misc/muic_max8997.c new file mode 100644 > > index 0000000..d332c09 > > --- /dev/null > > +++ b/drivers/misc/muic_max8997.c > > @@ -0,0 +1,80 @@ > > +/* > > + * Copyright (C) 2012 Samsung Electronics > > + * Lukasz Majewski <l.majewski@samsung.com> > > + * > > + * See file CREDITS for list of people who contributed to this > > + * project. > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation; either version 2 of > > + * the License, or (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public > > License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > > + * MA 02111-1307 USA > > + */ > > + > > +#include <common.h> > > +#include <power/pmic.h> > > +#include <power/power_chrg.h> > > +#include <power/max8997_muic.h> > > +#include <i2c.h> > > + > > +int power_muic_init(unsigned int bus) > > You add a different function to initialize it, but this is a version > opf pmic_init(). Why cannot we consider this one as all other pmics, > avoiding to introduce new and undocumented functions ? > > If I do not misunderstand, you use the different functions to select > the pmic. In other words, calling power_update_battery() in the trats > code selects automatically the fulel-gauge, because this code > implements this function. But this is against the goal of your > patches, that is to add multi instances of the pmics (and maybe a > second instance of fuel_gauge..). > > I think this should be solved moving this function to the general API > and calling it via pointer. For example, something like: > > p = pmic_get("MAX_8997); > p->read(...); > > p = pmic_get("MAX17042_FG"); > p->power_update_battery(..); > > What do you think ? I will think about that and give you an answer. > > Best regards, > Stefano Babic > > Regards, Lukasz
Hi Stefano, > On 14/09/2012 17:40, Lukasz Majewski wrote: > > Support for MUIC (Micro USB Integrated Circuit) built into the > > MAX8997 power management device. > > > > The MUIC device will work with redesigned PMIC framework. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Cc: Stefano Babic <sbabic@denx.de> > > --- > > Hi Lukasz, > > > drivers/misc/Makefile | 1 + > > drivers/misc/muic_max8997.c | 80 > > ++++++++++++++++++++++++++++++++++++++++++ > > include/power/max8997_muic.h | 61 > > ++++++++++++++++++++++++++++++++ include/power/power_chrg.h | > > 1 + 4 files changed, 143 insertions(+), 0 deletions(-) create mode > > 100644 drivers/misc/muic_max8997.c create mode 100644 > > include/power/max8997_muic.h > > > > I see now with this patch, but we have a mix between drivers/misc and > drivers/power. I think this was done because the power directory was > introduced later, but really all pmics under drivers/misc should be > moved into drivers/power. > > Maybe belong this one also to drivers/power ? > > You added several "add-ons" for MAX8997. Maybe it is clearer if > everything goes into a separate directory, such as power/max8997/ > > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > index 271463c..f08a800 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -41,6 +41,7 @@ COBJS-$(CONFIG_PMIC_I2C) += pmic_i2c.o > > COBJS-$(CONFIG_PMIC_SPI) += pmic_spi.o > > COBJS-$(CONFIG_PMIC_MAX8998) += pmic_max8998.o > > COBJS-$(CONFIG_PMIC_MAX8997) += pmic_max8997.o > > +COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o > > > > COBJS := $(COBJS-y) > > SRCS := $(COBJS:.o=.c) > > diff --git a/drivers/misc/muic_max8997.c > > b/drivers/misc/muic_max8997.c new file mode 100644 > > index 0000000..d332c09 > > --- /dev/null > > +++ b/drivers/misc/muic_max8997.c > > @@ -0,0 +1,80 @@ > > +/* > > + * Copyright (C) 2012 Samsung Electronics > > + * Lukasz Majewski <l.majewski@samsung.com> > > + * > > + * See file CREDITS for list of people who contributed to this > > + * project. > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation; either version 2 of > > + * the License, or (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public > > License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > > + * MA 02111-1307 USA > > + */ > > + > > +#include <common.h> > > +#include <power/pmic.h> > > +#include <power/power_chrg.h> > > +#include <power/max8997_muic.h> > > +#include <i2c.h> > > + > > +int power_muic_init(unsigned int bus) > > You add a different function to initialize it, but this is a version > opf pmic_init(). Why cannot we consider this one as all other pmics, > avoiding to introduce new and undocumented functions ? > > If I do not misunderstand, you use the different functions to select > the pmic. In other words, calling power_update_battery() in the trats > code selects automatically the fulel-gauge, because this code > implements this function. But this is against the goal of your > patches, that is to add multi instances of the pmics (and maybe a > second instance of fuel_gauge..). > > I think this should be solved moving this function to the general API > and calling it via pointer. For example, something like: > > p = pmic_get("MAX_8997); > p->read(...); > > p = pmic_get("MAX17042_FG"); > p->power_update_battery(..); > > What do you think ? I think that struct pmic shall be extended by another field: struct pmic { struct power_ops *p_ops; }; struct bat_ops { int (*check_battery) (struct pmic *p); int (*update_battery) (struct pmic *p); int (*init_battery) (struct pmic *p); [other battery related callbacks] }; Each instance of struct bat_ops is tied with power related device. I will post v2 of the framework very soon.
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 271463c..f08a800 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -41,6 +41,7 @@ COBJS-$(CONFIG_PMIC_I2C) += pmic_i2c.o COBJS-$(CONFIG_PMIC_SPI) += pmic_spi.o COBJS-$(CONFIG_PMIC_MAX8998) += pmic_max8998.o COBJS-$(CONFIG_PMIC_MAX8997) += pmic_max8997.o +COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/misc/muic_max8997.c b/drivers/misc/muic_max8997.c new file mode 100644 index 0000000..d332c09 --- /dev/null +++ b/drivers/misc/muic_max8997.c @@ -0,0 +1,80 @@ +/* + * Copyright (C) 2012 Samsung Electronics + * Lukasz Majewski <l.majewski@samsung.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <power/pmic.h> +#include <power/power_chrg.h> +#include <power/max8997_muic.h> +#include <i2c.h> + +int power_muic_init(unsigned int bus) +{ + struct pmic *p = pmic_alloc(); + static const char name[] = "MAX8997_MUIC"; + + puts("Board Micro USB Interface Controller init\n"); + + p->name = name; + p->interface = PMIC_I2C; + p->number_of_regs = MUIC_NUM_OF_REGS; + p->hw.i2c.addr = MAX8997_MUIC_I2C_ADDR; + p->hw.i2c.tx_num = 1; + p->bus = bus; + + return 0; +} + +int power_chrg_get_type(void) +{ + unsigned int val; + unsigned char charge_type, charger; + struct pmic *p = pmic_get("MAX8997_MUIC"); + + if (pmic_probe(p)) + return CHARGER_NO; + + pmic_reg_read(p, MAX8997_MUIC_STATUS2, &val); + charge_type = val & MAX8997_MUIC_CHG_MASK; + + switch (charge_type) { + case MAX8997_MUIC_CHG_NO: + charger = CHARGER_NO; + break; + case MAX8997_MUIC_CHG_USB: + case MAX8997_MUIC_CHG_USB_D: + charger = CHARGER_USB; + break; + case MAX8997_MUIC_CHG_TA: + case MAX8997_MUIC_CHG_TA_1A: + charger = CHARGER_TA; + break; + case MAX8997_MUIC_CHG_TA_500: + charger = CHARGER_TA_500; + break; + default: + charger = CHARGER_UNKNOWN; + break; + } + + return charger; +} diff --git a/include/power/max8997_muic.h b/include/power/max8997_muic.h new file mode 100644 index 0000000..0149c12 --- /dev/null +++ b/include/power/max8997_muic.h @@ -0,0 +1,61 @@ +/* + * Copyright (C) 2012 Samsung Electronics + * Lukasz Majewski <l.majewski@samsung.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef __MAX8997_MUIC_H_ +#define __MAX8997_MUIC_H_ + +#include <power/power_chrg.h> + +/* MAX8997_MUIC_STATUS2 */ +#define MAX8997_MUIC_CHG_NO 0x00 +#define MAX8997_MUIC_CHG_USB 0x01 +#define MAX8997_MUIC_CHG_USB_D 0x02 +#define MAX8997_MUIC_CHG_TA 0x03 +#define MAX8997_MUIC_CHG_TA_500 0x04 +#define MAX8997_MUIC_CHG_TA_1A 0x05 +#define MAX8997_MUIC_CHG_MASK 0x07 + +/* MAX 8997 MUIC registers */ +enum { + MAX8997_MUIC_ID = 0x00, + MAX8997_MUIC_INT1 = 0x01, + MAX8997_MUIC_INT2 = 0x02, + MAX8997_MUIC_INT3 = 0x03, + MAX8997_MUIC_STATUS1 = 0x04, + MAX8997_MUIC_STATUS2 = 0x05, + MAX8997_MUIC_STATUS3 = 0x06, + MAX8997_MUIC_INTMASK1 = 0x07, + MAX8997_MUIC_INTMASK2 = 0x08, + MAX8997_MUIC_INTMASK3 = 0x09, + MAX8997_MUIC_CDETCTRL = 0x0A, + MAX8997_MUIC_CONTROL1 = 0x0C, + MAX8997_MUIC_CONTROL2 = 0x0D, + MAX8997_MUIC_CONTROL3 = 0x0E, + + MUIC_NUM_OF_REGS = 0x0F, +}; + +#define MAX8997_MUIC_I2C_ADDR (0x4A >> 1) + +int power_muic_init(unsigned int bus); +#endif /* __MAX8997_MUIC_H_ */ diff --git a/include/power/power_chrg.h b/include/power/power_chrg.h index fb3c3a6..ecbad46 100644 --- a/include/power/power_chrg.h +++ b/include/power/power_chrg.h @@ -50,4 +50,5 @@ struct battery { unsigned int state; }; +int power_chrg_get_type(void); #endif /* __POWER_CHARGER_H_ */