[PATCH 0/2] kxcjk-1013 driver optimizations

[PATCH 0/2] kxcjk-1013 driver optimizations

[Irina Tirdea]

Adds optimization of i2c transactions in trigger handler and
usage of available_scan_masks for kxcjk-1013.

Adriana Reus (2):
  iio: accel: kxcjk-1013: use available_scan_masks
  iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

 drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH 1/2] iio: accel: kxcjk-1013: use available_scan_masks

[Irina Tirdea]

From: Adriana Reus <adriana.reus@intel.com>

Use available_scan_masks to allow the iio core to select
the data to send to userspace depending on which axes are
enabled, instead of doing this in the driver's interrupt
handler.

This also fixes the issue of accessing the buffer scan_mask
instead of active_scan_mask, since these might not be the
same due to client devices.

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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 567de26..5f27787 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -115,6 +115,7 @@ enum kxcjk1013_axis {
 	AXIS_X,
 	AXIS_Y,
 	AXIS_Z,
+	AXIS_MAX,
 };
 
 enum kxcjk1013_mode {
@@ -947,6 +948,8 @@ static const struct iio_info kxcjk1013_info = {
 	.driver_module		= THIS_MODULE,
 };
 
+static const unsigned long kxcjk1013_scan_masks[] = {0x7, 0};
+
 static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -955,9 +958,7 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
 	int bit, ret, i = 0;
 
 	mutex_lock(&data->mutex);
-
-	for_each_set_bit(bit, indio_dev->buffer->scan_mask,
-			 indio_dev->masklength) {
+	for (bit = 0; bit < AXIS_MAX; bit++) {
 		ret = kxcjk1013_get_acc_reg(data, bit);
 		if (ret < 0) {
 			mutex_unlock(&data->mutex);
@@ -1228,6 +1229,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->channels = kxcjk1013_channels;
 	indio_dev->num_channels = ARRAY_SIZE(kxcjk1013_channels);
+	indio_dev->available_scan_masks = kxcjk1013_scan_masks;
 	indio_dev->name = name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &kxcjk1013_info;
-- 
1.9.1

[PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Irina Tirdea]

From: Adriana Reus <adriana.reus@intel.com>

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 kxcjk-1013 accel driver
does one i2c transfer for each axis. This has an impact on the
frequency of the accelerometer 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. In case i2c block read is not supported,
fallback to reading each axis as a separate word.

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);
 };
 
 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;
+	}
+	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;
-- 
1.9.1

Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Peter Meerwald]

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

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

> +	}
> +	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)

RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Tirdea, Irina]

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

RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Tirdea, Irina]

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

> > +	}
> > +	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)

Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Pandruvada, Srinivas]

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

Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 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¥

Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Peter Meerwald]

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

Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Pandruvada, Srinivas]

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

Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 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¥

Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Jonathan Cameron]

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)

RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Tirdea, Irina]

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

RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler

[Tirdea, Irina]

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

Re: [PATCH 0/2] kxcjk-1013 driver optimizations

[Peter Meerwald]

> Adds optimization of i2c transactions in trigger handler and
> usage of available_scan_masks for kxcjk-1013.

the 'optimization' changes the capabilities of the driver, reading 
individual channel is not possible anymore

> Adriana Reus (2):
>   iio: accel: kxcjk-1013: use available_scan_masks
>   iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> 
>  drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 11 deletions(-)
> 
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

RE: [PATCH 0/2] kxcjk-1013 driver optimizations

[Tirdea, Irina]

> -----Original Message-----
> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
> Sent: 16 February, 2015 21:14
> To: Tirdea, Irina
> Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
> 
> 
> > Adds optimization of i2c transactions in trigger handler and
> > usage of available_scan_masks for kxcjk-1013.
> 
> the 'optimization' changes the capabilities of the driver, reading
> individual channel is not possible anymore
> 
We can still read individual channels, just that the driver does not see that anymore and the demux is handled by the iio core.
available_scan_masks contains the bitmap superset for all channels we can enable (I will remove the 0 available_scan_mask as it does not makes sense).
When a new channel is enabled, the iio core checks if the new mask is a subset of any entry in available_scan_masks. When the buffer is pushed from the driver, the iio core will select only the enabled channel values.

