On 28/03/16 17:03, Tirdea, Irina wrote: > > >> -----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? Send a patch against, fixes-togreg, or given timing fixes-togreg-post-rc1 as appropriate but put a comment in there saying it was also fixed in "adfasfaf" so that the merge is obvious when the two hit each other - hopefully in Greg's tree... Jonathan > > 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); >>>>> >>>> >>> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
