Re: [PATCH v3 2/5] power: supply: add support for max77759 fuel gauge

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Hi Thomas,

On Mon, Apr 21, 2025 at 08:13:33PM +0200, Thomas Antoine via B4 Relay wrote:
> From: Thomas Antoine <t.antoine@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/
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  drivers/power/supply/max1720x_battery.c | 270 ++++++++++++++++++++++++++++----
>  1 file changed, 237 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index cca5f8b5071fb731f9b60420239ea03d46cb1bf3..969d3a7c2baa7e1d23c5175942d975b277c8554c 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 */
> @@ -50,19 +51,32 @@
>  #define MAX172XX_DEV_NAME_TYPE_MASK	GENMASK(3, 0)
>  #define MAX172XX_DEV_NAME_TYPE_MAX17201	BIT(0)
>  #define MAX172XX_DEV_NAME_TYPE_MAX17205	(BIT(0) | BIT(2))
> +#define MAX77759_DEV_NAME_TYPE_MASK	GENMASK(15, 9)
> +#define MAX77759_DEV_NAME_TYPE_MAX77759	0x31
>  #define MAX172XX_QR_TABLE10		0x22
> +#define MAX77759_TASKPERIOD		0x3C
> +#define MAX77759_TASKPERIOD_175MS	0x1680
> +#define MAX77759_TASKPERIOD_351MS	0x2D00

I think it would be more readable if MAX77759_ defines are separated to
the MAX172XX defines instead of mixing them up.

>  #define MAX172XX_BATT			0xDA	/* Battery voltage */
>  #define MAX172XX_ATAVCAP		0xDF
>  
>  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;
> +	int charge_full_design;

Don't see charge_full_design is used somewhere besides reading it from
device-tree and it isn't part of the bindings. If not needed, remove it.

> +	enum chip_id id;
>  };
>  
> 

[...]

