[PATCH v4 2/5] power: supply: add support for max77759 fuel gauge
Peter Griffin
peter.griffin at linaro.org
Fri Jun 6 04:40:23 PDT 2025
Hi Thomas,
Thanks for your patch and working to get fuel gauge functional on
Pixel 6! I've tried to do quite an in-depth review comparing with the
downstream driver.
On Fri, 23 May 2025 at 13:52, Thomas Antoine via B4 Relay
<devnull+t.antoine.uclouvain.be at kernel.org> wrote:
>
> From: Thomas Antoine <t.antoine at uclouvain.be>
>
> The interface of the Maxim MAX77759 fuel gauge has a lot of common with the
> Maxim MAX1720x. A major difference is the lack of non-volatile memory
> slave address. No slave is available at address 0xb of the i2c bus, which
> is coherent with the following driver from google: line 5836 disables
> non-volatile memory for m5 gauge.
>
> Link: https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c
>
> Other differences include the lack of V_BATT register to read the battery
> level. The voltage must instead be read from V_CELL, the lowest voltage of
> all cells. The mask to identify the chip is different. The computation of
> the charge must also be changed to take into account TASKPERIOD, which
> can add a factor 2 to the result.
>
> Add support for the MAX77759 by taking into account all of those
> differences based on chip type.
>
> Do not advertise temp probes using the non-volatile memory as those are
> not available.
>
> The regmap was proposed by André Draszik in
>
> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/
I think it would be worth noting in the commit message this is basic
initial support for the M5 gauge in MAX77759, and things like loading
& saving the m5 model aren't implemented yet.
That's important as some values such as the REPSOC register value used
for POWER_SUPPLY_PROP_CAPACITY show the result after all processing
including ModelGauge mixing etc, so these values won't be as accurate
as downstream.
>
> Signed-off-by: Thomas Antoine <t.antoine at uclouvain.be>
> ---
> drivers/power/supply/max1720x_battery.c | 265 ++++++++++++++++++++++++++++----
> 1 file changed, 238 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
> --- a/drivers/power/supply/max1720x_battery.c
> +++ b/drivers/power/supply/max1720x_battery.c
> @@ -37,6 +37,7 @@
> #define MAX172XX_REPCAP 0x05 /* Average capacity */
> #define MAX172XX_REPSOC 0x06 /* Percentage of charge */
> #define MAX172XX_TEMP 0x08 /* Temperature */
> +#define MAX172XX_VCELL 0x09 /* Lowest cell voltage */
> #define MAX172XX_CURRENT 0x0A /* Actual current */
> #define MAX172XX_AVG_CURRENT 0x0B /* Average current */
> #define MAX172XX_FULL_CAP 0x10 /* Calculated full capacity */
> @@ -54,15 +55,28 @@
> #define MAX172XX_BATT 0xDA /* Battery voltage */
> #define MAX172XX_ATAVCAP 0xDF
>
> +#define MAX77759_DEV_NAME_TYPE_MASK GENMASK(15, 9)
> +#define MAX77759_DEV_NAME_TYPE_MAX77759 0x31
> +#define MAX77759_TASKPERIOD 0x3C
> +#define MAX77759_TASKPERIOD_175MS 0x1680
> +#define MAX77759_TASKPERIOD_351MS 0x2D00
> +
> static const char *const max1720x_manufacturer = "Maxim Integrated";
> static const char *const max17201_model = "MAX17201";
> static const char *const max17205_model = "MAX17205";
> +static const char *const max77759_model = "MAX77759";
> +
> +enum chip_id {
> + MAX1720X_ID,
> + MAX77759_ID,
> +};
>
> struct max1720x_device_info {
> struct regmap *regmap;
> struct regmap *regmap_nv;
> struct i2c_client *ancillary;
> int rsense;
> + enum chip_id id;
> };
>
> /*
> @@ -271,6 +285,80 @@ static const enum power_supply_property max1720x_battery_props[] = {
> POWER_SUPPLY_PROP_MANUFACTURER,
> };
>
> +/*
> + * Registers 0x80 up to 0xaf which contain the model for the fuel gauge
> + * algorithm (stored in nvmem for the max1720x) are locked. They can
> + * be unlocked by writing 0x59 to 0x62 and 0xc4 to 0x63. They should be
> + * enabled in the regmap if the driver is extended to manage the model.
> + */
> +static const struct regmap_range max77759_registers[] = {
> + regmap_reg_range(0x00, 0x4f),
> + regmap_reg_range(0xb0, 0xbf),
> + regmap_reg_range(0xd0, 0xd0),
> + regmap_reg_range(0xdc, 0xdf),
> + regmap_reg_range(0xfb, 0xfb),
> + regmap_reg_range(0xff, 0xff),
> +};
> +
> +static const struct regmap_range max77759_ro_registers[] = {
> + regmap_reg_range(0x3d, 0x3d),
> + regmap_reg_range(0xfb, 0xfb),
> + regmap_reg_range(0xff, 0xff),
> +};
> +
> +static const struct regmap_access_table max77759_write_table = {
> + .no_ranges = max77759_ro_registers,
> + .n_no_ranges = ARRAY_SIZE(max77759_ro_registers),
> +};
> +
> +static const struct regmap_access_table max77759_rd_table = {
> + .yes_ranges = max77759_registers,
> + .n_yes_ranges = ARRAY_SIZE(max77759_registers),
> +};
> +
> +static const struct regmap_config max77759_regmap_cfg = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .max_register = 0xff,
> + .wr_table = &max77759_write_table,
> + .rd_table = &max77759_rd_table,
> + .val_format_endian = REGMAP_ENDIAN_LITTLE,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> +static const enum power_supply_property max77759_battery_props[] = {
> + POWER_SUPPLY_PROP_PRESENT,
I checked the register values match downstream - this looks correct
> + POWER_SUPPLY_PROP_CAPACITY,
I checked the register offset matchs downstream. The value reported
varies a bit versus downstream. As mentioned above that is likely due
to the REPSOC register reporting after mixing with the m5 model which
is not loaded currently. Also the application specific values and cell
characterization information used by the model isn't configured
currently (see link below in _TEMP property below for the initial fuel
gauge params used by downstream.
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
I checked the register offset matchs downstream. Values reported look sensible.
> + POWER_SUPPLY_PROP_CHARGE_FULL,
Downstream has a slightly different implementation than upstream for
this property. See here
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c#2244
> + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
I checked the register offset value is correct. However this is
reporting 3000000 and downstream reports 4524000. I checked and it's
just converting the register reset value of DESIGNCAP which is 0xbb8.
This is listed as a "application specific" value, so it maybe we just
need to write the correct initial value to DESIGNCAP (see TEMP section
below)
> + POWER_SUPPLY_PROP_CHARGE_AVG,
This property isn't reported downstream. The value is changing and not
just the reset value. I noticed REPSOC is an output of the ModelGauge
algorithm so it is likely not to be completely accurate.
> + POWER_SUPPLY_PROP_TEMP,
I checked the register offset value is correct. However the
temperature is always being reported as the register reset value of
220. This is for obvious reasons quite an important one to report
correctly.
I started debugging this a bit, and it is caused by an incorrectly
configured CONFIG (0x1D) register. In particular the TEX[8] bit is 1
on reset in this register which means temperature measurements are
written from the host AP. When this bit is set to 0, measurements on
the AIN pin are converted to a temperature value and stored in the
Temperature register (I then saw values of 360 and the value
changing).
See here for the bits in that CONFIG register
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5_reg.h#403
In downstream all these initial register settings are taken from DT
here https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android14-gs-pixel-6.1/arch/arm64/boot/dts/google/gs101-fake-battery-data.dtsi#27
For temperature when TEX=0, TGAIN, TOFF and TCURVE registers should
also be configured to adjust the temperature measurement.
I think it would likely be worth initialising all the fuel gauge
registers referenced in maxim,fg-params as that includes some of the
application specific values for DESIGNCAP, also some of the cell
characterization information, and hopefully we will get more accurate
values from the fuel gauge generally.
> + POWER_SUPPLY_PROP_CURRENT_NOW,
I checked the register offset matches downstream. Values reported look
reasonable.
> + POWER_SUPPLY_PROP_CURRENT_AVG,
I checked the register offset matches downstream. Values reported look
reasonable.
> + POWER_SUPPLY_PROP_MODEL_NAME,
This property isn't reported downstream.
> + POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +
> +struct chip_data {
> + bool has_nvmem;
> + const struct regmap_config *regmap_cfg;
> + enum chip_id id;
> +};
> +
> +static const struct chip_data max1720x_data = {
> + .has_nvmem = true,
> + .regmap_cfg = &max1720x_regmap_cfg,
> + .id = MAX1720X_ID,
> +};
> +
> +static const struct chip_data max77759_data = {
> + .has_nvmem = false,
> + .regmap_cfg = &max77759_regmap_cfg,
> + .id = MAX77759_ID,
> +};
> +
> /* Convert regs value to power_supply units */
>
> static int max172xx_time_to_ps(unsigned int reg)
> @@ -288,12 +376,41 @@ static int max172xx_voltage_to_ps(unsigned int reg)
> return reg * 1250; /* in uV */
> }
>
> +static int max172xx_cell_voltage_to_ps(unsigned int reg)
> +{
> + return reg * 625 / 8; /* in uV */
> +}
> +
> static int max172xx_capacity_to_ps(unsigned int reg,
> struct max1720x_device_info *info)
> {
> return reg * (500000 / info->rsense); /* in uAh */
> }
>
> +static int max77759_capacity_lsb(struct max1720x_device_info *info,
> + unsigned int *lsb)
> +{
> + unsigned int reg_task_period;
> + int ret;
> +
> + ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, ®_task_period);
> + if (ret < 0)
> + return ret;
> +
> + switch (reg_task_period) {
> + case MAX77759_TASKPERIOD_175MS:
> + *lsb = 1;
> + break;
> + case MAX77759_TASKPERIOD_351MS:
> + *lsb = 2;
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Current and temperature is signed values, so unsigned regs
> * value must be converted to signed type
> @@ -390,16 +507,36 @@ static int max1720x_battery_get_property(struct power_supply *psy,
> val->intval = max172xx_percent_to_ps(reg_val);
> break;
> case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> - ret = regmap_read(info->regmap, MAX172XX_BATT, ®_val);
> - val->intval = max172xx_voltage_to_ps(reg_val);
> + if (info->id == MAX1720X_ID) {
> + ret = regmap_read(info->regmap, MAX172XX_BATT, ®_val);
> + val->intval = max172xx_voltage_to_ps(reg_val);
I think MAX1720X using MAX172XX_BATT register is likely a bug as the
downstream driver uses MAX172XX_VCELL for that variant see here
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x.h#304
Having said that, if we do need to cope with differing register
offsets for the different fuel gauge variants it would be nicer to
abstract them in a way similar to the downstream driver. See here
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c#1235.
I think that would be more scalable in supporting multiple variants in
one driver. Otherwise we will have an explosion of if(id==blah) else
if (id==blah) in the driver.
kind regards,
Peter
More information about the linux-arm-kernel
mailing list