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

[Nikhil Gautam]

On 03-11-2026 04:52 pm, Andy Shevchenko wrote:
> On Sun, Nov 01, 2026 at 11:10:19AM +0530, Nikhil Gautam wrote:

Hi Andy,
Thanks for the quick review.

> > +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.
>
> ...
Agreed, a shared index enum is cleaner than relying on "same order".
Will add it and use designated initializers in both tables.

> > +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.
No good reason, Will make it unsigned int and fix the declaration order.

> 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);
Ack, will add the local client pointer and use it in both calls.

> > +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++) {
Ack, will use a loop-scoped unsigned int.

> > +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.
Leftover from the earlier claim/release version.
Will drop it and return ccs811_set_sampling_freq() directly.

Will update comments on next version.


Regards,
Nikhil