Re: [PATCH v3 2/4] power: supply: max17042_battery: Initialize MAX17055 from battery info

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 17665 bytes --]

Hello Vincent,

On Tue, Jul 21, 2026 at 02:57:34PM -0400, Vincent Cloutier wrote:
> From: Vincent Cloutier <vincent@cloutier.co>
> 
> MAX17055 can use charge-full-design-microamp-hours and
> charge-term-current-microamp from monitored-battery for its POR
> configuration. Prepare quantized DesignCap, IChgTerm, and EZ Config dQAcc
> values in the power-supply registration callback after the core has parsed
> battery information.
> 
> Validate all supplied values before changing the sparse configuration.
> Reject zero, values that quantize to zero, capacities that cannot produce
> dQAcc, and positive termination currents outside the signed register range.
> Limit this path to MAX17055 because the other supported gauges require
> complete characterization data.
> 
> Follow the documented POR sequence. Wait for FSTAT.DNR, save HibCfg, exit
> hibernate, write stable battery registers once and verify them after 1 ms,
> issue command writes directly, poll until refresh clears, and restore
> HibCfg. Clear POR only after every step succeeds.
> 
> Keep driver-backed properties unavailable while initialization is
> incomplete. If an I/O error interrupts the transaction, restore hibernate
> before the next attempt and retry periodically so a transient startup
> failure does not become permanent.
> 
> Assisted-by: OpenCode:gpt-5.6-sol
> Signed-off-by: Vincent Cloutier <vincent@cloutier.co>
> ---

Please split this into smaller patches doing one thing at a time,
which makes it a lot easier to review and also helps to identify
potential bugs with git bisect in the future.

At least the init_complete and the change from 'struct work_struct
work' to 'struct delayed_work work' could be done in advance.