> > Adriana Reus (2):
> >   iio: accel: kxcjk-1013: use available_scan_masks
> >   iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >
> >  drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
> >  1 file changed, 39 insertions(+), 11 deletions(-)
> >
> >
> 
> --
> 
> Peter Meerwald
> +43-664-2444418 (mobile)

Re: [PATCH 0/2] kxcjk-1013 driver optimizations

[Jonathan Cameron]

On 20/02/15 12:03, Tirdea, Irina wrote:
> 
> 
>> -----Original Message-----
>> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
>> Sent: 16 February, 2015 21:14
>> To: Tirdea, Irina
>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
>> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
>>
>>
>>> Adds optimization of i2c transactions in trigger handler and
>>> usage of available_scan_masks for kxcjk-1013.
>>
>> the 'optimization' changes the capabilities of the driver, reading
>> individual channel is not possible anymore
>>
> We can still read individual channels, just that the driver does not see that anymore and the demux is handled by the iio core.
> available_scan_masks contains the bitmap superset for all channels we can enable (I will remove the 0 available_scan_mask as it does not makes sense).
Careful. That's there as a null terminator for this list of available scan masks.
> When a new channel is enabled, the iio core checks if the new mask is a subset of any entry in available_scan_masks. 
> When the buffer is pushed from the driver, the iio core will select only the enabled channel values.

Pretty much.  IIRC This originally went in as part of handling multiple clients on these
data streams, but it was rapidly pointed out that lots of drivers spun their own
versions to break out particular elements from a multichannel read.
As such, using the generic form saved on repetition in these cases as well.
A good, complex example is drivers/iio/adc/max1363 in which really weird combinations
of channels (but far from all) are available for bulk capture and reading.
> 
>>> Adriana Reus (2):
>>>   iio: accel: kxcjk-1013: use available_scan_masks
>>>   iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>>>
>>>  drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 39 insertions(+), 11 deletions(-)
>>>
>>>
>>
>> --
>>
>> Peter Meerwald
>> +43-664-2444418 (mobile)
> --
> 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
> 

RE: [PATCH 0/2] kxcjk-1013 driver optimizations

[Tirdea, Irina]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 21 February, 2015 20:27
> To: Tirdea, Irina; Peter Meerwald
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
> 
> On 20/02/15 12:03, Tirdea, Irina wrote:
> >
> >
> >> -----Original Message-----
> >> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
> >> Sent: 16 February, 2015 21:14
> >> To: Tirdea, Irina
> >> Cc: Jonathan Cameron; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Reus, Adriana
> >> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
> >>
> >>
> >>> Adds optimization of i2c transactions in trigger handler and
> >>> usage of available_scan_masks for kxcjk-1013.
> >>
> >> the 'optimization' changes the capabilities of the driver, reading
> >> individual channel is not possible anymore
> >>
> > We can still read individual channels, just that the driver does not see that anymore and the demux is handled by the iio core.
> > available_scan_masks contains the bitmap superset for all channels we can enable (I will remove the 0 available_scan_mask as it
> does not makes sense).
> Careful. That's there as a null terminator for this list of available scan masks.
Missed this. You are right, thanks! Seems it's good as is.

> > When a new channel is enabled, the iio core checks if the new mask is a subset of any entry in available_scan_masks.
> > When the buffer is pushed from the driver, the iio core will select only the enabled channel values.
> 
> Pretty much.  IIRC This originally went in as part of handling multiple clients on these
> data streams, but it was rapidly pointed out that lots of drivers spun their own
> versions to break out particular elements from a multichannel read.
> As such, using the generic form saved on repetition in these cases as well.
> A good, complex example is drivers/iio/adc/max1363 in which really weird combinations
> of channels (but far from all) are available for bulk capture and reading.
> >
> >>> Adriana Reus (2):
> >>>   iio: accel: kxcjk-1013: use available_scan_masks
> >>>   iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >>>
> >>>  drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
> >>>  1 file changed, 39 insertions(+), 11 deletions(-)
> >>>
> >>>
> >>
> >> --
> >>
> >> Peter Meerwald
> >> +43-664-2444418 (mobile)
> > --
> > 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
> >