Re: [PATCH RFC 2/8] power: supply: Add driver for Qualcomm PMI8998 fuel gauge

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 11/24/25 10:53 PM, David Heidelberg via B4 Relay wrote:
> From: Joel Selvaraj <foss@joelselvaraj.com>
> 
> Ths driver supports the fuel gauge hardware available on PMICs known as
> 3rd generation fuel gauge hardware available on PMI8998.
> 
> Co-developed-by: Casey Connolly <casey@connolly.tech>
> Co-developed-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> Co-developed-by: Yassine Oudjana <y.oudjana@protonmail.com>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---

[...]

> +/**
> + * @brief pmi8998_fg_read() - Read multiple registers with regmap_bulk_read

I think this won't pass kerneldoc checks (make W=1)

[...]

> +static int pmi8998_fg_write(struct pmi8998_fg_chip *chip, u8 *val, u16 addr, int len)
> +{
> +	bool sec_access = (addr & 0xff) > 0xd0;

Downstream checks if the address is > 0xBA which is what you want
at least for pmi8998

You can de-abbreviate this to 'secure_access' (not to be confused
with 'secondary' or so). There's a locking mechanism which needs a
0xa5 byte written to the base+0xd0 register (applies to all FG
peripherals with the 'last non-secure register' value possibly
varying).

[...]

> +	u8 sec_addr_val = 0xa5;
> +	int ret;
> +
> +	if (((chip->base + addr) & 0xff00) == 0)

The 'fuel gauge' consists of:

FG_BATT_SOC @ 0x4000 (state of charge monitor)
FG_BATT_INFO @ 0x4100 ("general fg minus SoC")
FG_BCL @ 0x4200 (battery current limiter)
FG_LMH @ 0x4300 (limits management hardware)
FG_MEM_IF @ 0x4400 (DMA engine)
RRADC @ 0x4500 (today handled by its own driver)

and a couple other peripherals that Linux doesn't need to worry about

Each one of them should have its own 'reg' entry (which is assumed
to be 0x100-long), which will let you skip such interesting checks
and rely on the regmap framework disallowing address spillover (or
you can just then make the addr argument a u8)

It would be good to keep in mind their relationship and think about how
to model them together. I don't think they must all necessarily be part
of a single big "fg" dt node, particularly the LMH/BCL part seems to be
rather self-contained

[...]

> +		return -EINVAL;
> +
> +	dev_vdbg(chip->dev, "%s: Writing 0x%x to 0x%x", __func__, *val, addr);
> +
> +	if (sec_access) {
> +		ret = regmap_bulk_write(chip->regmap,
> +					((chip->base + addr) & 0xff00) | 0xd0,
> +				&sec_addr_val, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_bulk_write(chip->regmap, chip->base + addr, val, len);
> +}
> +
> +/**
> + * @brief pmi8998_fg_masked_write() - like pmi8998_fg_write but applies
> + * a mask first.
> + *
> + * @param chip Pointer to chip
> + * @param val Pointer to write values from
> + * @param addr Address to write to
> + * @param len Number of registers (bytes) to write
> + * @return int 0 on success, negative errno on error
> + */
> +static int pmi8998_fg_masked_write(struct pmi8998_fg_chip *chip, u16 addr, u8 mask, u8 val)
> +{
> +	u8 reg;
> +	int ret;
> +
> +	ret = pmi8998_fg_read(chip, &reg, addr, 1);
> +	if (ret)
> +		return ret;
> +
> +	reg &= ~mask;
> +	reg |= val & mask;
> +
> +	return pmi8998_fg_write(chip, &reg, addr, 1);
> +}
> +
> +/*
> + * Battery status
> + */
> +
> +/**
> + * @brief pmi8998_fg_get_capacity() - Get remaining capacity of battery
> + *
> + * @param chip Pointer to chip
> + * @param val Pointer to store value at
> + * @return int 0 on success, negative errno on error
> + */
> +static int pmi8998_fg_get_capacity(struct pmi8998_fg_chip *chip, int *val)
> +{
> +	u8 cap[2];
> +	int ret;
> +
> +	ret = pmi8998_fg_read(chip, cap, BATT_MONOTONIC_SOC, 2);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to read capacity: %d", ret);
> +		return ret;
> +	}

Downstream tries for 5 times to get this (raw) pair of values and fails if
they don't match - 0x400a is a shadow register of 0x4009 and this is very
much intended

> +	if (cap[0] != cap[1])
> +		cap[0] = cap[0] < cap[1] ? cap[0] : cap[1];
> +
> +	*val = DIV_ROUND_CLOSEST((cap[0] - 1) * 98, 0xff - 2) + 1;

98 comes from "FULL_CAPACITY (100) - 2", 0xff denotes "FULL_SOC_RAW", i.e. the
raw value of this register that corresponds to 100% (again not sure where the
minus2 comes from - perhaps some rounding fixups)

> +
> +	return 0;
> +}
> +
> +/**
> + * @brief pmi8998_fg_get_temperature() - Get temperature of battery
> + *
> + * @param chip Pointer to chip
> + * @param val Pointer to store value at
> + * @return int 0 on success, negative errno on error
> + */
> +static int pmi8998_fg_get_temperature(struct pmi8998_fg_chip *chip, int *val)
> +{
> +	int ret, temp;
> +	u8 readval[2];
> +
> +	ret = pmi8998_fg_read(chip, readval, PARAM_ADDR_BATT_TEMP, 2);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to read temperature: %d\n", ret);
> +		return ret;
> +	}
> +
> +	temp = ((readval[1] & BATT_TEMP_MSB_MASK) << 8) |
> +		(readval[0] & BATT_TEMP_LSB_MASK);
> +	temp = DIV_ROUND_CLOSEST(temp * 10, 4);
> +
> +	*val = temp - 2730;
> +
> +	return 0;
> +}
> +
> +/**
> + * @brief pmi8998_fg_get_current() - Get current being drawn from battery
> + *
> + * @param chip Pointer to chip
> + * @param val Pointer to store value at
> + * @return int 0 on success, negative errno on error
> + */
> +static int pmi8998_fg_get_current(struct pmi8998_fg_chip *chip, int *val)
> +{
> +	s16 temp;
> +	u8 readval[2];
> +	int ret;
> +
> +	ret = pmi8998_fg_read(chip, readval, PARAM_ADDR_BATT_CURRENT, 2);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to read current: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* handle rev 1 too */

PMI8998v1 has flipped the order of the registers and I would guesstimate
that it wouldn't actually be present in the wild

> +	temp = (s16)(readval[1] << 8 | readval[0]);
> +	*val = div_s64((s64)temp * 488281, 1000);

This is a funny way to say that this is a 2s complement-encoded
16b value, where 5 bits are reserved for the integer portion

[...]

> +		power_supply_changed(chip->batt_psy);
> +
> +		if (chip->status == POWER_SUPPLY_STATUS_UNKNOWN) {
> +			/*
> +			 * REVISIT: Find better solution or remove current-based
> +			 * status checking once checking is properly implemented
> +			 * in charger drivers
> +
> +			 * Sometimes it take a while for current to stabilize,
> +			 * so signal property change again later to make sure
> +			 * current-based status is properly detected.
> +			 */

On downstream, it's the charger counterpart that signals PSY_STATUS_(DIS)CHARGING

Konrad