[PATCH v5 0/8] Add support for best effort block read emulation

[PATCH v5 0/8] Add support for best effort block read emulation

[Irina Tirdea]

This is version 5 for adding i2c_smbus_read_i2c_block_data_or_emulated
to i2c-core.

Jonathan, I did not add your Reviewed-by to the patch adding the 
i2c_smbus_read_i2c_block_data_or_emulated function since I made some
changes to the code.

Thanks,
Irina

Changes from v4:
 - optimized the code for i2c_smbus_read_i2c_block_data_or_emulated as
Wolfram suggested
 - dropped the error message from i2c_smbus_read_i2c_block_data_or_emulated
 - fixed documentation to specify I2C_SMBUS_BLOCK_MAX instead of 32

Changes from v3:
 - when reading an odd number of bytes using word emulation, read an even
number of bytes using word reads and the last byte using byte read
 - code styling changes to improve readability
 - add a comment about addressing assumptions to the
i2c_smbus_read_i2c_block_data_or_emulated function as Jonathan suggested
 - add Acked-by from Jonathan and Srinivas to the iio changes

Changes from v2:
 - changed bmc150-accel, kxcjk-1013 and bmg160 drivers to use
i2c_smbus_read_i2c_block_data_or_emulated

Changes from v1:
 - dropped the RFC tag
 - changed at24 to use i2c_smbus_read_i2c_block_data_or_emulated
 - when reading an odd number of bytes using word emulation, read an even
number of bytes and drop the last one
 - add a comment that this might not be suitable for all I2C slaves

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

Irina Tirdea (6):
  i2c: core: Add support for best effort block read emulation
  eeprom: at24: use i2c_smbus_read_i2c_block_data_or_emulated
  iio: accel: bmc150: use available_scan_masks
  iio: accel: bmc150: optimize i2c transfers in trigger handler
  iio: gyro: bmg160: use available_scan_masks
  iio: gyro: bmg160: optimize i2c transfers in trigger handler

 drivers/i2c/i2c-core.c           | 57 ++++++++++++++++++++++++++++++++++++++++
 drivers/iio/accel/bmc150-accel.c | 23 ++++++++--------
 drivers/iio/accel/kxcjk-1013.c   | 24 ++++++++---------
 drivers/iio/gyro/bmg160.c        | 23 ++++++++--------
 drivers/misc/eeprom/at24.c       | 37 +++++---------------------
 include/linux/i2c.h              |  3 +++
 6 files changed, 102 insertions(+), 65 deletions(-)

-- 
1.9.1

[PATCH v5 1/8] i2c: core: Add support for best effort block read emulation

[Irina Tirdea]

There are devices that need to handle block transactions
regardless of the capabilities exported by the adapter.
For performance reasons, they need to use i2c read blocks
if available, otherwise emulate the block transaction with word
or byte transactions.

Add support for a helper function that would read a data block
using the best transfer available: I2C_FUNC_SMBUS_READ_I2C_BLOCK,
I2C_FUNC_SMBUS_READ_WORD_DATA or I2C_FUNC_SMBUS_READ_BYTE_DATA.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/i2c/i2c-core.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |  3 +++
 2 files changed, 60 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 07a83f3..0c581bc 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2969,6 +2969,63 @@ trace:
 }
 EXPORT_SYMBOL(i2c_smbus_xfer);
 
+/**
+ * i2c_smbus_read_i2c_block_data_or_emulated - read block or emulate
+ * @client: Handle to slave device
+ * @command: Byte interpreted by slave
+ * @length: Size of data block; SMBus allows at most I2C_SMBUS_BLOCK_MAX bytes
+ * @values: Byte array into which data will be read; big enough to hold
+ *	the data returned by the slave.  SMBus allows at most
+ *	I2C_SMBUS_BLOCK_MAX bytes.
+ *
+ * This executes the SMBus "block read" protocol if supported by the adapter.
+ * If block read is not supported, it emulates it using either word or byte
+ * read protocols depending on availability.
+ *
+ * The addresses of the I2C slave device that are accessed with this function
+ * must be mapped to a linear region, so that a block read will have the same
+ * effect as a byte read. Before using this function you must double-check
+ * if the I2C slave does support exchanging a block transfer with a byte
+ * transfer.
+ */
+s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
+					      u8 command, u8 length, u8 *values)
+{
+	u8 i = 0;
+	int status;
+
+	if (length > I2C_SMBUS_BLOCK_MAX)
+		length = I2C_SMBUS_BLOCK_MAX;
+
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK))
+		return i2c_smbus_read_i2c_block_data(client, command, length, values);
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE_DATA))
+		return -EOPNOTSUPP;
+
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_WORD_DATA)) {
+		while ((i + 2) <= length) {
+			status = i2c_smbus_read_word_data(client, command + i);
+			if (status < 0)
+				return status;
+			values[i] = status & 0xff;
+			values[i + 1] = status >> 8;
+			i += 2;
+		}
+	}
+
+	while (i < length) {
+		status = i2c_smbus_read_byte_data(client, command + i);
+		if (status < 0)
+			return status;
+		values[i] = status;
+		i++;
+	}
+
+	return i;
+}
+EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data_or_emulated);
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
 {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e2c859b..32b925c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -121,6 +121,9 @@ extern s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client,
 extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
 					  u8 command, u8 length,
 					  const u8 *values);
