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

[Stefano Babic]

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);

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


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================