Message-ID: <20250628175921.1db14102@jic23-huawei>
Date: Sat, 28 Jun 2025 17:59:21 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@...bosch.com>
Cc: "lars@...afoo.de" <lars@...afoo.de>, "robh@...nel.org"
 <robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>, "dima.fedrau@...il.com"
 <dima.fedrau@...il.com>, "marcelo.schmitt1@...il.com"
 <marcelo.schmitt1@...il.com>, "linux-iio@...r.kernel.org"
 <linux-iio@...r.kernel.org>, "devicetree@...r.kernel.org"
 <devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "Lorenz Christian (ME-SE/EAD2)"
 <Christian.Lorenz3@...bosch.com>, "Frauendorf Ulrike (ME/PJ-SW3)"
 <Ulrike.Frauendorf@...bosch.com>, "Dolde Kai (ME-SE/PAE-A3)"
 <Kai.Dolde@...bosch.com>
Subject: Re: [PATCH v2 2/2] iio: imu: smi330: Add driver

On Thu, 26 Jun 2025 09:31:19 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@...bosch.com> wrote:

> 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. 

Fair enough.  Would have been good to put the compiler warning here.
I'd failed to realise you were intialising a pointer not a fixed
sized array.


> 
> 
> >> +
> >> +	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?

When you say resorting I'm not totally sure what you mean. If it's simply
removing entries we don't wan then provide available_scan_masks and the
IIO core code does it all for you.  If fewer elements are required
by the consumer (which may be userspace) then it repacks the data to include
only what you'd expect.
> 
> >
> >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.

The trade off is that in many case a loop of regmap_read() calls is much less
efficient than regmap_bulk_read() even though it may result in less data on the bus.

So the question then becomes how common is it for people to have
1 channel, 2 channels,  n - 1 channels, n channels.

It is common that reading n - 1 in a loop is slower than reading
n and dropping one + data mangling to realign everything after the
missing channel.  Normally we assume that if someone bought a fancy
device they probably want most of it's channels and optimise for that
rather than a small subset of channels.

The optimisation you have for all channels made me think that if that
was worth doing the overheads of separate reads must be significant!

> 
> >  
> >> +			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.

So what you are doing is preventing tearing, not a wrong value in the sense of
getting the next time stamp.

It's fairly unusual for a complex device to not have some form of explicit ack that stops us getting
a new interrupt until we've handled the last one (IRQF_ONESHOT can be appropriate as well).
I think the issue here is that the second interrupt might have nothing to do with data ready (which I'd
hope does need some form of clear) but we grab a timestamp anyway.  Rather than run the risk of a
very wrong timestamp it might make more sense to just grab a less accurate timestamp in
the threaded handler.

Jonathan

Powered by blists - more mailing lists