diff mbox

[V4,3/4] power_supply: Enable battery-charger for 88pm860x

Message ID 20120823034815.GB16674@lizard (mailing list archive)
State New, archived
Headers show

Commit Message

Anton Vorontsov Aug. 23, 2012, 3:48 a.m. UTC
On Fri, Jul 27, 2012 at 04:27:16PM +0800, Jett.Zhou wrote:
> There are charger and battery measurement feature for 88pm860x PMIC.
> 
> For charger, it can support pre-charge with small current when battery is
> nearly exausted and then changed into fast-charge with CC&CV mode.
> 
> For battery monitor, it can support battery measurement such as
> vbat,vsys,vchg and ibat etc,it can aslo accumulating the Coulomb value
> charged or discharged from battery based on Conlomb Counter, we use it
> to estimate battery capacity.
> 
> Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
> ---

A very nice driver, looks neat!

It slightly touches MFD parts, so Samuel, Mark, can I get your Acks
to pass the driver via battery tree? (For convenience I didn't snip
the MFD parts, they are at the very end of this email.)


Jett,

I also noticed these warnings:

  CHECK   drivers/power/88pm860x_battery.c
drivers/power/88pm860x_battery.c:128:5: warning: symbol 'array_soc' was not declared. Should it be static?
  CHECK   drivers/power/88pm860x_charger.c
drivers/power/88pm860x_charger.c:640:3: warning: symbol 'pm860x_irq_descs' was not declared. Should it be static?
  CHECK   drivers/mfd/88pm860x-core.c
drivers/mfd/88pm860x-core.c:803:53: warning: incorrect type in assignment (different base types)
drivers/mfd/88pm860x-core.c:803:53:    expected struct charger_regulator *charger_regulators
drivers/mfd/88pm860x-core.c:803:53:    got struct regulator_bulk_data static [toplevel] *

They are minor, except for the last one. You seemed to use
'regulator_bulk_data' struct (just as charger manager documentation
wrongly tells you, yup), but in real it should have been
'struct charger_regulator'. The only reason that it worked is
because both 'supply' and 'regulator_name' struct members are the
first in these structs. :-)

So, I had to apply these small fixes on top of you patch, just FYI.

--->8---->8--->8


--->8---->8--->8


And the MFD parts for which I'd need some Acks:

> diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
> index d09918c..229cb29 100644
> --- a/drivers/mfd/88pm860x-core.c
> +++ b/drivers/mfd/88pm860x-core.c
> @@ -18,6 +18,7 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/88pm860x.h>
>  #include <linux/regulator/machine.h>
> +#include <linux/power/charger-manager.h>
>  
>  #define INT_STATUS_NUM			3
>  
> @@ -84,7 +85,8 @@ static struct resource battery_resources[] __devinitdata = {
>  static struct resource charger_resources[] __devinitdata = {
>  	{PM8607_IRQ_CHG,  PM8607_IRQ_CHG,  "charger detect",  IORESOURCE_IRQ,},
>  	{PM8607_IRQ_CHG_DONE,  PM8607_IRQ_CHG_DONE,  "charging done",       IORESOURCE_IRQ,},
> -	{PM8607_IRQ_CHG_FAULT, PM8607_IRQ_CHG_FAULT, "charging timeout",    IORESOURCE_IRQ,},
> +	{PM8607_IRQ_CHG_FAIL,  PM8607_IRQ_CHG_FAIL,  "charging timeout",    IORESOURCE_IRQ,},
> +	{PM8607_IRQ_CHG_FAULT, PM8607_IRQ_CHG_FAULT, "charging fault",	    IORESOURCE_IRQ,},
>  	{PM8607_IRQ_GPADC1,    PM8607_IRQ_GPADC1,    "battery temperature", IORESOURCE_IRQ,},
>  	{PM8607_IRQ_VBAT, PM8607_IRQ_VBAT, "battery voltage", IORESOURCE_IRQ,},
>  	{PM8607_IRQ_VCHG, PM8607_IRQ_VCHG, "vchg voltage",    IORESOURCE_IRQ,},
> @@ -155,10 +157,15 @@ static struct regulator_init_data preg_init_data = {
>  	.consumer_supplies	= &preg_supply[0],
>  };
>  
> +static struct regulator_bulk_data chg_desc_regulator_data[] = {
> +	{ .supply = "preg", },
> +};
> +
>  static struct mfd_cell power_devs[] = {
>  	{"88pm860x-battery", -1,},
>  	{"88pm860x-charger", -1,},
>  	{"88pm860x-preg",    -1,},
> +	{"charger-manager", -1,},
>  };
>  
>  static struct mfd_cell rtc_devs[] = {
> @@ -791,6 +798,19 @@ static void __devinit device_power_init(struct pm860x_chip *chip,
>  			      &preg_resources[0], chip->irq_base);
>  	if (ret < 0)
>  		dev_err(chip->dev, "Failed to add preg subdev\n");
> +
> +	if (pdata->chg_desc) {
> +		pdata->chg_desc->charger_regulators =
> +			&chg_desc_regulator_data[0];
> +		pdata->chg_desc->num_charger_regulators	=
> +			ARRAY_SIZE(chg_desc_regulator_data),
> +		power_devs[3].platform_data = pdata->chg_desc;
> +		power_devs[3].pdata_size = sizeof(*pdata->chg_desc);
> +		ret = mfd_add_devices(chip->dev, 0, &power_devs[3], 1,
> +				      NULL, chip->irq_base);
> +		if (ret < 0)
> +			dev_err(chip->dev, "Failed to add chg-manager subdev\n");
> +	}
>  }
>  
>  static void __devinit device_onkey_init(struct pm860x_chip *chip,

> diff --git a/include/linux/mfd/88pm860x.h b/include/linux/mfd/88pm860x.h
> index 7b24943..b7c5a3c 100644
> --- a/include/linux/mfd/88pm860x.h
> +++ b/include/linux/mfd/88pm860x.h
> @@ -55,6 +55,21 @@ enum {
>  #define PM8606_DCM_BOOST		(0x00)
>  #define PM8606_PWM			(0x01)
>  
> +#define PM8607_MISC2			(0x42)
> +
> +/* Power Up Log Register */
> +#define PM8607_POWER_UP_LOG		(0x3F)
> +
> +/* Charger Control Registers */
> +#define PM8607_CCNT			(0x47)
> +#define PM8607_CHG_CTRL1		(0x48)
> +#define PM8607_CHG_CTRL2		(0x49)
> +#define PM8607_CHG_CTRL3		(0x4A)
> +#define PM8607_CHG_CTRL4		(0x4B)
> +#define PM8607_CHG_CTRL5		(0x4C)
> +#define PM8607_CHG_CTRL6		(0x4D)
> +#define PM8607_CHG_CTRL7		(0x4E)
> +
>  /* Backlight Registers */
>  #define PM8606_WLED1A			(0x02)
>  #define PM8606_WLED1B			(0x03)
> @@ -205,6 +220,71 @@ enum {
>  #define PM8607_PD_PREBIAS		(0x56)	/* prebias time */
>  #define PM8607_GPADC_MISC1		(0x57)
>  
> +/* bit definitions of  MEAS_EN1*/
> +#define PM8607_MEAS_EN1_VBAT		(1 << 0)
> +#define PM8607_MEAS_EN1_VCHG		(1 << 1)
> +#define PM8607_MEAS_EN1_VSYS		(1 << 2)
> +#define PM8607_MEAS_EN1_TINT		(1 << 3)
> +#define PM8607_MEAS_EN1_RFTMP		(1 << 4)
> +#define PM8607_MEAS_EN1_TBAT		(1 << 5)
> +#define PM8607_MEAS_EN1_GPADC2		(1 << 6)
> +#define PM8607_MEAS_EN1_GPADC3		(1 << 7)
> +
> +/* Battery Monitor Registers */
> +#define PM8607_GP_BIAS2			(0x5A)
> +#define PM8607_VBAT_LOWTH		(0x5B)
> +#define PM8607_VCHG_LOWTH		(0x5C)
> +#define PM8607_VSYS_LOWTH		(0x5D)
> +#define PM8607_TINT_LOWTH		(0x5E)
> +#define PM8607_GPADC0_LOWTH		(0x5F)
> +#define PM8607_GPADC1_LOWTH		(0x60)
> +#define PM8607_GPADC2_LOWTH		(0x61)
> +#define PM8607_GPADC3_LOWTH		(0x62)
> +#define PM8607_VBAT_HIGHTH		(0x63)
> +#define PM8607_VCHG_HIGHTH		(0x64)
> +#define PM8607_VSYS_HIGHTH		(0x65)
> +#define PM8607_TINT_HIGHTH		(0x66)
> +#define PM8607_GPADC0_HIGHTH		(0x67)
> +#define PM8607_GPADC1_HIGHTH		(0x68)
> +#define PM8607_GPADC2_HIGHTH		(0x69)
> +#define PM8607_GPADC3_HIGHTH		(0x6A)
> +#define PM8607_IBAT_MEAS1		(0x6B)
> +#define PM8607_IBAT_MEAS2		(0x6C)
> +#define PM8607_VBAT_MEAS1		(0x6D)
> +#define PM8607_VBAT_MEAS2		(0x6E)
> +#define PM8607_VCHG_MEAS1		(0x6F)
> +#define PM8607_VCHG_MEAS2		(0x70)
> +#define PM8607_VSYS_MEAS1		(0x71)
> +#define PM8607_VSYS_MEAS2		(0x72)
> +#define PM8607_TINT_MEAS1		(0x73)
> +#define PM8607_TINT_MEAS2		(0x74)
> +#define PM8607_GPADC0_MEAS1		(0x75)
> +#define PM8607_GPADC0_MEAS2		(0x76)
> +#define PM8607_GPADC1_MEAS1		(0x77)
> +#define PM8607_GPADC1_MEAS2		(0x78)
> +#define PM8607_GPADC2_MEAS1		(0x79)
> +#define PM8607_GPADC2_MEAS2		(0x7A)
> +#define PM8607_GPADC3_MEAS1		(0x7B)
> +#define PM8607_GPADC3_MEAS2		(0x7C)
> +#define PM8607_CCNT_MEAS1		(0x95)
> +#define PM8607_CCNT_MEAS2		(0x96)
> +#define PM8607_VBAT_AVG			(0x97)
> +#define PM8607_VCHG_AVG			(0x98)
> +#define PM8607_VSYS_AVG			(0x99)
> +#define PM8607_VBAT_MIN			(0x9A)
> +#define PM8607_VCHG_MIN			(0x9B)
> +#define PM8607_VSYS_MIN			(0x9C)
> +#define PM8607_VBAT_MAX			(0x9D)
> +#define PM8607_VCHG_MAX			(0x9E)
> +#define PM8607_VSYS_MAX			(0x9F)
> +
> +#define PM8607_GPADC_MISC2		(0x59)
> +#define PM8607_GPADC0_GP_BIAS_A0	(1 << 0)
> +#define PM8607_GPADC1_GP_BIAS_A1	(1 << 1)
> +#define PM8607_GPADC2_GP_BIAS_A2	(1 << 2)
> +#define PM8607_GPADC3_GP_BIAS_A3	(1 << 3)
> +#define PM8607_GPADC2_GP_BIAS_OUT2	(1 << 6)
> +
>  /* RTC Control Registers */
>  #define PM8607_RTC1			(0xA0)
>  #define PM8607_RTC_COUNTER1		(0xA1)
> @@ -370,7 +450,8 @@ struct pm860x_touch_pdata {
>  };
>  
>  struct pm860x_power_pdata {
> -	unsigned	fast_charge;	/* charge current */
> +	int		max_capacity;
> +	int		resistor;
>  };
>  
>  struct pm860x_platform_data {
> @@ -379,6 +460,7 @@ struct pm860x_platform_data {
>  	struct pm860x_rtc_pdata		*rtc;
>  	struct pm860x_touch_pdata	*touch;
>  	struct pm860x_power_pdata	*power;
> +	struct charger_desc		*chg_desc;
>  	struct regulator_init_data	*regulator;
>  
>  	unsigned short	companion_addr;	/* I2C address of companion chip */
> -- 
> 1.7.0.4

Comments

Anton Vorontsov Aug. 23, 2012, 4:43 a.m. UTC | #1
On Wed, Aug 22, 2012 at 08:48:15PM -0700, Anton Vorontsov wrote:
[....]
> drivers/mfd/88pm860x-core.c:803:53: warning: incorrect type in assignment (different base types)
> drivers/mfd/88pm860x-core.c:803:53:    expected struct charger_regulator *charger_regulators
> drivers/mfd/88pm860x-core.c:803:53:    got struct regulator_bulk_data static [toplevel] *
> 
> They are minor, except for the last one. You seemed to use
> 'regulator_bulk_data' struct (just as charger manager documentation
> wrongly tells you, yup), but in real it should have been
> 'struct charger_regulator'. The only reason that it worked is
> because both 'supply' and 'regulator_name' struct members are the
> first in these structs. :-)

Oh, actually, I see that this is a result of extcon rework, so not
your fault at all.

Anton.
jett zhou Aug. 23, 2012, 9:06 a.m. UTC | #2
Hi Anton
    Thanks for helping me to amend these patch.
    I got many good suggestions and learn a lot in this process:)
    BTW, you mean you will help to amend that several minor warning with
battery tree?
    If there is something wrong, I can correct it.

Thanks



2012/8/23 Anton Vorontsov <anton.vorontsov@linaro.org>

> On Wed, Aug 22, 2012 at 08:48:15PM -0700, Anton Vorontsov wrote:
> [....]
> > drivers/mfd/88pm860x-core.c:803:53: warning: incorrect type in
> assignment (different base types)
> > drivers/mfd/88pm860x-core.c:803:53:    expected struct charger_regulator
> *charger_regulators
> > drivers/mfd/88pm860x-core.c:803:53:    got struct regulator_bulk_data
> static [toplevel] *
> >
> > They are minor, except for the last one. You seemed to use
> > 'regulator_bulk_data' struct (just as charger manager documentation
> > wrongly tells you, yup), but in real it should have been
> > 'struct charger_regulator'. The only reason that it worked is
> > because both 'supply' and 'regulator_name' struct members are the
> > first in these structs. :-)
>
> Oh, actually, I see that this is a result of extcon rework, so not
> your fault at all.
>
> Anton.
>
Anton Vorontsov Aug. 23, 2012, 12:22 p.m. UTC | #3
On Thu, Aug 23, 2012 at 05:06:22PM +0800, jett zhou wrote:
> Thanks for helping me to amend these patch.  I got many good
> suggestions and learn a lot in this process:) BTW, you mean you will
> help to amend that several minor warning with battery tree?

Yup, I have these small fixups locally in my tree already, so if/once
Samuel gives me an Ack for the mfd part, I'll merge it into battery git
tree.

Thanks,

Anton.
jett zhou Aug. 23, 2012, 12:26 p.m. UTC | #4
Got it. Thanks

2012/8/23 Anton Vorontsov <anton.vorontsov@linaro.org>

> On Thu, Aug 23, 2012 at 05:06:22PM +0800, jett zhou wrote:
> > Thanks for helping me to amend these patch.  I got many good
> > suggestions and learn a lot in this process:) BTW, you mean you will
> > help to amend that several minor warning with battery tree?
>
> Yup, I have these small fixups locally in my tree already, so if/once
> Samuel gives me an Ack for the mfd part, I'll merge it into battery git
> tree.
>
> Thanks,
>
> Anton.
>
Haojian Zhuang Sept. 13, 2012, 7:48 a.m. UTC | #5
On Thu, Aug 23, 2012 at 8:22 PM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> On Thu, Aug 23, 2012 at 05:06:22PM +0800, jett zhou wrote:
>> Thanks for helping me to amend these patch.  I got many good
>> suggestions and learn a lot in this process:) BTW, you mean you will
>> help to amend that several minor warning with battery tree?
>
> Yup, I have these small fixups locally in my tree already, so if/once
> Samuel gives me an Ack for the mfd part, I'll merge it into battery git
> tree.
>
> Thanks,
>
> Anton.

Hi all,

Any updates?

Regards
Haojian
Anton Vorontsov Sept. 20, 2012, 10:35 p.m. UTC | #6
On Thu, Sep 13, 2012 at 03:48:11PM +0800, Haojian Zhuang wrote:
> On Thu, Aug 23, 2012 at 8:22 PM, Anton Vorontsov
> <anton.vorontsov@linaro.org> wrote:
> > On Thu, Aug 23, 2012 at 05:06:22PM +0800, jett zhou wrote:
> >> Thanks for helping me to amend these patch.  I got many good
> >> suggestions and learn a lot in this process:) BTW, you mean you will
> >> help to amend that several minor warning with battery tree?
> >
> > Yup, I have these small fixups locally in my tree already, so if/once
> > Samuel gives me an Ack for the mfd part, I'll merge it into battery git
> > tree.
> 
> Any updates?

