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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Oct 26, 2026 at 11:51:14AM -0500, Victor Chernyakin wrote:
> The FDC1004 is a high-precision 4-channel capacitive sensor that talks
> over I2C. It can report the values of each of its 4 channels individually,
> as well as the difference between any two of them.
> 
> This initial driver allows performing single reads and configuring
> the sensor's various settings (sample frequency, calibration
> gain/offset, etc.). It could be extended with support for triggered
> buffers, repeated measurement mode, and/or power management.

...

> +static int fdc1004_soft_reset(struct fdc1004_data *data)
> +{
> +	int ret;
> +	unsigned int val;

Try to keep all of them in the reversed xmas tree order.

> +	ret = regmap_write(data->regmap, FDC1004_REG_FDC_CONF,
> +			   FDC1004_FDC_CONF_RST_MASK);

	struct regmap *map = data->regmap;
	...

	ret = regmap_write(map, FDC1004_REG_FDC_CONF, FDC1004_FDC_CONF_RST_MASK);

(yes, it's fine to have 81 characters here)

> +	if (ret)
> +		return ret;
> +
> +	return regmap_read_poll_timeout(data->regmap, FDC1004_REG_FDC_CONF, val,
> +					!(ret & FDC1004_FDC_CONF_RST_MASK),
> +					1 * USEC_PER_MSEC, 200 * USEC_PER_MSEC);
> +}

...

> +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]);

We don't practice this style in IIO.

		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]);

> +		ret = regmap_write(map, FDC1004_REG_CONF_MEASn(0), reg_val);
> +		if (ret)
> +			return ret;
> +
> +		/* Start the measurement. */
> +		reg_val = FDC1004_FDC_CONF_MEAS_n_MASK(0)
> +			/* "+ 1" because the chip wants a 1-indexed value. */

Move comment on top of the assignment.

> +			| FIELD_PREP(FDC1004_FDC_CONF_RATE_MASK,
> +				     data->samp_freq_index + 1);

Same about the style.

> +		ret = regmap_write(map, FDC1004_REG_FDC_CONF, reg_val);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Wait for the measurement to complete.
> +		 * The slowest sampling frequency the chip supports is 100 SPS,
> +		 * i.e. 1 sample every 10 ms, so our timeout can't be shorter
> +		 * than that. Go with 40 ms to be safe.
> +		 */
> +		ret = regmap_read_poll_timeout(map, FDC1004_REG_FDC_CONF, reg_val,
> +					       reg_val & FDC1004_FDC_CONF_DONE_n_MASK(0),
> +					       1 * USEC_PER_MSEC, 40 * USEC_PER_MSEC);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Now read the two registers making up the measurement.
> +		 * The datasheet says to read MSB first, LSB second.
> +		 */
> +		ret = regmap_bulk_read(map, FDC1004_REG_MEASn_MSB(0),
> +				       reg_vals, ARRAY_SIZE(reg_vals));
> +		if (ret)
> +			return ret;
> +
> +		*val = (sign_extend32(reg_vals[0], 15) << 8) | (reg_vals[1] >> 8);

We still have two shift operators, so it might be not a bad idea after all to
have a combined variable with wordswap done in case of LITTLE_ENDIAN
architecture. Jonathan, what do you think about this? Reading the whole set as
32-bit value and convert it if required

		// also note in this case we don't need to have _MSB in the name
		ret = regmap_bulk_read(map, FDC1004_REG_MEASn(0), reg_val, sizeof(reg_val));
		if (ret)
			return ret;

#ifdef __LITTLE_ENDIAN
		reg_val = swahw32(reg_val);
#endif

		*val = sign_extend32(reg_val >> 8, 23);

		// I assume the following is also possible in this case
		// *val = (int)reg_val >> 8;
		// but usually we use sign_extend32() in IIO even when simple
		// casting is okay

This also reduces stack usage.

> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		guard(mutex)(&data->lock);
> +
> +		*val = fdc1004_samp_freqs[data->samp_freq_index];
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_OFFSET: {
> +		guard(mutex)(&data->lock);
> +
> +		*val = data->offsets[chan->channel] * fdc1004_offset[1];
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		ret = regmap_read(map, FDC1004_REG_OFFSET_CAL_CINn(chan->channel),
> +				  &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		iio_val_s64_decompose(sign_extend64(reg_val, 15) *
> +				      fdc1004_calibbias[1], val, val2);
> +		return IIO_VAL_DECIMAL64_FEMTO;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		ret = regmap_read(map, FDC1004_REG_GAIN_CAL_CINn(chan->channel),
> +				  &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		iio_val_s64_decompose(reg_val * fdc1004_calibscale[1], val, val2);
> +		return IIO_VAL_DECIMAL64_FEMTO;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +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]),
> +};

We still haven't decided on this one. I think the 2D array is better for these.
And with the above we effectively have a duplication of the tables.

...

> +#define FDC1004_DIFFERENTIAL_CAPACITANCE_CHANNEL(_chan1, _chan2)         \
> +	{                                                                \
> +		.type = IIO_CAPACITANCE,                                 \
> +		.indexed = true,                                         \
> +		.differential = true,                                    \
> +		.channel = _chan1,                                       \
> +		.channel2 = _chan2,                                      \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),            \
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +		.info_mask_shared_by_all_available =                     \
> +			BIT(IIO_CHAN_INFO_SAMP_FREQ),                    \
> +	}

Please, use tab for indenting the \ slashes (and only spaces at the end if you
want to shorten that a bit).

-- 
With Best Regards,
Andy Shevchenko