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

[Alexey Charkov]

On Thu, Mar 19, 2026 at 9:01 PM Lee Jones <lee@xxxxxxxxxx> wrote:
>
> 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?

Thanks for spotting this, Lee. It was removed in a prior version when
I was passing the type as platform data, which is no longer the case,
so the const can be reinstated. Will do.

> >       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.

Guess I'll do both: add matches to the `i2c_device_id` to enable
non-OF use, and make IDs 1-based to avoid misinterpreting a failed
match as a valid BQ25703A match. Thanks for the pointers!

> >       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.

I'll shift the enum values to start at 1, then an extra check on the
match data shouldn't be necessary, as NULL will fall through to the
error in `default`.

Thanks a lot,
Alexey