[PATCH 0/6] Driver optimizations in trigger handler

[PATCH 0/6] Driver optimizations in trigger handler

[Irina Tirdea]

This patchset adds optimization of i2c transactions in trigger handler
for bmc150, bmg160 and kxcjk-1013 drivers. It also introduces the
usage of available_scan_masks.

The code for bmc150 and bmg160 drivers is a rewrite of a previous
version [1] that takes into account the usage of regmap. The code
for kxcjk-1013 is the same as in the previous patch [1],
but included here for merging (since the
i2c_smbus_read_i2c_block_data_or_emulated API has been merged in
the iio tree).

[1] https://lkml.org/lkml/2015/8/12/609

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

Irina Tirdea (4):
  iio: accel: bmc150: use available_scan_masks
  iio: accel: bmc150: optimize transfers in trigger handler
  iio: gyro: bmg160: use available_scan_masks
  iio: accel: bmg160: optimize transfers in trigger handler

 drivers/iio/accel/bmc150-accel-core.c | 25 ++++++++++++-------------
 drivers/iio/accel/kxcjk-1013.c        | 24 ++++++++++++------------
 drivers/iio/gyro/bmg160_core.c        | 24 ++++++++++++------------
 3 files changed, 36 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH 1/6] iio: accel: bmc150: use available_scan_masks

[Irina Tirdea]

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.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index c73331f7..cc52366 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -138,6 +138,7 @@ enum bmc150_accel_axis {
 	AXIS_X,
 	AXIS_Y,
 	AXIS_Z,
+	AXIS_MAX,
 };
 
 enum bmc150_power_modes {
@@ -1104,6 +1105,10 @@ static const struct iio_info bmc150_accel_info_fifo = {
 	.driver_module		= THIS_MODULE,
 };
 
+static const unsigned long bmc150_accel_scan_masks[] = {
+					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+					0};
+
 static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1113,8 +1118,7 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
 	unsigned int raw_val;
 
 	mutex_lock(&data->mutex);
-	for_each_set_bit(bit, indio_dev->active_scan_mask,
-			 indio_dev->masklength) {
+	for (bit = 0; bit < AXIS_MAX; bit++) {
 		ret = regmap_bulk_read(data->regmap,
 				       BMC150_ACCEL_AXIS_TO_REG(bit), &raw_val,
 				       2);
@@ -1574,6 +1578,7 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	indio_dev->channels = data->chip_info->channels;
 	indio_dev->num_channels = data->chip_info->num_channels;
 	indio_dev->name = name ? name : data->chip_info->name;
+	indio_dev->available_scan_masks = bmc150_accel_scan_masks;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmc150_accel_info;
 
-- 
1.9.1

[PATCH 2/6] iio: accel: bmc150: optimize transfers in trigger handler

[Irina Tirdea]

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 bmc150 accel driver does
one bus 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 bus at each transfer.

Reading all axis values in one bus transfer reduces the delays
introduced by the bus.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index cc52366..58df97d 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -989,6 +989,7 @@ static const struct iio_event_spec bmc150_accel_event = {
 		.realbits = (bits),					\
 		.storagebits = 16,					\
 		.shift = 16 - (bits),					\
+		.endianness = IIO_LE,					\
 	},								\
 	.event_spec = &bmc150_accel_event,				\
 	.num_event_specs = 1						\
@@ -1114,21 +1115,14 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
-	int bit, ret, i = 0;
-	unsigned int raw_val;
+	int ret;
 
 	mutex_lock(&data->mutex);
-	for (bit = 0; bit < AXIS_MAX; bit++) {
-		ret = regmap_bulk_read(data->regmap,
-				       BMC150_ACCEL_AXIS_TO_REG(bit), &raw_val,
-				       2);
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			goto err_read;
-		}
-		data->buffer[i++] = raw_val;
-	}
+	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
+			       data->buffer, AXIS_MAX * 2);
 	mutex_unlock(&data->mutex);
+	if (ret < 0)
+		goto err_read;
 
 	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
 					   pf->timestamp);
-- 
1.9.1

[PATCH 3/6] iio: gyro: bmg160: use available_scan_masks