OK, I guess I can just go ahead and apply it, the MFD changes are very
trivial anyway. So, this is now applied (along with some fixups that
I was talking about in my previous reply).

Thank you!

Anton.
Haojian Zhuang Sept. 21, 2012, 1:01 a.m. UTC | #7
On Fri, Sep 21, 2012 at 6:35 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> On Thu, Sep 13, 2012 at 03:48:11PM +0800, Haojian Zhuang wrote:
>> On Thu, Aug 23, 2012 at 8:22 PM, Anton Vorontsov
>> <anton.vorontsov@linaro.org> wrote:
>> > On Thu, Aug 23, 2012 at 05:06:22PM +0800, jett zhou wrote:
>> >> Thanks for helping me to amend these patch.  I got many good
>> >> suggestions and learn a lot in this process:) BTW, you mean you will
>> >> help to amend that several minor warning with battery tree?
>> >
>> > Yup, I have these small fixups locally in my tree already, so if/once
>> > Samuel gives me an Ack for the mfd part, I'll merge it into battery git
>> > tree.
>>
>> Any updates?
>
> OK, I guess I can just go ahead and apply it, the MFD changes are very
> trivial anyway. So, this is now applied (along with some fixups that
> I was talking about in my previous reply).
>
> Thank you!
>
> Anton.

