RE: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Tirdea, Irina]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@xxxxxxxxxx]
> Sent: 28 March, 2016 17:15
> To: Peter Meerwald-Stadler
> Cc: Tirdea, Irina; linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Hartmut Knaack; Lars-Peter Clausen; Purdila, Octavian;
> Markus Pargmann; Pandruvada, Srinivas
> Subject: Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler
> 
> On 28/03/16 11:09, Peter Meerwald-Stadler wrote:
> >
> >>> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> >>> enable/disable the bus at each i2c transfer and must wait for
> >>> the enable/disable to happen before sending the data.
> >>>
> >>> When reading data in the trigger handler, the bmc150 accel driver does
> >
> > should refer to bmg160
> Good spot.  It's also a gyroscope, not an accelerometer... I just fixed this
> up and repushed out testing.
> >
Oops... too much copy-paste :)

> >>> one bus transfer for each axis. This has an impact on the frequency
> >>> of the accelerometer at high sample rates due to additional delays
> >>> introduced by the bus at each transfer.
> >>>
> >>> Reading all axis values in one bus transfer reduces the delays
> >>> introduced by the bus.
> >>>
> >>> Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx>
> >> I forgot to highlight on the earlier driver that there is also 'technically'
> >> a bit of an ABI change here because we are now exporting as LE rather than CPU
> >> order.  However, I 'hope' anyone actually accessing the buffered data is either
> >> doing it through a nice library or hasn't hacked the endian unwinding out of
> >> the generic_buffer example!
> >
> > the patch takes away the possibility to do buffered reads on individual
> > channels (not sure if this is useful per se)
> >
> > this optimizes for the common case, ok;
> >
> > wondering if adding
> > .endianness = IIO_LE
> > is actually an unrelated fix
> Good point, when I first read the code I assumed we were moving from an i2c_word
> read to a bulk read, thus necessitating this addition.  However, we aren't as it
> was previously as an i2c_bulk read of 2 bytes...
> 
> Irina, could you confirm if this was broken before your patches?
> 

Peter is right. I also had in mind the change from i2c_word to bulk read, but
the regmap API has changed this in the meantime.

Since the driver uses regmap_bulk_read to read 2 bytes for each
axis, the data read will have the endianness of the device (little endian)
and we should do endianness conversion or else it will not work on big
endian platforms.

> I'll leave this as is, perhaps we need an additional fix patch specifying LE to
> put out as a fix.

There is one more place in both bmc150 and bmg160 drivers where 
regmap_bulk_read is used without endianness conversion (when reading raw axes).
I will send a separate patch to fix all endianness issues.

While looking at the existent code, I also found another bug in the bmg160 driver 
that this patch fixes as a side effect: the error code returned by regmap_bulk_read
is saved in the data->buffer instead of the value read. Not sure how to handle this,
since it is fixed now by this patch. Jonathan, should I send a fix patch for this? 

Thanks,
Irina

> >
> >> Again, fingers crossed this doesn't break anything significant.
> >>
> >> Applied,
> >>
> >> Thanks,
> >>
> >> Jonathan
> >>> ---
> >>>  drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
> >>>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> >>> index 8d6e5b1..43570b8 100644
> >>> --- a/drivers/iio/gyro/bmg160_core.c
> >>> +++ b/drivers/iio/gyro/bmg160_core.c
> >>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
> >>>  		.sign = 's',						\
> >>>  		.realbits = 16,					\
> >>>  		.storagebits = 16,					\
> >>> +		.endianness = IIO_LE,					\
> >>>  	},								\
> >>>  	.event_spec = &bmg160_event,					\
> >>>  	.num_event_specs = 1						\
> >>> @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
> >>>  	struct iio_poll_func *pf = p;
> >>>  	struct iio_dev *indio_dev = pf->indio_dev;
> >>>  	struct bmg160_data *data = iio_priv(indio_dev);
> >>> -	int bit, ret, i = 0;
> >>> -	unsigned int val;
> >>> +	int ret;
> >>>
> >>>  	mutex_lock(&data->mutex);
> >>> -	for (bit = 0; bit < AXIS_MAX; bit++) {
> >>> -		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
> >>> -				       &val, 2);
> >>> -		if (ret < 0) {
> >>> -			mutex_unlock(&data->mutex);
> >>> -			goto err;
> >>> -		}
> >>> -		data->buffer[i++] = ret;
> >>> -	}
> >>> +	ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
> >>> +			       data->buffer, AXIS_MAX * 2);
> >>>  	mutex_unlock(&data->mutex);
> >>> +	if (ret < 0)
> >>> +		goto err;
> >>>
> >>>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >>>  					   pf->timestamp);
> >>>
> >>
> >