Re: [PATCH v2 3/3] iio: cdc: add Texas Instruments FDC1004 driver

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

On Sun, Oct 25, 2026 at 12:48:06AM -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.

...

> +/*
> + * OFFSET_CAL_CINn: 16 bits, signed. Format: IIO_VAL_DECIMAL64_FEMTO.
> + * 1 step = 1/(2^11) pF = 1/(1000 * 2^11) nF = 0.000000488281250 nF.
> + */
> +static const s64 fdc1004_calibbias[] = {
> +	-0x8000 * 488281250UL, /* min */
> +	+0x0001 * 488281250UL, /* step */
> +	+0x7FFF * 488281250UL, /* max */
> +};

Can you explain the negative capacitance meaning? Does it mean some vector
(direction) in the physical world?

...

> +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);
> +	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);
> +		reg_val |= FIELD_PREP(FDC1004_CONF_MEAS_CHB_MASK, chan->channel2);
> +		reg_val |= FIELD_PREP(FDC1004_CONF_MEAS_CAPDAC_MASK,
> +				      data->offsets[chan->channel]);
> +		ret = regmap_write(data->regmap, FDC1004_REG_CONF_MEASn(0), reg_val);

You can make a lot of lines to fit 80 characters limit with a simple trick,
id est add a local variable.

	struct regmap *map = data->regmap;
	...
		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. */
> +		reg_val |= FIELD_PREP(FDC1004_FDC_CONF_RATE_MASK,
> +				      data->samp_freq_index + 1);

Make it a single assignment (just multi-line).
Same for the rest of the similar cases.

> +		ret = regmap_write(data->regmap, 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(
> +			data->regmap, FDC1004_REG_FDC_CONF, reg_val,
> +			reg_val & FDC1004_FDC_CONF_DONE_n_MASK(0),
> +			1 * USEC_PER_MSEC, 40 * USEC_PER_MSEC);

The indentation is broken.

> +		if (ret)
> +			return ret;
> +
> +		/* The datasheet says to read MSB first, LSB second. */
> +		ret = regmap_bulk_read(data->regmap, 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);


So, why the value is not marked as __be32 with the respective conversion?

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

Why do we need these tables while already having them just in different
format? Perhaps you can always keep the decomposed variant and otherwise
join them at run-time?

-- 
With Best Regards,
Andy Shevchenko