[Irina Tirdea]

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.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/iio/gyro/bmg160_core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index bbce3b0..8d6e5b1 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -116,6 +116,7 @@ enum bmg160_axis {
 	AXIS_X,
 	AXIS_Y,
 	AXIS_Z,
+	AXIS_MAX,
 };
 
 static const struct {
@@ -763,6 +764,10 @@ static const struct iio_info bmg160_info = {
 	.driver_module		= THIS_MODULE,
 };
 
+static const unsigned long bmg160_accel_scan_masks[] = {
+					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+					0};
+
 static irqreturn_t bmg160_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -772,8 +777,7 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
 	unsigned int val;
 
 	mutex_lock(&data->mutex);
-	for_each_set_bit(bit, indio_dev->active_scan_mask,
-			 indio_dev->masklength) {
+	for (bit = 0; bit < AXIS_MAX; bit++) {
 		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
 				       &val, 2);
 		if (ret < 0) {
@@ -1019,6 +1023,7 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	indio_dev->channels = bmg160_channels;
 	indio_dev->num_channels = ARRAY_SIZE(bmg160_channels);
 	indio_dev->name = name;
+	indio_dev->available_scan_masks = bmg160_accel_scan_masks;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmg160_info;
 
-- 
1.9.1

[PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Irina Tirdea]

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 bmc150 accel driver does
one bus 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 bus at each transfer.

Reading all axis values in one bus transfer reduces the delays
introduced by the bus.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 8d6e5b1..43570b8 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
 		.sign = 's',						\
 		.realbits = 16,					\
 		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
 	},								\
 	.event_spec = &bmg160_event,					\
 	.num_event_specs = 1						\
@@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct bmg160_data *data = iio_priv(indio_dev);
-	int bit, ret, i = 0;
-	unsigned int val;
+	int ret;
 
 	mutex_lock(&data->mutex);
-	for (bit = 0; bit < AXIS_MAX; bit++) {
-		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
-				       &val, 2);
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			goto err;
-		}
-		data->buffer[i++] = ret;
-	}
+	ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
+			       data->buffer, AXIS_MAX * 2);
 	mutex_unlock(&data->mutex);
+	if (ret < 0)
+		goto err;
 
 	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
 					   pf->timestamp);
-- 
1.9.1

[PATCH 5/6] 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.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/accel/kxcjk-1013.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index edec1d0..3861fe9 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 {
@@ -953,6 +954,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;
@@ -962,8 +965,7 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
 
 	mutex_lock(&data->mutex);
 
-	for_each_set_bit(bit, indio_dev->active_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);
@@ -1204,6 +1206,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 6/6] 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. Uses i2c_smbus_read_i2c_block_data_or_emulated
that will fallback to reading each axis as a separate word in case i2c
block read is not supported.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/accel/kxcjk-1013.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 3861fe9..4881856 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -923,7 +923,7 @@ static const struct iio_event_spec kxcjk1013_event = {
 		.realbits = 12,						\
 		.storagebits = 16,					\
 		.shift = 4,						\
-		.endianness = IIO_CPU,					\
+		.endianness = IIO_LE,					\
 	},								\
 	.event_spec = &kxcjk1013_event,				\
 	.num_event_specs = 1						\
@@ -961,19 +961,16 @@ 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 = i2c_smbus_read_i2c_block_data_or_emulated(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);
-- 
1.9.1

Re: [PATCH 1/6] iio: accel: bmc150: use available_scan_masks

[Jonathan Cameron]

On 24/03/16 09:29, Irina Tirdea wrote:
> 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.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Applied to the togreg branch of iio.git.

