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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
---
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
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 <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
---
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
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 <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
---
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
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 <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
---
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
From: Adriana Reus <[email protected]>
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 <[email protected]>
Signed-off-by: Irina Tirdea <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
---
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
From: Adriana Reus <[email protected]>
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 <[email protected]>
Signed-off-by: Irina Tirdea <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
---
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
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 <[email protected]>
This looks concise, thanks for the rework
Applied to for-next, thanks!
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 <[email protected]>
Applied to for-next, thanks!
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 <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]>
> Acked-by: Jonathan Cameron <[email protected]>
> Acked-by: Srinivas Pandruvada <[email protected]>
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);
>
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 <[email protected]>
> > Acked-by: Jonathan Cameron <[email protected]>
> > Acked-by: Srinivas Pandruvada <[email protected]>
> 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 [email protected]
> 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 |
> -----Original Message-----
> From: Jonathan Cameron [mailto:[email protected]]
> Sent: 16 August, 2015 12:25
> To: Tirdea, Irina; Wolfram Sang; [email protected]; [email protected]
> Cc: [email protected]; 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 <[email protected]>
> > Acked-by: Jonathan Cameron <[email protected]>
> > Acked-by: Srinivas Pandruvada <[email protected]>
> 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);
> >
> -----Original Message-----
> From: Markus Pargmann [mailto:[email protected]]
> Sent: 17 August, 2015 12:10
> To: Jonathan Cameron
> Cc: Tirdea, Irina; Wolfram Sang; [email protected]; [email protected]; [email protected]; Pandruvada,
> Srinivas; Peter Meerwald
> Subject: Re: [PATCH v5 6/8] iio: gyro: bmg160: optimize i2c transfers in trigger handler
>
> 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 <[email protected]>
> > > Acked-by: Jonathan Cameron <[email protected]>
> > > Acked-by: Srinivas Pandruvada <[email protected]>
> > 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()?
I think it is. However, if that does not work for the i2c controllers I'm
testing with I'll take a look at the patches you mention below.
I'll rebase this patch on top of your next version for bmg160 and
resend.
Thanks,
Irina
>
> 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 [email protected]
> > 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 |
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> > > Acked-by: Jonathan Cameron <[email protected]>
> > > Acked-by: Srinivas Pandruvada <[email protected]>
> > 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
On Fri, 2015-08-21 at 12:21 +0200, Wolfram Sang wrote:
> 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 <[email protected]>
> > > > Acked-by: Jonathan Cameron <[email protected]>
> > > > Acked-by: Srinivas Pandruvada <
> > > > [email protected]>
> > > 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?
>
This is the only driver converted to use regmap, because of SPI mode
support. The other drivers will still use Irina's changes.
Thanks
Srinivas
> Thanks,
>
> Wolfram
> ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> > > 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.
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
> 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