+extern s32
+i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
+					  u8 command, u8 length, u8 *values);
 #endif /* I2C */
 
 /**
-- 
1.9.1

Re: [PATCH v5 1/8] i2c: core: Add support for best effort block read emulation

[Wolfram Sang]

On Wed, Aug 12, 2015 at 05:31:33PM +0300, Irina Tirdea wrote:
> There are devices that need to handle block transactions
> regardless of the capabilities exported by the adapter.
> For performance reasons, they need to use i2c read blocks
> if available, otherwise emulate the block transaction with word
> or byte transactions.
> 
> Add support for a helper function that would read a data block
> using the best transfer available: I2C_FUNC_SMBUS_READ_I2C_BLOCK,
> I2C_FUNC_SMBUS_READ_WORD_DATA or I2C_FUNC_SMBUS_READ_BYTE_DATA.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>

This looks concise, thanks for the rework

Applied to for-next, thanks!

[PATCH v5 2/8] eeprom: at24: use i2c_smbus_read_i2c_block_data_or_emulated

[Irina Tirdea]

For i2c busses that support only SMBUS extensions, the eeprom at24
driver reads data from the device using the SMBus block, word or byte
read protocols depending on availability.

Replace the block read emulation from the driver with the
i2c_smbus_read_i2c_block_data_or_emulated call from i2c core.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/misc/eeprom/at24.c | 37 ++++++-------------------------------
 1 file changed, 6 insertions(+), 31 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 2b254f3..c6cb7f8 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -186,19 +186,11 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 	if (count > io_limit)
 		count = io_limit;
 
-	switch (at24->use_smbus) {
-	case I2C_SMBUS_I2C_BLOCK_DATA:
+	if (at24->use_smbus) {
 		/* Smaller eeproms can work given some SMBus extension calls */
 		if (count > I2C_SMBUS_BLOCK_MAX)
 			count = I2C_SMBUS_BLOCK_MAX;
