On Thu, Feb 23, 2017 at 8:53 AM, Andrew F. Davis [off-list ref] wrote:

On 02/23/2017 10:38 AM, Liam Breck wrote:

quoted

On Thu, Feb 23, 2017 at 8:08 AM, Andrew F. Davis [off-list ref] wrote:

quoted

On 02/23/2017 09:49 AM, Liam Breck wrote:

quoted

On Thu, Feb 23, 2017 at 7:17 AM, Andrew F. Davis [off-list ref] wrote:

quoted

On 02/22/2017 06:54 PM, Liam Breck wrote:

quoted

On Wed, Feb 22, 2017 at 4:30 PM, Andrew F. Davis [off-list ref] wrote:

quoted

On 02/22/2017 03:29 PM, Liam Breck wrote:

quoted

On Wed, Feb 22, 2017 at 12:35 PM, Andrew F. Davis [off-list ref] wrote:

quoted

On 02/21/2017 03:30 PM, Liam Breck wrote:

quoted

From: Liam Breck <redacted>

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 <redacted>
Signed-off-by: Liam Breck <redacted>
---
 drivers/power/supply/bq27xxx_battery.c | 399 ++++++++++++++++++++++++++++++++-
 1 file changed, 397 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 7475a5f..8085d4a 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c

@@ -51,7 +51,7 @@

 #include <linux/power/bq27xxx_battery.h>

-#define DRIVER_VERSION               "1.2.0"
+#define DRIVER_VERSION               "1.3.0"

 #define BQ27XXX_MANUFACTURER "Texas Instruments"

@@ -452,6 +452,99 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);

+/* writable registers */
+#define BQ27XXX_CONTROL                      0x00
+#define BQ27XXX_DATA_CLASS           0x3E
+#define BQ27XXX_DATA_BLOCK           0x3F
+#define BQ27XXX_BLOCK_DATA           0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM  0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL   0x61
+

These all belong in the bq27xxx_regs array, you can add new register
types to the bq27xxx_reg_index enum.

They're the same on all chips. Let's dup this code 9+ times when we have cause.

They are not the same on all chips, some chips don't have these
registers at all. Plus, BQ27XXX_CONTROL is the same for all chips, and
we already have it in all the chips bq27xxx_regs.

Datasheets? Do those chips even support this feature set? Bqtool
doesn't appear to support any other memory update scheme.

Many chips don't, which is why we don't want these registers being
global defines, you can have the chips without these registers as
INVALID_REG_ADDR

Chips that don't support memory update won't have a bq27xxx_dm_regs
list; we look for that at the start of this code path and do nothing
if it's null for di->chip. All chips that do support memory update use
the same command registers, so there's no reason to dup those.

Someday they might use a different value, we don't lose anything by
putting these device registers in the table with the other device
registers. I would rather duplicate some lines if it makes the code more
uniform and easier to follow, again this is not a contest, we write code
for ourselves, let the compiler organize/optimize things.

I prefer the principle that every line you write must have a purpose
in the current codebase, vs a future one. But if you are adamant, I
capitulate.

Normally I would agree, but for listing registers on a device we would
have a register map that looks like swiss cheese.

Was that adamant? Keep the readable registers in tables; make the
writable ones universal since they're all the same.

quoted

quoted

quoted

Also you can delete REG_CTRL as it's not used by the existing code  :-)

True, but as before, I just like having the complete register set in a
nice simple table, as opposed to having to add and change things all
over if/when we may need that register.

Being a static table, I wonder if a sufficiently smart compiler could
see that it is not used and optimize the value out for us...

No :-p All the compiler can do for unused data is issue a warning. And
it can't do so for array elements.

I said "sufficiently smart compiler", it could find that no code within
the tables context accesses it outside of the set indexes, it could then
even remove the tables altogether and replace accesses with immediate
values. Not practical, but not impossible, I think.. :)

I'm not so interested in hypothetical compilers and chips. There are
reasons why compilers don't optimize away data.

quoted

You really should define those tables in a function which memcpy's the
right one onto di.regs.

What would that do for us? We still need to store the tables, it just
removes one level of pointers.

It would store 1 table for the life of the kernel, vs 9+