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

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

Hi Jonathan,

most findings are already fixed. For 3 findings we still have something unclear. It will be nice if you can give us little bit more indication.

>> +
>> +static const struct iio_chan_spec smi330_channels[] = {
>> +	SMI330_ACCEL_CHANNEL(IIO_ACCEL, X, SMI330_SCAN_ACCEL_X),
>> +	SMI330_ACCEL_CHANNEL(IIO_ACCEL, Y, SMI330_SCAN_ACCEL_Y),
>> +	SMI330_ACCEL_CHANNEL(IIO_ACCEL, Z, SMI330_SCAN_ACCEL_Z),
>> +	SMI330_GYRO_CHANNEL(IIO_ANGL_VEL, X, SMI330_SCAN_GYRO_X),
>> +	SMI330_GYRO_CHANNEL(IIO_ANGL_VEL, Y, SMI330_SCAN_GYRO_Y),
>> +	SMI330_GYRO_CHANNEL(IIO_ANGL_VEL, Z, SMI330_SCAN_GYRO_Z),
>> +	SMI330_TEMP_CHANNEL(SMI330_TEMP_OBJECT),
>> +	IIO_CHAN_SOFT_TIMESTAMP(SMI330_SCAN_TIMESTAMP),
>> +};
>> +
>> +static const struct smi330_sysfs_attr smi330_accel_scale_attr = {
>> +	.reg_vals = (int[]){ SMI330_ACCEL_RANGE_2G,
>SMI330_ACCEL_RANGE_4G,
>> +			     SMI330_ACCEL_RANGE_8G,
>SMI330_ACCEL_RANGE_16G },
>
>Do we need the (int[]) part here?

Remove it will lead to a compiler warning. 


>> +
>> +	if (*indio_dev->active_scan_mask == SMI330_ALL_CHAN_MSK) {
>> +		ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG,
>> +				       data->buf, ARRAY_SIZE(smi330_channels));
>> +		if (ret)
>> +			goto out;
>> +	} else {
>> +		iio_for_each_active_channel(indio_dev, chan) {
>> +			ret = regmap_read(data->regmap,
>> +					  SMI330_ACCEL_X_REG + chan, &sample);
>
>Given there is always a trade off between the efficiency of a bulk read and reading
>just the channels enabled, we often just set available_scan_masks, read the lot and
>let the IIO core worry about resorting the data as needed.

Is any IIO API available to take care the resorting data?

>
>For example, if you want all but 1 channel that is almost certainly quicker and that is
>more likely to be the common case than you want 1 channel only using the buffer.
>
>So I'd like some more explanation here on why this path is needed.

We try to read the data just for activated channel and put them in buffer. If just one channel activated, reading data for all channels leads to unnecessary data transfer on bus.

>
>> +			if (ret)
>> +				goto out;
>> +			data->buf[i++] = sample;
>> +		}
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
>> +					   data->current_timestamp);
>> +
>> +out:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}


>> +static irqreturn_t smi330_irq_handler(int irq, void *p) {
>> +	struct iio_dev *indio_dev = p;
>> +	struct smi330_data *data = iio_priv(indio_dev);
>> +
>> +	atomic64_set(&data->irq_timestamp, iio_get_time_ns(indio_dev));
>
>Why do you need the atomic here? If there is a top half / thread race then you could
>get garbage data anyway.

The atomic here is used to prevent simultaneous read (in bottom half) / write (in top half) to the "data->irq_timestamp"
This happens if the next interrupt comes too early when the current bottom half is still reading "data->irq_timestamp". 
The next top half will interrupt the execution of current bottom half and change the "data->irq_timestamp". 
Finally, the current bottom half get a wrong value.