Thanks,
> ---
>  drivers/iio/accel/bmc150-accel-core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index c73331f7..cc52366 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -138,6 +138,7 @@ enum bmc150_accel_axis {
>  	AXIS_X,
>  	AXIS_Y,
>  	AXIS_Z,
> +	AXIS_MAX,
>  };
>  
>  enum bmc150_power_modes {
> @@ -1104,6 +1105,10 @@ static const struct iio_info bmc150_accel_info_fifo = {
>  	.driver_module		= THIS_MODULE,
>  };
>  
> +static const unsigned long bmc150_accel_scan_masks[] = {
> +					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> +					0};
> +
>  static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -1113,8 +1118,7 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>  	unsigned int raw_val;
>  
>  	mutex_lock(&data->mutex);
> -	for_each_set_bit(bit, indio_dev->active_scan_mask,
> -			 indio_dev->masklength) {
> +	for (bit = 0; bit < AXIS_MAX; bit++) {
>  		ret = regmap_bulk_read(data->regmap,
>  				       BMC150_ACCEL_AXIS_TO_REG(bit), &raw_val,
>  				       2);
> @@ -1574,6 +1578,7 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  	indio_dev->channels = data->chip_info->channels;
>  	indio_dev->num_channels = data->chip_info->num_channels;
>  	indio_dev->name = name ? name : data->chip_info->name;
> +	indio_dev->available_scan_masks = bmc150_accel_scan_masks;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &bmc150_accel_info;
>  
> 

Re: [PATCH 2/6] iio: accel: bmc150: optimize transfers in trigger handler

[Jonathan Cameron]

On 24/03/16 09:29, Irina Tirdea wrote:
> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> enable/disable the bus at each i2c transfer and must wait for
> the enable/disable to happen before sending the data.
> 
> When reading data in the trigger handler, the bmc150 accel driver does
> one bus 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 bus at each transfer.
> 
> Reading all axis values in one bus transfer reduces the delays
> introduced by the bus.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Applied to the togreg branch of iio.git.  Thanks.

There is in theory a potential to slow down obscure cases here, but
what are the chances anyone is actually reading a single axis of
one of these accelerometers and cares about absolute maximum throughput.

Ah well, if they do, they may scream and we'll have to do something 
more complex to keep that case fast.  That would make this code
a lot more ugly so *fingers crossed* :)

Anyhow, I like this series and the underlying emulated reading
patch a lot.

Jonathan
> ---
>  drivers/iio/accel/bmc150-accel-core.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index cc52366..58df97d 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -989,6 +989,7 @@ static const struct iio_event_spec bmc150_accel_event = {
>  		.realbits = (bits),					\
>  		.storagebits = 16,					\
>  		.shift = 16 - (bits),					\
> +		.endianness = IIO_LE,					\
>  	},								\
>  	.event_spec = &bmc150_accel_event,				\
>  	.num_event_specs = 1						\
> @@ -1114,21 +1115,14 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> -	int bit, ret, i = 0;
> -	unsigned int raw_val;
> +	int ret;
>  
>  	mutex_lock(&data->mutex);
> -	for (bit = 0; bit < AXIS_MAX; bit++) {
> -		ret = regmap_bulk_read(data->regmap,
> -				       BMC150_ACCEL_AXIS_TO_REG(bit), &raw_val,
> -				       2);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			goto err_read;
> -		}
> -		data->buffer[i++] = raw_val;
> -	}
> +	ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
> +			       data->buffer, AXIS_MAX * 2);
>  	mutex_unlock(&data->mutex);
> +	if (ret < 0)
> +		goto err_read;
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>  					   pf->timestamp);
> 

Re: [PATCH 3/6] iio: gyro: bmg160: use available_scan_masks

[Jonathan Cameron]

On 24/03/16 09:29, Irina Tirdea wrote:
> 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.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Applied,

