From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Andrew F. Davis" Subject: Re: [PATCH v8 7/9] power: bq27xxx_battery: Add power_supply_battery_info support Date: Mon, 27 Feb 2017 15:47:46 -0600 Message-ID: <880a1d69-dc06-b921-e0be-e3ebff75938d@ti.com> References: <20170227071117.18934-1-liam@networkimprov.net> <20170227071117.18934-8-liam@networkimprov.net> <164968b9-e809-d7fc-176b-cbeded20d43d@ti.com> <4a6622a3-5fea-ac37-c906-4073ef655983@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Return-path: Received: from lelnx193.ext.ti.com ([198.47.27.77]:36264 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751644AbdB0Vsc (ORCPT ); Mon, 27 Feb 2017 16:48:32 -0500 In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Liam Breck Cc: Sebastian Reichel , linux-pm@vger.kernel.org, Matt Ranostay , Liam Breck On 02/27/2017 03:35 PM, Liam Breck wrote: > On Mon, Feb 27, 2017 at 1:21 PM, Andrew F. Davis wrote: >> On 02/27/2017 02:05 PM, Liam Breck wrote: >>> On Mon, Feb 27, 2017 at 10:06 AM, Andrew F. Davis wrote: >>>> On 02/27/2017 01:11 AM, Liam Breck wrote: >>>>> From: Liam Breck >>>>> >>>>> Previously there was no way to configure chip registers in the event that the >>>>> defaults didn't match the battery in question. >>>>> >>>>> BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs, >>>>> and writes battery data to chip RAM or non-volatile memory. >>>>> >>>>> Signed-off-by: Matt Ranostay >>>>> Signed-off-by: Liam Breck >>>>> --- >>>>> drivers/power/supply/bq27xxx_battery.c | 458 ++++++++++++++++++++++++++++++++- >>>>> 1 file changed, 456 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c >>>>> index 7475a5f..41d4ce7 100644 >>>>> --- a/drivers/power/supply/bq27xxx_battery.c >>>>> +++ b/drivers/power/supply/bq27xxx_battery.c >>>>> @@ -51,7 +51,7 @@ >>>>> >>>>> #include >>>>> >>>>> -#define DRIVER_VERSION "1.2.0" >>>>> +#define DRIVER_VERSION "1.3.0" >>>>> >>>>> #define BQ27XXX_MANUFACTURER "Texas Instruments" >>>>> >>>>> @@ -59,6 +59,7 @@ >>>>> #define BQ27XXX_FLAG_DSC BIT(0) >>>>> #define BQ27XXX_FLAG_SOCF BIT(1) /* State-of-Charge threshold final */ >>>>> #define BQ27XXX_FLAG_SOC1 BIT(2) /* State-of-Charge threshold 1 */ >>>>> +#define BQ27XXX_FLAG_CFGUP BIT(5) >>>>> #define BQ27XXX_FLAG_FC BIT(9) >>>>> #define BQ27XXX_FLAG_OTD BIT(14) >>>>> #define BQ27XXX_FLAG_OTC BIT(15) >>>>> @@ -72,6 +73,11 @@ >>>>> #define BQ27000_FLAG_FC BIT(5) >>>>> #define BQ27000_FLAG_CHGS BIT(7) /* Charge state flag */ >>>>> >>>>> +/* control register params */ >>>>> +#define BQ27XXX_SEALED 0x20 >>>>> +#define BQ27XXX_SET_CFGUPDATE 0x13 >>>>> +#define BQ27XXX_SOFT_RESET 0x42 >>>>> + >>>>> #define BQ27XXX_RS (20) /* Resistor sense mOhm */ >>>>> #define BQ27XXX_POWER_CONSTANT (29200) /* 29.2 µV^2 * 1000 */ >>>>> #define BQ27XXX_CURRENT_CONSTANT (3570) /* 3.57 µV * 1000 */ >>>>> @@ -102,6 +108,11 @@ enum bq27xxx_reg_index { >>>>> BQ27XXX_REG_SOC, /* State-of-Charge */ >>>>> BQ27XXX_REG_DCAP, /* Design Capacity */ >>>>> BQ27XXX_REG_AP, /* Average Power */ >>>>> + BQ27XXX_DM_CTRL, /* BlockDataControl() */ >>>> >>>> /* Block Data Control */ >>>> >>>>> + BQ27XXX_DM_CLASS, /* DataClass() */ >>>> >>>> /* Data Class */ >>> >>> I used the terms from the docs, so a search will find meaningful results. >>> >>>>> + BQ27XXX_DM_BLOCK, /* DataBlock() */ >>>>> + BQ27XXX_DM_DATA, /* BlockData() */ >>>>> + BQ27XXX_DM_CKSUM, /* BlockDataChecksum() */ >>>>> BQ27XXX_REG_MAX, /* sentinel */ >>>>> }; >>>>> >>>>> @@ -125,6 +136,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x0b, >>>>> [BQ27XXX_REG_DCAP] = 0x76, >>>>> [BQ27XXX_REG_AP] = 0x24, >>>>> + [BQ27XXX_DM_CTRL] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_CLASS] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_DATA] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR, >>>>> }, >>>>> [BQ27010] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -144,6 +160,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x0b, >>>>> [BQ27XXX_REG_DCAP] = 0x76, >>>>> [BQ27XXX_REG_AP] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_CTRL] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_CLASS] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_DATA] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR, >>>>> }, >>>>> [BQ27500] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -163,6 +184,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x2c, >>>>> [BQ27XXX_REG_DCAP] = 0x3c, >>>>> [BQ27XXX_REG_AP] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_CTRL] = 0x61, >>>>> + [BQ27XXX_DM_CLASS] = 0x3e, >>>>> + [BQ27XXX_DM_BLOCK] = 0x3f, >>>>> + [BQ27XXX_DM_DATA] = 0x40, >>>>> + [BQ27XXX_DM_CKSUM] = 0x60, >>>>> }, >>>>> [BQ27510] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -182,6 +208,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x20, >>>>> [BQ27XXX_REG_DCAP] = 0x2e, >>>>> [BQ27XXX_REG_AP] = INVALID_REG_ADDR, >>>>> + [BQ27XXX_DM_CTRL] = 0x61, >>>>> + [BQ27XXX_DM_CLASS] = 0x3e, >>>>> + [BQ27XXX_DM_BLOCK] = 0x3f, >>>>> + [BQ27XXX_DM_DATA] = 0x40, >>>>> + [BQ27XXX_DM_CKSUM] = 0x60, >>>>> }, >>>>> [BQ27530] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -201,6 +232,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x2c, >>>>> [BQ27XXX_REG_DCAP] = INVALID_REG_ADDR, >>>>> [BQ27XXX_REG_AP] = 0x24, >>>>> + [BQ27XXX_DM_CTRL] = 0x61, >>>>> + [BQ27XXX_DM_CLASS] = 0x3e, >>>>> + [BQ27XXX_DM_BLOCK] = 0x3f, >>>>> + [BQ27XXX_DM_DATA] = 0x40, >>>>> + [BQ27XXX_DM_CKSUM] = 0x60, >>>>> }, >>>>> [BQ27541] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -220,6 +256,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x2c, >>>>> [BQ27XXX_REG_DCAP] = 0x3c, >>>>> [BQ27XXX_REG_AP] = 0x24, >>>>> + [BQ27XXX_DM_CTRL] = 0x61, >>>>> + [BQ27XXX_DM_CLASS] = 0x3e, >>>>> + [BQ27XXX_DM_BLOCK] = 0x3f, >>>>> + [BQ27XXX_DM_DATA] = 0x40, >>>>> + [BQ27XXX_DM_CKSUM] = 0x60, >>>>> }, >>>>> [BQ27545] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -239,6 +280,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x2c, >>>>> [BQ27XXX_REG_DCAP] = INVALID_REG_ADDR, >>>>> [BQ27XXX_REG_AP] = 0x24, >>>>> + [BQ27XXX_DM_CTRL] = 0x61, >>>>> + [BQ27XXX_DM_CLASS] = 0x3e, >>>>> + [BQ27XXX_DM_BLOCK] = 0x3f, >>>>> + [BQ27XXX_DM_DATA] = 0x40, >>>>> + [BQ27XXX_DM_CKSUM] = 0x60, >>>>> }, >>>>> [BQ27421] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -258,6 +304,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x1c, >>>>> [BQ27XXX_REG_DCAP] = 0x3c, >>>>> [BQ27XXX_REG_AP] = 0x18, >>>>> + [BQ27XXX_DM_CTRL] = 0x61, >>>>> + [BQ27XXX_DM_CLASS] = 0x3e, >>>>> + [BQ27XXX_DM_BLOCK] = 0x3f, >>>>> + [BQ27XXX_DM_DATA] = 0x40, >>>>> + [BQ27XXX_DM_CKSUM] = 0x60, >>>>> }, >>>>> [BQ27425] = { >>>>> [BQ27XXX_REG_CTRL] = 0x00, >>>>> @@ -277,6 +328,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = { >>>>> [BQ27XXX_REG_SOC] = 0x1c, >>>>> [BQ27XXX_REG_DCAP] = 0x3c, >>>>> [BQ27XXX_REG_AP] = 0x18, >>>>> + [BQ27XXX_DM_CTRL] = 0x61, >>>>> + [BQ27XXX_DM_CLASS] = 0x3e, >>>>> + [BQ27XXX_DM_BLOCK] = 0x3f, >>>>> + [BQ27XXX_DM_DATA] = 0x40, >>>>> + [BQ27XXX_DM_CKSUM] = 0x60, >>>> >>>> That wasn't so painful was it :) >>> >>> It added 11% to the patch :-/ >>> >>>>> }, >>>>> }; >>>>> >>>>> @@ -452,6 +508,81 @@ static struct { >>>>> static DEFINE_MUTEX(bq27xxx_list_lock); >>>>> static LIST_HEAD(bq27xxx_battery_devices); >>>>> >>>>> +#define BQ27XXX_DM_SZ 32 >>>>> + >>>>> +#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500) >>>>> + >>>>> +struct bq27xxx_dm_reg { >>>>> + u8 subclass_id; >>>>> + u8 offset; >>>>> + u8 bytes; >>>>> + u16 min, max; >>>>> +}; >>>>> + >>>>> +struct bq27xxx_dm_buf { >>>>> + u8 class; >>>>> + u8 block; >>>>> + u8 a[BQ27XXX_DM_SZ]; >>>>> + bool full, updt; >>>>> +}; >>>>> + >>>>> +#define BQ27XXX_DM_BUF(di, i) { \ >>>>> + .class = bq27xxx_dm_regs[(di)->chip][i].subclass_id, \ >>>>> + .block = bq27xxx_dm_regs[(di)->chip][i].offset / BQ27XXX_DM_SZ, \ >>>>> +} >>>>> + >>>>> +static inline u16* bq27xxx_dm_buf_ptr(struct bq27xxx_dm_buf *buf, >>>>> + struct bq27xxx_dm_reg *reg) { >>>>> + if (buf->class == reg->subclass_id >>>>> + && buf->block == reg->offset / BQ27XXX_DM_SZ) >>>>> + return (u16*) (buf->a + reg->offset % BQ27XXX_DM_SZ); >>>>> + >>>>> + return NULL; >>>>> +} >>>>> + >>>>> +enum bq27xxx_dm_reg_id { >>>>> + BQ27XXX_DM_DESIGN_CAPACITY = 0, >>>>> + BQ27XXX_DM_DESIGN_ENERGY, >>>>> + BQ27XXX_DM_TERMINATE_VOLTAGE, >>>>> +}; >>>>> + >>>>> +static const char* bq27xxx_dm_reg_name[] = { >>>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity", >>>>> + [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy", >>>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", >>>>> +}; >>>>> + >>>>> +static struct bq27xxx_dm_reg bq27425_dm_regs[] = { >>>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 12, 2, 0, 32767 }, >>>>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 14, 2, 0, 32767 }, >>>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800, 3700 }, >>>>> +}; >>>>> + >>>>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = { /* not tested */ >>>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 10, 2, 0, 8000 }, >>>>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 12, 2, 0, 32767 }, >>>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500, 3700 }, >>>>> +}; >>>>> + >>>>> +//static struct bq27xxx_dm_reg bq27621_dm_regs[] = { /* not tested */ >>>>> +// [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 3, 2, 0, 8000 }, >>>>> +// [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 5, 2, 0, 32767 }, >>>>> +// [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500, 3700 }, >>>>> +//}; >>>> >>>> I don't think this comment style is allowed. >>> >>> This is a to-do item. >>> >>>>> + >>>>> +static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = { >>>>> + [BQ27421] = bq27421_dm_regs, /* and BQ27441 */ >>>>> + [BQ27425] = bq27425_dm_regs, >>>>> +// [BQ27621] = bq27621_dm_regs, >>>>> +}; >>>> >>>> I know it is not tested, but lets not comment it out, I'll do a round of >>>> testing for this part when this series is ready. >>> >>> ID for 621 is not yet defined. We need an efficient way to add IDs. >>> Perhaps via chip-id and group-id. Most current ID refs would be to >>> group-id. I'd like to defer that, so we should only add inputs for the >>> single-chip groups in this patchset: 500, 545, 425. >>> >>> We must NOT set one-time program (OTP) memory, mentioned in 421 & 441 >>> docs. It's not clear whether documented data-memory ops change OTP. >>> Can you ask? The 425 has rewritable NVM. The 621 docs don't say. All >>> the above have cfgupdate mode, which the rest of the family lacks. >>> >> >> This would be a good question for e2e[0]. >> >> [0] http://e2e.ti.com/support/power_management/battery_management/f/180 > > Can I ask you to handle discussions with TI folks? > You can ask, doesn't mean I will :) but really I don't have any special means of getting a hold of the gauge team that you don't have (except maybe walking over to their cubes and bugging them). We try to ask even internal inter-team questions on e2e, you can see questions asked by TI employees to other employees on that site, the idea is that if we have a problem, someday customers may have the same problem, so it's best not no keep these answers buried in some internal email chain. So the most I would do is post the same question to them on e2e, you can avoid me being a middle-man by asking them directly. Thanks, Andrew > Thanks :-) >