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

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

[Irina Tirdea]

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@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 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

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

[Jonathan Cameron]

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 
 
Jonathan
> 
> Regards,
> Viorel
> 
> 
> On Thu, Jan 29, 2015 at 3:15 PM, Irina Tirdea <irina.tirdea@intel.com <mailto:irina.tirdea@intel.com>> 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@intel.com <mailto:irina.tirdea@intel.com>>
>     Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com <mailto:srinivas.pandruvada@linux.intel.com>>
>     ---
>      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@vger.kernel.org <mailto:majordomo@vger.kernel.org>
>     More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

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

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4369 bytes --]

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,
Srinivas

>  
> Jonathan
> > 
> > Regards,
> > Viorel
> > 
> > 
> > On Thu, Jan 29, 2015 at 3:15 PM, Irina Tirdea <irina.tirdea@intel.com <mailto:irina.tirdea@intel.com>> 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@intel.com <mailto:irina.tirdea@intel.com>>
> >     Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com <mailto:srinivas.pandruvada@linux.intel.com>>
> >     ---
> >      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@vger.kernel.org <mailto:majordomo@vger.kernel.org>
> >     More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Tirdea, Irina]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4933 bytes --]



> -----Original Message-----
> From: Pandruvada, Srinivas
> Sent: 04 February, 2015 22:39
> To: jic23@kernel.org
> Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; Tirdea, Irina; viorel.suman@gmail.com
> 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@intel.com <mailto:irina.tirdea@intel.com>> 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@intel.com <mailto:irina.tirdea@intel.com>>
> > >     Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com <mailto:srinivas.pandruvada@linux.intel.com>>
> > >     ---
> > >      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@vger.kernel.org <mailto:majordomo@vger.kernel.org>
> > >     More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >
> > >
> >

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