Thanks

Haojian
Samuel Ortiz Sept. 21, 2012, 10:14 p.m. UTC | #8
Hi Anton,

On Thu, Sep 20, 2012 at 03:35:05PM -0700, Anton Vorontsov wrote:
> On Thu, Sep 13, 2012 at 03:48:11PM +0800, Haojian Zhuang wrote:
> > On Thu, Aug 23, 2012 at 8:22 PM, Anton Vorontsov
> > <anton.vorontsov@linaro.org> wrote:
> > > On Thu, Aug 23, 2012 at 05:06:22PM +0800, jett zhou wrote:
> > >> Thanks for helping me to amend these patch.  I got many good
> > >> suggestions and learn a lot in this process:) BTW, you mean you will
> > >> help to amend that several minor warning with battery tree?
> > >
> > > Yup, I have these small fixups locally in my tree already, so if/once
> > > Samuel gives me an Ack for the mfd part, I'll merge it into battery git
> > > tree.
> > 
> > Any updates?
> 
> OK, I guess I can just go ahead and apply it, the MFD changes are very
> trivial anyway. So, this is now applied (along with some fixups that
> I was talking about in my previous reply).
Applying this patch as is will not build as the mfd_add_devices() API has
changed. Either you rebase your patchset on top of Linus tree or I take this
patch and fix the mfd_add_devices() call. Please let me know how you want to
proceed.

Cheers,
Samuel.
Anton Vorontsov Sept. 21, 2012, 10:39 p.m. UTC | #9
On Sat, Sep 22, 2012 at 12:14:22AM +0200, Samuel Ortiz wrote:
> > > >> Thanks for helping me to amend these patch.  I got many good
> > > >> suggestions and learn a lot in this process:) BTW, you mean you will
> > > >> help to amend that several minor warning with battery tree?
> > > >
> > > > Yup, I have these small fixups locally in my tree already, so if/once
> > > > Samuel gives me an Ack for the mfd part, I'll merge it into battery git
> > > > tree.
> > > 
> > > Any updates?
> > 
> > OK, I guess I can just go ahead and apply it, the MFD changes are very
> > trivial anyway. So, this is now applied (along with some fixups that
> > I was talking about in my previous reply).
> Applying this patch as is will not build as the mfd_add_devices() API has
> changed. Either you rebase your patchset on top of Linus tree or I take this
> patch and fix the mfd_add_devices() call. Please let me know how you want to
> proceed.

It's not the first time this happens, and git let us fix it in a pretty
elegant way: what I'm usually doing is just pulling Linus' tree back into
my tree (but not rebasing), and in the merge commit I fixup the issue.
That way things are still bisectable, and everything should work before
and after mfd_add_devices() change.