Thanks,
> ---
>  drivers/iio/gyro/bmg160_core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index bbce3b0..8d6e5b1 100644
> --- a/drivers/iio/gyro/bmg160_core.c
> +++ b/drivers/iio/gyro/bmg160_core.c
> @@ -116,6 +116,7 @@ enum bmg160_axis {
>  	AXIS_X,
>  	AXIS_Y,
>  	AXIS_Z,
> +	AXIS_MAX,
>  };
>  
>  static const struct {
> @@ -763,6 +764,10 @@ static const struct iio_info bmg160_info = {
>  	.driver_module		= THIS_MODULE,
>  };
>  
> +static const unsigned long bmg160_accel_scan_masks[] = {
> +					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> +					0};
> +
>  static irqreturn_t bmg160_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -772,8 +777,7 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
>  	unsigned int val;
>  
>  	mutex_lock(&data->mutex);
> -	for_each_set_bit(bit, indio_dev->active_scan_mask,
> -			 indio_dev->masklength) {
> +	for (bit = 0; bit < AXIS_MAX; bit++) {
>  		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
>  				       &val, 2);
>  		if (ret < 0) {
> @@ -1019,6 +1023,7 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  	indio_dev->channels = bmg160_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(bmg160_channels);
>  	indio_dev->name = name;
> +	indio_dev->available_scan_masks = bmg160_accel_scan_masks;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &bmg160_info;
>  
> 

Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Jonathan Cameron]

On 24/03/16 09:29, Irina Tirdea wrote:
> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> enable/disable the bus at each i2c transfer and must wait for
> the enable/disable to happen before sending the data.
> 
> When reading data in the trigger handler, the bmc150 accel driver does
> one bus 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 bus at each transfer.
> 
> Reading all axis values in one bus transfer reduces the delays
> introduced by the bus.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
I forgot to highlight on the earlier driver that there is also 'technically'
a bit of an ABI change here because we are now exporting as LE rather than CPU
order.  However, I 'hope' anyone actually accessing the buffered data is either
doing it through a nice library or hasn't hacked the endian unwinding out of
the generic_buffer example!

Again, fingers crossed this doesn't break anything significant.

Applied,

Thanks,

Jonathan
> ---
>  drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index 8d6e5b1..43570b8 100644
> --- a/drivers/iio/gyro/bmg160_core.c
> +++ b/drivers/iio/gyro/bmg160_core.c
> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
>  		.sign = 's',						\
>  		.realbits = 16,					\
>  		.storagebits = 16,					\
> +		.endianness = IIO_LE,					\
>  	},								\
>  	.event_spec = &bmg160_event,					\
>  	.num_event_specs = 1						\
> @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct bmg160_data *data = iio_priv(indio_dev);
> -	int bit, ret, i = 0;
> -	unsigned int val;
> +	int ret;
>  
>  	mutex_lock(&data->mutex);
> -	for (bit = 0; bit < AXIS_MAX; bit++) {
> -		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
> -				       &val, 2);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			goto err;
> -		}
> -		data->buffer[i++] = ret;
> -	}
> +	ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
> +			       data->buffer, AXIS_MAX * 2);
>  	mutex_unlock(&data->mutex);
> +	if (ret < 0)
> +		goto err;
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>  					   pf->timestamp);
> 

Re: [PATCH 5/6] iio: accel: kxcjk-1013: use available_scan_masks

[Jonathan Cameron]

On 24/03/16 09:29, Irina Tirdea wrote:
> 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.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Applied, thanks,

Jonathan
> ---
>  drivers/iio/accel/kxcjk-1013.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index edec1d0..3861fe9 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 {
> @@ -953,6 +954,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;
> @@ -962,8 +965,7 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
>  
>  	mutex_lock(&data->mutex);
>  
> -	for_each_set_bit(bit, indio_dev->active_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);
> @@ -1204,6 +1206,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;
> 

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

[Jonathan Cameron]

On 24/03/16 09:29, Irina Tirdea wrote:
> 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. Uses i2c_smbus_read_i2c_block_data_or_emulated
> that will fallback to reading each axis as a separate word in case i2c
> block read is not supported.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Applied - with all the others to the togreg branch of iio.git - initially pushed out
as testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/accel/kxcjk-1013.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 3861fe9..4881856 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -923,7 +923,7 @@ static const struct iio_event_spec kxcjk1013_event = {
>  		.realbits = 12,						\
>  		.storagebits = 16,					\
>  		.shift = 4,						\
> -		.endianness = IIO_CPU,					\
> +		.endianness = IIO_LE,					\
>  	},								\
>  	.event_spec = &kxcjk1013_event,				\
>  	.num_event_specs = 1						\
> @@ -961,19 +961,16 @@ 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 = i2c_smbus_read_i2c_block_data_or_emulated(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);
> 

Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Peter Meerwald-Stadler]

> > 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 bmc150 accel driver does

should refer to bmg160

