Re: [PATCH v4 09/11] mfd: bq257xx: Add BQ25792 support

[Lee Jones]

On Wed, 11 Mar 2026, Alexey Charkov wrote:

> Add register definitions and a new 'type' enum to be passed via MFD
> private data to support the BQ25792, which is a newer variant of the
> BQ257xx family.
> 
> BQ25792 shares similar logic of operation with the already supported
> BQ25703A but has a completely different register map and different
> electrical constraints.
> 
> Tested-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> Signed-off-by: Alexey Charkov <alchark@xxxxxxxxxxx>
> ---
>  drivers/mfd/bq257xx.c       |  54 +++++-
>  include/linux/mfd/bq257xx.h | 414 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 465 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/bq257xx.c b/drivers/mfd/bq257xx.c
> index e9d49dac0a16..4445ded5b2eb 100644
> --- a/drivers/mfd/bq257xx.c
> +++ b/drivers/mfd/bq257xx.c
> @@ -39,13 +39,47 @@ static const struct regmap_config bq25703_regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
>  };
>  
> -static const struct mfd_cell cells[] = {
> +static const struct regmap_range bq25792_writeable_reg_ranges[] = {
> +	regmap_reg_range(BQ25792_REG00_MIN_SYS_VOLTAGE,
> +			 BQ25792_REG18_NTC_CONTROL_1),
> +	regmap_reg_range(BQ25792_REG28_CHARGER_MASK_0,
> +			 BQ25792_REG30_ADC_FUNCTION_DISABLE_1),
> +};
> +
> +static const struct regmap_access_table bq25792_writeable_regs = {
> +	.yes_ranges = bq25792_writeable_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(bq25792_writeable_reg_ranges),
> +};
> +
> +static const struct regmap_range bq25792_volatile_reg_ranges[] = {
> +	regmap_reg_range(BQ25792_REG19_ICO_CURRENT_LIMIT,
> +			 BQ25792_REG27_FAULT_FLAG_1),
> +	regmap_reg_range(BQ25792_REG31_IBUS_ADC,
> +			 BQ25792_REG47_DPDM_DRIVER),
> +};
> +
> +static const struct regmap_access_table bq25792_volatile_regs = {
> +	.yes_ranges = bq25792_volatile_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(bq25792_volatile_reg_ranges),
> +};
> +
> +static const struct regmap_config bq25792_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = BQ25792_REG48_PART_INFORMATION,
> +	.cache_type = REGCACHE_MAPLE,
> +	.wr_table = &bq25792_writeable_regs,
> +	.volatile_table = &bq25792_volatile_regs,
> +};
> +
> +static struct mfd_cell cells[] = {

This was `static const` before. And I don't see any code in here making
changes to it.  Was the `const` dropped intentionally?

>  	MFD_CELL_NAME("bq257xx-regulator"),
>  	MFD_CELL_NAME("bq257xx-charger"),
>  };
>  
>  static int bq257xx_probe(struct i2c_client *client)
>  {
> +	const struct regmap_config *rcfg;
>  	struct bq257xx_device *ddata;
>  	int ret;
>  
> @@ -53,9 +87,21 @@ static int bq257xx_probe(struct i2c_client *client)
>  	if (!ddata)
>  		return -ENOMEM;
>  
> +	ddata->type = (uintptr_t)device_get_match_data(&client->dev);

This logic seems a bit fragile. For non-DeviceTree platforms,
`device_get_match_data()` will return `NULL` since the `i2c_device_id`
table doesn't provide any match data. Casting `NULL` to `uintptr_t` will
result in `0`, which corresponds to `BQ25703A`. This means the new
`bq25792` will be misidentified as a `bq25703a` when instantiated
without a DeviceTree node.  Even with DeviceTree, if a compatible string
is matched but has no `.data` property, the same misidentification will
occur.

Perhaps it would be safer to explicitly handle the `NULL` case and make the
identification logic more robust?  Or perhaps start your IDs from 1.

>  	ddata->client = client;
>  
> -	ddata->regmap = devm_regmap_init_i2c(client, &bq25703_regmap_config);
> +	switch (ddata->type) {
> +	case BQ25703A:
> +		rcfg = &bq25703_regmap_config;
> +		break;
> +	case BQ25792:
> +		rcfg = &bq25792_regmap_config;
> +		break;
> +	default:
> +		return dev_err_probe(&client->dev, -EINVAL, "Unsupported device type\n");

Given the potential for `device_get_match_data()` to return `NULL` (which
becomes `0`), this `default` case might not be reachable for invalid or
un-matchable devices. A more explicit check on the match data before this
`switch` might be better.

> +	}
> +
> +	ddata->regmap = devm_regmap_init_i2c(client, rcfg);
>  	if (IS_ERR(ddata->regmap)) {
>  		return dev_err_probe(&client->dev, PTR_ERR(ddata->regmap),
>  				     "Failed to allocate register map\n");
> @@ -74,12 +120,14 @@ static int bq257xx_probe(struct i2c_client *client)
>  
>  static const struct i2c_device_id bq257xx_i2c_ids[] = {
>  	{ "bq25703a" },
> +	{ "bq25792" },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, bq257xx_i2c_ids);
>  
>  static const struct of_device_id bq257xx_of_match[] = {
> -	{ .compatible = "ti,bq25703a" },
> +	{ .compatible = "ti,bq25703a", .data = (void *)BQ25703A },
> +	{ .compatible = "ti,bq25792", .data = (void *)BQ25792 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, bq257xx_of_match);

[...]

-- 
Lee Jones [李琼斯]