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

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

On Fri, Oct 23, 2026 at 04:01:43AM +0100, Jonathan Cameron wrote:
> On Thu, 22 Oct 2026 18:40:32 -0500
> Victor Chernyakin <chernyakin.victor.j@outlook.com> wrote:

...

> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>

> > +#include <linux/bitops.h>
> > +#include <linux/bits.h>

The latter one is covered by the former, so drop bits.h.

+ cleanup.h // guard()()

With this, please follow IWYU principle (see other IIO drivers that has been
submitted in the past couple of years for the style of include section).

> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/math.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/types.h>

...

> > +static const s64 fdc1004_calibbias[] = {
> > +	-0x8000 * 488281250UL, /* min */
> > +	+0x0001 * 488281250UL, /* step */
> > +	+0x7FFF * 488281250UL, /* max */
> > +};

Looking at these data types, can you check if linear_range.h APIs
can be applied instead?

...

> > +struct fdc1004_data {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> 
> Add a comment on what data the lock is actually protecting.
> 
> > +	u8 samp_freq_index;
> > +	u8 offsets[4];

Swap these two, it *might* be that compiler can retrieve all offsets into
32-bit register (due to alignment) at once and optimise the code. You can
check the result with `bloat-o-meter`, btw.

> > +};

...

> > +	return poll_timeout_us(
> > +		{
> > +			ret = i2c_smbus_read_word_swapped(client,
> > +							  FDC1004_REG_FDC_CONF);
> > +			if (ret < 0)
> > +				return ret;
> 
> common convention to add the error condition to the exit one rather than do it
> in here.  Then check ret after the exit from teh poll.
> Tends to give nice code to read.
> 
> > +		},
> > +		ret & FDC1004_FDC_CONF_DONE_n_MASK(meas), USEC_PER_MSEC,

And please split on logical borders, also use 1 * USEC_PER_MSEC.

> > +		40 * USEC_PER_MSEC, false);
> > +}

Yes, and this macro should not be used like this. Your case is read_poll_timeout().

...

> > +static int fdc1004_read_raw(struct iio_dev *indio_dev,
> > +			    const struct iio_chan_spec *chan, int *val,
> > +			    int *val2, long mask)

Logical borders:

static int fdc1004_read_raw(struct iio_dev *indio_dev,
			    const struct iio_chan_spec *chan,
			    int *val, int *val2, long mask)

...

> > +	case IIO_CHAN_INFO_RAW: {
> > +		guard(mutex)(&data->lock);
> > +
> > +		/* Configure the measurement we want. */
> > +		ret = i2c_smbus_write_word_swapped(
> > +			client, FDC1004_REG_CONF_MEASn(0),
> > +			chan->address |
> > +				FIELD_PREP(FDC1004_CONF_MEAS_CAPDAC_MASK,
> > +					   data->offsets[chan->channel]));

Indentation is broken...

> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Start the measurement. */
> > +		ret = i2c_smbus_write_word_swapped(
> > +			client, FDC1004_REG_FDC_CONF,
> > +			FDC1004_FDC_CONF_MEAS_n_MASK(0) |
> > +				/* "+ 1" because the chip wants a 1-indexed value. */
> > +				FIELD_PREP(FDC1004_FDC_CONF_RATE_MASK,
> > +					   data->samp_freq_index + 1));
> 
> For some of these more complex values I'd use a local variable and build them
> up in steps.

...and this is a good advice to fix readability,

> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = fdc1004_wait_until_measurement_is_ready(client, 0);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = fdc1004_read_measurement(client, 0, val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +	}

> > +	case IIO_CHAN_INFO_SAMP_FREQ: {
> > +		guard(mutex)(&data->lock);

Always add a blank line after guard()().

> > +		*val = fdc1004_samp_freqs[data->samp_freq_index];
> > +		return IIO_VAL_INT;
> > +	}

...

> > +		iio_val_s64_decompose(
> > +			sign_extend64(ret, 15) * fdc1004_calibbias[1],

Again, broken indentation.

> > +			val, val2);

...

> > +static int fdc1004_probe(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct fdc1004_data *data;
> > +	int ret;
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> 
> I'd look at using regmap even though you only have a single bus. It provides
> a rich set of helpers and the caching functionality that will save a bunch
> of bus traffic.
> 
> > +		return -EOPNOTSUPP;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +
> > +	ret = devm_mutex_init(&client->dev, &data->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_regulator_get_enable(&client->dev, "vdd");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = fdc1004_soft_reset(client);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->name = "fdc1004";
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &fdc1004_info;
> > +	indio_dev->channels = fdc1004_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(fdc1004_channels);
> > +
> > +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);

Why out of a sudden indio_dev->dev.parent?

> > +}

-- 
With Best Regards,
Andy Shevchenko