diff mbox

[v5,6/8] iio: gyro: bmg160: optimize i2c transfers in trigger handler

Message ID 1439389900-10108-7-git-send-email-irina.tirdea@intel.com
State Not Applicable
Headers show

Commit Message

Irina Tirdea Aug. 12, 2015, 2:31 p.m. UTC 
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. Uses i2c_smbus_read_i2c_block_data_or_emulated
that will fallback to reading each axis as a separate word in case i2c
block read is not supported.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/gyro/bmg160.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Jonathan Cameron Aug. 16, 2015, 9:24 a.m. UTC | #1 
On 12/08/15 15:31, Irina Tirdea 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. Uses i2c_smbus_read_i2c_block_data_or_emulated
> that will fallback to reading each axis as a separate word in case i2c
> block read is not supported.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Note, that in the meantime the bmg160 driver just went all regmap
on us (as part of adding SPI support - though that step hasn't
happened yet).  Hence we'll need a means of telling regmap about this
possibility.

> ---
>  drivers/iio/gyro/bmg160.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
> index b2a6ccb..1ff306d 100644
> --- a/drivers/iio/gyro/bmg160.c
> +++ b/drivers/iio/gyro/bmg160.c
> @@ -772,6 +772,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						\
> @@ -809,19 +810,16 @@ 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;
> +	int ret = 0;
>  
>  	mutex_lock(&data->mutex);
> -	for (bit = 0; bit < AXIS_MAX; bit++) {
> -		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_or_emulated(data->client,
> +							BMG160_REG_XOUT_L,
> +							AXIS_MAX * 2,
> +							(u8 *)data->buffer);
>  	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-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Pargmann Aug. 17, 2015, 9:09 a.m. UTC | #2 
On Sun, Aug 16, 2015 at 10:24:47AM +0100, Jonathan Cameron wrote:
> On 12/08/15 15:31, Irina Tirdea 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. Uses i2c_smbus_read_i2c_block_data_or_emulated
> > that will fallback to reading each axis as a separate word in case i2c
> > block read is not supported.
> > 
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Acked-by: Jonathan Cameron <jic23@kernel.org>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Note, that in the meantime the bmg160 driver just went all regmap
> on us (as part of adding SPI support - though that step hasn't
> happened yet).  Hence we'll need a means of telling regmap about this
> possibility.

Perhaps this is covered by a regmap_bulk_read()?

The series[1] I am working on implements a i2c smbus block data regmap
bus driver. Regmap should then automatically do a block read in
regmap_bulk_read.

Patch 15 introduces the i2c block data regmap bus driver[2].
I am only implementing this so I don't break bmc150 behavior. I do not
have the hardware available to test this regmap driver so it would be great
if someone else could test one of the next versions of this bus driver.

Best regards,

Markus

[1] http://thread.gmane.org/gmane.linux.kernel/2018643
[2] http://thread.gmane.org/gmane.linux.kernel/2018639

> 
> > ---
> >  drivers/iio/gyro/bmg160.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
> > index b2a6ccb..1ff306d 100644
> > --- a/drivers/iio/gyro/bmg160.c
> > +++ b/drivers/iio/gyro/bmg160.c
> > @@ -772,6 +772,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						\
> > @@ -809,19 +810,16 @@ 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;
> > +	int ret = 0;
> >  
> >  	mutex_lock(&data->mutex);
> > -	for (bit = 0; bit < AXIS_MAX; bit++) {
> > -		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_or_emulated(data->client,
> > +							BMG160_REG_XOUT_L,
> > +							AXIS_MAX * 2,
> > +							(u8 *)data->buffer);
> >  	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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Irina Tirdea Aug. 17, 2015, 9:34 a.m. UTC | #3 
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 16 August, 2015 12:25
> To: Tirdea, Irina; Wolfram Sang; linux-iio@vger.kernel.org; linux-i2c@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Peter Meerwald
> Subject: Re: [PATCH v5 6/8] iio: gyro: bmg160: optimize i2c transfers in trigger handler
> 
> On 12/08/15 15:31, Irina Tirdea 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. Uses i2c_smbus_read_i2c_block_data_or_emulated
> > that will fallback to reading each axis as a separate word in case i2c
> > block read is not supported.
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Acked-by: Jonathan Cameron <jic23@kernel.org>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Note, that in the meantime the bmg160 driver just went all regmap
> on us (as part of adding SPI support - though that step hasn't
> happened yet).  Hence we'll need a means of telling regmap about this
> possibility.

I think regmap_bulk_read might be enough in my case, but I'll have to
test this. I have also seen there are similar changes for the bmc150 driver,
so I will rebase my changes on top of the regmap ones. That leaves only the
kxcjk-1013 changes to be applied from this patchset.

Thanks,
Irina

> 
> > ---
> >  drivers/iio/gyro/bmg160.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
> > index b2a6ccb..1ff306d 100644
> > --- a/drivers/iio/gyro/bmg160.c
> > +++ b/drivers/iio/gyro/bmg160.c
> > @@ -772,6 +772,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						\
> > @@ -809,19 +810,16 @@ 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;
> > +	int ret = 0;
> >
> >  	mutex_lock(&data->mutex);
> > -	for (bit = 0; bit < AXIS_MAX; bit++) {
> > -		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_or_emulated(data->client,
> > +							BMG160_REG_XOUT_L,
> > +							AXIS_MAX * 2,
> > +							(u8 *)data->buffer);
> >  	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-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Irina Tirdea Aug. 17, 2015, 9:48 a.m. UTC | #4 
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFya3VzIFBhcmdtYW5u
IFttYWlsdG86bXBhQHBlbmd1dHJvbml4LmRlXQ0KPiBTZW50OiAxNyBBdWd1c3QsIDIwMTUgMTI6
MTANCj4gVG86IEpvbmF0aGFuIENhbWVyb24NCj4gQ2M6IFRpcmRlYSwgSXJpbmE7IFdvbGZyYW0g
U2FuZzsgbGludXgtaWlvQHZnZXIua2VybmVsLm9yZzsgbGludXgtaTJjQHZnZXIua2VybmVsLm9y
ZzsgbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgUGFuZHJ1dmFkYSwNCj4gU3Jpbml2YXM7
IFBldGVyIE1lZXJ3YWxkDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjUgNi84XSBpaW86IGd5cm86
IGJtZzE2MDogb3B0aW1pemUgaTJjIHRyYW5zZmVycyBpbiB0cmlnZ2VyIGhhbmRsZXINCj4gDQo+
IE9uIFN1biwgQXVnIDE2LCAyMDE1IGF0IDEwOjI0OjQ3QU0gKzAxMDAsIEpvbmF0aGFuIENhbWVy
b24gd3JvdGU6DQo+ID4gT24gMTIvMDgvMTUgMTU6MzEsIElyaW5hIFRpcmRlYSB3cm90ZToNCj4g
PiA+IFNvbWUgaTJjIGJ1c3NlcyAoZS5nLjogU3lub3BzeXMgRGVzaWduV2FyZSBJMkMgYWRhcHRl
cikgbmVlZCB0bw0KPiA+ID4gZW5hYmxlL2Rpc2FibGUgdGhlIGJ1cyBhdCBlYWNoIGkyYyB0cmFu
c2ZlciBhbmQgbXVzdCB3YWl0IGZvcg0KPiA+ID4gdGhlIGVuYWJsZS9kaXNhYmxlIHRvIGhhcHBl
biBiZWZvcmUgc2VuZGluZyB0aGUgZGF0YS4NCj4gPiA+DQo+ID4gPiBXaGVuIHJlYWRpbmcgZGF0
YSBpbiB0aGUgdHJpZ2dlciBoYW5kbGVyLCB0aGUgYm1nMTYwIGRyaXZlciBkb2VzDQo+ID4gPiBv
bmUgaTJjIHRyYW5zZmVyIGZvciBlYWNoIGF4aXMuIFRoaXMgaGFzIGFuIGltcGFjdCBvbiB0aGUg
ZnJlcXVlbmN5DQo+ID4gPiBvZiB0aGUgZ3lyb3Njb3BlIGF0IGhpZ2ggc2FtcGxlIHJhdGVzIGR1
ZSB0byBhZGRpdGlvbmFsIGRlbGF5cw0KPiA+ID4gaW50cm9kdWNlZCBieSB0aGUgaTJjIGJ1cyBh
dCBlYWNoIHRyYW5zZmVyLg0KPiA+ID4NCj4gPiA+IFJlYWRpbmcgYWxsIGF4aXMgdmFsdWVzIGlu
IG9uZSBpMmMgdHJhbnNmZXIgcmVkdWNlcyB0aGUgZGVsYXlzDQo+ID4gPiBpbnRyb2R1Y2VkIGJ5
IHRoZSBpMmMgYnVzLiBVc2VzIGkyY19zbWJ1c19yZWFkX2kyY19ibG9ja19kYXRhX29yX2VtdWxh
dGVkDQo+ID4gPiB0aGF0IHdpbGwgZmFsbGJhY2sgdG8gcmVhZGluZyBlYWNoIGF4aXMgYXMgYSBz
ZXBhcmF0ZSB3b3JkIGluIGNhc2UgaTJjDQo+ID4gPiBibG9jayByZWFkIGlzIG5vdCBzdXBwb3J0
ZWQuDQo+ID4gPg0KPiA+ID4gU2lnbmVkLW9mZi1ieTogSXJpbmEgVGlyZGVhIDxpcmluYS50aXJk
ZWFAaW50ZWwuY29tPg0KPiA+ID4gQWNrZWQtYnk6IEpvbmF0aGFuIENhbWVyb24gPGppYzIzQGtl
cm5lbC5vcmc+DQo+ID4gPiBBY2tlZC1ieTogU3Jpbml2YXMgUGFuZHJ1dmFkYSA8c3Jpbml2YXMu
cGFuZHJ1dmFkYUBsaW51eC5pbnRlbC5jb20+DQo+ID4gTm90ZSwgdGhhdCBpbiB0aGUgbWVhbnRp
bWUgdGhlIGJtZzE2MCBkcml2ZXIganVzdCB3ZW50IGFsbCByZWdtYXANCj4gPiBvbiB1cyAoYXMg
cGFydCBvZiBhZGRpbmcgU1BJIHN1cHBvcnQgLSB0aG91Z2ggdGhhdCBzdGVwIGhhc24ndA0KPiA+
IGhhcHBlbmVkIHlldCkuICBIZW5jZSB3ZSdsbCBuZWVkIGEgbWVhbnMgb2YgdGVsbGluZyByZWdt
YXAgYWJvdXQgdGhpcw0KPiA+IHBvc3NpYmlsaXR5Lg0KPiANCj4gUGVyaGFwcyB0aGlzIGlzIGNv
dmVyZWQgYnkgYSByZWdtYXBfYnVsa19yZWFkKCk/DQoNCkkgdGhpbmsgaXQgaXMuIEhvd2V2ZXIs
IGlmIHRoYXQgZG9lcyBub3Qgd29yayBmb3IgdGhlIGkyYyBjb250cm9sbGVycyBJJ20NCnRlc3Rp
bmcgd2l0aCBJJ2xsIHRha2UgYSBsb29rIGF0IHRoZSBwYXRjaGVzIHlvdSBtZW50aW9uIGJlbG93
Lg0KSSdsbCByZWJhc2UgdGhpcyBwYXRjaCBvbiB0b3Agb2YgeW91ciBuZXh0IHZlcnNpb24gZm9y
IGJtZzE2MCBhbmQNCnJlc2VuZC4NCg0KVGhhbmtzLA0KSXJpbmENCg0KPiANCj4gVGhlIHNlcmll
c1sxXSBJIGFtIHdvcmtpbmcgb24gaW1wbGVtZW50cyBhIGkyYyBzbWJ1cyBibG9jayBkYXRhIHJl
Z21hcA0KPiBidXMgZHJpdmVyLiBSZWdtYXAgc2hvdWxkIHRoZW4gYXV0b21hdGljYWxseSBkbyBh
IGJsb2NrIHJlYWQgaW4NCj4gcmVnbWFwX2J1bGtfcmVhZC4NCj4gDQo+IFBhdGNoIDE1IGludHJv
ZHVjZXMgdGhlIGkyYyBibG9jayBkYXRhIHJlZ21hcCBidXMgZHJpdmVyWzJdLg0KPiBJIGFtIG9u
bHkgaW1wbGVtZW50aW5nIHRoaXMgc28gSSBkb24ndCBicmVhayBibWMxNTAgYmVoYXZpb3IuIEkg
ZG8gbm90DQo+IGhhdmUgdGhlIGhhcmR3YXJlIGF2YWlsYWJsZSB0byB0ZXN0IHRoaXMgcmVnbWFw
IGRyaXZlciBzbyBpdCB3b3VsZCBiZSBncmVhdA0KPiBpZiBzb21lb25lIGVsc2UgY291bGQgdGVz
dCBvbmUgb2YgdGhlIG5leHQgdmVyc2lvbnMgb2YgdGhpcyBidXMgZHJpdmVyLg0KPiANCj4gQmVz
dCByZWdhcmRzLA0KPiANCj4gTWFya3VzDQo+IA0KPiBbMV0gaHR0cDovL3RocmVhZC5nbWFuZS5v
cmcvZ21hbmUubGludXgua2VybmVsLzIwMTg2NDMNCj4gWzJdIGh0dHA6Ly90aHJlYWQuZ21hbmUu
b3JnL2dtYW5lLmxpbnV4Lmtlcm5lbC8yMDE4NjM5DQo+IA0KPiA+DQo+ID4gPiAtLS0NCj4gPiA+
ICBkcml2ZXJzL2lpby9neXJvL2JtZzE2MC5jIHwgMTggKysrKysrKystLS0tLS0tLS0tDQo+ID4g
PiAgMSBmaWxlIGNoYW5nZWQsIDggaW5zZXJ0aW9ucygrKSwgMTAgZGVsZXRpb25zKC0pDQo+ID4g
Pg0KPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvaWlvL2d5cm8vYm1nMTYwLmMgYi9kcml2ZXJz
L2lpby9neXJvL2JtZzE2MC5jDQo+ID4gPiBpbmRleCBiMmE2Y2NiLi4xZmYzMDZkIDEwMDY0NA0K
PiA+ID4gLS0tIGEvZHJpdmVycy9paW8vZ3lyby9ibWcxNjAuYw0KPiA+ID4gKysrIGIvZHJpdmVy
cy9paW8vZ3lyby9ibWcxNjAuYw0KPiA+ID4gQEAgLTc3Miw2ICs3NzIsNyBAQCBzdGF0aWMgY29u
c3Qgc3RydWN0IGlpb19ldmVudF9zcGVjIGJtZzE2MF9ldmVudCA9IHsNCj4gPiA+ICAJCS5zaWdu
ID0gJ3MnLAkJCQkJCVwNCj4gPiA+ICAJCS5yZWFsYml0cyA9IDE2LAkJCQkJXA0KPiA+ID4gIAkJ
LnN0b3JhZ2ViaXRzID0gMTYsCQkJCQlcDQo+ID4gPiArCQkuZW5kaWFubmVzcyA9IElJT19MRSwJ
CQkJCVwNCj4gPiA+ICAJfSwJCQkJCQkJCVwNCj4gPiA+ICAJLmV2ZW50X3NwZWMgPSAmYm1nMTYw
X2V2ZW50LAkJCQkJXA0KPiA+ID4gIAkubnVtX2V2ZW50X3NwZWNzID0gMQkJCQkJCVwNCj4gPiA+
IEBAIC04MDksMTkgKzgxMCwxNiBAQCBzdGF0aWMgaXJxcmV0dXJuX3QgYm1nMTYwX3RyaWdnZXJf
aGFuZGxlcihpbnQgaXJxLCB2b2lkICpwKQ0KPiA+ID4gIAlzdHJ1Y3QgaWlvX3BvbGxfZnVuYyAq
cGYgPSBwOw0KPiA+ID4gIAlzdHJ1Y3QgaWlvX2RldiAqaW5kaW9fZGV2ID0gcGYtPmluZGlvX2Rl
djsNCj4gPiA+ICAJc3RydWN0IGJtZzE2MF9kYXRhICpkYXRhID0gaWlvX3ByaXYoaW5kaW9fZGV2
KTsNCj4gPiA+IC0JaW50IGJpdCwgcmV0LCBpID0gMDsNCj4gPiA+ICsJaW50IHJldCA9IDA7DQo+
ID4gPg0KPiA+ID4gIAltdXRleF9sb2NrKCZkYXRhLT5tdXRleCk7DQo+ID4gPiAtCWZvciAoYml0
ID0gMDsgYml0IDwgQVhJU19NQVg7IGJpdCsrKSB7DQo+ID4gPiAtCQlyZXQgPSBpMmNfc21idXNf
cmVhZF93b3JkX2RhdGEoZGF0YS0+Y2xpZW50LA0KPiA+ID4gLQkJCQkJICAgICAgIEJNRzE2MF9B
WElTX1RPX1JFRyhiaXQpKTsNCj4gPiA+IC0JCWlmIChyZXQgPCAwKSB7DQo+ID4gPiAtCQkJbXV0
ZXhfdW5sb2NrKCZkYXRhLT5tdXRleCk7DQo+ID4gPiAtCQkJZ290byBlcnI7DQo+ID4gPiAtCQl9
DQo+ID4gPiAtCQlkYXRhLT5idWZmZXJbaSsrXSA9IHJldDsNCj4gPiA+IC0JfQ0KPiA+ID4gKwly
ZXQgPSBpMmNfc21idXNfcmVhZF9pMmNfYmxvY2tfZGF0YV9vcl9lbXVsYXRlZChkYXRhLT5jbGll
bnQsDQo+ID4gPiArCQkJCQkJCUJNRzE2MF9SRUdfWE9VVF9MLA0KPiA+ID4gKwkJCQkJCQlBWElT
X01BWCAqIDIsDQo+ID4gPiArCQkJCQkJCSh1OCAqKWRhdGEtPmJ1ZmZlcik7DQo+ID4gPiAgCW11
dGV4X3VubG9jaygmZGF0YS0+bXV0ZXgpOw0KPiA+ID4gKwlpZiAocmV0IDwgMCkNCj4gPiA+ICsJ
CWdvdG8gZXJyOw0KPiA+ID4NCj4gPiA+ICAJaWlvX3B1c2hfdG9fYnVmZmVyc193aXRoX3RpbWVz
dGFtcChpbmRpb19kZXYsIGRhdGEtPmJ1ZmZlciwNCj4gPiA+ICAJCQkJCSAgIHBmLT50aW1lc3Rh
bXApOw0KPiA+ID4NCj4gPg0KPiA+IC0tDQo+ID4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxp
c3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LWlpbyIgaW4NCj4gPiB0aGUgYm9k
eSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZw0KPiA+IE1vcmUgbWFq
b3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRt
bA0KPiA+DQo+IA0KPiAtLQ0KPiBQZW5ndXRyb25peCBlLksuICAgICAgICAgICAgICAgICAgICAg
ICAgICAgfCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfA0KPiBJbmR1c3RyaWFsIExpbnV4
IFNvbHV0aW9ucyAgICAgICAgICAgICAgICAgfCBodHRwOi8vd3d3LnBlbmd1dHJvbml4LmRlLyAg
fA0KPiBQZWluZXIgU3RyLiA2LTgsIDMxMTM3IEhpbGRlc2hlaW0sIEdlcm1hbnkgfCBQaG9uZTog
KzQ5LTUxMjEtMjA2OTE3LTAgICAgfA0KPiBBbXRzZ2VyaWNodCBIaWxkZXNoZWltLCBIUkEgMjY4
NiAgICAgICAgICAgfCBGYXg6ICAgKzQ5LTUxMjEtMjA2OTE3LTU1NTUgfA0K
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Aug. 21, 2015, 10:21 a.m. UTC | #5 
On Mon, Aug 17, 2015 at 11:09:43AM +0200, Markus Pargmann wrote:
> On Sun, Aug 16, 2015 at 10:24:47AM +0100, Jonathan Cameron wrote:
> > On 12/08/15 15:31, Irina Tirdea 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. Uses i2c_smbus_read_i2c_block_data_or_emulated
> > > that will fallback to reading each axis as a separate word in case i2c
> > > block read is not supported.
> > > 
> > > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > > Acked-by: Jonathan Cameron <jic23@kernel.org>
> > > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Note, that in the meantime the bmg160 driver just went all regmap
> > on us (as part of adding SPI support - though that step hasn't
> > happened yet).  Hence we'll need a means of telling regmap about this
> > possibility.
> 
> Perhaps this is covered by a regmap_bulk_read()?
> 
> The series[1] I am working on implements a i2c smbus block data regmap
> bus driver. Regmap should then automatically do a block read in
> regmap_bulk_read.

Hmm, so doesn't your series make Irina's series obsolete? It addresses
the same problem only at a different layer (i2c core vs. regmap), or? It
would mean that i2c client drivers which want to support byte, word, or
block transfers should be converted to regmap. I assume most of the
potential candidates are register based devices anyhow. Then, regmap
would be the proper abstraction layer. Have I overlooked something?

Thanks,

   Wolfram
Pandruvada, Srinivas Aug. 21, 2015, 3:59 p.m. UTC | #6 
T24gRnJpLCAyMDE1LTA4LTIxIGF0IDEyOjIxICswMjAwLCBXb2xmcmFtIFNhbmcgd3JvdGU6DQo+
IE9uIE1vbiwgQXVnIDE3LCAyMDE1IGF0IDExOjA5OjQzQU0gKzAyMDAsIE1hcmt1cyBQYXJnbWFu
biB3cm90ZToNCj4gPiBPbiBTdW4sIEF1ZyAxNiwgMjAxNSBhdCAxMDoyNDo0N0FNICswMTAwLCBK
b25hdGhhbiBDYW1lcm9uIHdyb3RlOg0KPiA+ID4gT24gMTIvMDgvMTUgMTU6MzEsIElyaW5hIFRp
cmRlYSB3cm90ZToNCj4gPiA+ID4gU29tZSBpMmMgYnVzc2VzIChlLmcuOiBTeW5vcHN5cyBEZXNp
Z25XYXJlIEkyQyBhZGFwdGVyKSBuZWVkIHRvDQo+ID4gPiA+IGVuYWJsZS9kaXNhYmxlIHRoZSBi
dXMgYXQgZWFjaCBpMmMgdHJhbnNmZXIgYW5kIG11c3Qgd2FpdCBmb3INCj4gPiA+ID4gdGhlIGVu
YWJsZS9kaXNhYmxlIHRvIGhhcHBlbiBiZWZvcmUgc2VuZGluZyB0aGUgZGF0YS4NCj4gPiA+ID4g
DQo+ID4gPiA+IFdoZW4gcmVhZGluZyBkYXRhIGluIHRoZSB0cmlnZ2VyIGhhbmRsZXIsIHRoZSBi
bWcxNjAgZHJpdmVyIA0KPiA+ID4gPiBkb2VzDQo+ID4gPiA+IG9uZSBpMmMgdHJhbnNmZXIgZm9y
IGVhY2ggYXhpcy4gVGhpcyBoYXMgYW4gaW1wYWN0IG9uIHRoZSANCj4gPiA+ID4gZnJlcXVlbmN5
DQo+ID4gPiA+IG9mIHRoZSBneXJvc2NvcGUgYXQgaGlnaCBzYW1wbGUgcmF0ZXMgZHVlIHRvIGFk
ZGl0aW9uYWwgZGVsYXlzDQo+ID4gPiA+IGludHJvZHVjZWQgYnkgdGhlIGkyYyBidXMgYXQgZWFj
aCB0cmFuc2Zlci4NCj4gPiA+ID4gDQo+ID4gPiA+IFJlYWRpbmcgYWxsIGF4aXMgdmFsdWVzIGlu
IG9uZSBpMmMgdHJhbnNmZXIgcmVkdWNlcyB0aGUgZGVsYXlzDQo+ID4gPiA+IGludHJvZHVjZWQg
YnkgdGhlIGkyYyBidXMuIFVzZXMgDQo+ID4gPiA+IGkyY19zbWJ1c19yZWFkX2kyY19ibG9ja19k
YXRhX29yX2VtdWxhdGVkDQo+ID4gPiA+IHRoYXQgd2lsbCBmYWxsYmFjayB0byByZWFkaW5nIGVh
Y2ggYXhpcyBhcyBhIHNlcGFyYXRlIHdvcmQgaW4gDQo+ID4gPiA+IGNhc2UgaTJjDQo+ID4gPiA+
IGJsb2NrIHJlYWQgaXMgbm90IHN1cHBvcnRlZC4NCj4gPiA+ID4gDQo+ID4gPiA+IFNpZ25lZC1v
ZmYtYnk6IElyaW5hIFRpcmRlYSA8aXJpbmEudGlyZGVhQGludGVsLmNvbT4NCj4gPiA+ID4gQWNr
ZWQtYnk6IEpvbmF0aGFuIENhbWVyb24gPGppYzIzQGtlcm5lbC5vcmc+DQo+ID4gPiA+IEFja2Vk
LWJ5OiBTcmluaXZhcyBQYW5kcnV2YWRhIDwNCj4gPiA+ID4gc3Jpbml2YXMucGFuZHJ1dmFkYUBs
aW51eC5pbnRlbC5jb20+DQo+ID4gPiBOb3RlLCB0aGF0IGluIHRoZSBtZWFudGltZSB0aGUgYm1n
MTYwIGRyaXZlciBqdXN0IHdlbnQgYWxsIHJlZ21hcA0KPiA+ID4gb24gdXMgKGFzIHBhcnQgb2Yg
YWRkaW5nIFNQSSBzdXBwb3J0IC0gdGhvdWdoIHRoYXQgc3RlcCBoYXNuJ3QNCj4gPiA+IGhhcHBl
bmVkIHlldCkuICBIZW5jZSB3ZSdsbCBuZWVkIGEgbWVhbnMgb2YgdGVsbGluZyByZWdtYXAgYWJv
dXQgDQo+ID4gPiB0aGlzDQo+ID4gPiBwb3NzaWJpbGl0eS4NCj4gPiANCj4gPiBQZXJoYXBzIHRo
aXMgaXMgY292ZXJlZCBieSBhIHJlZ21hcF9idWxrX3JlYWQoKT8NCj4gPiANCj4gPiBUaGUgc2Vy
aWVzWzFdIEkgYW0gd29ya2luZyBvbiBpbXBsZW1lbnRzIGEgaTJjIHNtYnVzIGJsb2NrIGRhdGEg
DQo+ID4gcmVnbWFwDQo+ID4gYnVzIGRyaXZlci4gUmVnbWFwIHNob3VsZCB0aGVuIGF1dG9tYXRp
Y2FsbHkgZG8gYSBibG9jayByZWFkIGluDQo+ID4gcmVnbWFwX2J1bGtfcmVhZC4NCj4gDQo+IEht
bSwgc28gZG9lc24ndCB5b3VyIHNlcmllcyBtYWtlIElyaW5hJ3Mgc2VyaWVzIG9ic29sZXRlPyBJ
dCANCj4gYWRkcmVzc2VzDQo+IHRoZSBzYW1lIHByb2JsZW0gb25seSBhdCBhIGRpZmZlcmVudCBs
YXllciAoaTJjIGNvcmUgdnMuIHJlZ21hcCksIG9yPyANCj4gSXQNCj4gd291bGQgbWVhbiB0aGF0
IGkyYyBjbGllbnQgZHJpdmVycyB3aGljaCB3YW50IHRvIHN1cHBvcnQgYnl0ZSwgd29yZCwgDQo+
IG9yDQo+IGJsb2NrIHRyYW5zZmVycyBzaG91bGQgYmUgY29udmVydGVkIHRvIHJlZ21hcC4gSSBh
c3N1bWUgbW9zdCBvZiB0aGUNCj4gcG90ZW50aWFsIGNhbmRpZGF0ZXMgYXJlIHJlZ2lzdGVyIGJh
c2VkIGRldmljZXMgYW55aG93LiBUaGVuLCByZWdtYXANCj4gd291bGQgYmUgdGhlIHByb3BlciBh
YnN0cmFjdGlvbiBsYXllci4gSGF2ZSBJIG92ZXJsb29rZWQgc29tZXRoaW5nPw0KPiANClRoaXMg
aXMgdGhlIG9ubHkgZHJpdmVyIGNvbnZlcnRlZCB0byB1c2UgcmVnbWFwLCBiZWNhdXNlIG9mIFNQ
SSBtb2RlDQpzdXBwb3J0LiBUaGUgb3RoZXIgZHJpdmVycyB3aWxsIHN0aWxsIHVzZSBJcmluYSdz
IGNoYW5nZXMuDQoNClRoYW5rcw0KU3Jpbml2YXMNCg0KDQo+IFRoYW5rcywNCj4gDQo+ICAgIFdv
bGZyYW0NCj4g
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Aug. 22, 2015, 4:02 a.m. UTC | #7 
> > > The series[1] I am working on implements a i2c smbus block data 
> > > regmap
> > > bus driver. Regmap should then automatically do a block read in
> > > regmap_bulk_read.
> > 
> > Hmm, so doesn't your series make Irina's series obsolete? It 
> > addresses
> > the same problem only at a different layer (i2c core vs. regmap), or? 
> > It
> > would mean that i2c client drivers which want to support byte, word, 
> > or
> > block transfers should be converted to regmap. I assume most of the
> > potential candidates are register based devices anyhow. Then, regmap
> > would be the proper abstraction layer. Have I overlooked something?
> > 
> This is the only driver converted to use regmap, because of SPI mode
> support. The other drivers will still use Irina's changes.

The question is if they should. Or rather be converted to regmap. It is
an open question and I am seeking for further input.
Jonathan Cameron Aug. 22, 2015, 5:29 p.m. UTC | #8 
On 22/08/15 05:02, Wolfram Sang wrote:
> 
>>>> The series[1] I am working on implements a i2c smbus block data 
>>>> regmap
>>>> bus driver. Regmap should then automatically do a block read in
>>>> regmap_bulk_read.
>>>
>>> Hmm, so doesn't your series make Irina's series obsolete? It 
>>> addresses
>>> the same problem only at a different layer (i2c core vs. regmap), or? 
>>> It
>>> would mean that i2c client drivers which want to support byte, word, 
>>> or
>>> block transfers should be converted to regmap. I assume most of the
>>> potential candidates are register based devices anyhow. Then, regmap
>>> would be the proper abstraction layer. Have I overlooked something?
>>>
>> This is the only driver converted to use regmap, because of SPI mode
>> support. The other drivers will still use Irina's changes.
> 
> The question is if they should. Or rather be converted to regmap. It is
> an open question and I am seeking for further input.
> 
There are a fairly large number of legacy drivers where this might apply.
Do we have any real idea of how many?  I'm assuming Irina only made use
of it in ones that were of personal interest.  A quick grep suggests
10's of drivers use the block call. I'm guessing quite a few would use
it if needed for a particular board.

Do we want to insist on a much larger change (conversion to regmap)
when if this in place, a simple single functional call change will do the
job?

Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Aug. 24, 2015, 11:49 a.m. UTC | #9 
> Do we want to insist on a much larger change (conversion to regmap)
> when if this in place, a simple single functional call change will do the
> job?

I'd assume that regmap conversion will happen later quite likely anyhow.
Most of those devices will have I2C/SPI dual interfaces; or people will
find out that caching registers can reduce the bus load, etc...

That being said, I'll keep Irina's patches for the next merge window.
But we should keep an eye how/if the new function is used. Kernel
growth is an issue at times...

Thanks,

   Wolfram
diff mbox

Patch

diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
index b2a6ccb..1ff306d 100644
--- a/drivers/iio/gyro/bmg160.c
+++ b/drivers/iio/gyro/bmg160.c
@@ -772,6 +772,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						\
@@ -809,19 +810,16 @@  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;
+	int ret = 0;
 
 	mutex_lock(&data->mutex);
-	for (bit = 0; bit < AXIS_MAX; bit++) {
-		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_or_emulated(data->client,
+							BMG160_REG_XOUT_L,
+							AXIS_MAX * 2,
+							(u8 *)data->buffer);
 	mutex_unlock(&data->mutex);
+	if (ret < 0)
+		goto err;
 
 	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
 					   pf->timestamp);