RE: [PATCH] iio: gyro: bmg160: optimize i2c transfers in trigger handler

[Tirdea, Irina]

> -----Original Message-----
> From: Pandruvada, Srinivas
> Sent: 04 February, 2015 22:39
> To: jic23@xxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; Tirdea, Irina; viorel.suman@xxxxxxxxx
> Subject: Re: [PATCH] iio: gyro: bmg160: optimize i2c transfers in trigger handler
> 
> On Wed, 2015-02-04 at 17:27 +0000, Jonathan Cameron wrote:
> > On 30/01/15 04:03, Viorel Suman wrote:
> > > Hi,
> > >
> > > You might need more space in "values" buffer, for more details please check the
> > > "iio_push_to_buffers_with_timestamp" description and implementation.
> > Whilst that function is a little 'odd', this patch doesn't change the use
> > of buffer (simply how it if filled).  The buffer is 16bytes long
> > (8 x s16) so should be fine (4 x 16 bit and 1 x 64 bit).
> >
> > More problematically, we have using a new smbus capability here
> > (to read it one transaction).  It's one you can check for, so I'm guessing
> > we aren't guaranteed it is present.
> > I2C_FUNC_SMBUS_READ_I2C_BLOCK
> Good catch. We should add
>   if (i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_READ_I2C_BLOCK) in probe and use this otherwise revert to
> old method.
> 
Thanks for the review, Jonathan and Srinivas!
Will fix this in next version.
> Thanks,
> Srinivas
> 
> >
> > Jonathan
> > >
> > > Regards,
> > > Viorel
> > >
> > >
> > > On Thu, Jan 29, 2015 at 3:15 PM, Irina Tirdea <irina.tirdea@xxxxxxxxx <mailto:irina.tirdea@xxxxxxxxx>> 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 bmg160 driver does
> > >     one i2c transfer for each axis. This has an impact on the frequency
> > >     of the gyroscope at high sample rates due to additional delays
> > >     introduced by the i2c bus at each transfer.
> > >
> > >     Reading all axis values in one i2c transfer reduces the delays
> > >     introduced by the i2c bus.
> > >
> > >     Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx <mailto:irina.tirdea@xxxxxxxxx>>
> > >     Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx <mailto:srinivas.pandruvada@xxxxxxxxxxxxxxx>>
> > >     ---
> > >      drivers/iio/gyro/bmg160.c | 20 +++++++++++---------
> > >      1 file changed, 11 insertions(+), 9 deletions(-)
> > >
> > >     diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
> > >     index 60451b3..814eb20 100644
> > >     --- a/drivers/iio/gyro/bmg160.c
> > >     +++ b/drivers/iio/gyro/bmg160.c
> > >     @@ -115,6 +115,7 @@ enum bmg160_axis {
> > >             AXIS_X,
> > >             AXIS_Y,
> > >             AXIS_Z,
> > >     +       AXIS_MAX,
> > >      };
> > >
> > >      static const struct {
> > >     @@ -820,20 +821,21 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
> > >             struct iio_dev *indio_dev = pf->indio_dev;
> > >             struct bmg160_data *data = iio_priv(indio_dev);
> > >             int bit, ret, i = 0;
> > >     +       s16 values[AXIS_MAX];
> > >
> > >             mutex_lock(&data->mutex);
> > >     -       for_each_set_bit(bit, indio_dev->buffer->scan_mask,
> > >     -                        indio_dev->masklength) {
> > >     -               ret = i2c_smbus_read_word_data(data->client,
> > >     -                                              BMG160_AXIS_TO_REG(bit));
> > >     -               if (ret < 0) {
> > >     -                       mutex_unlock(&data->mutex);
> > >     -                       goto err;
> > >     -               }
> > >     -               data->buffer[i++] = ret;
> > >     +       ret = i2c_smbus_read_i2c_block_data(data->client, BMG160_REG_XOUT_L,
> > >     +                                           sizeof(values), (u8 *) values);
> > >     +       if (ret < 0) {
> > >     +               mutex_unlock(&data->mutex);
> > >     +               goto err;
> > >             }
> > >             mutex_unlock(&data->mutex);
> > >
> > >     +       for_each_set_bit(bit, indio_dev->buffer->scan_mask,
> > >     +                        indio_dev->masklength)
> > >     +               data->buffer[i++] = values[bit];
> > >     +
> > >             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > >                                                data->timestamp);
> > >      err:
> > >     --
> > >     1.9.1
> > >
> > >     --
> > >     To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > >     the body of a message to majordomo@xxxxxxxxxxxxxxx <mailto:majordomo@xxxxxxxxxxxxxxx>
> > >     More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >
> > >
> >

N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå