Re: [PATCH v4] iio: chemical: ccs811: Make sampling frequency configurable

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

On Sun, Nov 01, 2026 at 11:10:19AM +0530, Nikhil Gautam wrote:
> Allow userspace to configure the CCS811 IAQ measurement rate through
> the IIO sampling_frequency attribute.
> 
> Support the three IAQ measurement modes with sampling frequencies of
> 1 Hz, 0.1 Hz, and approximately 0.016667 Hz, corresponding to
> measurement intervals of 1, 10, and 60 seconds.
> 
> Preserve unrelated bits in the MEAS_MODE register when updating the
> drive mode, and cache the selected mode after a successful register
> write. Expose the supported frequencies through
> sampling_frequency_available.

...

> +static const int ccs811_samp_freq[][2] = {
> +	{ 1, 0 },
> +	{ 0, 100000 },
> +	{ 0, 16667 },
> +};

Now it would be better to have a common enum to index above and below.
So it will be kept in sync

enum {
	CCS811_MODE_IAQ_1SEC_IDX,
	CCS811_MODE_IAQ_10SEC_IDX,
	CCS811_MODE_IAQ_60SEC_IDX,
};

static const int ccs811_samp_freq[][2] = {
	[CCS811_MODE_IAQ_1SEC_IDX] = { 1, 0 },
	...
};

> +/* Drive mode for each entry of ccs811_samp_freq[], same order */
> +static const u8 ccs811_drive_mode[] = {
> +	CCS811_MODE_IAQ_1SEC,
> +	CCS811_MODE_IAQ_10SEC,
> +	CCS811_MODE_IAQ_60SEC,
> +};

...and here in similar way.

...

> +static int ccs811_update_sampling_freq(struct ccs811_data *data,
> +				       int val, int val2)
> +{
> +	u8 drive_mode;
> +	u8 mode;
> +	int i, ret;

Why is 'i' signed? Also keep reversed xmas tree order.

> +	for (i = 0; i < ARRAY_SIZE(ccs811_samp_freq); i++) {
> +		if (val == ccs811_samp_freq[i][0] &&
> +		    val2 == ccs811_samp_freq[i][1])
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(ccs811_samp_freq))
> +		return -EINVAL;
> +
> +	drive_mode = ccs811_drive_mode[i];


With

	struct i2c_client *client = data->client;

> +	ret = i2c_smbus_read_byte_data(data->client, CCS811_MEAS_MODE);

	ret = i2c_smbus_read_byte_data(client, CCS811_MEAS_MODE);

> +	if (ret < 0)
> +		return ret;
> +
> +	mode = (ret & ~CCS811_MODE_MASK) | drive_mode;

> +	ret = i2c_smbus_write_byte_data(data->client, CCS811_MEAS_MODE,
> +					mode);

	ret = i2c_smbus_write_byte_data(client, CCS811_MEAS_MODE, mode);

> +	if (ret < 0)
> +		return ret;
> +
> +	data->drive_mode = drive_mode;
> +
> +	return 0;
> +}

...

> +static int ccs811_get_sampling_freq(struct ccs811_data *data,
> +				    int *val, int *val2)
> +{
> +	int i;
> +
> +	guard(mutex)(&data->lock);

> +	for (i = 0; i < ARRAY_SIZE(ccs811_drive_mode); i++) {

	for (unsigned int i = 0; i < ARRAY_SIZE(ccs811_drive_mode); i++) {

> +		if (ccs811_drive_mode[i] != data->drive_mode)
> +			continue;
> +
> +		*val = ccs811_samp_freq[i][0];
> +		*val2 = ccs811_samp_freq[i][1];
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +
> +	return -EINVAL;
> +}

...

> +static int ccs811_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ccs811_data *data = iio_priv(indio_dev);
> +	int ret;

Why? I don't see any followup, without which this variable is redundant.

> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		ret = ccs811_set_sampling_freq(data, val, val2);
> +
> +		return ret;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko