* RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
@ 2015-02-20 12:02 ` Tirdea, Irina
0 siblings, 0 replies; 18+ messages in thread
From: Tirdea, Irina @ 2015-02-20 12:02 UTC (permalink / raw)
To: Peter Meerwald
Cc: Jonathan Cameron, linux-iio, linux-kernel, Pandruvada, Srinivas,
Reus, Adriana
> -----Original Message-----
> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
> Sent: 16 February, 2015 21:26
> To: Tirdea, Irina
> Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.kernel=
.org; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers i=
n trigger handler
>=20
>=20
> > Reading all axis values in one i2c transfer reduces the delays
> > introduced by the i2c bus. In case i2c block read is not supported,
> > fallback to reading each axis as a separate word.
>=20
> see comments inline below
>=20
Thanks for the review, Peter!
> > Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> > drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++-=
--------
> > 1 file changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1=
013.c
> > index 5f27787..bfa2899 100644
> > --- a/drivers/iio/accel/kxcjk-1013.c
> > +++ b/drivers/iio/accel/kxcjk-1013.c
> > @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> > int64_t timestamp;
> > enum kx_chipset chipset;
> > bool is_smo8500_device;
> > + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> > + u8 length, u8 *values);
>=20
> probably this could/should be done in the i2c layer or we end up adding a
> similar function in every driver?
>=20
Actually this is exactly what I did: adding this code in a couple of driver=
s :)
You are right, this belongs to the i2c core. Will move it there.
> > };
> >
> > enum kxcjk1013_axis {
> > @@ -216,6 +218,23 @@ static const struct {
> > {800, 0, 0x06},
> > {1600, 0, 0x06} };
> >
> > +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
> > + u8 command, u8 length, u8 *values)
> > +{
> > + s32 data;
> > + u8 i;
> > +
> > + for (i =3D 0; i < length; i +=3D 2) {
> > + data =3D i2c_smbus_read_word_data(client, command + i);
> > + if (data < 0)
> > + return data;
> > +
> > + values[i] =3D data & 0xFF;
> > + values[i+1] =3D data >> 8;
>=20
> this is incorrect; it forces the data to be little endian, however, the
> endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> code breaks for big-endian CPUs
>=20
> since _read_i2c_block_data() can't do endianness conversion (and the chip
> does i2c endianness, i.e. little-endian), the .scan_type should become
> IIO_LE and above code is correct again but still ugly :)
>=20
> bottom line: change .scan_type to IIO_LE
>=20
Good point. Changing the endianess to IIO_LE is correct for either kxcjk101=
3_read_block_data or i2c_smbus_read_i2c_block_data.
Will fix this in the next version. Thanks for catching this!
> > + }
> > + return i;
> > +}
> > +
> > static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> > enum kxcjk1013_mode mode)
> > {
> > @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int =
irq, void *p)
> > struct iio_poll_func *pf =3D p;
> > struct iio_dev *indio_dev =3D pf->indio_dev;
> > struct kxcjk1013_data *data =3D iio_priv(indio_dev);
> > - int bit, ret, i =3D 0;
> > + int ret;
> >
> > mutex_lock(&data->mutex);
> > - for (bit =3D 0; bit < AXIS_MAX; bit++) {
> > - ret =3D kxcjk1013_get_acc_reg(data, bit);
> > - if (ret < 0) {
> > - mutex_unlock(&data->mutex);
> > - goto err;
> > - }
> > - data->buffer[i++] =3D ret;
> > - }
> > + ret =3D data->read_block_data(data->client, KXCJK1013_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,
> > data->timestamp);
> > @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *cl=
ient,
> > const char *name;
> > int ret;
> >
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_BYTE_DATA |
> > + I2C_FUNC_SMBUS_READ_WORD_DATA))
> > + return -ENODEV;
> > +
> > indio_dev =3D devm_iio_device_alloc(&client->dev, sizeof(*data));
> > if (!indio_dev)
> > return -ENOMEM;
> > @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *cl=
ient,
> > i2c_set_clientdata(client, indio_dev);
> > data->client =3D client;
> >
> > + if (i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > + data->read_block_data =3D i2c_smbus_read_i2c_block_data;
> > + else
> > + data->read_block_data =3D kxcjk1013_read_block_data;
> > +
> > pdata =3D dev_get_platdata(&client->dev);
> > if (pdata)
> > data->active_high_intr =3D pdata->active_high_intr;
> >
>=20
> --
>=20
> Peter Meerwald
> +43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
2015-02-20 12:02 ` Tirdea, Irina
@ 2015-02-20 14:58 ` Pandruvada, Srinivas
-1 siblings, 0 replies; 18+ messages in thread
From: Pandruvada, Srinivas @ 2015-02-20 14:58 UTC (permalink / raw)
To: Tirdea, Irina; +Cc: linux-kernel, pmeerw, jic23, Reus, Adriana, linux-iio
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5391 bytes --]
On Fri, 2015-02-20 at 12:02 +0000, Tirdea, Irina wrote:
>
> > -----Original Message-----
> > From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
> > Sent: 16 February, 2015 21:26
> > To: Tirdea, Irina
> > Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
> > Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >
> >
> > > Reading all axis values in one i2c transfer reduces the delays
> > > introduced by the i2c bus. In case i2c block read is not supported,
> > > fallback to reading each axis as a separate word.
> >
> > see comments inline below
> >
> Thanks for the review, Peter!
>
> > > Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> > > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > > Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > ---
> > > drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 35 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> > > index 5f27787..bfa2899 100644
> > > --- a/drivers/iio/accel/kxcjk-1013.c
> > > +++ b/drivers/iio/accel/kxcjk-1013.c
> > > @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> > > int64_t timestamp;
> > > enum kx_chipset chipset;
> > > bool is_smo8500_device;
> > > + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> > > + u8 length, u8 *values);
> >
> > probably this could/should be done in the i2c layer or we end up adding a
> > similar function in every driver?
> >
> Actually this is exactly what I did: adding this code in a couple of drivers :)
> You are right, this belongs to the i2c core. Will move it there.
>
> > > };
> > >
> > > enum kxcjk1013_axis {
> > > @@ -216,6 +218,23 @@ static const struct {
> > > {800, 0, 0x06},
> > > {1600, 0, 0x06} };
> > >
> > > +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
> > > + u8 command, u8 length, u8 *values)
> > > +{
> > > + s32 data;
> > > + u8 i;
> > > +
> > > + for (i = 0; i < length; i += 2) {
> > > + data = i2c_smbus_read_word_data(client, command + i);
> > > + if (data < 0)
> > > + return data;
> > > +
> > > + values[i] = data & 0xFF;
> > > + values[i+1] = data >> 8;
> >
> > this is incorrect; it forces the data to be little endian, however, the
> > endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> > code breaks for big-endian CPUs
> >
> > since _read_i2c_block_data() can't do endianness conversion (and the chip
> > does i2c endianness, i.e. little-endian), the .scan_type should become
> > IIO_LE and above code is correct again but still ugly :)
> >
> > bottom line: change .scan_type to IIO_LE
> >
> Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> Will fix this in the next version. Thanks for catching this!
>
I don't think changing to IIO_LE is good idea as when i2c_read_bock..
then the scan type will be CPU. So better to fix endianness in this
function.
> > > + }
> > > + return i;
> > > +}
> > > +
> > > static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> > > enum kxcjk1013_mode mode)
> > > {
> > > @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
> > > struct iio_poll_func *pf = p;
> > > struct iio_dev *indio_dev = pf->indio_dev;
> > > struct kxcjk1013_data *data = iio_priv(indio_dev);
> > > - int bit, ret, i = 0;
> > > + int ret;
> > >
> > > mutex_lock(&data->mutex);
> > > - for (bit = 0; bit < AXIS_MAX; bit++) {
> > > - ret = kxcjk1013_get_acc_reg(data, bit);
> > > - if (ret < 0) {
> > > - mutex_unlock(&data->mutex);
> > > - goto err;
> > > - }
> > > - data->buffer[i++] = ret;
> > > - }
> > > + ret = data->read_block_data(data->client, KXCJK1013_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,
> > > data->timestamp);
> > > @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
> > > const char *name;
> > > int ret;
> > >
> > > + if (!i2c_check_functionality(client->adapter,
> > > + I2C_FUNC_SMBUS_BYTE_DATA |
> > > + I2C_FUNC_SMBUS_READ_WORD_DATA))
> > > + return -ENODEV;
> > > +
> > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > if (!indio_dev)
> > > return -ENOMEM;
> > > @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
> > > i2c_set_clientdata(client, indio_dev);
> > > data->client = client;
> > >
> > > + if (i2c_check_functionality(client->adapter,
> > > + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > > + data->read_block_data = i2c_smbus_read_i2c_block_data;
> > > + else
> > > + data->read_block_data = kxcjk1013_read_block_data;
> > > +
> > > pdata = dev_get_platdata(&client->dev);
> > > if (pdata)
> > > data->active_high_intr = pdata->active_high_intr;
> > >
> >
> > --
> >
> > Peter Meerwald
> > +43-664-2444418 (mobile)
ÿôèº{.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] 18+ messages in thread
* Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
@ 2015-02-20 14:58 ` Pandruvada, Srinivas
0 siblings, 0 replies; 18+ messages in thread
From: Pandruvada, Srinivas @ 2015-02-20 14:58 UTC (permalink / raw)
To: Tirdea, Irina; +Cc: linux-kernel, pmeerw, jic23, Reus, Adriana, linux-iio
T24gRnJpLCAyMDE1LTAyLTIwIGF0IDEyOjAyICswMDAwLCBUaXJkZWEsIElyaW5hIHdyb3RlOg0K
PiANCj4gPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+IEZyb206IFBldGVyIE1lZXJ3
YWxkIFttYWlsdG86cG1lZXJ3QHBtZWVydy5uZXRdDQo+ID4gU2VudDogMTYgRmVicnVhcnksIDIw
MTUgMjE6MjYNCj4gPiBUbzogVGlyZGVhLCBJcmluYQ0KPiA+IENjOiBKb25hdGhhbiBDYW1lcm9u
OyBsaW51eC1paW9Admdlci5rZXJuZWwub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3Jn
OyBQYW5kcnV2YWRhLCBTcmluaXZhczsgUmV1cywgQWRyaWFuYQ0KPiA+IFN1YmplY3Q6IFJlOiBb
UEFUQ0ggMi8yXSBpaW86IGFjY2VsOiBreGNqay0xMDEzOiBvcHRpbWl6ZSBpMmMgdHJhbnNmZXJz
IGluIHRyaWdnZXIgaGFuZGxlcg0KPiA+IA0KPiA+IA0KPiA+ID4gUmVhZGluZyBhbGwgYXhpcyB2
YWx1ZXMgaW4gb25lIGkyYyB0cmFuc2ZlciByZWR1Y2VzIHRoZSBkZWxheXMNCj4gPiA+IGludHJv
ZHVjZWQgYnkgdGhlIGkyYyBidXMuIEluIGNhc2UgaTJjIGJsb2NrIHJlYWQgaXMgbm90IHN1cHBv
cnRlZCwNCj4gPiA+IGZhbGxiYWNrIHRvIHJlYWRpbmcgZWFjaCBheGlzIGFzIGEgc2VwYXJhdGUg
d29yZC4NCj4gPiANCj4gPiBzZWUgY29tbWVudHMgaW5saW5lIGJlbG93DQo+ID4gDQo+IFRoYW5r
cyBmb3IgdGhlIHJldmlldywgUGV0ZXIhDQo+IA0KPiA+ID4gU2lnbmVkLW9mZi1ieTogQWRyaWFu
YSBSZXVzIDxhZHJpYW5hLnJldXNAaW50ZWwuY29tPg0KPiA+ID4gU2lnbmVkLW9mZi1ieTogSXJp
bmEgVGlyZGVhIDxpcmluYS50aXJkZWFAaW50ZWwuY29tPg0KPiA+ID4gUmV2aWV3ZWQtYnk6IFNy
aW5pdmFzIFBhbmRydXZhZGEgPHNyaW5pdmFzLnBhbmRydXZhZGFAbGludXguaW50ZWwuY29tPg0K
PiA+ID4gLS0tDQo+ID4gPiAgZHJpdmVycy9paW8vYWNjZWwva3hjamstMTAxMy5jIHwgNDQgKysr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tDQo+ID4gPiAgMSBmaWxlIGNo
YW5nZWQsIDM1IGluc2VydGlvbnMoKyksIDkgZGVsZXRpb25zKC0pDQo+ID4gPg0KPiA+ID4gZGlm
ZiAtLWdpdCBhL2RyaXZlcnMvaWlvL2FjY2VsL2t4Y2prLTEwMTMuYyBiL2RyaXZlcnMvaWlvL2Fj
Y2VsL2t4Y2prLTEwMTMuYw0KPiA+ID4gaW5kZXggNWYyNzc4Ny4uYmZhMjg5OSAxMDA2NDQNCj4g
PiA+IC0tLSBhL2RyaXZlcnMvaWlvL2FjY2VsL2t4Y2prLTEwMTMuYw0KPiA+ID4gKysrIGIvZHJp
dmVycy9paW8vYWNjZWwva3hjamstMTAxMy5jDQo+ID4gPiBAQCAtMTA5LDYgKzEwOSw4IEBAIHN0
cnVjdCBreGNqazEwMTNfZGF0YSB7DQo+ID4gPiAgCWludDY0X3QgdGltZXN0YW1wOw0KPiA+ID4g
IAllbnVtIGt4X2NoaXBzZXQgY2hpcHNldDsNCj4gPiA+ICAJYm9vbCBpc19zbW84NTAwX2Rldmlj
ZTsNCj4gPiA+ICsJczMyICgqcmVhZF9ibG9ja19kYXRhKShjb25zdCBzdHJ1Y3QgaTJjX2NsaWVu
dCAqY2xpZW50LCB1OCBjb21tYW5kLA0KPiA+ID4gKwkJCSAgICAgICB1OCBsZW5ndGgsIHU4ICp2
YWx1ZXMpOw0KPiA+IA0KPiA+IHByb2JhYmx5IHRoaXMgY291bGQvc2hvdWxkIGJlIGRvbmUgaW4g
dGhlIGkyYyBsYXllciBvciB3ZSBlbmQgdXAgYWRkaW5nIGENCj4gPiBzaW1pbGFyIGZ1bmN0aW9u
IGluIGV2ZXJ5IGRyaXZlcj8NCj4gPiANCj4gQWN0dWFsbHkgdGhpcyBpcyBleGFjdGx5IHdoYXQg
SSBkaWQ6IGFkZGluZyB0aGlzIGNvZGUgaW4gYSBjb3VwbGUgb2YgZHJpdmVycyA6KQ0KPiBZb3Ug
YXJlIHJpZ2h0LCB0aGlzIGJlbG9uZ3MgdG8gdGhlIGkyYyBjb3JlLiBXaWxsIG1vdmUgaXQgdGhl
cmUuDQo+IA0KPiA+ID4gIH07DQo+ID4gPg0KPiA+ID4gIGVudW0ga3hjamsxMDEzX2F4aXMgew0K
PiA+ID4gQEAgLTIxNiw2ICsyMTgsMjMgQEAgc3RhdGljIGNvbnN0IHN0cnVjdCB7DQo+ID4gPiAg
CQkJCSB7ODAwLCAwLCAweDA2fSwNCj4gPiA+ICAJCQkJIHsxNjAwLCAwLCAweDA2fSB9Ow0KPiA+
ID4NCj4gPiA+ICtzdGF0aWMgczMyIGt4Y2prMTAxM19yZWFkX2Jsb2NrX2RhdGEoY29uc3Qgc3Ry
dWN0IGkyY19jbGllbnQgKmNsaWVudCwNCj4gPiA+ICsJCQkJICAgICB1OCBjb21tYW5kLCB1OCBs
ZW5ndGgsIHU4ICp2YWx1ZXMpDQo+ID4gPiArew0KPiA+ID4gKwlzMzIgZGF0YTsNCj4gPiA+ICsJ
dTggaTsNCj4gPiA+ICsNCj4gPiA+ICsJZm9yIChpID0gMDsgaSA8IGxlbmd0aDsgaSArPSAyKSB7
DQo+ID4gPiArCQlkYXRhID0gaTJjX3NtYnVzX3JlYWRfd29yZF9kYXRhKGNsaWVudCwgY29tbWFu
ZCArIGkpOw0KPiA+ID4gKwkJaWYgKGRhdGEgPCAwKQ0KPiA+ID4gKwkJCXJldHVybiBkYXRhOw0K
PiA+ID4gKw0KPiA+ID4gKwkJdmFsdWVzW2ldID0gZGF0YSAmIDB4RkY7DQo+ID4gPiArCQl2YWx1
ZXNbaSsxXSA9IGRhdGEgPj4gODsNCj4gPiANCj4gPiB0aGlzIGlzIGluY29ycmVjdDsgaXQgZm9y
Y2VzIHRoZSBkYXRhIHRvIGJlIGxpdHRsZSBlbmRpYW4sIGhvd2V2ZXIsIHRoZQ0KPiA+IGVuZGlh
bm5lc3MgKGFzIHNwZWNpZmllZCBpbiB0aGUgZHJpdmVyJ3MgLnNjYW5fdHlwZSkgaXMgSUlPX0NQ
VSAtLSB0aGUNCj4gPiBjb2RlIGJyZWFrcyBmb3IgYmlnLWVuZGlhbiBDUFVzDQo+ID4gDQo+ID4g
c2luY2UgX3JlYWRfaTJjX2Jsb2NrX2RhdGEoKSBjYW4ndCBkbyBlbmRpYW5uZXNzIGNvbnZlcnNp
b24gKGFuZCB0aGUgY2hpcA0KPiA+IGRvZXMgaTJjIGVuZGlhbm5lc3MsIGkuZS4gbGl0dGxlLWVu
ZGlhbiksIHRoZSAuc2Nhbl90eXBlIHNob3VsZCBiZWNvbWUNCj4gPiBJSU9fTEUgYW5kIGFib3Zl
IGNvZGUgaXMgY29ycmVjdCBhZ2FpbiBidXQgc3RpbGwgdWdseSA6KQ0KPiA+IA0KPiA+IGJvdHRv
bSBsaW5lOiBjaGFuZ2UgLnNjYW5fdHlwZSB0byBJSU9fTEUNCj4gPiANCj4gR29vZCBwb2ludC4g
Q2hhbmdpbmcgdGhlIGVuZGlhbmVzcyB0byBJSU9fTEUgaXMgY29ycmVjdCBmb3IgZWl0aGVyIGt4
Y2prMTAxM19yZWFkX2Jsb2NrX2RhdGEgb3IgaTJjX3NtYnVzX3JlYWRfaTJjX2Jsb2NrX2RhdGEu
DQo+IFdpbGwgZml4IHRoaXMgaW4gdGhlIG5leHQgdmVyc2lvbi4gVGhhbmtzIGZvciBjYXRjaGlu
ZyB0aGlzIQ0KPiANCkkgZG9uJ3QgdGhpbmsgY2hhbmdpbmcgdG8gSUlPX0xFIGlzIGdvb2QgaWRl
YSBhcyB3aGVuIGkyY19yZWFkX2JvY2suLg0KdGhlbiB0aGUgc2NhbiB0eXBlIHdpbGwgYmUgQ1BV
LiBTbyBiZXR0ZXIgdG8gZml4IGVuZGlhbm5lc3MgaW4gdGhpcw0KZnVuY3Rpb24uDQoNCj4gPiA+
ICsJfQ0KPiA+ID4gKwlyZXR1cm4gaTsNCj4gPiA+ICt9DQo+ID4gPiArDQo+ID4gPiAgc3RhdGlj
IGludCBreGNqazEwMTNfc2V0X21vZGUoc3RydWN0IGt4Y2prMTAxM19kYXRhICpkYXRhLA0KPiA+
ID4gIAkJCSAgICAgIGVudW0ga3hjamsxMDEzX21vZGUgbW9kZSkNCj4gPiA+ICB7DQo+ID4gPiBA
QCAtOTU1LDE4ICs5NzQsMTQgQEAgc3RhdGljIGlycXJldHVybl90IGt4Y2prMTAxM190cmlnZ2Vy
X2hhbmRsZXIoaW50IGlycSwgdm9pZCAqcCkNCj4gPiA+ICAJc3RydWN0IGlpb19wb2xsX2Z1bmMg
KnBmID0gcDsNCj4gPiA+ICAJc3RydWN0IGlpb19kZXYgKmluZGlvX2RldiA9IHBmLT5pbmRpb19k
ZXY7DQo+ID4gPiAgCXN0cnVjdCBreGNqazEwMTNfZGF0YSAqZGF0YSA9IGlpb19wcml2KGluZGlv
X2Rldik7DQo+ID4gPiAtCWludCBiaXQsIHJldCwgaSA9IDA7DQo+ID4gPiArCWludCByZXQ7DQo+
ID4gPg0KPiA+ID4gIAltdXRleF9sb2NrKCZkYXRhLT5tdXRleCk7DQo+ID4gPiAtCWZvciAoYml0
ID0gMDsgYml0IDwgQVhJU19NQVg7IGJpdCsrKSB7DQo+ID4gPiAtCQlyZXQgPSBreGNqazEwMTNf
Z2V0X2FjY19yZWcoZGF0YSwgYml0KTsNCj4gPiA+IC0JCWlmIChyZXQgPCAwKSB7DQo+ID4gPiAt
CQkJbXV0ZXhfdW5sb2NrKCZkYXRhLT5tdXRleCk7DQo+ID4gPiAtCQkJZ290byBlcnI7DQo+ID4g
PiAtCQl9DQo+ID4gPiAtCQlkYXRhLT5idWZmZXJbaSsrXSA9IHJldDsNCj4gPiA+IC0JfQ0KPiA+
ID4gKwlyZXQgPSBkYXRhLT5yZWFkX2Jsb2NrX2RhdGEoZGF0YS0+Y2xpZW50LCBLWENKSzEwMTNf
UkVHX1hPVVRfTCwNCj4gPiA+ICsJCQkJICAgIEFYSVNfTUFYICogMiwgKHU4ICopIGRhdGEtPmJ1
ZmZlcik7DQo+ID4gPiAgCW11dGV4X3VubG9jaygmZGF0YS0+bXV0ZXgpOw0KPiA+ID4gKwlpZiAo
cmV0IDwgMCkNCj4gPiA+ICsJCWdvdG8gZXJyOw0KPiA+ID4NCj4gPiA+ICAJaWlvX3B1c2hfdG9f
YnVmZmVyc193aXRoX3RpbWVzdGFtcChpbmRpb19kZXYsIGRhdGEtPmJ1ZmZlciwNCj4gPiA+ICAJ
CQkJCSAgIGRhdGEtPnRpbWVzdGFtcCk7DQo+ID4gPiBAQCAtMTE5Niw2ICsxMjExLDExIEBAIHN0
YXRpYyBpbnQga3hjamsxMDEzX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQsDQo+ID4g
PiAgCWNvbnN0IGNoYXIgKm5hbWU7DQo+ID4gPiAgCWludCByZXQ7DQo+ID4gPg0KPiA+ID4gKwlp
ZiAoIWkyY19jaGVja19mdW5jdGlvbmFsaXR5KGNsaWVudC0+YWRhcHRlciwNCj4gPiA+ICsJCQkJ
ICAgICBJMkNfRlVOQ19TTUJVU19CWVRFX0RBVEEgfA0KPiA+ID4gKwkJCQkgICAgIEkyQ19GVU5D
X1NNQlVTX1JFQURfV09SRF9EQVRBKSkNCj4gPiA+ICsJCXJldHVybiAtRU5PREVWOw0KPiA+ID4g
Kw0KPiA+ID4gIAlpbmRpb19kZXYgPSBkZXZtX2lpb19kZXZpY2VfYWxsb2MoJmNsaWVudC0+ZGV2
LCBzaXplb2YoKmRhdGEpKTsNCj4gPiA+ICAJaWYgKCFpbmRpb19kZXYpDQo+ID4gPiAgCQlyZXR1
cm4gLUVOT01FTTsNCj4gPiA+IEBAIC0xMjA0LDYgKzEyMjQsMTIgQEAgc3RhdGljIGludCBreGNq
azEwMTNfcHJvYmUoc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCwNCj4gPiA+ICAJaTJjX3NldF9j
bGllbnRkYXRhKGNsaWVudCwgaW5kaW9fZGV2KTsNCj4gPiA+ICAJZGF0YS0+Y2xpZW50ID0gY2xp
ZW50Ow0KPiA+ID4NCj4gPiA+ICsJaWYgKGkyY19jaGVja19mdW5jdGlvbmFsaXR5KGNsaWVudC0+
YWRhcHRlciwNCj4gPiA+ICsJCQkJICAgIEkyQ19GVU5DX1NNQlVTX1JFQURfSTJDX0JMT0NLKSkN
Cj4gPiA+ICsJCWRhdGEtPnJlYWRfYmxvY2tfZGF0YSA9IGkyY19zbWJ1c19yZWFkX2kyY19ibG9j
a19kYXRhOw0KPiA+ID4gKwllbHNlDQo+ID4gPiArCQlkYXRhLT5yZWFkX2Jsb2NrX2RhdGEgPSBr
eGNqazEwMTNfcmVhZF9ibG9ja19kYXRhOw0KPiA+ID4gKw0KPiA+ID4gIAlwZGF0YSA9IGRldl9n
ZXRfcGxhdGRhdGEoJmNsaWVudC0+ZGV2KTsNCj4gPiA+ICAJaWYgKHBkYXRhKQ0KPiA+ID4gIAkJ
ZGF0YS0+YWN0aXZlX2hpZ2hfaW50ciA9IHBkYXRhLT5hY3RpdmVfaGlnaF9pbnRyOw0KPiA+ID4N
Cj4gPiANCj4gPiAtLQ0KPiA+IA0KPiA+IFBldGVyIE1lZXJ3YWxkDQo+ID4gKzQzLTY2NC0yNDQ0
NDE4IChtb2JpbGUpDQoNCg==
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
2015-02-20 14:58 ` Pandruvada, Srinivas
(?)
@ 2015-02-20 15:23 ` Peter Meerwald
2015-02-20 15:27 ` Pandruvada, Srinivas
-1 siblings, 1 reply; 18+ messages in thread
From: Peter Meerwald @ 2015-02-20 15:23 UTC (permalink / raw)
To: Pandruvada, Srinivas
Cc: Tirdea, Irina, linux-kernel, jic23, Reus, Adriana, linux-iio
> > > > +
> > > > + values[i] = data & 0xFF;
> > > > + values[i+1] = data >> 8;
> > >
> > > this is incorrect; it forces the data to be little endian, however, the
> > > endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> > > code breaks for big-endian CPUs
> > >
> > > since _read_i2c_block_data() can't do endianness conversion (and the chip
> > > does i2c endianness, i.e. little-endian), the .scan_type should become
> > > IIO_LE and above code is correct again but still ugly :)
> > >
> > > bottom line: change .scan_type to IIO_LE
> > >
> > Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> > Will fix this in the next version. Thanks for catching this!
> >
> I don't think changing to IIO_LE is good idea as when i2c_read_bock..
> then the scan type will be CPU. So better to fix endianness in this
> function.
the chip has little-endian data registers; i2c_read_block() just transfers
the data (no endianness conversion), so the data will still be
little-endian
p.
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
2015-02-20 15:23 ` Peter Meerwald
@ 2015-02-20 15:27 ` Pandruvada, Srinivas
0 siblings, 0 replies; 18+ messages in thread
From: Pandruvada, Srinivas @ 2015-02-20 15:27 UTC (permalink / raw)
To: pmeerw; +Cc: linux-kernel, linux-iio, Tirdea, Irina, jic23, Reus, Adriana
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1372 bytes --]
On Fri, 2015-02-20 at 16:23 +0100, Peter Meerwald wrote:
> > > > > +
> > > > > + values[i] = data & 0xFF;
> > > > > + values[i+1] = data >> 8;
> > > >
> > > > this is incorrect; it forces the data to be little endian, however, the
> > > > endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> > > > code breaks for big-endian CPUs
> > > >
> > > > since _read_i2c_block_data() can't do endianness conversion (and the chip
> > > > does i2c endianness, i.e. little-endian), the .scan_type should become
> > > > IIO_LE and above code is correct again but still ugly :)
> > > >
> > > > bottom line: change .scan_type to IIO_LE
> > > >
> > > Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> > > Will fix this in the next version. Thanks for catching this!
> > >
> > I don't think changing to IIO_LE is good idea as when i2c_read_bock..
> > then the scan type will be CPU. So better to fix endianness in this
> > function.
>
> the chip has little-endian data registers; i2c_read_block() just transfers
> the data (no endianness conversion), so the data will still be
> little-endian
You are right.
>
> p.
>
ÿôèº{.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] 18+ messages in thread
* Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
@ 2015-02-20 15:27 ` Pandruvada, Srinivas
0 siblings, 0 replies; 18+ messages in thread
From: Pandruvada, Srinivas @ 2015-02-20 15:27 UTC (permalink / raw)
To: pmeerw; +Cc: linux-kernel, linux-iio, Tirdea, Irina, jic23, Reus, Adriana
T24gRnJpLCAyMDE1LTAyLTIwIGF0IDE2OjIzICswMTAwLCBQZXRlciBNZWVyd2FsZCB3cm90ZToN
Cj4gPiA+ID4gPiArDQo+ID4gPiA+ID4gKwkJdmFsdWVzW2ldID0gZGF0YSAmIDB4RkY7DQo+ID4g
PiA+ID4gKwkJdmFsdWVzW2krMV0gPSBkYXRhID4+IDg7DQo+ID4gPiA+IA0KPiA+ID4gPiB0aGlz
IGlzIGluY29ycmVjdDsgaXQgZm9yY2VzIHRoZSBkYXRhIHRvIGJlIGxpdHRsZSBlbmRpYW4sIGhv
d2V2ZXIsIHRoZQ0KPiA+ID4gPiBlbmRpYW5uZXNzIChhcyBzcGVjaWZpZWQgaW4gdGhlIGRyaXZl
cidzIC5zY2FuX3R5cGUpIGlzIElJT19DUFUgLS0gdGhlDQo+ID4gPiA+IGNvZGUgYnJlYWtzIGZv
ciBiaWctZW5kaWFuIENQVXMNCj4gPiA+ID4gDQo+ID4gPiA+IHNpbmNlIF9yZWFkX2kyY19ibG9j
a19kYXRhKCkgY2FuJ3QgZG8gZW5kaWFubmVzcyBjb252ZXJzaW9uIChhbmQgdGhlIGNoaXANCj4g
PiA+ID4gZG9lcyBpMmMgZW5kaWFubmVzcywgaS5lLiBsaXR0bGUtZW5kaWFuKSwgdGhlIC5zY2Fu
X3R5cGUgc2hvdWxkIGJlY29tZQ0KPiA+ID4gPiBJSU9fTEUgYW5kIGFib3ZlIGNvZGUgaXMgY29y
cmVjdCBhZ2FpbiBidXQgc3RpbGwgdWdseSA6KQ0KPiA+ID4gPiANCj4gPiA+ID4gYm90dG9tIGxp
bmU6IGNoYW5nZSAuc2Nhbl90eXBlIHRvIElJT19MRQ0KPiA+ID4gPiANCj4gPiA+IEdvb2QgcG9p
bnQuIENoYW5naW5nIHRoZSBlbmRpYW5lc3MgdG8gSUlPX0xFIGlzIGNvcnJlY3QgZm9yIGVpdGhl
ciBreGNqazEwMTNfcmVhZF9ibG9ja19kYXRhIG9yIGkyY19zbWJ1c19yZWFkX2kyY19ibG9ja19k
YXRhLg0KPiA+ID4gV2lsbCBmaXggdGhpcyBpbiB0aGUgbmV4dCB2ZXJzaW9uLiBUaGFua3MgZm9y
IGNhdGNoaW5nIHRoaXMhDQo+ID4gPiANCj4gPiBJIGRvbid0IHRoaW5rIGNoYW5naW5nIHRvIElJ
T19MRSBpcyBnb29kIGlkZWEgYXMgd2hlbiBpMmNfcmVhZF9ib2NrLi4NCj4gPiB0aGVuIHRoZSBz
Y2FuIHR5cGUgd2lsbCBiZSBDUFUuIFNvIGJldHRlciB0byBmaXggZW5kaWFubmVzcyBpbiB0aGlz
DQo+ID4gZnVuY3Rpb24uDQo+IA0KPiB0aGUgY2hpcCBoYXMgbGl0dGxlLWVuZGlhbiBkYXRhIHJl
Z2lzdGVyczsgaTJjX3JlYWRfYmxvY2soKSBqdXN0IHRyYW5zZmVycyANCj4gdGhlIGRhdGEgKG5v
IGVuZGlhbm5lc3MgY29udmVyc2lvbiksIHNvIHRoZSBkYXRhIHdpbGwgc3RpbGwgYmUgDQo+IGxp
dHRsZS1lbmRpYW4NCllvdSBhcmUgcmlnaHQuDQo+IA0KPiBwLg0KPiANCg0K
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
2015-02-20 12:02 ` Tirdea, Irina
(?)
(?)
@ 2015-02-21 18:29 ` Jonathan Cameron
2015-02-24 17:25 ` Tirdea, Irina
-1 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2015-02-21 18:29 UTC (permalink / raw)
To: Tirdea, Irina, Peter Meerwald
Cc: linux-iio, linux-kernel, Pandruvada, Srinivas, Reus, Adriana
On 20/02/15 12:02, Tirdea, Irina wrote:
>
>
>> -----Original Message-----
>> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
>> Sent: 16 February, 2015 21:26
>> To: Tirdea, Irina
>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
>> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>>
>>
>>> Reading all axis values in one i2c transfer reduces the delays
>>> introduced by the i2c bus. In case i2c block read is not supported,
>>> fallback to reading each axis as a separate word.
>>
>> see comments inline below
>>
> Thanks for the review, Peter!
>
>>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
>>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>>> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> ---
>>> drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 35 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>>> index 5f27787..bfa2899 100644
>>> --- a/drivers/iio/accel/kxcjk-1013.c
>>> +++ b/drivers/iio/accel/kxcjk-1013.c
>>> @@ -109,6 +109,8 @@ struct kxcjk1013_data {
>>> int64_t timestamp;
>>> enum kx_chipset chipset;
>>> bool is_smo8500_device;
>>> + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
>>> + u8 length, u8 *values);
>>
>> probably this could/should be done in the i2c layer or we end up adding a
>> similar function in every driver?
>>
> Actually this is exactly what I did: adding this code in a couple of drivers :)
> You are right, this belongs to the i2c core. Will move it there.
A good idea. Might be possible to make this a transparent fallback so no
special handling is needed in drivers at all (e.g. emulate the block read using
the biggest read that is available) - taking care about the endian fun and
games that results.
Propose it to Wolfram and the i2c list and see what people think of it.
Jonathan
>
>>> };
>>>
>>> enum kxcjk1013_axis {
>>> @@ -216,6 +218,23 @@ static const struct {
>>> {800, 0, 0x06},
>>> {1600, 0, 0x06} };
>>>
>>> +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
>>> + u8 command, u8 length, u8 *values)
>>> +{
>>> + s32 data;
>>> + u8 i;
>>> +
>>> + for (i = 0; i < length; i += 2) {
>>> + data = i2c_smbus_read_word_data(client, command + i);
>>> + if (data < 0)
>>> + return data;
>>> +
>>> + values[i] = data & 0xFF;
>>> + values[i+1] = data >> 8;
>>
>> this is incorrect; it forces the data to be little endian, however, the
>> endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
>> code breaks for big-endian CPUs
>>
>> since _read_i2c_block_data() can't do endianness conversion (and the chip
>> does i2c endianness, i.e. little-endian), the .scan_type should become
>> IIO_LE and above code is correct again but still ugly :)
>>
>> bottom line: change .scan_type to IIO_LE
>>
> Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> Will fix this in the next version. Thanks for catching this!
>
>>> + }
>>> + return i;
>>> +}
>>> +
>>> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
>>> enum kxcjk1013_mode mode)
>>> {
>>> @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
>>> struct iio_poll_func *pf = p;
>>> struct iio_dev *indio_dev = pf->indio_dev;
>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> - int bit, ret, i = 0;
>>> + int ret;
>>>
>>> mutex_lock(&data->mutex);
>>> - for (bit = 0; bit < AXIS_MAX; bit++) {
>>> - ret = kxcjk1013_get_acc_reg(data, bit);
>>> - if (ret < 0) {
>>> - mutex_unlock(&data->mutex);
>>> - goto err;
>>> - }
>>> - data->buffer[i++] = ret;
>>> - }
>>> + ret = data->read_block_data(data->client, KXCJK1013_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,
>>> data->timestamp);
>>> @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>> const char *name;
>>> int ret;
>>>
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_BYTE_DATA |
>>> + I2C_FUNC_SMBUS_READ_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>> if (!indio_dev)
>>> return -ENOMEM;
>>> @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>> i2c_set_clientdata(client, indio_dev);
>>> data->client = client;
>>>
>>> + if (i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
>>> + data->read_block_data = i2c_smbus_read_i2c_block_data;
>>> + else
>>> + data->read_block_data = kxcjk1013_read_block_data;
>>> +
>>> pdata = dev_get_platdata(&client->dev);
>>> if (pdata)
>>> data->active_high_intr = pdata->active_high_intr;
>>>
>>
>> --
>>
>> Peter Meerwald
>> +43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
2015-02-21 18:29 ` Jonathan Cameron
@ 2015-02-24 17:25 ` Tirdea, Irina
0 siblings, 0 replies; 18+ messages in thread
From: Tirdea, Irina @ 2015-02-24 17:25 UTC (permalink / raw)
To: Jonathan Cameron, Peter Meerwald
Cc: linux-iio, linux-kernel, Pandruvada, Srinivas, Reus, Adriana
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 21 February, 2015 20:30
> To: Tirdea, Irina; Peter Meerwald
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>
> On 20/02/15 12:02, Tirdea, Irina wrote:
> >
> >
> >> -----Original Message-----
> >> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
> >> Sent: 16 February, 2015 21:26
> >> To: Tirdea, Irina
> >> Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
> >> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >>
> >>
> >>> Reading all axis values in one i2c transfer reduces the delays
> >>> introduced by the i2c bus. In case i2c block read is not supported,
> >>> fallback to reading each axis as a separate word.
> >>
> >> see comments inline below
> >>
> > Thanks for the review, Peter!
> >
> >>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> >>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> >>> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> >>> ---
> >>> drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
> >>> 1 file changed, 35 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> >>> index 5f27787..bfa2899 100644
> >>> --- a/drivers/iio/accel/kxcjk-1013.c
> >>> +++ b/drivers/iio/accel/kxcjk-1013.c
> >>> @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> >>> int64_t timestamp;
> >>> enum kx_chipset chipset;
> >>> bool is_smo8500_device;
> >>> + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> >>> + u8 length, u8 *values);
> >>
> >> probably this could/should be done in the i2c layer or we end up adding a
> >> similar function in every driver?
> >>
> > Actually this is exactly what I did: adding this code in a couple of drivers :)
> > You are right, this belongs to the i2c core. Will move it there.
> A good idea. Might be possible to make this a transparent fallback so no
> special handling is needed in drivers at all (e.g. emulate the block read using
> the biggest read that is available) - taking care about the endian fun and
> games that results.
>
> Propose it to Wolfram and the i2c list and see what people think of it.
>
I'll do the block emulation code first and send it on the i2c list (still checking some endianness issues).
Once I'll have a response for the i2c part, I'll resend these patchsets with the required fixes.
Thanks,
Irina
> Jonathan
> >
> >>> };
> >>>
> >>> enum kxcjk1013_axis {
> >>> @@ -216,6 +218,23 @@ static const struct {
> >>> {800, 0, 0x06},
> >>> {1600, 0, 0x06} };
> >>>
> >>> +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
> >>> + u8 command, u8 length, u8 *values)
> >>> +{
> >>> + s32 data;
> >>> + u8 i;
> >>> +
> >>> + for (i = 0; i < length; i += 2) {
> >>> + data = i2c_smbus_read_word_data(client, command + i);
> >>> + if (data < 0)
> >>> + return data;
> >>> +
> >>> + values[i] = data & 0xFF;
> >>> + values[i+1] = data >> 8;
> >>
> >> this is incorrect; it forces the data to be little endian, however, the
> >> endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> >> code breaks for big-endian CPUs
> >>
> >> since _read_i2c_block_data() can't do endianness conversion (and the chip
> >> does i2c endianness, i.e. little-endian), the .scan_type should become
> >> IIO_LE and above code is correct again but still ugly :)
> >>
> >> bottom line: change .scan_type to IIO_LE
> >>
> > Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> > Will fix this in the next version. Thanks for catching this!
> >
> >>> + }
> >>> + return i;
> >>> +}
> >>> +
> >>> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> >>> enum kxcjk1013_mode mode)
> >>> {
> >>> @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
> >>> struct iio_poll_func *pf = p;
> >>> struct iio_dev *indio_dev = pf->indio_dev;
> >>> struct kxcjk1013_data *data = iio_priv(indio_dev);
> >>> - int bit, ret, i = 0;
> >>> + int ret;
> >>>
> >>> mutex_lock(&data->mutex);
> >>> - for (bit = 0; bit < AXIS_MAX; bit++) {
> >>> - ret = kxcjk1013_get_acc_reg(data, bit);
> >>> - if (ret < 0) {
> >>> - mutex_unlock(&data->mutex);
> >>> - goto err;
> >>> - }
> >>> - data->buffer[i++] = ret;
> >>> - }
> >>> + ret = data->read_block_data(data->client, KXCJK1013_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,
> >>> data->timestamp);
> >>> @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
> >>> const char *name;
> >>> int ret;
> >>>
> >>> + if (!i2c_check_functionality(client->adapter,
> >>> + I2C_FUNC_SMBUS_BYTE_DATA |
> >>> + I2C_FUNC_SMBUS_READ_WORD_DATA))
> >>> + return -ENODEV;
> >>> +
> >>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >>> if (!indio_dev)
> >>> return -ENOMEM;
> >>> @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
> >>> i2c_set_clientdata(client, indio_dev);
> >>> data->client = client;
> >>>
> >>> + if (i2c_check_functionality(client->adapter,
> >>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> >>> + data->read_block_data = i2c_smbus_read_i2c_block_data;
> >>> + else
> >>> + data->read_block_data = kxcjk1013_read_block_data;
> >>> +
> >>> pdata = dev_get_platdata(&client->dev);
> >>> if (pdata)
> >>> data->active_high_intr = pdata->active_high_intr;
> >>>
> >>
> >> --
> >>
> >> Peter Meerwald
> >> +43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
@ 2015-02-24 17:25 ` Tirdea, Irina
0 siblings, 0 replies; 18+ messages in thread
From: Tirdea, Irina @ 2015-02-24 17:25 UTC (permalink / raw)
To: Jonathan Cameron, Peter Meerwald
Cc: linux-iio, linux-kernel, Pandruvada, Srinivas, Reus, Adriana
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 21 February, 2015 20:30
> To: Tirdea, Irina; Peter Meerwald
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, =
Srinivas; Reus, Adriana
> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers i=
n trigger handler
>=20
> On 20/02/15 12:02, Tirdea, Irina wrote:
> >
> >
> >> -----Original Message-----
> >> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
> >> Sent: 16 February, 2015 21:26
> >> To: Tirdea, Irina
> >> Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.ker=
nel.org; Pandruvada, Srinivas; Reus, Adriana
> >> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfer=
s in trigger handler
> >>
> >>
> >>> Reading all axis values in one i2c transfer reduces the delays
> >>> introduced by the i2c bus. In case i2c block read is not supported,
> >>> fallback to reading each axis as a separate word.
> >>
> >> see comments inline below
> >>
> > Thanks for the review, Peter!
> >
> >>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> >>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> >>> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com=
>
> >>> ---
> >>> drivers/iio/accel/kxcjk-1013.c | 44 ++++++++++++++++++++++++++++++++=
+---------
> >>> 1 file changed, 35 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk=
-1013.c
> >>> index 5f27787..bfa2899 100644
> >>> --- a/drivers/iio/accel/kxcjk-1013.c
> >>> +++ b/drivers/iio/accel/kxcjk-1013.c
> >>> @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> >>> int64_t timestamp;
> >>> enum kx_chipset chipset;
> >>> bool is_smo8500_device;
> >>> + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> >>> + u8 length, u8 *values);
> >>
> >> probably this could/should be done in the i2c layer or we end up addin=
g a
> >> similar function in every driver?
> >>
> > Actually this is exactly what I did: adding this code in a couple of dr=
ivers :)
> > You are right, this belongs to the i2c core. Will move it there.
> A good idea. Might be possible to make this a transparent fallback so no
> special handling is needed in drivers at all (e.g. emulate the block read=
using
> the biggest read that is available) - taking care about the endian fun an=
d
> games that results.
>=20
> Propose it to Wolfram and the i2c list and see what people think of it.
>=20
I'll do the block emulation code first and send it on the i2c list (still c=
hecking some endianness issues).=20
Once I'll have a response for the i2c part, I'll resend these patchsets wit=
h the required fixes.
Thanks,
Irina
> Jonathan
> >
> >>> };
> >>>
> >>> enum kxcjk1013_axis {
> >>> @@ -216,6 +218,23 @@ static const struct {
> >>> {800, 0, 0x06},
> >>> {1600, 0, 0x06} };
> >>>
> >>> +static s32 kxcjk1013_read_block_data(const struct i2c_client *client=
,
> >>> + u8 command, u8 length, u8 *values)
> >>> +{
> >>> + s32 data;
> >>> + u8 i;
> >>> +
> >>> + for (i =3D 0; i < length; i +=3D 2) {
> >>> + data =3D i2c_smbus_read_word_data(client, command + i);
> >>> + if (data < 0)
> >>> + return data;
> >>> +
> >>> + values[i] =3D data & 0xFF;
> >>> + values[i+1] =3D data >> 8;
> >>
> >> this is incorrect; it forces the data to be little endian, however, th=
e
> >> endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> >> code breaks for big-endian CPUs
> >>
> >> since _read_i2c_block_data() can't do endianness conversion (and the c=
hip
> >> does i2c endianness, i.e. little-endian), the .scan_type should become
> >> IIO_LE and above code is correct again but still ugly :)
> >>
> >> bottom line: change .scan_type to IIO_LE
> >>
> > Good point. Changing the endianess to IIO_LE is correct for either kxcj=
k1013_read_block_data or i2c_smbus_read_i2c_block_data.
> > Will fix this in the next version. Thanks for catching this!
> >
> >>> + }
> >>> + return i;
> >>> +}
> >>> +
> >>> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> >>> enum kxcjk1013_mode mode)
> >>> {
> >>> @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(in=
t irq, void *p)
> >>> struct iio_poll_func *pf =3D p;
> >>> struct iio_dev *indio_dev =3D pf->indio_dev;
> >>> struct kxcjk1013_data *data =3D iio_priv(indio_dev);
> >>> - int bit, ret, i =3D 0;
> >>> + int ret;
> >>>
> >>> mutex_lock(&data->mutex);
> >>> - for (bit =3D 0; bit < AXIS_MAX; bit++) {
> >>> - ret =3D kxcjk1013_get_acc_reg(data, bit);
> >>> - if (ret < 0) {
> >>> - mutex_unlock(&data->mutex);
> >>> - goto err;
> >>> - }
> >>> - data->buffer[i++] =3D ret;
> >>> - }
> >>> + ret =3D data->read_block_data(data->client, KXCJK1013_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,
> >>> data->timestamp);
> >>> @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *=
client,
> >>> const char *name;
> >>> int ret;
> >>>
> >>> + if (!i2c_check_functionality(client->adapter,
> >>> + I2C_FUNC_SMBUS_BYTE_DATA |
> >>> + I2C_FUNC_SMBUS_READ_WORD_DATA))
> >>> + return -ENODEV;
> >>> +
> >>> indio_dev =3D devm_iio_device_alloc(&client->dev, sizeof(*data));
> >>> if (!indio_dev)
> >>> return -ENOMEM;
> >>> @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *=
client,
> >>> i2c_set_clientdata(client, indio_dev);
> >>> data->client =3D client;
> >>>
> >>> + if (i2c_check_functionality(client->adapter,
> >>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> >>> + data->read_block_data =3D i2c_smbus_read_i2c_block_data;
> >>> + else
> >>> + data->read_block_data =3D kxcjk1013_read_block_data;
> >>> +
> >>> pdata =3D dev_get_platdata(&client->dev);
> >>> if (pdata)
> >>> data->active_high_intr =3D pdata->active_high_intr;
> >>>
> >>
> >> --
> >>
> >> Peter Meerwald
> >> +43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 18+ messages in thread