LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] iio: gyro: bmg160: optimize i2c transfers in trigger handler
@ 2015-01-29 13:15 Irina Tirdea
       [not found] ` <CAAH5R_OYTbY3AW-pA3-p4+Gzo3MdGYX0a52UtJTbt=F8HSOj0A@mail.gmail.com>
  0 siblings, 1 reply; 4+ messages in thread
From: Irina Tirdea @ 2015-01-29 13:15 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: linux-kernel, Srinivas Pandruvada, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iio: gyro: bmg160: optimize i2c transfers in trigger handler
       [not found] ` <CAAH5R_OYTbY3AW-pA3-p4+Gzo3MdGYX0a52UtJTbt=F8HSOj0A@mail.gmail.com>
@ 2015-02-04 17:27   ` Jonathan Cameron
  2015-02-04 20:39     ` Pandruvada, Srinivas
  0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2015-02-04 17:27 UTC (permalink / raw)
  To: Viorel Suman, Irina Tirdea; +Cc: linux-iio, linux-kernel, Srinivas Pandruvada

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iio: gyro: bmg160: optimize i2c transfers in trigger handler
  2015-02-04 17:27   ` Jonathan Cameron
@ 2015-02-04 20:39     ` Pandruvada, Srinivas
  2015-02-16 17:58       ` Tirdea, Irina
  0 siblings, 1 reply; 4+ messages in thread
From: Pandruvada, Srinivas @ 2015-02-04 20:39 UTC (permalink / raw)
  To: jic23; +Cc: linux-kernel, linux-iio, Tirdea, Irina, viorel.suman

[-- 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¥

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] iio: gyro: bmg160: optimize i2c transfers in trigger handler
  2015-02-04 20:39     ` Pandruvada, Srinivas
@ 2015-02-16 17:58       ` Tirdea, Irina
  0 siblings, 0 replies; 4+ messages in thread
From: Tirdea, Irina @ 2015-02-16 17:58 UTC (permalink / raw)
  To: Pandruvada, Srinivas, jic23; +Cc: linux-kernel, linux-iio, viorel.suman

[-- 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¥

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-02-16 17:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-29 13:15 [PATCH] iio: gyro: bmg160: optimize i2c transfers in trigger handler Irina Tirdea
     [not found] ` <CAAH5R_OYTbY3AW-pA3-p4+Gzo3MdGYX0a52UtJTbt=F8HSOj0A@mail.gmail.com>
2015-02-04 17:27   ` Jonathan Cameron
2015-02-04 20:39     ` Pandruvada, Srinivas
2015-02-16 17:58       ` Tirdea, Irina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).