-		break;
-	case I2C_SMBUS_WORD_DATA:
-		count = 2;
-		break;
-	case I2C_SMBUS_BYTE_DATA:
-		count = 1;
-		break;
-	default:
+	} else {
 		/*
 		 * When we have a better choice than SMBus calls, use a
 		 * combined I2C message. Write address; then read up to
@@ -229,27 +221,10 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
 	timeout = jiffies + msecs_to_jiffies(write_timeout);
 	do {
 		read_time = jiffies;
-		switch (at24->use_smbus) {
-		case I2C_SMBUS_I2C_BLOCK_DATA:
-			status = i2c_smbus_read_i2c_block_data(client, offset,
-					count, buf);
-			break;
-		case I2C_SMBUS_WORD_DATA:
-			status = i2c_smbus_read_word_data(client, offset);
-			if (status >= 0) {
-				buf[0] = status & 0xff;
-				buf[1] = status >> 8;
-				status = count;
-			}
-			break;
-		case I2C_SMBUS_BYTE_DATA:
-			status = i2c_smbus_read_byte_data(client, offset);
-			if (status >= 0) {
-				buf[0] = status;
-				status = count;
-			}
-			break;
-		default:
+		if (at24->use_smbus) {
+			status = i2c_smbus_read_i2c_block_data_or_emulated(client, offset,
+									   count, buf);
+		} else {
 			status = i2c_transfer(client->adapter, msg, 2);
 			if (status == 2)
 				status = count;
-- 
1.9.1

Re: [PATCH v5 2/8] eeprom: at24: use i2c_smbus_read_i2c_block_data_or_emulated

[Wolfram Sang]

On Wed, Aug 12, 2015 at 05:31:34PM +0300, Irina Tirdea wrote:
> For i2c busses that support only SMBUS extensions, the eeprom at24
> driver reads data from the device using the SMBus block, word or byte
> read protocols depending on availability.
> 
> Replace the block read emulation from the driver with the
> i2c_smbus_read_i2c_block_data_or_emulated call from i2c core.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>

Applied to for-next, thanks!

Re: [PATCH v5 2/8] eeprom: at24: use i2c_smbus_read_i2c_block_data_or_emulated

[Jonathan Cameron]

On 14/08/15 19:24, Wolfram Sang wrote:
> On Wed, Aug 12, 2015 at 05:31:34PM +0300, Irina Tirdea wrote:
>> For i2c busses that support only SMBUS extensions, the eeprom at24
>> driver reads data from the device using the SMBus block, word or byte
>> read protocols depending on availability.
>>
>> Replace the block read emulation from the driver with the
>> i2c_smbus_read_i2c_block_data_or_emulated call from i2c core.
>>
>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> 
> Applied to for-next, thanks!
Cool.  These will presumably make it in during the coming
merge window.  I'll pick up the IIO ones next cycle
(IIO merge is probably now closed anyway as Linus is talking about
merge window opening next week I think).

Good work Irina,

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

[PATCH v5 3/8] 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>
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index cc5a357..40da2d0 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -136,6 +136,7 @@ enum bmc150_accel_axis {
 	AXIS_X,
 	AXIS_Y,
 	AXIS_Z,
+	AXIS_MAX,
 };
 
 enum bmc150_power_modes {
@@ -1200,6 +1201,8 @@ static const struct iio_info bmc150_accel_info_fifo = {
 	.driver_module		= THIS_MODULE,
 };
 
+static const unsigned long bmc150_accel_scan_masks[] = {0x7, 0};
+
 static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1208,8 +1211,7 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
 	int bit, ret, i = 0;
 
 	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 = i2c_smbus_read_word_data(data->client,
 					       BMC150_ACCEL_AXIS_TO_REG(bit));
 		if (ret < 0) {
@@ -1647,6 +1649,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->channels = data->chip_info->channels;
 	indio_dev->num_channels = data->chip_info->num_channels;
+	indio_dev->available_scan_masks = bmc150_accel_scan_masks;
 	indio_dev->name = name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmc150_accel_info;
-- 
1.9.1

[PATCH v5 4/8] iio: accel: bmc150: optimize i2c 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 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: 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/bmc150-accel.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 40da2d0..a38e3f4 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -1082,6 +1082,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						\
@@ -1208,19 +1209,16 @@ 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;
+	int ret;
 
 	mutex_lock(&data->mutex);
-	for (bit = 0; bit < AXIS_MAX; bit++) {
-		ret = i2c_smbus_read_word_data(data->client,
-					       BMC150_ACCEL_AXIS_TO_REG(bit));
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			goto err_read;
-		}
-		data->buffer[i++] = ret;
-	}
+	ret = i2c_smbus_read_i2c_block_data_or_emulated(data->client,
+							BMC150_ACCEL_REG_XOUT_L,
+							AXIS_MAX * 2,
+							(u8 *)data->buffer);
 	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 v5 5/8] 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>
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/gyro/bmg160.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
index 460bf71..b2a6ccb 100644
--- a/drivers/iio/gyro/bmg160.c
+++ b/drivers/iio/gyro/bmg160.c
@@ -114,6 +114,7 @@ enum bmg160_axis {
 	AXIS_X,
 	AXIS_Y,
 	AXIS_Z,
+	AXIS_MAX,
 };
 
 static const struct {
@@ -801,6 +802,8 @@ static const struct iio_info bmg160_info = {
 	.driver_module		= THIS_MODULE,
 };
 
+static const unsigned long bmg160_scan_masks[] = {0x7, 0};
+
 static irqreturn_t bmg160_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -809,8 +812,7 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
 	int bit, ret, i = 0;
 
 	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 = i2c_smbus_read_word_data(data->client,
 					       BMG160_AXIS_TO_REG(bit));
 		if (ret < 0) {
@@ -1062,6 +1064,7 @@ static int bmg160_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->channels = bmg160_channels;
 	indio_dev->num_channels = ARRAY_SIZE(bmg160_channels);
+	indio_dev->available_scan_masks = bmg160_scan_masks;
 	indio_dev->name = name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmg160_info;
-- 
1.9.1

[PATCH v5 6/8] iio: gyro: bmg160: optimize i2c 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 bmg160 driver does
one i2c transfer for each axis. This has an impact on the frequency
of the gyroscope at high sample rates due to additional delays
introduced by the i2c bus at each transfer.

Reading all axis values in one i2c transfer reduces the delays
introduced by the i2c bus. Uses i2c_smbus_read_i2c_block_data_or_emulated
that will fallback to reading each axis as a separate word in case i2c
block read is not supported.

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

diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
index b2a6ccb..1ff306d 100644
--- a/drivers/iio/gyro/bmg160.c
+++ b/drivers/iio/gyro/bmg160.c
@@ -772,6 +772,7 @@ static const struct iio_event_spec bmg160_event = {
 		.sign = 's',						\
 		.realbits = 16,					\
 		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
 	},								\
 	.event_spec = &bmg160_event,					\
 	.num_event_specs = 1						\
@@ -809,19 +810,16 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct bmg160_data *data = iio_priv(indio_dev);
-	int bit, ret, i = 0;
+	int ret = 0;
 
 	mutex_lock(&data->mutex);
-	for (bit = 0; bit < AXIS_MAX; bit++) {
-		ret = i2c_smbus_read_word_data(data->client,
-					       BMG160_AXIS_TO_REG(bit));
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
-			goto err;
-		}
-		data->buffer[i++] = ret;
-	}
+	ret = i2c_smbus_read_i2c_block_data_or_emulated(data->client,
+							BMG160_REG_XOUT_L,
+							AXIS_MAX * 2,
+							(u8 *)data->buffer);
 	mutex_unlock(&data->mutex);
+	if (ret < 0)
+		goto err;
 
 	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
 					   pf->timestamp);
-- 
1.9.1

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

[Jonathan Cameron]

On 12/08/15 15:31, Irina Tirdea wrote:
> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> enable/disable the bus at each i2c transfer and must wait for
> the enable/disable to happen before sending the data.
> 
> When reading data in the trigger handler, the bmg160 driver does
> one i2c transfer for each axis. This has an impact on the frequency
> of the gyroscope at high sample rates due to additional delays
> introduced by the i2c bus at each transfer.
> 
> Reading all axis values in one i2c transfer reduces the delays
> introduced by the i2c bus. Uses i2c_smbus_read_i2c_block_data_or_emulated
> that will fallback to reading each axis as a separate word in case i2c
> block read is not supported.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Note, that in the meantime the bmg160 driver just went all regmap
on us (as part of adding SPI support - though that step hasn't
happened yet).  Hence we'll need a means of telling regmap about this
possibility.

> ---
>  drivers/iio/gyro/bmg160.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
> index b2a6ccb..1ff306d 100644
> --- a/drivers/iio/gyro/bmg160.c
> +++ b/drivers/iio/gyro/bmg160.c
> @@ -772,6 +772,7 @@ static const struct iio_event_spec bmg160_event = {
>  		.sign = 's',						\
>  		.realbits = 16,					\
>  		.storagebits = 16,					\
> +		.endianness = IIO_LE,					\
>  	},								\
>  	.event_spec = &bmg160_event,					\
>  	.num_event_specs = 1						\
> @@ -809,19 +810,16 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct bmg160_data *data = iio_priv(indio_dev);
> -	int bit, ret, i = 0;
> +	int ret = 0;
>  
>  	mutex_lock(&data->mutex);
> -	for (bit = 0; bit < AXIS_MAX; bit++) {
> -		ret = i2c_smbus_read_word_data(data->client,
> -					       BMG160_AXIS_TO_REG(bit));
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			goto err;
> -		}
> -		data->buffer[i++] = ret;
> -	}
> +	ret = i2c_smbus_read_i2c_block_data_or_emulated(data->client,
> +							BMG160_REG_XOUT_L,
> +							AXIS_MAX * 2,
> +							(u8 *)data->buffer);
>  	mutex_unlock(&data->mutex);
> +	if (ret < 0)
> +		goto err;
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>  					   pf->timestamp);
> 

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

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

On Sun, Aug 16, 2015 at 10:24:47AM +0100, Jonathan Cameron wrote:
> On 12/08/15 15:31, Irina Tirdea wrote:
> > Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> > enable/disable the bus at each i2c transfer and must wait for
> > the enable/disable to happen before sending the data.
> > 
> > When reading data in the trigger handler, the bmg160 driver does
> > one i2c transfer for each axis. This has an impact on the frequency
> > of the gyroscope at high sample rates due to additional delays
> > introduced by the i2c bus at each transfer.
> > 
> > Reading all axis values in one i2c transfer reduces the delays
> > introduced by the i2c bus. Uses i2c_smbus_read_i2c_block_data_or_emulated
> > that will fallback to reading each axis as a separate word in case i2c
> > block read is not supported.
> > 
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Acked-by: Jonathan Cameron <jic23@kernel.org>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Note, that in the meantime the bmg160 driver just went all regmap
> on us (as part of adding SPI support - though that step hasn't
> happened yet).  Hence we'll need a means of telling regmap about this
> possibility.

Perhaps this is covered by a regmap_bulk_read()?

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

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

Best regards,

Markus

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

> 
> > ---
> >  drivers/iio/gyro/bmg160.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
> > index b2a6ccb..1ff306d 100644
> > --- a/drivers/iio/gyro/bmg160.c
> > +++ b/drivers/iio/gyro/bmg160.c
> > @@ -772,6 +772,7 @@ static const struct iio_event_spec bmg160_event = {
> >  		.sign = 's',						\
> >  		.realbits = 16,					\
> >  		.storagebits = 16,					\
> > +		.endianness = IIO_LE,					\
> >  	},								\
> >  	.event_spec = &bmg160_event,					\
> >  	.num_event_specs = 1						\
> > @@ -809,19 +810,16 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
> >  	struct iio_poll_func *pf = p;
> >  	struct iio_dev *indio_dev = pf->indio_dev;
> >  	struct bmg160_data *data = iio_priv(indio_dev);
> > -	int bit, ret, i = 0;
> > +	int ret = 0;
> >  
> >  	mutex_lock(&data->mutex);
> > -	for (bit = 0; bit < AXIS_MAX; bit++) {
> > -		ret = i2c_smbus_read_word_data(data->client,
> > -					       BMG160_AXIS_TO_REG(bit));
> > -		if (ret < 0) {
> > -			mutex_unlock(&data->mutex);
> > -			goto err;
> > -		}
> > -		data->buffer[i++] = ret;
> > -	}
> > +	ret = i2c_smbus_read_i2c_block_data_or_emulated(data->client,
> > +							BMG160_REG_XOUT_L,
> > +							AXIS_MAX * 2,
> > +							(u8 *)data->buffer);
> >  	mutex_unlock(&data->mutex);
> > +	if (ret < 0)
> > +		goto err;
> >  
> >  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >  					   pf->timestamp);
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Tirdea, Irina]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFya3VzIFBhcmdtYW5u
IFttYWlsdG86bXBhQHBlbmd1dHJvbml4LmRlXQ0KPiBTZW50OiAxNyBBdWd1c3QsIDIwMTUgMTI6
MTANCj4gVG86IEpvbmF0aGFuIENhbWVyb24NCj4gQ2M6IFRpcmRlYSwgSXJpbmE7IFdvbGZyYW0g
U2FuZzsgbGludXgtaWlvQHZnZXIua2VybmVsLm9yZzsgbGludXgtaTJjQHZnZXIua2VybmVsLm9y
ZzsgbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgUGFuZHJ1dmFkYSwNCj4gU3Jpbml2YXM7
IFBldGVyIE1lZXJ3YWxkDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjUgNi84XSBpaW86IGd5cm86
IGJtZzE2MDogb3B0aW1pemUgaTJjIHRyYW5zZmVycyBpbiB0cmlnZ2VyIGhhbmRsZXINCj4gDQo+
IE9uIFN1biwgQXVnIDE2LCAyMDE1IGF0IDEwOjI0OjQ3QU0gKzAxMDAsIEpvbmF0aGFuIENhbWVy
b24gd3JvdGU6DQo+ID4gT24gMTIvMDgvMTUgMTU6MzEsIElyaW5hIFRpcmRlYSB3cm90ZToNCj4g
PiA+IFNvbWUgaTJjIGJ1c3NlcyAoZS5nLjogU3lub3BzeXMgRGVzaWduV2FyZSBJMkMgYWRhcHRl
cikgbmVlZCB0bw0KPiA+ID4gZW5hYmxlL2Rpc2FibGUgdGhlIGJ1cyBhdCBlYWNoIGkyYyB0cmFu
c2ZlciBhbmQgbXVzdCB3YWl0IGZvcg0KPiA+ID4gdGhlIGVuYWJsZS9kaXNhYmxlIHRvIGhhcHBl
biBiZWZvcmUgc2VuZGluZyB0aGUgZGF0YS4NCj4gPiA+DQo+ID4gPiBXaGVuIHJlYWRpbmcgZGF0
YSBpbiB0aGUgdHJpZ2dlciBoYW5kbGVyLCB0aGUgYm1nMTYwIGRyaXZlciBkb2VzDQo+ID4gPiBv
bmUgaTJjIHRyYW5zZmVyIGZvciBlYWNoIGF4aXMuIFRoaXMgaGFzIGFuIGltcGFjdCBvbiB0aGUg
ZnJlcXVlbmN5DQo+ID4gPiBvZiB0aGUgZ3lyb3Njb3BlIGF0IGhpZ2ggc2FtcGxlIHJhdGVzIGR1
ZSB0byBhZGRpdGlvbmFsIGRlbGF5cw0KPiA+ID4gaW50cm9kdWNlZCBieSB0aGUgaTJjIGJ1cyBh
dCBlYWNoIHRyYW5zZmVyLg0KPiA+ID4NCj4gPiA+IFJlYWRpbmcgYWxsIGF4aXMgdmFsdWVzIGlu
IG9uZSBpMmMgdHJhbnNmZXIgcmVkdWNlcyB0aGUgZGVsYXlzDQo+ID4gPiBpbnRyb2R1Y2VkIGJ5
IHRoZSBpMmMgYnVzLiBVc2VzIGkyY19zbWJ1c19yZWFkX2kyY19ibG9ja19kYXRhX29yX2VtdWxh
dGVkDQo+ID4gPiB0aGF0IHdpbGwgZmFsbGJhY2sgdG8gcmVhZGluZyBlYWNoIGF4aXMgYXMgYSBz
ZXBhcmF0ZSB3b3JkIGluIGNhc2UgaTJjDQo+ID4gPiBibG9jayByZWFkIGlzIG5vdCBzdXBwb3J0
ZWQuDQo+ID4gPg0KPiA+ID4gU2lnbmVkLW9mZi1ieTogSXJpbmEgVGlyZGVhIDxpcmluYS50aXJk
ZWFAaW50ZWwuY29tPg0KPiA+ID4gQWNrZWQtYnk6IEpvbmF0aGFuIENhbWVyb24gPGppYzIzQGtl
cm5lbC5vcmc+DQo+ID4gPiBBY2tlZC1ieTogU3Jpbml2YXMgUGFuZHJ1dmFkYSA8c3Jpbml2YXMu
cGFuZHJ1dmFkYUBsaW51eC5pbnRlbC5jb20+DQo+ID4gTm90ZSwgdGhhdCBpbiB0aGUgbWVhbnRp
bWUgdGhlIGJtZzE2MCBkcml2ZXIganVzdCB3ZW50IGFsbCByZWdtYXANCj4gPiBvbiB1cyAoYXMg
cGFydCBvZiBhZGRpbmcgU1BJIHN1cHBvcnQgLSB0aG91Z2ggdGhhdCBzdGVwIGhhc24ndA0KPiA+
IGhhcHBlbmVkIHlldCkuICBIZW5jZSB3ZSdsbCBuZWVkIGEgbWVhbnMgb2YgdGVsbGluZyByZWdt
YXAgYWJvdXQgdGhpcw0KPiA+IHBvc3NpYmlsaXR5Lg0KPiANCj4gUGVyaGFwcyB0aGlzIGlzIGNv
dmVyZWQgYnkgYSByZWdtYXBfYnVsa19yZWFkKCk/DQoNCkkgdGhpbmsgaXQgaXMuIEhvd2V2ZXIs
IGlmIHRoYXQgZG9lcyBub3Qgd29yayBmb3IgdGhlIGkyYyBjb250cm9sbGVycyBJJ20NCnRlc3Rp
bmcgd2l0aCBJJ2xsIHRha2UgYSBsb29rIGF0IHRoZSBwYXRjaGVzIHlvdSBtZW50aW9uIGJlbG93
Lg0KSSdsbCByZWJhc2UgdGhpcyBwYXRjaCBvbiB0b3Agb2YgeW91ciBuZXh0IHZlcnNpb24gZm9y
IGJtZzE2MCBhbmQNCnJlc2VuZC4NCg0KVGhhbmtzLA0KSXJpbmENCg0KPiANCj4gVGhlIHNlcmll
c1sxXSBJIGFtIHdvcmtpbmcgb24gaW1wbGVtZW50cyBhIGkyYyBzbWJ1cyBibG9jayBkYXRhIHJl
Z21hcA0KPiBidXMgZHJpdmVyLiBSZWdtYXAgc2hvdWxkIHRoZW4gYXV0b21hdGljYWxseSBkbyBh
IGJsb2NrIHJlYWQgaW4NCj4gcmVnbWFwX2J1bGtfcmVhZC4NCj4gDQo+IFBhdGNoIDE1IGludHJv
ZHVjZXMgdGhlIGkyYyBibG9jayBkYXRhIHJlZ21hcCBidXMgZHJpdmVyWzJdLg0KPiBJIGFtIG9u
bHkgaW1wbGVtZW50aW5nIHRoaXMgc28gSSBkb24ndCBicmVhayBibWMxNTAgYmVoYXZpb3IuIEkg
ZG8gbm90DQo+IGhhdmUgdGhlIGhhcmR3YXJlIGF2YWlsYWJsZSB0byB0ZXN0IHRoaXMgcmVnbWFw
IGRyaXZlciBzbyBpdCB3b3VsZCBiZSBncmVhdA0KPiBpZiBzb21lb25lIGVsc2UgY291bGQgdGVz
dCBvbmUgb2YgdGhlIG5leHQgdmVyc2lvbnMgb2YgdGhpcyBidXMgZHJpdmVyLg0KPiANCj4gQmVz
dCByZWdhcmRzLA0KPiANCj4gTWFya3VzDQo+IA0KPiBbMV0gaHR0cDovL3RocmVhZC5nbWFuZS5v
cmcvZ21hbmUubGludXgua2VybmVsLzIwMTg2NDMNCj4gWzJdIGh0dHA6Ly90aHJlYWQuZ21hbmUu
b3JnL2dtYW5lLmxpbnV4Lmtlcm5lbC8yMDE4NjM5DQo+IA0KPiA+DQo+ID4gPiAtLS0NCj4gPiA+
ICBkcml2ZXJzL2lpby9neXJvL2JtZzE2MC5jIHwgMTggKysrKysrKystLS0tLS0tLS0tDQo+ID4g
PiAgMSBmaWxlIGNoYW5nZWQsIDggaW5zZXJ0aW9ucygrKSwgMTAgZGVsZXRpb25zKC0pDQo+ID4g
Pg0KPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvaWlvL2d5cm8vYm1nMTYwLmMgYi9kcml2ZXJz
L2lpby9neXJvL2JtZzE2MC5jDQo+ID4gPiBpbmRleCBiMmE2Y2NiLi4xZmYzMDZkIDEwMDY0NA0K
PiA+ID4gLS0tIGEvZHJpdmVycy9paW8vZ3lyby9ibWcxNjAuYw0KPiA+ID4gKysrIGIvZHJpdmVy
cy9paW8vZ3lyby9ibWcxNjAuYw0KPiA+ID4gQEAgLTc3Miw2ICs3NzIsNyBAQCBzdGF0aWMgY29u
c3Qgc3RydWN0IGlpb19ldmVudF9zcGVjIGJtZzE2MF9ldmVudCA9IHsNCj4gPiA+ICAJCS5zaWdu
ID0gJ3MnLAkJCQkJCVwNCj4gPiA+ICAJCS5yZWFsYml0cyA9IDE2LAkJCQkJXA0KPiA+ID4gIAkJ
LnN0b3JhZ2ViaXRzID0gMTYsCQkJCQlcDQo+ID4gPiArCQkuZW5kaWFubmVzcyA9IElJT19MRSwJ
CQkJCVwNCj4gPiA+ICAJfSwJCQkJCQkJCVwNCj4gPiA+ICAJLmV2ZW50X3NwZWMgPSAmYm1nMTYw
X2V2ZW50LAkJCQkJXA0KPiA+ID4gIAkubnVtX2V2ZW50X3NwZWNzID0gMQkJCQkJCVwNCj4gPiA+
IEBAIC04MDksMTkgKzgxMCwxNiBAQCBzdGF0aWMgaXJxcmV0dXJuX3QgYm1nMTYwX3RyaWdnZXJf
aGFuZGxlcihpbnQgaXJxLCB2b2lkICpwKQ0KPiA+ID4gIAlzdHJ1Y3QgaWlvX3BvbGxfZnVuYyAq
cGYgPSBwOw0KPiA+ID4gIAlzdHJ1Y3QgaWlvX2RldiAqaW5kaW9fZGV2ID0gcGYtPmluZGlvX2Rl
djsNCj4gPiA+ICAJc3RydWN0IGJtZzE2MF9kYXRhICpkYXRhID0gaWlvX3ByaXYoaW5kaW9fZGV2
KTsNCj4gPiA+IC0JaW50IGJpdCwgcmV0LCBpID0gMDsNCj4gPiA+ICsJaW50IHJldCA9IDA7DQo+
ID4gPg0KPiA+ID4gIAltdXRleF9sb2NrKCZkYXRhLT5tdXRleCk7DQo+ID4gPiAtCWZvciAoYml0
ID0gMDsgYml0IDwgQVhJU19NQVg7IGJpdCsrKSB7DQo+ID4gPiAtCQlyZXQgPSBpMmNfc21idXNf
cmVhZF93b3JkX2RhdGEoZGF0YS0+Y2xpZW50LA0KPiA+ID4gLQkJCQkJICAgICAgIEJNRzE2MF9B
WElTX1RPX1JFRyhiaXQpKTsNCj4gPiA+IC0JCWlmIChyZXQgPCAwKSB7DQo+ID4gPiAtCQkJbXV0
ZXhfdW5sb2NrKCZkYXRhLT5tdXRleCk7DQo+ID4gPiAtCQkJZ290byBlcnI7DQo+ID4gPiAtCQl9
DQo+ID4gPiAtCQlkYXRhLT5idWZmZXJbaSsrXSA9IHJldDsNCj4gPiA+IC0JfQ0KPiA+ID4gKwly
ZXQgPSBpMmNfc21idXNfcmVhZF9pMmNfYmxvY2tfZGF0YV9vcl9lbXVsYXRlZChkYXRhLT5jbGll
bnQsDQo+ID4gPiArCQkJCQkJCUJNRzE2MF9SRUdfWE9VVF9MLA0KPiA+ID4gKwkJCQkJCQlBWElT
X01BWCAqIDIsDQo+ID4gPiArCQkJCQkJCSh1OCAqKWRhdGEtPmJ1ZmZlcik7DQo+ID4gPiAgCW11
dGV4X3VubG9jaygmZGF0YS0+bXV0ZXgpOw0KPiA+ID4gKwlpZiAocmV0IDwgMCkNCj4gPiA+ICsJ
CWdvdG8gZXJyOw0KPiA+ID4NCj4gPiA+ICAJaWlvX3B1c2hfdG9fYnVmZmVyc193aXRoX3RpbWVz
dGFtcChpbmRpb19kZXYsIGRhdGEtPmJ1ZmZlciwNCj4gPiA+ICAJCQkJCSAgIHBmLT50aW1lc3Rh
bXApOw0KPiA+ID4NCj4gPg0KPiA+IC0tDQo+ID4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxp
c3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LWlpbyIgaW4NCj4gPiB0aGUgYm9k
eSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZw0KPiA+IE1vcmUgbWFq
b3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRt
bA0KPiA+DQo+IA0KPiAtLQ0KPiBQZW5ndXRyb25peCBlLksuICAgICAgICAgICAgICAgICAgICAg
ICAgICAgfCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfA0KPiBJbmR1c3RyaWFsIExpbnV4
IFNvbHV0aW9ucyAgICAgICAgICAgICAgICAgfCBodHRwOi8vd3d3LnBlbmd1dHJvbml4LmRlLyAg
fA0KPiBQZWluZXIgU3RyLiA2LTgsIDMxMTM3IEhpbGRlc2hlaW0sIEdlcm1hbnkgfCBQaG9uZTog
KzQ5LTUxMjEtMjA2OTE3LTAgICAgfA0KPiBBbXRzZ2VyaWNodCBIaWxkZXNoZWltLCBIUkEgMjY4
NiAgICAgICAgICAgfCBGYXg6ICAgKzQ5LTUxMjEtMjA2OTE3LTU1NTUgfA0K

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2035 bytes --]

On Mon, Aug 17, 2015 at 11:09:43AM +0200, Markus Pargmann wrote:
> On Sun, Aug 16, 2015 at 10:24:47AM +0100, Jonathan Cameron wrote:
> > On 12/08/15 15:31, Irina Tirdea wrote:
> > > Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> > > enable/disable the bus at each i2c transfer and must wait for
> > > the enable/disable to happen before sending the data.
> > > 
> > > When reading data in the trigger handler, the bmg160 driver does
> > > one i2c transfer for each axis. This has an impact on the frequency
> > > of the gyroscope at high sample rates due to additional delays
> > > introduced by the i2c bus at each transfer.
> > > 
> > > Reading all axis values in one i2c transfer reduces the delays
> > > introduced by the i2c bus. Uses i2c_smbus_read_i2c_block_data_or_emulated
> > > that will fallback to reading each axis as a separate word in case i2c
> > > block read is not supported.
> > > 
> > > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > > Acked-by: Jonathan Cameron <jic23@kernel.org>
> > > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Note, that in the meantime the bmg160 driver just went all regmap
> > on us (as part of adding SPI support - though that step hasn't
> > happened yet).  Hence we'll need a means of telling regmap about this
> > possibility.
> 
> Perhaps this is covered by a regmap_bulk_read()?
> 
> The series[1] I am working on implements a i2c smbus block data regmap
> bus driver. Regmap should then automatically do a block read in
> regmap_bulk_read.

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

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Pandruvada, Srinivas]

T24gRnJpLCAyMDE1LTA4LTIxIGF0IDEyOjIxICswMjAwLCBXb2xmcmFtIFNhbmcgd3JvdGU6DQo+
IE9uIE1vbiwgQXVnIDE3LCAyMDE1IGF0IDExOjA5OjQzQU0gKzAyMDAsIE1hcmt1cyBQYXJnbWFu
biB3cm90ZToNCj4gPiBPbiBTdW4sIEF1ZyAxNiwgMjAxNSBhdCAxMDoyNDo0N0FNICswMTAwLCBK
b25hdGhhbiBDYW1lcm9uIHdyb3RlOg0KPiA+ID4gT24gMTIvMDgvMTUgMTU6MzEsIElyaW5hIFRp
cmRlYSB3cm90ZToNCj4gPiA+ID4gU29tZSBpMmMgYnVzc2VzIChlLmcuOiBTeW5vcHN5cyBEZXNp
Z25XYXJlIEkyQyBhZGFwdGVyKSBuZWVkIHRvDQo+ID4gPiA+IGVuYWJsZS9kaXNhYmxlIHRoZSBi
dXMgYXQgZWFjaCBpMmMgdHJhbnNmZXIgYW5kIG11c3Qgd2FpdCBmb3INCj4gPiA+ID4gdGhlIGVu
YWJsZS9kaXNhYmxlIHRvIGhhcHBlbiBiZWZvcmUgc2VuZGluZyB0aGUgZGF0YS4NCj4gPiA+ID4g
DQo+ID4gPiA+IFdoZW4gcmVhZGluZyBkYXRhIGluIHRoZSB0cmlnZ2VyIGhhbmRsZXIsIHRoZSBi
bWcxNjAgZHJpdmVyIA0KPiA+ID4gPiBkb2VzDQo+ID4gPiA+IG9uZSBpMmMgdHJhbnNmZXIgZm9y
IGVhY2ggYXhpcy4gVGhpcyBoYXMgYW4gaW1wYWN0IG9uIHRoZSANCj4gPiA+ID4gZnJlcXVlbmN5
DQo+ID4gPiA+IG9mIHRoZSBneXJvc2NvcGUgYXQgaGlnaCBzYW1wbGUgcmF0ZXMgZHVlIHRvIGFk
ZGl0aW9uYWwgZGVsYXlzDQo+ID4gPiA+IGludHJvZHVjZWQgYnkgdGhlIGkyYyBidXMgYXQgZWFj
aCB0cmFuc2Zlci4NCj4gPiA+ID4gDQo+ID4gPiA+IFJlYWRpbmcgYWxsIGF4aXMgdmFsdWVzIGlu
IG9uZSBpMmMgdHJhbnNmZXIgcmVkdWNlcyB0aGUgZGVsYXlzDQo+ID4gPiA+IGludHJvZHVjZWQg
YnkgdGhlIGkyYyBidXMuIFVzZXMgDQo+ID4gPiA+IGkyY19zbWJ1c19yZWFkX2kyY19ibG9ja19k
YXRhX29yX2VtdWxhdGVkDQo+ID4gPiA+IHRoYXQgd2lsbCBmYWxsYmFjayB0byByZWFkaW5nIGVh
Y2ggYXhpcyBhcyBhIHNlcGFyYXRlIHdvcmQgaW4gDQo+ID4gPiA+IGNhc2UgaTJjDQo+ID4gPiA+
IGJsb2NrIHJlYWQgaXMgbm90IHN1cHBvcnRlZC4NCj4gPiA+ID4gDQo+ID4gPiA+IFNpZ25lZC1v
ZmYtYnk6IElyaW5hIFRpcmRlYSA8aXJpbmEudGlyZGVhQGludGVsLmNvbT4NCj4gPiA+ID4gQWNr
ZWQtYnk6IEpvbmF0aGFuIENhbWVyb24gPGppYzIzQGtlcm5lbC5vcmc+DQo+ID4gPiA+IEFja2Vk
LWJ5OiBTcmluaXZhcyBQYW5kcnV2YWRhIDwNCj4gPiA+ID4gc3Jpbml2YXMucGFuZHJ1dmFkYUBs
aW51eC5pbnRlbC5jb20+DQo+ID4gPiBOb3RlLCB0aGF0IGluIHRoZSBtZWFudGltZSB0aGUgYm1n
MTYwIGRyaXZlciBqdXN0IHdlbnQgYWxsIHJlZ21hcA0KPiA+ID4gb24gdXMgKGFzIHBhcnQgb2Yg
YWRkaW5nIFNQSSBzdXBwb3J0IC0gdGhvdWdoIHRoYXQgc3RlcCBoYXNuJ3QNCj4gPiA+IGhhcHBl
bmVkIHlldCkuICBIZW5jZSB3ZSdsbCBuZWVkIGEgbWVhbnMgb2YgdGVsbGluZyByZWdtYXAgYWJv
dXQgDQo+ID4gPiB0aGlzDQo+ID4gPiBwb3NzaWJpbGl0eS4NCj4gPiANCj4gPiBQZXJoYXBzIHRo
aXMgaXMgY292ZXJlZCBieSBhIHJlZ21hcF9idWxrX3JlYWQoKT8NCj4gPiANCj4gPiBUaGUgc2Vy
aWVzWzFdIEkgYW0gd29ya2luZyBvbiBpbXBsZW1lbnRzIGEgaTJjIHNtYnVzIGJsb2NrIGRhdGEg
DQo+ID4gcmVnbWFwDQo+ID4gYnVzIGRyaXZlci4gUmVnbWFwIHNob3VsZCB0aGVuIGF1dG9tYXRp
Y2FsbHkgZG8gYSBibG9jayByZWFkIGluDQo+ID4gcmVnbWFwX2J1bGtfcmVhZC4NCj4gDQo+IEht
bSwgc28gZG9lc24ndCB5b3VyIHNlcmllcyBtYWtlIElyaW5hJ3Mgc2VyaWVzIG9ic29sZXRlPyBJ
dCANCj4gYWRkcmVzc2VzDQo+IHRoZSBzYW1lIHByb2JsZW0gb25seSBhdCBhIGRpZmZlcmVudCBs
YXllciAoaTJjIGNvcmUgdnMuIHJlZ21hcCksIG9yPyANCj4gSXQNCj4gd291bGQgbWVhbiB0aGF0
IGkyYyBjbGllbnQgZHJpdmVycyB3aGljaCB3YW50IHRvIHN1cHBvcnQgYnl0ZSwgd29yZCwgDQo+
IG9yDQo+IGJsb2NrIHRyYW5zZmVycyBzaG91bGQgYmUgY29udmVydGVkIHRvIHJlZ21hcC4gSSBh
c3N1bWUgbW9zdCBvZiB0aGUNCj4gcG90ZW50aWFsIGNhbmRpZGF0ZXMgYXJlIHJlZ2lzdGVyIGJh
c2VkIGRldmljZXMgYW55aG93LiBUaGVuLCByZWdtYXANCj4gd291bGQgYmUgdGhlIHByb3BlciBh
YnN0cmFjdGlvbiBsYXllci4gSGF2ZSBJIG92ZXJsb29rZWQgc29tZXRoaW5nPw0KPiANClRoaXMg
aXMgdGhlIG9ubHkgZHJpdmVyIGNvbnZlcnRlZCB0byB1c2UgcmVnbWFwLCBiZWNhdXNlIG9mIFNQ
SSBtb2RlDQpzdXBwb3J0LiBUaGUgb3RoZXIgZHJpdmVycyB3aWxsIHN0aWxsIHVzZSBJcmluYSdz
IGNoYW5nZXMuDQoNClRoYW5rcw0KU3Jpbml2YXMNCg0KDQo+IFRoYW5rcywNCj4gDQo+ICAgIFdv
bGZyYW0NCj4g

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]


> > > The series[1] I am working on implements a i2c smbus block data 
> > > regmap
> > > bus driver. Regmap should then automatically do a block read in
> > > regmap_bulk_read.
> > 
> > Hmm, so doesn't your series make Irina's series obsolete? It 
> > addresses
> > the same problem only at a different layer (i2c core vs. regmap), or? 
> > It
> > would mean that i2c client drivers which want to support byte, word, 
> > or
> > block transfers should be converted to regmap. I assume most of the
> > potential candidates are register based devices anyhow. Then, regmap
> > would be the proper abstraction layer. Have I overlooked something?
> > 
> This is the only driver converted to use regmap, because of SPI mode
> support. The other drivers will still use Irina's changes.

The question is if they should. Or rather be converted to regmap. It is
an open question and I am seeking for further input.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jonathan Cameron]

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

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

Jonathan

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]


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

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

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

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Tirdea, Irina]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 16 August, 2015 12:25
> To: Tirdea, Irina; Wolfram Sang; linux-iio@vger.kernel.org; linux-i2c@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Pandruvada, Srinivas; Peter Meerwald
> Subject: Re: [PATCH v5 6/8] iio: gyro: bmg160: optimize i2c transfers in trigger handler
> 
> On 12/08/15 15:31, Irina Tirdea wrote:
> > Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> > enable/disable the bus at each i2c transfer and must wait for
> > the enable/disable to happen before sending the data.
> >
> > When reading data in the trigger handler, the bmg160 driver does
> > one i2c transfer for each axis. This has an impact on the frequency
> > of the gyroscope at high sample rates due to additional delays
> > introduced by the i2c bus at each transfer.
> >
> > Reading all axis values in one i2c transfer reduces the delays
> > introduced by the i2c bus. Uses i2c_smbus_read_i2c_block_data_or_emulated
> > that will fallback to reading each axis as a separate word in case i2c
> > block read is not supported.
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > Acked-by: Jonathan Cameron <jic23@kernel.org>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Note, that in the meantime the bmg160 driver just went all regmap
> on us (as part of adding SPI support - though that step hasn't
> happened yet).  Hence we'll need a means of telling regmap about this
> possibility.

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

Thanks,
Irina

> 
> > ---
> >  drivers/iio/gyro/bmg160.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/gyro/bmg160.c b/drivers/iio/gyro/bmg160.c
> > index b2a6ccb..1ff306d 100644
> > --- a/drivers/iio/gyro/bmg160.c
> > +++ b/drivers/iio/gyro/bmg160.c
> > @@ -772,6 +772,7 @@ static const struct iio_event_spec bmg160_event = {
> >  		.sign = 's',						\
> >  		.realbits = 16,					\
> >  		.storagebits = 16,					\
> > +		.endianness = IIO_LE,					\
> >  	},								\
> >  	.event_spec = &bmg160_event,					\
> >  	.num_event_specs = 1						\
> > @@ -809,19 +810,16 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
> >  	struct iio_poll_func *pf = p;
> >  	struct iio_dev *indio_dev = pf->indio_dev;
> >  	struct bmg160_data *data = iio_priv(indio_dev);
> > -	int bit, ret, i = 0;
> > +	int ret = 0;
> >
> >  	mutex_lock(&data->mutex);
> > -	for (bit = 0; bit < AXIS_MAX; bit++) {
> > -		ret = i2c_smbus_read_word_data(data->client,
> > -					       BMG160_AXIS_TO_REG(bit));
> > -		if (ret < 0) {
> > -			mutex_unlock(&data->mutex);
> > -			goto err;
> > -		}
> > -		data->buffer[i++] = ret;
> > -	}
> > +	ret = i2c_smbus_read_i2c_block_data_or_emulated(data->client,
> > +							BMG160_REG_XOUT_L,
> > +							AXIS_MAX * 2,
> > +							(u8 *)data->buffer);
> >  	mutex_unlock(&data->mutex);
> > +	if (ret < 0)
> > +		goto err;
> >
> >  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >  					   pf->timestamp);
> >

[PATCH v5 7/8] 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 0d9bd35..d4b80e7 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 {
@@ -956,6 +957,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;
@@ -965,8 +968,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);
@@ -1236,6 +1238,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 v5 8/8] 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 d4b80e7..e8f7b45 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -926,7 +926,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						\
@@ -964,19 +964,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