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

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

On Thu, Oct 22, 2026 at 08:36:52PM +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.

...

>   * TODO:
> - * 1. Make the drive mode selectable form userspace
> - * 2. Add support for interrupts
> - * 3. Adjust time to wait for data to be ready based on selected operation mode
> - * 4. Read error register and put the information in logs
> + * 1. Add support for interrupts
> + * 2. Adjust time to wait for data to be ready based on selected operation mode
> + * 3. Read error register and put the information in logs

I would just change the 1st line as

 * 1. DONE. Was: "Make the drive mode selectable form userspace"

And renumber / kill when it's close to complete.

>   */

...

> +static int ccs811_set_sampling_freq(struct ccs811_data *data,
> +				    int val, int val2)
> +{
> +	u8 drive_mode;
> +	int ret;
> +
> +	if (val == 1 && val2 == 0)
> +		drive_mode = CCS811_MODE_IAQ_1SEC;
> +	else if (val == 0 && val2 == 100000)
> +		drive_mode = CCS811_MODE_IAQ_10SEC;
> +	else if (val == 0 && val2 == 16667)
> +		drive_mode = CCS811_MODE_IAQ_60SEC;
> +	else
> +		return -EINVAL;

You have an array, use it instead of duplicating values here.


> +	guard(mutex)(&data->lock);

Always make sure the guard()() is put standalone (id est surrounded by
blank lines).

> +	ccs811_set_wakeup(data, true);
> +
> +	ret = i2c_smbus_read_byte_data(data->client, CCS811_MEAS_MODE);
> +	if (ret < 0)
> +		goto out;

> +	ret &= ~CCS811_MODE_MASK;
> +	ret |= drive_mode;
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					CCS811_MEAS_MODE, ret);

A single line, and the ret = foo(ret) is not good style, add a temporary
variable with good name and use it.

	u8 mode;
	...
	mode = (ret & _MASK) | drive_mode;
	ret = _write_byte_data(mode);

> +	if (ret < 0)
> +		goto out;
> +
> +	data->drive_mode = drive_mode;
> +	ret = 0;
> +
> +out:

Instead, make a wrapper that does this wakeup calls, guard()(), bu calls the
body only once, so you will have here

	ccs811_set_wakeup(data, true);
	ret = my_new_kool_body(...);

> +	ccs811_set_wakeup(data, false);
> +	return ret;
> +}

...

> +static int ccs811_get_sampling_freq(struct ccs811_data *data,
> +				    int *val, int *val2)
> +{
> +	guard(mutex)(&data->lock);
> +
> +	switch (data->drive_mode) {
> +	case CCS811_MODE_IAQ_1SEC:
> +		*val = 1;
> +		*val2 = 0;
> +		break;
> +	case CCS811_MODE_IAQ_10SEC:
> +		*val = 0;
> +		*val2 = 100000;
> +		break;
> +	case CCS811_MODE_IAQ_60SEC:
> +		*val = 0;
> +		*val2 = 16667;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

> +	return IIO_VAL_INT_PLUS_MICRO;

Replace break:s with this return.
Also use an array you have.

> +}

-- 
With Best Regards,
Andy Shevchenko