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

[Oleksij Rempel <o.rempel@pengutronix.de>]

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 |