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

[Jonathan Cameron]

On Wed,  4 Nov 2026 20:41:38 +0530
Nikhil Gautam <nikhilgtr@xxxxxxxxx> 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.
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@xxxxxxxxx>
A few trivial things + a note on why you are seeing those build failures.

If it wasn't for that scope issue I might have applied this version
and possibly tidied up the trivial stuff.

There is also one more significant suggestion inline if you want
to take it on.  Up to you though!

Jonathan

> @@ -163,16 +185,21 @@ static int ccs811_start_sensor_application(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static int ccs811_setup(struct i2c_client *client)
> +static int ccs811_setup(struct ccs811_data *data)
>  {
>  	int ret;
>  
> -	ret = ccs811_start_sensor_application(client);
> +	ret = ccs811_start_sensor_application(data->client);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, CCS811_MEAS_MODE,
> +					CCS811_MODE_IAQ_1SEC);
>  	if (ret < 0)
>  		return ret;
>  
> -	return i2c_smbus_write_byte_data(client, CCS811_MEAS_MODE,
> -					 CCS811_MODE_IAQ_1SEC);
> +	data->drive_mode = CCS811_MODE_IAQ_1SEC;

Trivial but a blank line here would be a tiny readability 
improvement.  Lets the eye immediately spot the 'good' return path.

> +	return 0;
>  }

> +
> +static int ccs811_set_sampling_freq(struct ccs811_data *data,
> +				    int val, int val2)
> +{
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ccs811_set_wakeup(data, true);
> +	ret = ccs811_update_sampling_freq(data, val, val2);
> +	ccs811_set_wakeup(data, false);

If you are feeling adventurous and want a follow up, it might
be worth thinking about defining a guard for ccs811_set_wakeup.

From a quick glance it looks to me like it would simplify some
existing paths in the driver as well.  I suspect it doesn't need
to be as tightly scoped as it is in probe() today and can wait
for the end of that function.  To prevent races it might be necessary
to take data->lock in probe though - I haven't walked through exactly
what that is protecting but as wakeup isn't reference counted it
seems likely that is part of the protected state.

Anyhow, entirely optional - I just noticed it because it would have
saved a few lines of code here.

> +
> +	return ret;
> +}

>  static int ccs811_read_info_raw(struct ccs811_data *data,
>  				struct iio_chan_spec const *chan,
>  				int *val, int mask)
> @@ -249,6 +343,24 @@ static int ccs811_read_info_raw(struct ccs811_data *data,
>  	}
>  }
>  
> +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);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:

The build warnings are because this doesn't have scope defined
around the declaration of the variable cleanup that is
the magic behind these.
	case IIO_CHAN_INFO_SAMP_FREQ: {
		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
		if (IIO_DEV_ACQUIRE_FAILED(claim))
			return -EBUSY;

		return ccs811_set_sampling_freq(data, val, val2);
	}
	default:

should fix that up.

> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		return ccs811_set_sampling_freq(data, val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int ccs811_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan,
> +				    long mask)
> +{
> +	if (mask == IIO_CHAN_INFO_SAMP_FREQ)
> +		return IIO_VAL_INT_PLUS_MICRO;

This is the the default, so should be no need to provide get_fmt.

> +
> +	return -EINVAL;
> +}

>  		return ret;