[U-Boot] [PATCH v2 09/21] pmic: Extend struct pmic to support battery and charger related operations

[Lukasz Majewski]

Hi Stefano,

> Am 05/10/2012 10:16, schrieb Lukasz Majewski:
> > Now it is possible to provide specific function per PMIC/power
> > device instance.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> > ---
> > Changes for v2:
> > - New at patch v2
> > ---
> 
> Hi Lucasz,
> 
> >  include/power/battery.h |   38
> > ++++++++++++++++++++++++++++++++++++++ include/power/pmic.h    |
> > 23 ++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1
> > deletions(-) create mode 100644 include/power/battery.h
> > 
> > diff --git a/include/power/battery.h b/include/power/battery.h
> > new file mode 100644
> > index 0000000..e2fec68
> > --- /dev/null
> > +++ b/include/power/battery.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + *  Copyright (C) 2012 Samsung Electronics
> > + *  Lukasz Majewski <l.majewski at 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 __POWER_BATTERY_H_
> > +#define __POWER_BATTERY_H_
> > +
> > +struct battery {
> > +	unsigned int version;
> > +	unsigned int state_of_chrg;
> > +	unsigned int time_to_empty;
> > +	unsigned int capacity;
> > +	unsigned int voltage_uV;
> > +
> > +	unsigned int state;
> > +};
> > +
> > +int power_bat_init(unsigned char bus);
> > +#endif /* __POWER_BATTERY_H_ */
> > diff --git a/include/power/pmic.h b/include/power/pmic.h
> > index 3583342..5ec7bae 100644
> > --- a/include/power/pmic.h
> > +++ b/include/power/pmic.h
> > @@ -27,8 +27,9 @@
> >  #include <common.h>
> >  #include <linux/list.h>
> >  #include <i2c.h>
> > +#include <power/power_chrg.h>
> >  
> > -enum { PMIC_I2C, PMIC_SPI, };
> > +enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
> >  enum { I2C_PMIC, I2C_NUM, };
> >  enum { PMIC_READ, PMIC_WRITE, };
> >  enum { PMIC_BYTE_ORDER_REVERSED, PMIC_BYTE_ORDER_NORMAL, };
> > @@ -48,6 +49,21 @@ struct p_spi {
> >  	u32 (*prepare_tx)(u32 reg, u32 *val, u32 write);
> >  };
> >  
> > +struct pmic;
> > +struct power_battery {
> > +	struct battery *bat;
> > +
> > +	int (*fg_battery_check) (struct pmic *p, struct pmic *bat);
> > +	int (*fg_battery_update) (struct pmic *p, struct pmic
> > *bat); +
> > +	int (*chrg_type) (struct pmic *p);
> > +	int (*chrg_bat_present) (struct pmic *p);
> > +	int (*chrg_state) (struct pmic *p, int state, int current);
> > +
> > +	/* Keep info about power devices involved with battery
> > operation */
> > +	struct pmic *chrg, *fg, *muic;
> > +};
> > +
> >  struct pmic {
> >  	const char *name;
> >  	unsigned char bus;
> > @@ -59,6 +75,11 @@ struct pmic {
> >  		struct p_spi spi;
> >  	} hw;
> >  
> > +	struct power_battery *pwr_bat;
> > +	int (*battery_init) (struct pmic *p, struct pmic *bat);
> 
> If we add entry points to the pmic structure, I do not expect to pass
> the pointer of the structure itself, or I could call the function with
> another pointer:
> 
> 	p->battery_init(p1, bat);
> 
> So p should be hide, stored during the initialization of PMIC itself
> 
> > +	int (*battery_charge) (struct pmic *bat);
> > +	void (*low_power_mode) (void);
> > +
> >  	struct list_head list;
> >  };
> >  
> 
> I have some concerns regarding this structure. It seems to me very
> coupled to the PMIC you are using.
> 
> Reading this and the next patches it seems to me that is due because
> your PMIC includes other three PMICs. However, IMHO the interface
> presented before this patch is very clear.
> 
> I have expected something as:
> 
> 	P = get_pmic("CHRG");
> 	p->chrg_state(current);

I use explicit name to get access to e.g. charger (instead of "CHRG"
I use "MAX8997_PMIC"). We can define the "CHRG"
at ./include/configs/{board.h} file as #define CHRG MAX8997_PMIC. This
is not a problem.

> 
> And if the problem is that this PMIC really descend from another one,
> we could add a "struct pmic *parent" in the pmic itself. You already
> introduced lists, and we can work with pointer instead of hard-code
> the subpmics as in :
> 	struct pmic *chrg, *fg, *muic;

I've considered using of "parents" there. But for the sake of simpler
code (or rather simpler logical view), I wanted to use the "flat"
structure.

The "flat" structure is OK with PMIC, MUIC, Fuel gauge (since those are
in the same "level".

The problem is with battery, since it hasn't got an physical interface
(I2C) and uses other PMIC devices to retain its state (charging state)

> 
> In this case, if we have a pmic that logically can be seen with a
> different number of pmics, we can still support.
> You introduce PMIC_NONE to tell that this pmic does not have an own
> data transfer and rely on another one. But again, this could be
> solved using
> 
> 	if (p->parent)
> 		p->i2c....
> 
> or something like this to use the transfer method of the "main" PMIC.

I will think how redesign the proposed code to use "parent" child
structure.

(I hope that the patch series won't grow much more then :-) )


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group