Re: [PATCH v7] power: supply: bq27xxx: detect bq27541 behind bq27411 compatible

[Pali Rohár]

On Monday 19 October 2026 02:03:39 Rinat Muhamedgaliev via B4 Relay wrote:
> From: Rinat Muhamedgaliev <rinat.muhamedgaliev@xxxxxxxxx>
> 
> OnePlus 6 and 6T replacement battery packs may contain either a bq27411
> or a bq27541 fuel gauge at I2C address 0x55. The device tree currently
> identifies the gauge as bq27411, but a bq27541 uses a different register
> map and then reports invalid battery values.
> 
> Read the DeviceType control subcommand when probing a bq27411. Keep the
> existing profile for DeviceType 0x0421, but select the bq27541 profile
> for DeviceType 0x0541. This retains the established DT ABI and supports
> replacement packs without introducing a generic compatible.
> 
> Tested on OnePlus 6T (fajita) with DeviceType 0x0541: voltage, state of
> charge, and temperature were reported correctly. Testing on hardware with
> DeviceType 0x0421 would be appreciated.
> 
> Signed-off-by: Rinat Muhamedgaliev <rinat.muhamedgaliev@xxxxxxxxx>
> ---
>  drivers/power/supply/bq27xxx_battery_i2c.c | 67 ++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
> index 94b00bb89c17..a9982953e4f2 100644
> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> @@ -16,6 +16,11 @@
>  static DEFINE_IDR(battery_id);
>  static DEFINE_MUTEX(battery_mutex);
>  
> +#define BQ27XXX_REG_CTRL		0x00
> +#define BQ27XXX_DEVICE_TYPE		0x0001
> +#define BQ27411_DEVICE_TYPE		0x0421
> +#define BQ27541_DEVICE_TYPE		0x0541
> +
>  static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
>  {
>  	struct bq27xxx_device_info *di = data;
> @@ -136,6 +141,56 @@ static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
>  	return 0;
>  }
>  
> +static int bq27xxx_battery_i2c_check_device_type(struct bq27xxx_device_info *di)
> +{
> +	enum bq27xxx_chip detected_chip;
> +	int expected_type;
> +	int ret;
> +
> +	switch (di->chip) {
> +	case BQ27411:
> +		expected_type = BQ27411_DEVICE_TYPE;
> +		break;
> +	case BQ27541:
> +		expected_type = BQ27541_DEVICE_TYPE;
> +		break;
> +	default:
> +		dev_warn(di->dev, "expected DeviceType undefined\n");
> +		return 0;
> +	}
> +
> +	ret = di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_DEVICE_TYPE,
> +			    false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = di->bus.read(di, BQ27XXX_REG_CTRL, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case BQ27411_DEVICE_TYPE:
> +		detected_chip = BQ27411;
> +		break;
> +	case BQ27541_DEVICE_TYPE:
> +		detected_chip = BQ27541;
> +		break;
> +	default:
> +		dev_err(di->dev, "unrecognized DeviceType 0x%04x\n", ret);
> +		return -ENODEV;
> +	}
> +
> +	if (detected_chip == di->chip)
> +		return 0;
> +
> +	dev_warn(di->dev,
> +		 "detected DeviceType 0x%04x instead of expected 0x%04x\n",
> +		 ret, expected_type);
> +	di->chip = detected_chip;
> +
> +	return 0;
> +}
> +
>  static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>  				     const struct i2c_device_id *id)
>  {
> @@ -169,6 +224,18 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>  	di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
>  	di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
>  
> +	/* Add fuel gauges with supported DeviceType reporting to this list. */
> +	switch (di->chip) {
> +	case BQ27411:
> +	case BQ27541:
> +		ret = bq27xxx_battery_i2c_check_device_type(di);
> +		if (ret)
> +			goto err_failed;
> +		break;
> +	default:
> +		break;
> +	}
> +
>  	ret = bq27xxx_battery_setup(di);
>  	if (ret)
>  		goto err_failed;
> 
> -- 
> 2.56.0
> 
> 

Hello, my understanding is that this whole code is not i2c specific as
it uses the bq27xxx_device_info abstraction APIs. Therefore should not
be this code rather part of the bus-neutral bq27xxx_battery_setup()
function which is in the bq27xxx_battery.c file? Also that file already
contains definitions of register maps. This change is adding new
register reading/writing which looks to be inconsistent with the way how
the whole driver is structured and how it access registers. For example
the BQ27XXX_REG_CTRL is already defined in the bq27xxx_battery.c file.
And there is also enum /* control register params */ where the
BQ27XXX_DEVICE_TYPE constant should belong (as there are all other
commands defined).