So, it's all OK, I'll just merge Linus' -rc6 back into my tree
(and Stephen already has a fix in linux-next).

Thanks!
Anton.
diff mbox

Patch

diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
index 229cb29..76b5b7d 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -157,8 +157,8 @@  static struct regulator_init_data preg_init_data = {
 	.consumer_supplies	= &preg_supply[0],
 };
 
-static struct regulator_bulk_data chg_desc_regulator_data[] = {
-	{ .supply = "preg", },
+static struct charger_regulator chg_desc_regulator_data[] = {
+	{ .regulator_name = "preg", },
 };
 
 static struct mfd_cell power_devs[] = {
diff --git a/drivers/power/88pm860x_battery.c b/drivers/power/88pm860x_battery.c
index e84e98d..1c19828 100644
--- a/drivers/power/88pm860x_battery.c
+++ b/drivers/power/88pm860x_battery.c
@@ -125,7 +125,7 @@  struct ccnt {
  * State of Charge.
  * The first number is mAh(=3.6C), and the second number is percent point.
  */
-int array_soc[][2] = {
+static int array_soc[][2] = {
 	{4170, 100}, {4154, 99}, {4136, 98}, {4122, 97}, {4107, 96},
 	{4102, 95}, {4088, 94}, {4081, 93}, {4070, 92}, {4060, 91},
 	{4053, 90}, {4044, 89}, {4035, 88}, {4028, 87}, {4019, 86},
diff --git a/drivers/power/88pm860x_charger.c b/drivers/power/88pm860x_charger.c
index 52b59d8..4eeef9b 100644
--- a/drivers/power/88pm860x_charger.c
+++ b/drivers/power/88pm860x_charger.c
@@ -634,7 +634,7 @@  static int pm860x_init_charger(struct pm860x_charger_info *info)
 	return 0;
 }
 
-struct pm860x_irq_desc {
+static struct pm860x_irq_desc {
 	const char *name;
 	irqreturn_t (*handler)(int irq, void *data);
 } pm860x_irq_descs[] = {