AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

["Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com>]

Hi Jonathan,

"available_scan_masks" works not as expected.  We test it using kernel version v6.16. see the test result inline.

Best Regards 
Jianping 

>> +
>> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct smi330_data *data = iio_priv(indio_dev);
>> +	int ret, chan;
>> +	int i = 0;
>> +
>> +	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data-
>>buf,
>> +			       ARRAY_SIZE(smi330_channels));
>> +	if (ret)
>> +		goto out;
>> +
>> +	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
>> +		iio_for_each_active_channel(indio_dev, chan)
>> +			data->buf[i++] = data->buf[chan];
>
>If I follow this correctly you are reading all the channels and just copying out the
>ones you want.  Just let the IIO core do that for you by setting iio_dev-
>>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the whole
>buffer every time.

For the most frequent use cases, we define available_scan_masks = { SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK, SMI330_GYRO_XYZ_MSK, 0 }; and push the whole buffer every time.
From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z. Then we enable buffer and expect that only the gyro values and timestamp in iio_buffer. Nevertheless, we have 3 accelerometer values and the timestamp in iio_buffer.
It seems that the iio core does not take care which channel is enabled,  just copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code still needs to take care and just copy the enabled channel value to buffer.

Another side effect after using available_scan_masks is that the active_scan_masks sometimes does not reflect current channel activation status.

Is some step missing to properly use available_scan_masks ?  How can a user find out from user space which channel combination is defined in available_scan_masks ?

>
>The handling the core code is reasonably sophisticated and will use bulk
>copying where appropriate.
>
>If there is a strong reason to not use that, add a comment here so we don't
>have anyone 'fix' this code in future.
>
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
>> +pf->timestamp);
>> +
>> +out:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}