Re: [PATCH v5 4/4] iio: cdc: add Texas Instruments FDC1004 driver

[Victor Chernyakin]

> https://sashiko.dev/#/patchset/SN1PR12MB2528805DB17DD015E191111FA5762%40SN1PR12MB2528.namprd12.prod.outlook.com

> > +static const struct regmap_config fdc1004_regmap_config = {
> > +	.name = "fdc1004",
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.val_format_endian = REGMAP_ENDIAN_BIG,
> > +	.max_register = FDC1004_REG_GAIN_CAL_CINn(3),
> > +	.rd_table = &fdc1004_readable_reg_table,
> > +	.wr_table = &fdc1004_writeable_reg_table,
> > +	.volatile_table = &fdc1004_volatile_reg_table,
> > +	.cache_type = REGCACHE_MAPLE,
> > +};
> Should fdc1004_regmap_config set .use_single_read = true?
> On adapters that support I2C_FUNC_I2C, regmap_bulk_read() in
> fdc1004_read_raw() coalesces the read of FDC1004_REG_MEASn_MSB(0) and
> FDC1004_REG_MEASn_LSB(0) into a single 4-byte I2C read at register 0x00,
> whereas the FDC1004 does not auto-increment its register pointer across
> multi-register reads and requires two separate 16-bit reads at 0x00 and
> 0x01.

Hmm, I think that's right. The relevant quote is (section 6.5.2, p. 12):

"When reading from the FDC1004, the last value stored in the pointer by
a write operation is used to determine which register is read by a read
operation. To change the pointer register for a read operation, a new value
must be written to the pointer."

> > +static int fdc1004_soft_reset(struct fdc1004_data *data)
> > +{
> > +	struct regmap *map = data->regmap;
> > +	unsigned int reg_val;
> > +	int ret;
> > +
> > +	ret = regmap_write(map, FDC1004_REG_FDC_CONF, FDC1004_FDC_CONF_RST_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_read_poll_timeout(map, FDC1004_REG_FDC_CONF, reg_val,
> > +					!(ret & FDC1004_FDC_CONF_RST_MASK),
> > +					1 * USEC_PER_MSEC, 200 * USEC_PER_MSEC);
> > +}
> Should the poll condition in fdc1004_soft_reset() check reg_val instead of
> ret?  At this point ret is 0, so regmap_read_poll_timeout() succeeds on the
> first read without waiting for FDC1004_FDC_CONF_RST_MASK to clear.

Yikes, that's a good catch. I broke this between v1 and v2.

> > +
> > +static int fdc1004_read_raw(struct iio_dev *indio_dev,
> > +			    const struct iio_chan_spec *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct fdc1004_data *data = iio_priv(indio_dev);
> > +	struct regmap *map = data->regmap;
> > +	unsigned int reg_val;
> > +	u16 reg_vals[2];
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		iio_val_s64_decompose(fdc1004_scale, val, val2);
> > +		return IIO_VAL_DECIMAL64_YOCTO;
> > +	case IIO_CHAN_INFO_RAW: {
> > +		guard(mutex)(&data->lock);
> > +
> > +		/* Configure the measurement we want. */
> > +		reg_val = FIELD_PREP(FDC1004_CONF_MEAS_CHA_MASK, chan->channel) |
> > +			  FIELD_PREP(FDC1004_CONF_MEAS_CHB_MASK, chan->channel2) |
> > +			  FIELD_PREP(FDC1004_CONF_MEAS_CAPDAC_MASK,
> > +				     data->offsets[chan->channel]);
> Should FDC1004_CONF_MEAS_CAPDAC_MASK be left at 0 when reading a
> differential channel?
> Differential channels do not use CAPDAC (chan->channel2 is a physical
> channel rather than FDC1004_CONF_MEAS_CHB_CAPDAC) and do not expose
> IIO_CHAN_INFO_OFFSET, but this unconditionally writes the single-ended
> offset from data->offsets[chan->channel] into the CAPDAC field, which the
> datasheet specifies must be 0b00000 for differential measurements.

The datasheet does not specify that. Section 6.5.3.1 (p. 14) says:
"Note that the CAPDAC setting has no effect for a differential measurement."

> [ ... ]
> > +static const int fdc1004_calibbias_decomposed[] = {
> > +	lower_32_bits(fdc1004_calibbias[0]), upper_32_bits(fdc1004_calibbias[0]),
> > +	lower_32_bits(fdc1004_calibbias[1]), upper_32_bits(fdc1004_calibbias[1]),
> > +	lower_32_bits(fdc1004_calibbias[2]), upper_32_bits(fdc1004_calibbias[2]),
> > +};
> > +
> > +static const int fdc1004_calibscale_decomposed[] = {
> > +	lower_32_bits(fdc1004_calibscale[0]), upper_32_bits(fdc1004_calibscale[0]),
> > +	lower_32_bits(fdc1004_calibscale[1]), upper_32_bits(fdc1004_calibscale[1]),
> > +	lower_32_bits(fdc1004_calibscale[2]), upper_32_bits(fdc1004_calibscale[2]),
> > +};
> Does initializing fdc1004_calibbias_decomposed[] and
> fdc1004_calibscale_decomposed[] from elements of const arrays break Clang
> builds?
> In C11/GNU11 mode, subscripting a const array such as fdc1004_calibbias[]
> is not an integer constant expression, so compiling with Clang (make
> LLVM=1) fails with "initializer element is not a compile-time constant".

Clang compiles this just fine as long as both arrays are const:
https://godbolt.org/z/3Kn3T8fq5

> > +static int fdc1004_write_raw(struct iio_dev *indio_dev,
> > +			     const struct iio_chan_spec *chan,
> > +			     int val, int val2, long mask)
> > +{
> [ ... ]
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		val64 = iio_val_s64_compose(val, val2);
> > +		if (val64 < fdc1004_calibbias[0] || val64 > fdc1004_calibbias[2])
> > +			return -EINVAL;
> > +
> > +		return regmap_write(map, FDC1004_REG_OFFSET_CAL_CINn(chan->channel),
> > +				    DIV_S64_ROUND_CLOSEST(val64, fdc1004_calibbias[1]));
> Does passing a negative DIV_S64_ROUND_CLOSEST() result directly to
> regmap_write() fail when writing a negative calibration bias?
> When val64 is negative, the s64 quotient is implicitly converted to an
> unsigned int in 0xffff8000..0xffffffff rather than a 16-bit value in
> 0x8000..0xffff.  _regmap_write() caches that 32-bit value in
> REGCACHE_MAPLE, and on SMBus word adapters regmap_smbus_word_write_swapped()
> rejects values greater than 0xffff with -EINVAL:
> drivers/base/regmap/regmap-i2c.c:regmap_smbus_word_write_swapped() {
> 	if (val > 0xffff || reg > 0xff)
> 		return -EINVAL;
>     ...
> }
> Casting the quotient to u16 before passing it to regmap_write() avoids
> both issues.

Negative values work with my setup, but that may be because my adapter
doesn't use that code path. I've added the cast just in case.