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

Now it is possible to provide specific function per PMIC/power
device instance.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes for v2:
- New at patch v2
---
 include/power/battery.h |   38 ++++++++++++++++++++++++++++++++++++++
 include/power/pmic.h    |   23 ++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 1 deletions(-)
 create mode 100644 include/power/battery.h

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@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@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@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);

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;

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.

Best regards,
Stefano Babic

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@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@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@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 :-) )

Hi Stefano,

I'd like to present an overview of my idea (pseudo code):

struct battery {
	int (* battery_charge) ()
	struct pmic *fg, *muic, *chrg
}

struct chrg {
	int (*chrg_type) ()
	int (*chrg_bat_present) ()
	int (*chrg_state) ()
}

struct fg {
	int (*fg_bat_check) ()
	int (*fg_bat_update) ()
}

struct muic {

}

Then extend struct pmic:

struct pmic {

struct battery *bat_p;
struct chrg *chrg_p;
struct fg *fg_p;
struct muic *muic_p;

struct pmic *parent;
void (*low_power_mode) ();
}

The struct [battery|chrg|fg|muic] is provided during pmic device
initialization (it is defined as a static in a device's translation unit
- e.g. fg_max17042.c)


To solve the problem with multi instances of the same devices (e.g. two
identical HW devices - MAX17042 are connected to SOC), methods defined
for pmic/power devices accept struct pmic *p of the device instance.

You have suggested, that pmic device shall be a parent. However, I see
it differently (at least from my HW):

			struct pmic *fg, *muic, *chrg
			-----------------
		--------| BAT		|------------
		|	|		|	    |
		|	-----------------	    |
		|		|		    |
	       \|/	       \|/		   \|/
	-----------	-----------------	---------
	|FG       |	|MUIC		|	|CHRG	|
	|         |	|		| 	|	|
	-----------	-----------------	---------
  	struct	pmic *parent = &bat (for FG, MUIC, CHRG)

(PMIC is also connected to the rest via list)


I think that BAT is parent here, since when we want to e.g. charge the
battery we would look for it with:
p_bat = pmic_get("BAT_TRATS");

In my opinion it is natural to call p_bat->battery_charge(p_bat)
to enable charging and in the same time don't be concerned with "helper"
devices (like FG, MUIC, CHRG) - which provide methods to check if
battery is charged properly.

Moreover BAT shall be treated as a PMIC device and thereof has its own
instance of struct pmic. It is desirable to have all power related
devices connected in the list.

The __real__ problem here is how to "connect" functionality provided by
FG, CHRG, MUIC with battery, to have easy access:
p_bat->battery_charge(p_bat).

One option would be to fill: 
struct battery *bat_p;
struct chrg *chrg_p;
struct fg *fg_p;
struct muic *muic_p;

for struct pmic bat.

Then we could access relevant functions just from p_bat.