> +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)
> +				   struct max1720x_device_info *info,
> +				   int *intval)
>  {
> -	return reg * (500000 / info->rsense);	/* in uAh */
> +	int lsb = 1;
> +	int reg_val;

The naming of reg_val is somehow confusing because of reg. Better rename
it to something like reg_task_period or similar and reg_val should be of
type unsigned int. 

> +	int ret;
> +
> +	if (info->id == MAX77759_ID) {
> +		ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		switch (reg_val) {
> +		case MAX77759_TASKPERIOD_175MS:
> +			break;
> +		case MAX77759_TASKPERIOD_351MS:
> +			lsb = 2;
> +			break;
> +		default:
> +			return -ENODEV;
> +		}
> +	}
> +	*intval = reg * (500000 / info->rsense) * lsb;	/* in uAh */
> +	return 0;

nit: add newline before return.

>  }
>  
>  /*
> @@ -306,6 +420,28 @@ static int max172xx_temperature_to_ps(unsigned int reg)
>  	return val * 10 / 256; /* in tenths of deg. C */
>  }
>  
> +static const char *max1720x_devname_to_model(unsigned int reg_val,
> +					     union power_supply_propval *val,
> +					     struct max1720x_device_info *info)
> +{
> +	switch (info->id) {
> +	case MAX1720X_ID:
> +		reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
> +		if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
> +			return max17201_model;
> +		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> +			return max17205_model;
> +		return NULL;

nit: return NULL in else case.

> +	case MAX77759_ID:
> +		reg_val = FIELD_GET(MAX77759_DEV_NAME_TYPE_MASK, reg_val);
> +		if (reg_val == MAX77759_DEV_NAME_TYPE_MAX77759)
> +			return max77759_model;
> +		return NULL;

nit: return NULL in else case.

> +	default:
> +		return NULL;
> +	}
> +}
> +
>  /*
>   * Calculating current registers resolution:
>   *
> @@ -390,19 +526,31 @@ 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, &reg_val);
> -		val->intval = max172xx_voltage_to_ps(reg_val);
> +		if (info->id == MAX1720X_ID) {
> +			ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
> +			val->intval = max172xx_voltage_to_ps(reg_val);
> +		} else if (info->id == MAX77759_ID) {
> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
> +		} else
> +			return -ENODEV;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>  		ret = regmap_read(info->regmap, MAX172XX_DESIGN_CAP, &reg_val);
> -		val->intval = max172xx_capacity_to_ps(reg_val);
> +		if (ret)
> +			break;

I would keep max172xx_capacity_to_ps as it was before and add the
calculation for the MAX77759 after handling the MAX1720X case. Creating
a function max77759_capacity_to_ps that further processes the value
calculated by max172xx_capacity_to_ps or just inline this code.
Otherwise the naming of the function is somehow confusing.

> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_AVG:
>  		ret = regmap_read(info->regmap, MAX172XX_REPCAP, &reg_val);
> -		val->intval = max172xx_capacity_to_ps(reg_val);
> +		if (ret)
> +			break;
> +

Same as above.

> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
>  		ret = regmap_read(info->regmap, MAX172XX_TTE, &reg_val);
> +		pr_info("RAW TTE: %d", reg_val);

Remove pr_info.

>  		val->intval = max172xx_time_to_ps(reg_val);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
> @@ -423,17 +571,15 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL:
>  		ret = regmap_read(info->regmap, MAX172XX_FULL_CAP, &reg_val);
> -		val->intval = max172xx_capacity_to_ps(reg_val);

...

> +		if (ret)
> +			break;
> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>  		break;
>  	case POWER_SUPPLY_PROP_MODEL_NAME:
>  		ret = regmap_read(info->regmap, MAX172XX_DEV_NAME, &reg_val);
> -		reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
> -		if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
> -			val->strval = max17201_model;
> -		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> -			val->strval = max17205_model;
> -		else
> -			return -ENODEV;
> +		val->strval = max1720x_devname_to_model(reg_val, val, info);

Wouldn't it be better to just inline this function ?

> +		if (!val->strval)
> +			ret = -ENODEV;
>  {

[...]

>  	struct power_supply_config psy_cfg = {};
>  	struct device *dev = &client->dev;
>  	struct max1720x_device_info *info;
>  	struct power_supply *bat;
> +	const struct chip_data *data;
> +	const struct power_supply_desc *bat_desc;
>  	int ret;
>  
>  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
>  
> +	data = device_get_match_data(dev);
> +	if (!data)
> +		return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
> +
>  	psy_cfg.drv_data = info;
>  	psy_cfg.fwnode = dev_fwnode(dev);
> -	psy_cfg.attr_grp = max1720x_groups;
> +	switch (data->id) {
> +	case MAX1720X_ID:
> +		psy_cfg.attr_grp = max1720x_groups;
> +		bat_desc = &max1720x_bat_desc;
> +		break;
> +	case MAX77759_ID:
> +		bat_desc = &max77759_bat_desc;
> +		break;
> +	default:
> +		return dev_err_probe(dev, -EINVAL, "Unsupported chip\n");
> +	}

nit: add empty line

>  	i2c_set_clientdata(client, info);
> -	info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
> +
> +	info->id = data->id;
> +	info->regmap = devm_regmap_init_i2c(client, data->regmap_cfg);
>  	if (IS_ERR(info->regmap))
>  		return dev_err_probe(dev, PTR_ERR(info->regmap),
>  				     "regmap initialization failed\n");
>  
> -	ret = max1720x_probe_nvmem(client, info);
> +	if (data->has_nvmem) {
> +		ret = max1720x_probe_nvmem(client, info);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node,
> +				   "charge-full-design-microamp-hours", &info->charge_full_design);
> +	if (ret)
> +		info->charge_full_design = 0;
> +
> +	ret = max1720x_get_rsense(dev, info, data);
>  	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
> +		return dev_err_probe(dev, ret, "Failed to get RSense\n");
>  
> -	bat = devm_power_supply_register(dev, &max1720x_bat_desc, &psy_cfg);
> +	bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
>  	if (IS_ERR(bat))
>  		return dev_err_probe(dev, PTR_ERR(bat),
>  				     "Failed to register power supply\n");
> @@ -613,7 +816,8 @@ static int max1720x_probe(struct i2c_client *client)
>  }
>  
>  static const struct of_device_id max1720x_of_match[] = {
> -	{ .compatible = "maxim,max17201" },
> +	{ .compatible = "maxim,max17201", .data = (void *) &max1720x_data },
> +	{ .compatible = "maxim,max77759-fg", .data = (void *) &max77759_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, max1720x_of_match);
> 
> -- 
> 2.49.0
> 
> 

Best regards,
Dimitri Fedrau