> > one bus 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 bus at each transfer.
> > 
> > Reading all axis values in one bus transfer reduces the delays
> > introduced by the bus.
> > 
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> I forgot to highlight on the earlier driver that there is also 'technically'
> a bit of an ABI change here because we are now exporting as LE rather than CPU
> order.  However, I 'hope' anyone actually accessing the buffered data is either
> doing it through a nice library or hasn't hacked the endian unwinding out of
> the generic_buffer example!

the patch takes away the possibility to do buffered reads on individual 
channels (not sure if this is useful per se)

this optimizes for the common case, ok;

wondering if adding 
.endianness = IIO_LE
is actually an unrelated fix

> Again, fingers crossed this doesn't break anything significant.
> 
> Applied,
> 
> Thanks,
> 
> Jonathan
> > ---
> >  drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> > index 8d6e5b1..43570b8 100644
> > --- a/drivers/iio/gyro/bmg160_core.c
> > +++ b/drivers/iio/gyro/bmg160_core.c
> > @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
> >  		.sign = 's',						\
> >  		.realbits = 16,					\
> >  		.storagebits = 16,					\
> > +		.endianness = IIO_LE,					\
> >  	},								\
> >  	.event_spec = &bmg160_event,					\
> >  	.num_event_specs = 1						\
> > @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
> >  	struct iio_poll_func *pf = p;
> >  	struct iio_dev *indio_dev = pf->indio_dev;
> >  	struct bmg160_data *data = iio_priv(indio_dev);
> > -	int bit, ret, i = 0;
> > -	unsigned int val;
> > +	int ret;
> >  
> >  	mutex_lock(&data->mutex);
> > -	for (bit = 0; bit < AXIS_MAX; bit++) {
> > -		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
> > -				       &val, 2);
> > -		if (ret < 0) {
> > -			mutex_unlock(&data->mutex);
> > -			goto err;
> > -		}
> > -		data->buffer[i++] = ret;
> > -	}
> > +	ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
> > +			       data->buffer, AXIS_MAX * 2);
> >  	mutex_unlock(&data->mutex);
> > +	if (ret < 0)
> > +		goto err;
> >  
> >  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >  					   pf->timestamp);
> > 
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Jonathan Cameron]

On 28/03/16 11:09, Peter Meerwald-Stadler wrote:
> 
>>> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
>>> enable/disable the bus at each i2c transfer and must wait for
>>> the enable/disable to happen before sending the data.
>>>
>>> When reading data in the trigger handler, the bmc150 accel driver does
> 
> should refer to bmg160
Good spot.  It's also a gyroscope, not an accelerometer... I just fixed this
up and repushed out testing.
> 
>>> one bus 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 bus at each transfer.
>>>
>>> Reading all axis values in one bus transfer reduces the delays
>>> introduced by the bus.
>>>
>>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>> I forgot to highlight on the earlier driver that there is also 'technically'
>> a bit of an ABI change here because we are now exporting as LE rather than CPU
>> order.  However, I 'hope' anyone actually accessing the buffered data is either
>> doing it through a nice library or hasn't hacked the endian unwinding out of
>> the generic_buffer example!
> 
> the patch takes away the possibility to do buffered reads on individual 
> channels (not sure if this is useful per se)
> 
> this optimizes for the common case, ok;
> 
> wondering if adding 
> .endianness = IIO_LE
> is actually an unrelated fix
Good point, when I first read the code I assumed we were moving from an i2c_word
read to a bulk read, thus necessitating this addition.  However, we aren't as it
was previously as an i2c_bulk read of 2 bytes... 

Irina, could you confirm if this was broken before your patches?

I'll leave this as is, perhaps we need an additional fix patch specifying LE to
put out as a fix.
> 
>> Again, fingers crossed this doesn't break anything significant.
>>
>> Applied,
>>
>> Thanks,
>>
>> Jonathan
>>> ---
>>>  drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
>>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
>>> index 8d6e5b1..43570b8 100644
>>> --- a/drivers/iio/gyro/bmg160_core.c
>>> +++ b/drivers/iio/gyro/bmg160_core.c
>>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
>>>  		.sign = 's',						\
>>>  		.realbits = 16,					\
>>>  		.storagebits = 16,					\
>>> +		.endianness = IIO_LE,					\
>>>  	},								\
>>>  	.event_spec = &bmg160_event,					\
>>>  	.num_event_specs = 1						\
>>> @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
>>>  	struct iio_poll_func *pf = p;
>>>  	struct iio_dev *indio_dev = pf->indio_dev;
>>>  	struct bmg160_data *data = iio_priv(indio_dev);
>>> -	int bit, ret, i = 0;
>>> -	unsigned int val;
>>> +	int ret;
>>>  
>>>  	mutex_lock(&data->mutex);
>>> -	for (bit = 0; bit < AXIS_MAX; bit++) {
>>> -		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
>>> -				       &val, 2);
>>> -		if (ret < 0) {
>>> -			mutex_unlock(&data->mutex);
>>> -			goto err;
>>> -		}
>>> -		data->buffer[i++] = ret;
>>> -	}
>>> +	ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
>>> +			       data->buffer, AXIS_MAX * 2);
>>>  	mutex_unlock(&data->mutex);
>>> +	if (ret < 0)
>>> +		goto err;
>>>  
>>>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>  					   pf->timestamp);
>>>
>>
> 

RE: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Tirdea, Irina]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 28 March, 2016 17:15
> To: Peter Meerwald-Stadler
> Cc: Tirdea, Irina; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Hartmut Knaack; Lars-Peter Clausen; Purdila, Octavian;
> Markus Pargmann; Pandruvada, Srinivas
> Subject: Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler
> 
> On 28/03/16 11:09, Peter Meerwald-Stadler wrote:
> >
> >>> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> >>> enable/disable the bus at each i2c transfer and must wait for
> >>> the enable/disable to happen before sending the data.
> >>>
> >>> When reading data in the trigger handler, the bmc150 accel driver does
> >
> > should refer to bmg160
> Good spot.  It's also a gyroscope, not an accelerometer... I just fixed this
> up and repushed out testing.
> >
Oops... too much copy-paste :)

> >>> one bus 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 bus at each transfer.
> >>>
> >>> Reading all axis values in one bus transfer reduces the delays
> >>> introduced by the bus.
> >>>
> >>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> >> I forgot to highlight on the earlier driver that there is also 'technically'
> >> a bit of an ABI change here because we are now exporting as LE rather than CPU
> >> order.  However, I 'hope' anyone actually accessing the buffered data is either
> >> doing it through a nice library or hasn't hacked the endian unwinding out of
> >> the generic_buffer example!
> >
> > the patch takes away the possibility to do buffered reads on individual
> > channels (not sure if this is useful per se)
> >
> > this optimizes for the common case, ok;
> >
> > wondering if adding
> > .endianness = IIO_LE
> > is actually an unrelated fix
> Good point, when I first read the code I assumed we were moving from an i2c_word
> read to a bulk read, thus necessitating this addition.  However, we aren't as it
> was previously as an i2c_bulk read of 2 bytes...
> 
> Irina, could you confirm if this was broken before your patches?
> 

Peter is right. I also had in mind the change from i2c_word to bulk read, but
the regmap API has changed this in the meantime.

Since the driver uses regmap_bulk_read to read 2 bytes for each
axis, the data read will have the endianness of the device (little endian)
and we should do endianness conversion or else it will not work on big
endian platforms.

> I'll leave this as is, perhaps we need an additional fix patch specifying LE to
> put out as a fix.

There is one more place in both bmc150 and bmg160 drivers where 
regmap_bulk_read is used without endianness conversion (when reading raw axes).
I will send a separate patch to fix all endianness issues.

While looking at the existent code, I also found another bug in the bmg160 driver 
that this patch fixes as a side effect: the error code returned by regmap_bulk_read
is saved in the data->buffer instead of the value read. Not sure how to handle this,
since it is fixed now by this patch. Jonathan, should I send a fix patch for this? 

Thanks,
Irina

