LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] kxcjk-1013 driver optimizations
@ 2015-02-16 18:21 Irina Tirdea
  2015-02-16 18:21 ` [PATCH 1/2] iio: accel: kxcjk-1013: use available_scan_masks Irina Tirdea
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Irina Tirdea @ 2015-02-16 18:21 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: linux-kernel, Srinivas Pandruvada, Adriana Reus, 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


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

* [PATCH 1/2] iio: accel: kxcjk-1013: use available_scan_masks
  2015-02-16 18:21 [PATCH 0/2] kxcjk-1013 driver optimizations Irina Tirdea
@ 2015-02-16 18:21 ` Irina Tirdea
  2015-02-16 18:21 ` [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler Irina Tirdea
  2015-02-16 19:14 ` [PATCH 0/2] kxcjk-1013 driver optimizations Peter Meerwald
  2 siblings, 0 replies; 14+ messages in thread
From: Irina Tirdea @ 2015-02-16 18:21 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: linux-kernel, Srinivas Pandruvada, Adriana Reus, 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


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

* [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
  2015-02-16 18:21 [PATCH 0/2] kxcjk-1013 driver optimizations Irina Tirdea
  2015-02-16 18:21 ` [PATCH 1/2] iio: accel: kxcjk-1013: use available_scan_masks Irina Tirdea
@ 2015-02-16 18:21 ` Irina Tirdea
  2015-02-16 19:26   ` Peter Meerwald
  2015-02-16 19:14 ` [PATCH 0/2] kxcjk-1013 driver optimizations Peter Meerwald
  2 siblings, 1 reply; 14+ messages in thread
From: Irina Tirdea @ 2015-02-16 18:21 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: linux-kernel, Srinivas Pandruvada, Adriana Reus, 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


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

* Re: [PATCH 0/2] kxcjk-1013 driver optimizations
  2015-02-16 18:21 [PATCH 0/2] kxcjk-1013 driver optimizations Irina Tirdea
  2015-02-16 18:21 ` [PATCH 1/2] iio: accel: kxcjk-1013: use available_scan_masks Irina Tirdea
  2015-02-16 18:21 ` [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler Irina Tirdea
@ 2015-02-16 19:14 ` Peter Meerwald
  2015-02-20 12:03   ` Tirdea, Irina
  2 siblings, 1 reply; 14+ messages in thread
From: Peter Meerwald @ 2015-02-16 19:14 UTC (permalink / raw)
  To: Irina Tirdea
  Cc: Jonathan Cameron, linux-iio, linux-kernel, Srinivas Pandruvada,
	Adriana Reus


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

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

* Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
  2015-02-16 18:21 ` [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler Irina Tirdea
@ 2015-02-16 19:26   ` Peter Meerwald
  2015-02-20 12:02     ` Tirdea, Irina
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Meerwald @ 2015-02-16 19:26 UTC (permalink / raw)
  To: Irina Tirdea
  Cc: Jonathan Cameron, linux-iio, linux-kernel, Srinivas Pandruvada,
	Adriana Reus


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

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

* RE: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
  2015-02-16 19:26   ` Peter Meerwald
@ 2015-02-20 12:02     ` Tirdea, Irina
  2015-02-20 14:58       ` Pandruvada, Srinivas
  2015-02-21 18:29       ` Jonathan Cameron
  0 siblings, 2 replies; 14+ 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 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)

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

* RE: [PATCH 0/2] kxcjk-1013 driver optimizations
  2015-02-16 19:14 ` [PATCH 0/2] kxcjk-1013 driver optimizations Peter Meerwald
@ 2015-02-20 12:03   ` Tirdea, Irina
  2015-02-21 18:26     ` Jonathan Cameron
  0 siblings, 1 reply; 14+ messages in thread
From: Tirdea, Irina @ 2015-02-20 12:03 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: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)

^ permalink raw reply	[flat|nested] 14+ 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
  2015-02-20 15:23         ` Peter Meerwald
  2015-02-21 18:29       ` Jonathan Cameron
  1 sibling, 1 reply; 14+ 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] 14+ 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
  0 siblings, 1 reply; 14+ 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] 14+ 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; 14+ 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] 14+ messages in thread

* Re: [PATCH 0/2] kxcjk-1013 driver optimizations
  2015-02-20 12:03   ` Tirdea, Irina
@ 2015-02-21 18:26     ` Jonathan Cameron
  2015-02-24 17:22       ` Tirdea, Irina
  0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2015-02-21 18:26 UTC (permalink / raw)
  To: Tirdea, Irina, Peter Meerwald
  Cc: linux-iio, linux-kernel, Pandruvada, Srinivas, Reus, Adriana

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
> 


^ permalink raw reply	[flat|nested] 14+ 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
@ 2015-02-21 18:29       ` Jonathan Cameron
  2015-02-24 17:25         ` Tirdea, Irina
  1 sibling, 1 reply; 14+ 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] 14+ messages in thread

* RE: [PATCH 0/2] kxcjk-1013 driver optimizations
  2015-02-21 18:26     ` Jonathan Cameron
@ 2015-02-24 17:22       ` Tirdea, Irina
  0 siblings, 0 replies; 14+ messages in thread
From: Tirdea, Irina @ 2015-02-24 17:22 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: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
> >


^ permalink raw reply	[flat|nested] 14+ 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; 14+ 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] 14+ messages in thread

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

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-16 18:21 [PATCH 0/2] kxcjk-1013 driver optimizations Irina Tirdea
2015-02-16 18:21 ` [PATCH 1/2] iio: accel: kxcjk-1013: use available_scan_masks Irina Tirdea
2015-02-16 18:21 ` [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler Irina Tirdea
2015-02-16 19:26   ` Peter Meerwald
2015-02-20 12:02     ` Tirdea, Irina
2015-02-20 14:58       ` Pandruvada, Srinivas
2015-02-20 15:23         ` Peter Meerwald
2015-02-20 15:27           ` Pandruvada, Srinivas
2015-02-21 18:29       ` Jonathan Cameron
2015-02-24 17:25         ` Tirdea, Irina
2015-02-16 19:14 ` [PATCH 0/2] kxcjk-1013 driver optimizations Peter Meerwald
2015-02-20 12:03   ` Tirdea, Irina
2015-02-21 18:26     ` Jonathan Cameron
2015-02-24 17:22       ` Tirdea, Irina

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