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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1423082350.3211.1.camel@spandruv-mobl2>
Date:	Wed, 4 Feb 2015 20:39:09 +0000
From:	"Pandruvada, Srinivas" <srinivas.pandruvada@...el.com>
To:	"jic23@...nel.org" <jic23@...nel.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"Tirdea, Irina" <irina.tirdea@...el.com>,
	"viorel.suman@...il.com" <viorel.suman@...il.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,
Srinivas

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