> >
> >> Again, fingers crossed this doesn't break anything significant.
> >>
> >> Applied,
> >>
> >> Thanks,
> >>
> >> Jonathan
> >>> ---
> >>>  drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
> >>>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> >>> index 8d6e5b1..43570b8 100644
> >>> --- a/drivers/iio/gyro/bmg160_core.c
> >>> +++ b/drivers/iio/gyro/bmg160_core.c
> >>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
> >>>  		.sign = 's',						\
> >>>  		.realbits = 16,					\
> >>>  		.storagebits = 16,					\
> >>> +		.endianness = IIO_LE,					\
> >>>  	},								\
> >>>  	.event_spec = &bmg160_event,					\
> >>>  	.num_event_specs = 1						\
> >>> @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
> >>>  	struct iio_poll_func *pf = p;
> >>>  	struct iio_dev *indio_dev = pf->indio_dev;
> >>>  	struct bmg160_data *data = iio_priv(indio_dev);
> >>> -	int bit, ret, i = 0;
> >>> -	unsigned int val;
> >>> +	int ret;
> >>>
> >>>  	mutex_lock(&data->mutex);
> >>> -	for (bit = 0; bit < AXIS_MAX; bit++) {
> >>> -		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
> >>> -				       &val, 2);
> >>> -		if (ret < 0) {
> >>> -			mutex_unlock(&data->mutex);
> >>> -			goto err;
> >>> -		}
> >>> -		data->buffer[i++] = ret;
> >>> -	}
> >>> +	ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
> >>> +			       data->buffer, AXIS_MAX * 2);
> >>>  	mutex_unlock(&data->mutex);
> >>> +	if (ret < 0)
> >>> +		goto err;
> >>>
> >>>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >>>  					   pf->timestamp);
> >>>
> >>
> >

RE: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Tirdea, Irina]

> -----Original Message-----
> From: Peter Meerwald-Stadler [mailto:pmeerw@pmeerw.net]
> Sent: 28 March, 2016 13:09
> To: Jonathan Cameron
> Cc: Tirdea, Irina; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.or=
g; Hartmut Knaack; Lars-Peter Clausen; Purdila, Octavian;
> Markus Pargmann; Pandruvada, Srinivas
> Subject: Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigge=
r handler
>=20
>=20

Thanks for the review, Peter!

> > > 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 bmc150 accel driver doe=
s
>=20
> should refer to bmg160
>=20
> > > one bus 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 bus at each transfer.
> > >
> > > Reading all axis values in one bus transfer reduces the delays
> > > introduced by the bus.
> > >
> > > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > I forgot to highlight on the earlier driver that there is also 'technic=
ally'
> > a bit of an ABI change here because we are now exporting as LE rather t=
han CPU
> > order.  However, I 'hope' anyone actually accessing the buffered data i=
s either
> > doing it through a nice library or hasn't hacked the endian unwinding o=
ut of
> > the generic_buffer example!
>=20
> the patch takes away the possibility to do buffered reads on individual
> channels (not sure if this is useful per se)

We can still read individual channels, but the demux is now handled by the =
iio core
(through available_scan_masks, added in the previous patch).

As Jonathan mentioned in a previous patch, this will impact performance for=
 reading
only a subset of the available channels  (since we will read all 3 axes reg=
ardless of
how many axes the user actually requested  and will receive).
>=20
> this optimizes for the common case, ok;
>=20
> wondering if adding
> .endianness =3D IIO_LE
> is actually an unrelated fix
>=20

Thanks for catching this!=20
I already covered this point in the reply to Jonathan.

Thanks,
Irina=20

Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler

[Jonathan Cameron]