>  drivers/power/supply/max17042_battery.c | 335 +++++++++++++++++++++---
>  include/linux/power/max17042_battery.h  |   2 +
>  2 files changed, 306 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index d409d2f0d383..b25bf26135e0 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -62,18 +62,30 @@
>  #define MAX17042_RESISTANCE_LSB		1 / 4096 /* Ω */
>  #define MAX17042_TEMPERATURE_LSB	1 / 256 /* °C */
>  
> +#define MAX17055_DQACC_DIV		32
> +#define MAX17055_DPACC_FACTOR		44138
> +#define MAX17055_DPACC_VCHG_FACTOR	51200
> +#define MAX17055_FSTAT_DNR_BIT		BIT(0)
> +#define MAX17055_DNR_POLL_US		10000
> +#define MAX17055_DNR_TIMEOUT_US		2000000
> +#define MAX17055_INIT_RETRY_DELAY_MS	10000
> +#define MAX17055_REFRESH_POLL_US		10000
> +#define MAX17055_REFRESH_TIMEOUT_US	1000000
> +
>  struct max17042_chip {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	struct power_supply *battery;
>  	enum max170xx_chip_type chip_type;
>  	struct max17042_config_data *config_data;
> -	struct work_struct work;
> -	int    init_complete;
> +	struct delayed_work work;
>  	int    irq;
>  	int    task_period;
>  	bool   enable_current_sense;
>  	bool   enable_por_init;
> +	bool   init_complete;
> +	bool   hib_restore_pending;
> +	u16    hib_cfg;
>  	unsigned int r_sns;
>  	int    vmin;	/* in millivolts */
>  	int    vmax;	/* in millivolts */
> @@ -256,7 +268,7 @@ static int max17042_get_property(struct power_supply *psy,
>  	u32 data;
>  	u64 data64;
>  
> -	if (!chip->init_complete)
> +	if (!READ_ONCE(chip->init_complete))
>  		return -EAGAIN;
>  
>  	switch (psp) {
> @@ -566,6 +578,24 @@ static int max17042_write_verify_reg(struct regmap *map, u8 reg, u32 value)
>  	return ret;
>  }
>  
> +static int max17055_write_verify_reg(struct regmap *map, u8 reg, u32 value)
> +{
> +	u32 read_value;
> +	int ret;
> +
> +	ret = regmap_write(map, reg, value);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(1000, 2000);
> +
> +	ret = regmap_read(map, reg, &read_value);
> +	if (ret)
> +		return ret;
> +
> +	return read_value == value ? 0 : -EIO;
> +}
> +
>  static inline void max17042_override_por(struct regmap *map,
>  					 u8 reg, u16 value)
>  {
> @@ -801,8 +831,12 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
>  	max17042_override_por(map, MAX17042_CONFIG, config->config);
>  	max17042_override_por(map, MAX17042_SHDNTIMER, config->shdntimer);
>  
> -	max17042_override_por(map, MAX17042_DesignCap, config->design_cap);
> -	max17042_override_por(map, MAX17042_ICHGTerm, config->ichgt_term);
> +	if (chip->chip_type != MAXIM_DEVICE_TYPE_MAX17055) {
> +		max17042_override_por(map, MAX17042_DesignCap,
> +				      config->design_cap);
> +		max17042_override_por(map, MAX17042_ICHGTerm,
> +				      config->ichgt_term);
> +	}
>  
>  	max17042_override_por(map, MAX17042_AtRate, config->at_rate);
>  	max17042_override_por(map, MAX17042_LearnCFG, config->learn_cfg);
> @@ -812,8 +846,10 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
>  
>  	max17042_override_por(map, MAX17042_FullCAP, config->fullcap);
>  	max17042_override_por(map, MAX17042_FullCAPNom, config->fullcapnom);
> -	max17042_override_por(map, MAX17042_dQacc, config->dqacc);
> -	max17042_override_por(map, MAX17042_dPacc, config->dpacc);
> +	if (chip->chip_type != MAXIM_DEVICE_TYPE_MAX17055) {
> +		max17042_override_por(map, MAX17042_dQacc, config->dqacc);
> +		max17042_override_por(map, MAX17042_dPacc, config->dpacc);
> +	}
>  
>  	max17042_override_por(map, MAX17042_RCOMP0, config->rcomp0);
>  	max17042_override_por(map, MAX17042_TempCo, config->tcompc0);
> @@ -847,26 +883,162 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
>  		max17042_override_por(map, MAX17055_ModelCfg, config->model_cfg);
>  }
>  
> -static int max17042_init_chip(struct max17042_chip *chip)
> +static int max17055_override_battery_values(struct max17042_chip *chip)
>  {
> +	struct max17042_config_data *config = chip->config_data;
>  	struct regmap *map = chip->regmap;
> +	unsigned int design_cap;
> +	unsigned int model_cfg;
> +	unsigned int dqacc;
> +	u64 dpacc;
>  	int ret;
>  
> +	if (config->design_cap) {
> +		ret = max17055_write_verify_reg(map, MAX17042_DesignCap,
> +						config->design_cap);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (config->dqacc) {
> +		ret = max17055_write_verify_reg(map, MAX17042_dQacc,
> +						config->dqacc);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (config->ichgt_term) {
> +		ret = max17055_write_verify_reg(map, MAX17042_ICHGTerm,
> +						config->ichgt_term);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!config->design_cap && !config->dqacc)
> +		return 0;
> +
> +	ret = regmap_read(map, MAX17042_DesignCap, &design_cap);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, MAX17042_dQacc, &dqacc);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, MAX17055_ModelCfg, &model_cfg);
> +	if (ret)
> +		return ret;
> +
> +	if (!design_cap || !dqacc)
> +		return -ERANGE;
> +
> +	dpacc = (u64)dqacc *
> +		(model_cfg & MAX17055_MODELCFG_VCHG_BIT ?
> +		 MAX17055_DPACC_VCHG_FACTOR : MAX17055_DPACC_FACTOR);
> +	do_div(dpacc, design_cap);
> +	if (dpacc > U16_MAX)
> +		return -ERANGE;
> +
> +	return max17055_write_verify_reg(map, MAX17042_dPacc, (u16)dpacc);
> +}
> +
> +static int max17055_restore_hibernate(struct max17042_chip *chip)
> +{
> +	int restore_hib_ret;
> +	int soft_wakeup_ret;
> +
> +	soft_wakeup_ret = regmap_write(chip->regmap, MAX17055_SoftWakeup, 0);
> +	restore_hib_ret = max17055_write_verify_reg(chip->regmap,
> +						    MAX17055_HibCfg,
> +						    chip->hib_cfg);
> +	if (!soft_wakeup_ret && !restore_hib_ret)
> +		chip->hib_restore_pending = false;
> +
> +	return soft_wakeup_ret ?: restore_hib_ret;
> +}
> +
> +static int max17055_init_chip(struct max17042_chip *chip)
> +{
> +	struct regmap *map = chip->regmap;
> +	unsigned int hib_cfg;
> +	unsigned int model_cfg;
> +	unsigned int fstat;
> +	int restore_ret;
> +	int ret;
> +
> +	if (chip->hib_restore_pending) {
> +		ret = max17055_restore_hibernate(chip);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read_poll_timeout(map, MAX17042_FSTAT, fstat,
> +				       !(fstat & MAX17055_FSTAT_DNR_BIT),
> +				       MAX17055_DNR_POLL_US,
> +				       MAX17055_DNR_TIMEOUT_US);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, MAX17055_HibCfg, &hib_cfg);
> +	if (ret)
> +		return ret;
> +
> +	chip->hib_cfg = hib_cfg;
> +	chip->hib_restore_pending = true;
> +
> +	ret = regmap_write(map, MAX17055_SoftWakeup, 0x0090);
> +	if (ret)
> +		goto restore_hibernate;
> +
> +	ret = max17055_write_verify_reg(map, MAX17055_HibCfg, 0);
> +	if (ret)
> +		goto restore_hibernate;
> +
> +	ret = regmap_write(map, MAX17055_SoftWakeup, 0);
> +	if (ret)
> +		goto restore_hibernate;
> +
>  	max17042_override_por_values(chip);
>  
> +	ret = max17055_override_battery_values(chip);
> +	if (ret)
> +		goto restore_hibernate;
> +
> +	ret = regmap_write_bits(map, MAX17055_ModelCfg,
> +				MAX17055_MODELCFG_REFRESH_BIT,
> +				MAX17055_MODELCFG_REFRESH_BIT);
> +	if (ret)
> +		goto restore_hibernate;
> +
> +	ret = regmap_read_poll_timeout(map, MAX17055_ModelCfg, model_cfg,
> +				       !(model_cfg &
> +					 MAX17055_MODELCFG_REFRESH_BIT),
> +				       MAX17055_REFRESH_POLL_US,
> +				       MAX17055_REFRESH_TIMEOUT_US);
> +
> +restore_hibernate:
> +	restore_ret = max17055_restore_hibernate(chip);
> +	if (restore_ret)
> +		return restore_ret;
> +
> +	return ret;
> +}
> +
> +static int max17042_init_chip(struct max17042_chip *chip)
> +{
> +	struct regmap *map = chip->regmap;
> +	int ret;
> +
>  	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> -		regmap_write_bits(map, MAX17055_ModelCfg,
> -				  MAX17055_MODELCFG_REFRESH_BIT,
> -				  MAX17055_MODELCFG_REFRESH_BIT);
> -	}
> +		ret = max17055_init_chip(chip);
> +		if (ret)
> +			return ret;
> +	} else {
> +		max17042_override_por_values(chip);
>  
> -	/* After Power up, the MAX17042 requires 500mS in order
> -	 * to perform signal debouncing and initial SOC reporting
> -	 */
> -	msleep(500);
> +		/* Allow signal debouncing and initial SOC reporting. */
> +		msleep(500);
>  
> -	if (chip->chip_type != MAXIM_DEVICE_TYPE_MAX17055) {
> -		/* Initialize configuration */
>  		max17042_write_config_regs(chip);
>  
>  		/* write cell characterization data */
> @@ -902,8 +1074,7 @@ static int max17042_init_chip(struct max17042_chip *chip)
>  	}
>  
>  	/* Init complete, Clear the POR bit */
> -	regmap_update_bits(map, MAX17042_STATUS, STATUS_POR_BIT, 0x0);
> -	return 0;
> +	return regmap_clear_bits(map, MAX17042_STATUS, STATUS_POR_BIT);

another correct, but unrelated change.

>  }
>  
>  static void max17042_set_soc_threshold(struct max17042_chip *chip, u16 off)
> @@ -989,18 +1160,28 @@ static irqreturn_t max17042_thread_handler(int id, void *dev)
>  
>  static void max17042_init_worker(struct work_struct *work)
>  {
> -	struct max17042_chip *chip = container_of(work,
> +	struct max17042_chip *chip = container_of(to_delayed_work(work),
>  				struct max17042_chip, work);
> -	int ret;
> +	int ret = 0;
>  
>  	/* Initialize registers according to values from config_data */
> -	if (chip->enable_por_init && chip->config_data) {
> +	if (chip->enable_por_init && chip->config_data)
>  		ret = max17042_init_chip(chip);
> -		if (ret)
> -			return;
> +
> +	if (ret) {
> +		if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> +			dev_warn_ratelimited(chip->dev,
> +				"initialization failed: %d, retrying\n", ret);
> +			queue_delayed_work(system_freezable_wq, &chip->work,
> +				msecs_to_jiffies(MAX17055_INIT_RETRY_DELAY_MS));
> +		} else {
> +			dev_err(chip->dev, "initialization failed: %d\n", ret);
> +		}
> +		return;
>  	}
>  
> -	chip->init_complete = 1;
> +	WRITE_ONCE(chip->init_complete, true);
> +	power_supply_changed(chip->battery);
>  }
>  
>  #ifdef CONFIG_OF
> @@ -1060,6 +1241,93 @@ static int max17042_init_defaults(struct max17042_chip *chip)
>  	return 0;
>  }
>  
> +static int max17042_apply_battery_properties(struct max17042_chip *chip,
> +					     struct power_supply_battery_info *info)
> +{
> +	struct max17042_config_data *config;
> +	struct device *dev = chip->dev;
> +	bool have_design_cap;
> +	bool have_ichgt_term;
> +	u16 design_cap = 0;
> +	u16 ichgt_term = 0;
> +	u16 dqacc = 0;
> +	u64 data64;
> +
> +	if (!info || chip->chip_type != MAXIM_DEVICE_TYPE_MAX17055)
> +		return 0;
> +
> +	have_design_cap = chip->enable_current_sense &&
> +		info->charge_full_design_uah >= 0;
> +	have_ichgt_term = chip->enable_current_sense &&
> +		info->charge_term_current_ua >= 0;
> +	if (!have_design_cap && !have_ichgt_term)
> +		return 0;
> +
> +	if (have_design_cap) {
> +		if (!info->charge_full_design_uah)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "battery design capacity must be positive\n");

If it's negative you set have_design_cap=false. This would only be
printed for charge_full_design_uah = 0. It seems more sensible to
also set have_design_cap to false for this case and drop this extra
check.

Greetings,

-- Sebastian

> +		data64 = (u64)info->charge_full_design_uah * chip->r_sns;
> +		do_div(data64, MAX17042_CAPACITY_LSB);
> +		if (!data64)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "battery design capacity is too small for sense resistor\n");
> +		if (data64 > U16_MAX)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "battery design capacity exceeds register range\n");
> +
> +		design_cap = (u16)data64;
> +		dqacc = design_cap / MAX17055_DQACC_DIV;
> +		if (!dqacc)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "battery design capacity is too small for EZ config\n");
> +	}
> +
> +	if (have_ichgt_term) {
> +		if (!info->charge_term_current_ua)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "charge termination current must be positive\n");
> +
> +		data64 = (u64)info->charge_term_current_ua * chip->r_sns;
> +		do_div(data64, MAX17042_CURRENT_LSB);
> +		if (!data64)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "charge termination current is too small for sense resistor\n");
> +		if (data64 > S16_MAX)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "charge termination current exceeds positive register range\n");
> +
> +		ichgt_term = (u16)data64;
> +	}
> +
> +	config = chip->config_data;
> +	if (!config) {
> +		config = devm_kzalloc(dev, sizeof(*config), GFP_KERNEL);
> +		if (!config)
> +			return -ENOMEM;
> +	}
> +
> +	if (have_design_cap) {
> +		config->design_cap = design_cap;
> +		config->dqacc = dqacc;
> +	}
> +	if (have_ichgt_term)
> +		config->ichgt_term = ichgt_term;
> +
> +	chip->config_data = config;
> +	chip->enable_por_init = true;
> +
> +	return 0;
> +}
> +
> +static int max17042_init_battery(struct power_supply *psy)
> +{
> +	struct max17042_chip *chip = power_supply_get_drvdata(psy);
> +
> +	return max17042_apply_battery_properties(chip, psy->battery_info);
> +}
> +
>  static const struct regmap_config max17042_regmap_config = {
>  	.name = "max17042",
>  	.reg_bits = 8,
> @@ -1113,6 +1381,7 @@ static const struct power_supply_desc max17042_psy_desc = {
>  	.set_property	= max17042_set_property,
>  	.property_is_writeable	= max17042_property_is_writeable,
>  	.external_power_changed	= power_supply_changed,
> +	.init		= max17042_init_battery,
>  	.properties	= max17042_battery_props,
>  	.num_properties	= ARRAY_SIZE(max17042_battery_props),
>  };
> @@ -1123,6 +1392,7 @@ static const struct power_supply_desc max17042_no_current_sense_psy_desc = {
>  	.get_property	= max17042_get_property,
>  	.set_property	= max17042_set_property,
>  	.property_is_writeable	= max17042_property_is_writeable,
> +	.init		= max17042_init_battery,
>  	.properties	= max17042_battery_props,
>  	.num_properties	= ARRAY_SIZE(max17042_battery_props) - 2,
>  };
> @@ -1242,15 +1512,18 @@ static int max17042_probe(struct i2c_client *client, struct device *dev, int irq
>  
>  	chip->irq = irq;
>  
> -	regmap_read(chip->regmap, MAX17042_STATUS, &val);
> +	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to read status\n");
> +
>  	if (val & STATUS_POR_BIT) {
> -		ret = devm_work_autocancel(dev, &chip->work,
> -					   max17042_init_worker);
> +		ret = devm_delayed_work_autocancel(dev, &chip->work,
> +						   max17042_init_worker);
>  		if (ret)
>  			return ret;
> -		schedule_work(&chip->work);
> +		queue_delayed_work(system_freezable_wq, &chip->work, 0);
>  	} else {
> -		chip->init_complete = 1;
> +		WRITE_ONCE(chip->init_complete, true);
>  	}
>  
>  	return 0;
> diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
> index 13aeab1597c6..810e068eafd2 100644
> --- a/include/linux/power/max17042_battery.h
> +++ b/include/linux/power/max17042_battery.h
> @@ -25,6 +25,7 @@
>  #define MAX17042_CHARACTERIZATION_DATA_SIZE 48
>  
>  #define MAX17055_MODELCFG_REFRESH_BIT	BIT(15)
> +#define MAX17055_MODELCFG_VCHG_BIT	BIT(10)
>  
>  enum max17042_register {
>  	MAX17042_STATUS		= 0x00,
> @@ -124,6 +125,7 @@ enum max17055_register {
>  
>  	MAX17055_ConvgCfg	= 0x49,
>  	MAX17055_VFRemCap	= 0x4A,
> +	MAX17055_SoftWakeup	= 0x60,
>  
>  	MAX17055_STATUS2	= 0xB0,
>  	MAX17055_POWER		= 0xB1,
> -- 
> 2.55.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]