Re: [PATCH v2 5/8] iio: dac: ds4424: convert to regmap

Oleksij Rempel
2026-02-01T16:16:56Z

Hi Sander,

On Sun, Feb 01, 2026 at 03:42:28PM +0100, Sander Vanheule wrote:
> Hi Oleksij,
>
> On Tue, 2026-01-27 at 07:09 +0100, Oleksij Rempel wrote:
> > Refactor the driver to use the regmap API.
> >
> > Replace the driver-specific mutex and manual shadow buffers with the
> > standard regmap infrastructure for locking and caching.
> >
> > This ensures the cache is populated from hardware at probe, preventing
> > state desynchronization (e.g. across suspend/resume).
>
>
> [...]
>
> > +static const struct regmap_access_table ds44x4_table = {
> > + .yes_ranges = ds44x4_ranges,
> > + .n_yes_ranges = ARRAY_SIZE(ds44x4_ranges),
> > +};
> >
> > -static int ds4424_set_value(struct iio_dev *indio_dev,
> > -      int val, struct iio_chan_spec const *chan)
> > +static const struct regmap_config ds44x2_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .cache_type = REGCACHE_FLAT,
> > + .max_register = DS4424_DAC_ADDR(1),
> > + .rd_table = &ds44x2_table,
> > + .wr_table = &ds44x2_table,
> > +};
>
> Note that REGCACHE_FLAT will allocate 0xF8 unsigned longs you will never use.
> REGCACHE_MAPLE will probably be much closer to the size of the original value
> cache, for a small look-up performance penalty (but always fast compared to the
> I2C bus).

ACK, already migrated to REGCACHE_MAPLE in the v3:
https://lore.kernel.org/all/20260128153824.3679187-8-o.rempel@pengutronix.de/

Which works mostly fine except of the cache initialisation. If I use
num_reg_defaults_raw with REGCACHE_MAPLE as proposed by Andy
Shevchenko, first access to regmap values over debugfs will explode with
NULL pointer etc...
If I remove num_reg_defaults_raw, I need to read register manually
to init defaul values as implemented in v3.

The REGCACHE_FLAT has one more problem, the cache will be inited with
i2c NACKs (0xFF) if used with num_reg_defaults_raw.

> [...]
>
> > @@ -163,49 +184,52 @@ static int ds4424_write_raw(struct iio_dev *indio_dev,
> >
> >  static int ds4424_verify_chip(struct iio_dev *indio_dev)
> >  {
> > - int ret, val;
> > + struct ds4424_data *data = iio_priv(indio_dev);
> > + u8 raw_values[DS4424_MAX_DAC_CHANNELS];
> > + int ret;
> >
> > - ret = ds4424_get_value(indio_dev, &val, 0);
> > - if (ret < 0)
> > - dev_err(&indio_dev->dev,
> > - "%s failed. ret: %d\n", __func__, ret);
> > + /* Bulk read all channels starting at 0xf8.
> > + * This populates the regmap cache with current HW values.
> > + */
> > + ret = regmap_bulk_read(data->regmap, DS4424_DAC_ADDR(0),
> > +        raw_values, indio_dev->num_channels);
>
> Are you forcing a HW-to-cache sync for performance?

See response above.

> Previously this function would just read a single value to verify a reply was
> sent, instead of seeding the cache in data->raw, so that's a change in behavior
> (and purpose) of this function, meaning you may want to change the name if you
> keep this.
>
> You could (should IMHO) use a sparse cache, which is actually aware of its
> content's validity and will transparently access the device on the first access
> to initialize itself. I would recommend REGCACHE_MAPLE as above, but
> REGCACHE_FLAT_S also works. REGCACHE_FLAT is tricky to properly initialize [1],
> and I would not recommend using it in new code.

Ok, thx!

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |