Re: [PATCH v4 1/3] power: supply: Add support for the Qualcomm Battery Monitoring System

[Craig Tatlor]

Thanks for review, replies inline.

On Thu, Apr 26, 2018 at 01:34:00PM +0200, Linus Walleij wrote:
> On Sat, Apr 7, 2018 at 7:57 PM, Craig Tatlor  wrote:
> 
> Hi Craig! Thanks for your patch!
> 
> > This patch adds a driver for the BMS (Battery Monitoring System)
> > block of the PM8941 PMIC, it uses a lookup table defined in the
> > device tree to generate a capacity from the BMS supplied OCV, it
> > then ammends the coulomb counter to that to increase the accuracy
> > of the estimated capacity.
> >
> > Signed-off-by: Craig Tatlor 
> 
> Just some minor remarks.
> 
> NB: I see that you are writing from a private email address
> so if you're working as a hobbyist on your precious sparetime
> I have lower expectation on how much work you will put into
> this, so see it more as suggestions than demands.
Yeah, I am a just hobbyist :)
> 
> My overall feedback is that for algorithmic charger what
> we need is infrastructure. What is currently piling up in
> drivers/power/supply scares me a bit in it's lack of
> framework and code reuse.
> 
> It also scares me because this is vital technology dealing
> with physical devices and as such really need to have
> modularized reusable reviewed code with several users
> and deployments.
> 
> Code reuse would include:
> 
> - Mathematical helpers such as interpolation of
>   values from absolute values or tables
>   Suggestions below!
> - State machines and transitions
> - CC/CV alorithms (using the above)
> - Many other things
> 
> Not that *I* can make the situation much better, I'm just
> sharing my fears,
> 
> > +static s64 sign_extend_s36(uint64_t raw)
> > +{
> > +   raw = raw & CC_36_BIT_MASK;
> > +
> > +   return (raw >> 35) == 0LL ?
> > +   raw : (SIGN_EXTEND_36_TO_64_MASK | raw);
> > +}
> 
> #include 
> 
> Use sign_extend32()
Right.
> 
> > +static unsigned int interpolate(int y0, int x0, int y1, int x1, int x)
> > +{
> > +   if (y0 == y1 || x == x0)
> > +   return y0;
> > +   if (x1 == x0 || x == x1)
> > +   return y1;
> > +
> > +   return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
> > +}
> > +
> > +static unsigned int between(int left, int right, int val)
> > +{
> > +   if (left <= val && val <= right)
> > +   return 1;
> > +
> > +   return 0;
> > +}
> 
> How are these things not library functions?
> 
> Every cell of my brain says this code should be reusable.
> 
> Can you put this in ?
> 
> I bet a million to one that the video people will sooner or later
> need linear interpolation and there are more users in the kernel
> than drivers/power/, certainly drivers/iio as well.
> 
> If noone else says anything I vote to put at least the linear
> interpolation into  with a bonus if you
> move some of the current users in drivers/power over
> while you're at it.
I was pretty surprised there wasn't a library function for it
aswell, i will add it there. 
> 
> > +static unsigned int interpolate_capacity(int temp, u16 ocv,
> > +   struct bms_ocv_lut ocv_lut)
> > +{
> > +   unsigned int pcj_minus_one = 0, pcj = 0;
> > +   int i, j;
> > +
> > +   for (j = 0; j < TEMPERATURE_COLS; j++)
> > +   if (temp <= ocv_lut.temp_legend[j])
> > +   break;
> > +
> > +   if (ocv >= ocv_lut.lut[0][j])
> > +   return ocv_lut.capacity_legend[0];
> > +
> > +   if (ocv <= ocv_lut.lut[ocv_lut.rows - 1][j - 1])
> > +   return ocv_lut.capacity_legend[ocv_lut.rows - 1];
> > +
> > +   for (i = 0; i < ocv_lut.rows - 1; i++) {
> > +   if (pcj == 0 && between(ocv_lut.lut[i][j],
> > +   ocv_lut.lut[i+1][j], ocv))
> > +   pcj = interpolate(ocv_lut.capacity_legend[i],
> > + ocv_lut.lut[i][j],
> > + ocv_lut.capacity_legend[i + 1],
> > + ocv_lut.lut[i+1][j],
> > + ocv);
> > +
> > +   if (pcj_minus_one == 0 && between(ocv_lut.lut[i][j-1],
> > + ocv_lut.lut[i+1][j-1], 
> > ocv))
> > +   pcj_minus_one = 
> > interpolate(ocv_lut.capacity_legend[i],
> > +   ocv_lut.lut[i][j-1],
> > +   
> > ocv_lut.capacity_legend[i + 1],
> > +   ocv_lut.lut[i+1][j-1],
> > +   ocv);
> > +
> > +   if (pcj && pcj_minus_one)
> > +   return interpolate(pcj_minus_one,
> > +  ocv_lut.temp_legend[j-1],
> > +  pcj,
> > +  ocv_lut.temp_legend[j],
> > +  

Re: [PATCH v4 1/3] power: supply: Add support for the Qualcomm Battery Monitoring System

[Craig Tatlor]

Thanks for review, replies inline.

On Thu, Apr 26, 2018 at 01:34:00PM +0200, Linus Walleij wrote:
> On Sat, Apr 7, 2018 at 7:57 PM, Craig Tatlor  wrote:
> 
> Hi Craig! Thanks for your patch!
> 
> > This patch adds a driver for the BMS (Battery Monitoring System)
> > block of the PM8941 PMIC, it uses a lookup table defined in the
> > device tree to generate a capacity from the BMS supplied OCV, it
> > then ammends the coulomb counter to that to increase the accuracy
> > of the estimated capacity.
> >
> > Signed-off-by: Craig Tatlor 
> 
> Just some minor remarks.
> 
> NB: I see that you are writing from a private email address
> so if you're working as a hobbyist on your precious sparetime
> I have lower expectation on how much work you will put into
> this, so see it more as suggestions than demands.
Yeah, I am a just hobbyist :)
> 
> My overall feedback is that for algorithmic charger what
> we need is infrastructure. What is currently piling up in
> drivers/power/supply scares me a bit in it's lack of
> framework and code reuse.
> 
> It also scares me because this is vital technology dealing
> with physical devices and as such really need to have
> modularized reusable reviewed code with several users
> and deployments.
> 
> Code reuse would include:
> 
> - Mathematical helpers such as interpolation of
>   values from absolute values or tables
>   Suggestions below!
> - State machines and transitions
> - CC/CV alorithms (using the above)
> - Many other things
> 
> Not that *I* can make the situation much better, I'm just
> sharing my fears,
> 
> > +static s64 sign_extend_s36(uint64_t raw)
> > +{
> > +   raw = raw & CC_36_BIT_MASK;
> > +
> > +   return (raw >> 35) == 0LL ?
> > +   raw : (SIGN_EXTEND_36_TO_64_MASK | raw);
> > +}
> 
> #include 
> 
> Use sign_extend32()
Right.
> 
> > +static unsigned int interpolate(int y0, int x0, int y1, int x1, int x)
> > +{
> > +   if (y0 == y1 || x == x0)
> > +   return y0;
> > +   if (x1 == x0 || x == x1)
> > +   return y1;
> > +
> > +   return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
> > +}
> > +
> > +static unsigned int between(int left, int right, int val)
> > +{
> > +   if (left <= val && val <= right)
> > +   return 1;
> > +
> > +   return 0;
> > +}
> 
> How are these things not library functions?
> 
> Every cell of my brain says this code should be reusable.
> 
> Can you put this in ?
> 
> I bet a million to one that the video people will sooner or later
> need linear interpolation and there are more users in the kernel
> than drivers/power/, certainly drivers/iio as well.
> 
> If noone else says anything I vote to put at least the linear
> interpolation into  with a bonus if you
> move some of the current users in drivers/power over
> while you're at it.
I was pretty surprised there wasn't a library function for it
aswell, i will add it there. 
> 
> > +static unsigned int interpolate_capacity(int temp, u16 ocv,
> > +   struct bms_ocv_lut ocv_lut)
> > +{
> > +   unsigned int pcj_minus_one = 0, pcj = 0;
> > +   int i, j;
> > +
> > +   for (j = 0; j < TEMPERATURE_COLS; j++)
> > +   if (temp <= ocv_lut.temp_legend[j])
> > +   break;
> > +
> > +   if (ocv >= ocv_lut.lut[0][j])
> > +   return ocv_lut.capacity_legend[0];
> > +
> > +   if (ocv <= ocv_lut.lut[ocv_lut.rows - 1][j - 1])
> > +   return ocv_lut.capacity_legend[ocv_lut.rows - 1];
> > +
> > +   for (i = 0; i < ocv_lut.rows - 1; i++) {
> > +   if (pcj == 0 && between(ocv_lut.lut[i][j],
> > +   ocv_lut.lut[i+1][j], ocv))
> > +   pcj = interpolate(ocv_lut.capacity_legend[i],
> > + ocv_lut.lut[i][j],
> > + ocv_lut.capacity_legend[i + 1],
> > + ocv_lut.lut[i+1][j],
> > + ocv);
> > +
> > +   if (pcj_minus_one == 0 && between(ocv_lut.lut[i][j-1],
> > + ocv_lut.lut[i+1][j-1], 
> > ocv))
> > +   pcj_minus_one = 
> > interpolate(ocv_lut.capacity_legend[i],
> > +   ocv_lut.lut[i][j-1],
> > +   
> > ocv_lut.capacity_legend[i + 1],
> > +   ocv_lut.lut[i+1][j-1],
> > +   ocv);
> > +
> > +   if (pcj && pcj_minus_one)
> > +   return interpolate(pcj_minus_one,
> > +  ocv_lut.temp_legend[j-1],
> > +  pcj,
> > +  ocv_lut.temp_legend[j],
> > +  temp);
> > +   }
> 
> 

Re: [PATCH v4 1/3] power: supply: Add support for the Qualcomm Battery Monitoring System

[Linus Walleij]

On Sat, Apr 7, 2018 at 7:57 PM, Craig Tatlor  wrote:

Hi Craig! Thanks for your patch!

> This patch adds a driver for the BMS (Battery Monitoring System)
> block of the PM8941 PMIC, it uses a lookup table defined in the
> device tree to generate a capacity from the BMS supplied OCV, it
> then ammends the coulomb counter to that to increase the accuracy
> of the estimated capacity.
>
> Signed-off-by: Craig Tatlor 

Just some minor remarks.

NB: I see that you are writing from a private email address
so if you're working as a hobbyist on your precious sparetime
I have lower expectation on how much work you will put into
this, so see it more as suggestions than demands.

My overall feedback is that for algorithmic charger what
we need is infrastructure. What is currently piling up in
drivers/power/supply scares me a bit in it's lack of
framework and code reuse.

It also scares me because this is vital technology dealing
with physical devices and as such really need to have
modularized reusable reviewed code with several users
and deployments.

Code reuse would include:

- Mathematical helpers such as interpolation of
  values from absolute values or tables
  Suggestions below!
- State machines and transitions
- CC/CV alorithms (using the above)
- Many other things

Not that *I* can make the situation much better, I'm just
sharing my fears,

> +static s64 sign_extend_s36(uint64_t raw)
> +{
> +   raw = raw & CC_36_BIT_MASK;
> +
> +   return (raw >> 35) == 0LL ?
> +   raw : (SIGN_EXTEND_36_TO_64_MASK | raw);
> +}

#include 

Use sign_extend32()

> +static unsigned int interpolate(int y0, int x0, int y1, int x1, int x)
> +{
> +   if (y0 == y1 || x == x0)
> +   return y0;
> +   if (x1 == x0 || x == x1)
> +   return y1;
> +
> +   return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
> +}
> +
> +static unsigned int between(int left, int right, int val)
> +{
> +   if (left <= val && val <= right)
> +   return 1;
> +
> +   return 0;
> +}

How are these things not library functions?

Every cell of my brain says this code should be reusable.

Can you put this in ?

I bet a million to one that the video people will sooner or later
need linear interpolation and there are more users in the kernel
than drivers/power/, certainly drivers/iio as well.

If noone else says anything I vote to put at least the linear
interpolation into  with a bonus if you
move some of the current users in drivers/power over
while you're at it.

> +static unsigned int interpolate_capacity(int temp, u16 ocv,
> +   struct bms_ocv_lut ocv_lut)
> +{
> +   unsigned int pcj_minus_one = 0, pcj = 0;
> +   int i, j;
> +
> +   for (j = 0; j < TEMPERATURE_COLS; j++)
> +   if (temp <= ocv_lut.temp_legend[j])
> +   break;
> +
> +   if (ocv >= ocv_lut.lut[0][j])
> +   return ocv_lut.capacity_legend[0];
> +
> +   if (ocv <= ocv_lut.lut[ocv_lut.rows - 1][j - 1])
> +   return ocv_lut.capacity_legend[ocv_lut.rows - 1];
> +
> +   for (i = 0; i < ocv_lut.rows - 1; i++) {
> +   if (pcj == 0 && between(ocv_lut.lut[i][j],
> +   ocv_lut.lut[i+1][j], ocv))
> +   pcj = interpolate(ocv_lut.capacity_legend[i],
> + ocv_lut.lut[i][j],
> + ocv_lut.capacity_legend[i + 1],
> + ocv_lut.lut[i+1][j],
> + ocv);
> +
> +   if (pcj_minus_one == 0 && between(ocv_lut.lut[i][j-1],
> + ocv_lut.lut[i+1][j-1], ocv))
> +   pcj_minus_one = 
> interpolate(ocv_lut.capacity_legend[i],
> +   ocv_lut.lut[i][j-1],
> +   ocv_lut.capacity_legend[i 
> + 1],
> +   ocv_lut.lut[i+1][j-1],
> +   ocv);
> +
> +   if (pcj && pcj_minus_one)
> +   return interpolate(pcj_minus_one,
> +  ocv_lut.temp_legend[j-1],
> +  pcj,
> +  ocv_lut.temp_legend[j],
> +  temp);
> +   }

This code really needs some comments to tell what is going on
here. Also I sense that you can break out a smaller function
for interpolation based on table values, such as a function
that would take a standard format of tables, look up where we
are in that table and interpolate from the neighboring values.

People can then later go in and refine the algorithms if they
e.g. want to introduce spline or RMS interpolation instead
and we can get 

Re: [PATCH v4 1/3] power: supply: Add support for the Qualcomm Battery Monitoring System

[Linus Walleij]

On Sat, Apr 7, 2018 at 7:57 PM, Craig Tatlor  wrote:

Hi Craig! Thanks for your patch!

> This patch adds a driver for the BMS (Battery Monitoring System)
> block of the PM8941 PMIC, it uses a lookup table defined in the
> device tree to generate a capacity from the BMS supplied OCV, it
> then ammends the coulomb counter to that to increase the accuracy
> of the estimated capacity.
>
> Signed-off-by: Craig Tatlor 

Just some minor remarks.

NB: I see that you are writing from a private email address
so if you're working as a hobbyist on your precious sparetime
I have lower expectation on how much work you will put into
this, so see it more as suggestions than demands.

My overall feedback is that for algorithmic charger what
we need is infrastructure. What is currently piling up in
drivers/power/supply scares me a bit in it's lack of
framework and code reuse.

It also scares me because this is vital technology dealing
with physical devices and as such really need to have
modularized reusable reviewed code with several users
and deployments.

Code reuse would include:

- Mathematical helpers such as interpolation of
  values from absolute values or tables
  Suggestions below!
- State machines and transitions
- CC/CV alorithms (using the above)
- Many other things

Not that *I* can make the situation much better, I'm just
sharing my fears,

> +static s64 sign_extend_s36(uint64_t raw)
> +{
> +   raw = raw & CC_36_BIT_MASK;
> +
> +   return (raw >> 35) == 0LL ?
> +   raw : (SIGN_EXTEND_36_TO_64_MASK | raw);
> +}

#include 

Use sign_extend32()

> +static unsigned int interpolate(int y0, int x0, int y1, int x1, int x)
> +{
> +   if (y0 == y1 || x == x0)
> +   return y0;
> +   if (x1 == x0 || x == x1)
> +   return y1;
> +
> +   return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
> +}
> +
> +static unsigned int between(int left, int right, int val)
> +{
> +   if (left <= val && val <= right)
> +   return 1;
> +
> +   return 0;
> +}

How are these things not library functions?

Every cell of my brain says this code should be reusable.

Can you put this in ?

I bet a million to one that the video people will sooner or later
need linear interpolation and there are more users in the kernel
than drivers/power/, certainly drivers/iio as well.

If noone else says anything I vote to put at least the linear
interpolation into  with a bonus if you
move some of the current users in drivers/power over
while you're at it.

> +static unsigned int interpolate_capacity(int temp, u16 ocv,
> +   struct bms_ocv_lut ocv_lut)
> +{
> +   unsigned int pcj_minus_one = 0, pcj = 0;
> +   int i, j;
> +
> +   for (j = 0; j < TEMPERATURE_COLS; j++)
> +   if (temp <= ocv_lut.temp_legend[j])
> +   break;
> +
> +   if (ocv >= ocv_lut.lut[0][j])
> +   return ocv_lut.capacity_legend[0];
> +
> +   if (ocv <= ocv_lut.lut[ocv_lut.rows - 1][j - 1])
> +   return ocv_lut.capacity_legend[ocv_lut.rows - 1];
> +
> +   for (i = 0; i < ocv_lut.rows - 1; i++) {
> +   if (pcj == 0 && between(ocv_lut.lut[i][j],
> +   ocv_lut.lut[i+1][j], ocv))
> +   pcj = interpolate(ocv_lut.capacity_legend[i],
> + ocv_lut.lut[i][j],
> + ocv_lut.capacity_legend[i + 1],
> + ocv_lut.lut[i+1][j],
> + ocv);
> +
> +   if (pcj_minus_one == 0 && between(ocv_lut.lut[i][j-1],
> + ocv_lut.lut[i+1][j-1], ocv))
> +   pcj_minus_one = 
> interpolate(ocv_lut.capacity_legend[i],
> +   ocv_lut.lut[i][j-1],
> +   ocv_lut.capacity_legend[i 
> + 1],
> +   ocv_lut.lut[i+1][j-1],
> +   ocv);
> +
> +   if (pcj && pcj_minus_one)
> +   return interpolate(pcj_minus_one,
> +  ocv_lut.temp_legend[j-1],
> +  pcj,
> +  ocv_lut.temp_legend[j],
> +  temp);
> +   }

This code really needs some comments to tell what is going on
here. Also I sense that you can break out a smaller function
for interpolation based on table values, such as a function
that would take a standard format of tables, look up where we
are in that table and interpolate from the neighboring values.

People can then later go in and refine the algorithms if they
e.g. want to introduce spline or RMS interpolation instead
and we can get better interpolation for everybody.

> +static 

[PATCH v4 1/3] power: supply: Add support for the Qualcomm Battery Monitoring System

[Craig Tatlor]

This patch adds a driver for the BMS (Battery Monitoring System)
block of the PM8941 PMIC, it uses a lookup table defined in the
device tree to generate a capacity from the BMS supplied OCV, it
then ammends the coulomb counter to that to increase the accuracy
of the estimated capacity.

Signed-off-by: Craig Tatlor 
---
 drivers/power/supply/Kconfig|   9 +
 drivers/power/supply/Makefile   |   1 +
 drivers/power/supply/qcom_bms.c | 500 
 3 files changed, 510 insertions(+)
 create mode 100644 drivers/power/supply/qcom_bms.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 428b426842f4..6c354c37bc55 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -82,6 +82,15 @@ config BATTERY_ACT8945A
  Say Y here to enable support for power supply provided by
  Active-semi ActivePath ACT8945A charger.
 
+config BATTERY_BMS
+   tristate "Qualcomm Battery Monitoring System driver"
+   depends on MFD_SPMI_PMIC || COMPILE_TEST
+   depends on OF
+   depends on REGMAP_SPMI
+   help
+ Say Y to include support for the Battery Monitoring hardware
+ found in some Qualcomm PM series PMICs.
+
 config BATTERY_CPCAP
tristate "Motorola CPCAP PMIC battery driver"
depends on MFD_CPCAP && IIO
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index e83aa843bcc6..04204174b047 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_BATTERY_88PM860X)+= 88pm860x_battery.o
 obj-$(CONFIG_BATTERY_ACT8945A) += act8945a_charger.o
 obj-$(CONFIG_BATTERY_AXP20X)   += axp20x_battery.o
 obj-$(CONFIG_CHARGER_AXP20X)   += axp20x_ac_power.o
+obj-$(CONFIG_BATTERY_BMS)  += qcom_bms.o
 obj-$(CONFIG_BATTERY_CPCAP)+= cpcap-battery.o
 obj-$(CONFIG_BATTERY_DS2760)   += ds2760_battery.o
 obj-$(CONFIG_BATTERY_DS2780)   += ds2780_battery.o
diff --git a/drivers/power/supply/qcom_bms.c b/drivers/power/supply/qcom_bms.c
new file mode 100644
index ..f31c99c03518
--- /dev/null
+++ b/drivers/power/supply/qcom_bms.c
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL
+
+/*
+ * Qualcomm Battery Monitoring System driver
+ *
+ * Copyright (C) 2018 Craig Tatlor 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define REG_BMS_OCV_FOR_SOC_DATA0  0x90
+#define REG_BMS_SHDW_CC_DATA0  0xA8
+#define REG_BMS_CC_DATA_CTL0x42
+#define REG_BMS_CC_CLEAR_CTL   0x4
+
+#define BMS_HOLD_OREG_DATA BIT(0)
+#define BMS_CLEAR_SHDW_CC  BIT(6)
+
+#define CC_36_BIT_MASK 0xFLL
+#define SIGN_EXTEND_36_TO_64_MASK  (-1LL ^ CC_36_BIT_MASK)
+
+#define BMS_CC_READING_RESOLUTION_N542535
+#define BMS_CC_READING_RESOLUTION_D1
+#define BMS_CC_READING_TICKS   56
+#define BMS_SLEEP_CLK_HZ   32764
+
+#define SECONDS_PER_HOUR   3600
+#define TEMPERATURE_COLS   5
+#define MAX_CAPACITY_ROWS  50
+
+/* lookup table for ocv -> capacity conversion */
+struct bms_ocv_lut {
+   int rows;
+   s8 temp_legend[TEMPERATURE_COLS];
+   u8 capacity_legend[MAX_CAPACITY_ROWS];
+   u16 lut[MAX_CAPACITY_ROWS][TEMPERATURE_COLS];
+};
+
+/* lookup table for battery temperature -> fcc conversion */
+struct bms_fcc_lut {
+   s8 temp_legend[TEMPERATURE_COLS];
+   u16 lut[TEMPERATURE_COLS];
+};
+
+struct bms_device_info {
+   struct device *dev;
+   struct regmap *regmap;
+   struct power_supply *bat;
+   struct power_supply_desc bat_desc;
+   struct bms_ocv_lut ocv_lut;
+   struct bms_fcc_lut fcc_lut;
+   struct iio_channel *adc;
+   spinlock_t bms_output_lock;
+   int base_addr;
+
+   int ocv_thr_irq;
+   int ocv;
+};
+
+static s64 sign_extend_s36(uint64_t raw)
+{
+   raw = raw & CC_36_BIT_MASK;
+
+   return (raw >> 35) == 0LL ?
+   raw : (SIGN_EXTEND_36_TO_64_MASK | raw);
+}
+
+static unsigned int interpolate(int y0, int x0, int y1, int x1, int x)
+{
+   if (y0 == y1 || x == x0)
+   return y0;
+   if (x1 == x0 || x == x1)
+   return y1;
+
+   return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
+}
+
+static unsigned int between(int left, int right, int val)
+{
+   if (left <= val && val <= right)
+   return 1;
+
+   return 0;
+}
+
+static unsigned int interpolate_capacity(int temp, u16 ocv,
+   struct bms_ocv_lut ocv_lut)
+{
+   unsigned int pcj_minus_one = 0, pcj = 0;
+   int i, j;
+
+   for (j = 0; j < TEMPERATURE_COLS; j++)
+   if (temp <= ocv_lut.temp_legend[j])
+   break;
+
+   if (ocv >= ocv_lut.lut[0][j])
+   return ocv_lut.capacity_legend[0];
+
+   if (ocv <= 

[PATCH v4 1/3] power: supply: Add support for the Qualcomm Battery Monitoring System

[Craig Tatlor]

This patch adds a driver for the BMS (Battery Monitoring System)
block of the PM8941 PMIC, it uses a lookup table defined in the
device tree to generate a capacity from the BMS supplied OCV, it
then ammends the coulomb counter to that to increase the accuracy
of the estimated capacity.

Signed-off-by: Craig Tatlor 
---
 drivers/power/supply/Kconfig|   9 +
 drivers/power/supply/Makefile   |   1 +
 drivers/power/supply/qcom_bms.c | 500 
 3 files changed, 510 insertions(+)
 create mode 100644 drivers/power/supply/qcom_bms.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 428b426842f4..6c354c37bc55 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -82,6 +82,15 @@ config BATTERY_ACT8945A
  Say Y here to enable support for power supply provided by
  Active-semi ActivePath ACT8945A charger.
 
+config BATTERY_BMS
+   tristate "Qualcomm Battery Monitoring System driver"
+   depends on MFD_SPMI_PMIC || COMPILE_TEST
+   depends on OF
+   depends on REGMAP_SPMI
+   help
+ Say Y to include support for the Battery Monitoring hardware
+ found in some Qualcomm PM series PMICs.
+
 config BATTERY_CPCAP
tristate "Motorola CPCAP PMIC battery driver"
depends on MFD_CPCAP && IIO
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index e83aa843bcc6..04204174b047 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_BATTERY_88PM860X)+= 88pm860x_battery.o
 obj-$(CONFIG_BATTERY_ACT8945A) += act8945a_charger.o
 obj-$(CONFIG_BATTERY_AXP20X)   += axp20x_battery.o
 obj-$(CONFIG_CHARGER_AXP20X)   += axp20x_ac_power.o
+obj-$(CONFIG_BATTERY_BMS)  += qcom_bms.o
 obj-$(CONFIG_BATTERY_CPCAP)+= cpcap-battery.o
 obj-$(CONFIG_BATTERY_DS2760)   += ds2760_battery.o
 obj-$(CONFIG_BATTERY_DS2780)   += ds2780_battery.o
diff --git a/drivers/power/supply/qcom_bms.c b/drivers/power/supply/qcom_bms.c
new file mode 100644
index ..f31c99c03518
--- /dev/null
+++ b/drivers/power/supply/qcom_bms.c
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL
+
+/*
+ * Qualcomm Battery Monitoring System driver
+ *
+ * Copyright (C) 2018 Craig Tatlor 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define REG_BMS_OCV_FOR_SOC_DATA0  0x90
+#define REG_BMS_SHDW_CC_DATA0  0xA8
+#define REG_BMS_CC_DATA_CTL0x42
+#define REG_BMS_CC_CLEAR_CTL   0x4
+
+#define BMS_HOLD_OREG_DATA BIT(0)
+#define BMS_CLEAR_SHDW_CC  BIT(6)
+
+#define CC_36_BIT_MASK 0xFLL
+#define SIGN_EXTEND_36_TO_64_MASK  (-1LL ^ CC_36_BIT_MASK)
+
+#define BMS_CC_READING_RESOLUTION_N542535
+#define BMS_CC_READING_RESOLUTION_D1
+#define BMS_CC_READING_TICKS   56
+#define BMS_SLEEP_CLK_HZ   32764
+
+#define SECONDS_PER_HOUR   3600
+#define TEMPERATURE_COLS   5
+#define MAX_CAPACITY_ROWS  50
+
+/* lookup table for ocv -> capacity conversion */
+struct bms_ocv_lut {
+   int rows;
+   s8 temp_legend[TEMPERATURE_COLS];
+   u8 capacity_legend[MAX_CAPACITY_ROWS];
+   u16 lut[MAX_CAPACITY_ROWS][TEMPERATURE_COLS];
+};
+
+/* lookup table for battery temperature -> fcc conversion */
+struct bms_fcc_lut {
+   s8 temp_legend[TEMPERATURE_COLS];
+   u16 lut[TEMPERATURE_COLS];
+};
+
+struct bms_device_info {
+   struct device *dev;
+   struct regmap *regmap;
+   struct power_supply *bat;
+   struct power_supply_desc bat_desc;
+   struct bms_ocv_lut ocv_lut;
+   struct bms_fcc_lut fcc_lut;
+   struct iio_channel *adc;
+   spinlock_t bms_output_lock;
+   int base_addr;
+
+   int ocv_thr_irq;
+   int ocv;
+};
+
+static s64 sign_extend_s36(uint64_t raw)
+{
+   raw = raw & CC_36_BIT_MASK;
+
+   return (raw >> 35) == 0LL ?
+   raw : (SIGN_EXTEND_36_TO_64_MASK | raw);
+}
+
+static unsigned int interpolate(int y0, int x0, int y1, int x1, int x)
+{
+   if (y0 == y1 || x == x0)
+   return y0;
+   if (x1 == x0 || x == x1)
+   return y1;
+
+   return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
+}
+
+static unsigned int between(int left, int right, int val)
+{
+   if (left <= val && val <= right)
+   return 1;
+
+   return 0;
+}
+
+static unsigned int interpolate_capacity(int temp, u16 ocv,
+   struct bms_ocv_lut ocv_lut)
+{
+   unsigned int pcj_minus_one = 0, pcj = 0;
+   int i, j;
+
+   for (j = 0; j < TEMPERATURE_COLS; j++)
+   if (temp <= ocv_lut.temp_legend[j])
+   break;
+
+   if (ocv >= ocv_lut.lut[0][j])
+   return ocv_lut.capacity_legend[0];
+
+   if (ocv <= ocv_lut.lut[ocv_lut.rows - 1][j - 1])
+