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

[Nikhil Gautam <nikhilgtr@gmail.com>]

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