On 28/03/16 17:03, Tirdea, Irina wrote:
> 
> 
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:jic23@kernel.org]
>> Sent: 28 March, 2016 17:15
>> To: Peter Meerwald-Stadler
>> Cc: Tirdea, Irina; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Hartmut Knaack; Lars-Peter Clausen; Purdila, Octavian;
>> Markus Pargmann; Pandruvada, Srinivas
>> Subject: Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler
>>
>> On 28/03/16 11:09, Peter Meerwald-Stadler wrote:
>>>
>>>>> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
>>>>> enable/disable the bus at each i2c transfer and must wait for
>>>>> the enable/disable to happen before sending the data.
>>>>>
>>>>> When reading data in the trigger handler, the bmc150 accel driver does
>>>
>>> should refer to bmg160
>> Good spot.  It's also a gyroscope, not an accelerometer... I just fixed this
>> up and repushed out testing.
>>>
> Oops... too much copy-paste :)
> 
>>>>> one bus 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 bus at each transfer.
>>>>>
>>>>> Reading all axis values in one bus transfer reduces the delays
>>>>> introduced by the bus.
>>>>>
>>>>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>>>> I forgot to highlight on the earlier driver that there is also 'technically'
>>>> a bit of an ABI change here because we are now exporting as LE rather than CPU
>>>> order.  However, I 'hope' anyone actually accessing the buffered data is either
>>>> doing it through a nice library or hasn't hacked the endian unwinding out of
>>>> the generic_buffer example!
>>>
>>> the patch takes away the possibility to do buffered reads on individual
>>> channels (not sure if this is useful per se)
>>>
>>> this optimizes for the common case, ok;
>>>
>>> wondering if adding
>>> .endianness = IIO_LE
>>> is actually an unrelated fix
>> Good point, when I first read the code I assumed we were moving from an i2c_word
>> read to a bulk read, thus necessitating this addition.  However, we aren't as it
>> was previously as an i2c_bulk read of 2 bytes...
>>
>> Irina, could you confirm if this was broken before your patches?
>>
> 
> Peter is right. I also had in mind the change from i2c_word to bulk read, but
> the regmap API has changed this in the meantime.
> 
> Since the driver uses regmap_bulk_read to read 2 bytes for each
> axis, the data read will have the endianness of the device (little endian)
> and we should do endianness conversion or else it will not work on big
> endian platforms.
> 
>> I'll leave this as is, perhaps we need an additional fix patch specifying LE to
>> put out as a fix.
> 
> There is one more place in both bmc150 and bmg160 drivers where 
> regmap_bulk_read is used without endianness conversion (when reading raw axes).
> I will send a separate patch to fix all endianness issues.
> 
> While looking at the existent code, I also found another bug in the bmg160 driver 
> that this patch fixes as a side effect: the error code returned by regmap_bulk_read
> is saved in the data->buffer instead of the value read. Not sure how to handle this,
> since it is fixed now by this patch. Jonathan, should I send a fix patch for this? 
Send a patch against, fixes-togreg, or given timing fixes-togreg-post-rc1 as appropriate
but put a comment in there saying it was also fixed in "adfasfaf" so that the merge
is obvious when the two hit each other - hopefully in Greg's tree...

Jonathan
> 
> Thanks,
> Irina
> 
>>>
>>>> Again, fingers crossed this doesn't break anything significant.
>>>>
>>>> Applied,
>>>>
>>>> Thanks,
>>>>
>>>> Jonathan
>>>>> ---
>>>>>  drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
>>>>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
>>>>> index 8d6e5b1..43570b8 100644
>>>>> --- a/drivers/iio/gyro/bmg160_core.c
>>>>> +++ b/drivers/iio/gyro/bmg160_core.c
>>>>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
>>>>>  		.sign = 's',						\
>>>>>  		.realbits = 16,					\
>>>>>  		.storagebits = 16,					\
>>>>> +		.endianness = IIO_LE,					\
>>>>>  	},								\
>>>>>  	.event_spec = &bmg160_event,					\
>>>>>  	.num_event_specs = 1						\
>>>>> @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
>>>>>  	struct iio_poll_func *pf = p;
>>>>>  	struct iio_dev *indio_dev = pf->indio_dev;
>>>>>  	struct bmg160_data *data = iio_priv(indio_dev);
>>>>> -	int bit, ret, i = 0;
>>>>> -	unsigned int val;
>>>>> +	int ret;
>>>>>
>>>>>  	mutex_lock(&data->mutex);
>>>>> -	for (bit = 0; bit < AXIS_MAX; bit++) {
>>>>> -		ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
>>>>> -				       &val, 2);
>>>>> -		if (ret < 0) {
>>>>> -			mutex_unlock(&data->mutex);
>>>>> -			goto err;
>>>>> -		}
>>>>> -		data->buffer[i++] = ret;
>>>>> -	}
>>>>> +	ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
>>>>> +			       data->buffer, AXIS_MAX * 2);
>>>>>  	mutex_unlock(&data->mutex);
>>>>> +	if (ret < 0)
>>>>> +		goto err;
>>>>>
>>>>>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>>  					   pf->timestamp);
>>>>>
>>>>
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>