Adds optimization of i2c transactions in trigger handler and
usage of available_scan_masks for kxcjk-1013.
Adriana Reus (2):
iio: accel: kxcjk-1013: use available_scan_masks
iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
1 file changed, 39 insertions(+), 11 deletions(-)
--
1.9.1
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.
This also fixes the issue of accessing the buffer scan_mask
instead of active_scan_mask, since these might not be the
same due to client devices.
Signed-off-by: Adriana Reus <[email protected]>
Signed-off-by: Irina Tirdea <[email protected]>
Reviewed-by: Srinivas Pandruvada <[email protected]>
---
drivers/iio/accel/kxcjk-1013.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 567de26..5f27787 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -115,6 +115,7 @@ enum kxcjk1013_axis {
AXIS_X,
AXIS_Y,
AXIS_Z,
+ AXIS_MAX,
};
enum kxcjk1013_mode {
@@ -947,6 +948,8 @@ static const struct iio_info kxcjk1013_info = {
.driver_module = THIS_MODULE,
};
+static const unsigned long kxcjk1013_scan_masks[] = {0x7, 0};
+
static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf = p;
@@ -955,9 +958,7 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
int bit, ret, i = 0;
mutex_lock(&data->mutex);
-
- for_each_set_bit(bit, indio_dev->buffer->scan_mask,
- indio_dev->masklength) {
+ for (bit = 0; bit < AXIS_MAX; bit++) {
ret = kxcjk1013_get_acc_reg(data, bit);
if (ret < 0) {
mutex_unlock(&data->mutex);
@@ -1228,6 +1229,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
indio_dev->dev.parent = &client->dev;
indio_dev->channels = kxcjk1013_channels;
indio_dev->num_channels = ARRAY_SIZE(kxcjk1013_channels);
+ indio_dev->available_scan_masks = kxcjk1013_scan_masks;
indio_dev->name = name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &kxcjk1013_info;
--
1.9.1
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. In case i2c block read is not supported,
fallback to reading each axis as a separate word.
Signed-off-by: Adriana Reus <[email protected]>
Signed-off-by: Irina Tirdea <[email protected]>
Reviewed-by: Srinivas Pandruvada <[email protected]>
---
drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 5f27787..bfa2899 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -109,6 +109,8 @@ struct kxcjk1013_data {
int64_t timestamp;
enum kx_chipset chipset;
bool is_smo8500_device;
+ s32 (*read_block_data)(const struct i2c_client *client, u8 command,
+ u8 length, u8 *values);
};
enum kxcjk1013_axis {
@@ -216,6 +218,23 @@ static const struct {
{800, 0, 0x06},
{1600, 0, 0x06} };
+static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
+ u8 command, u8 length, u8 *values)
+{
+ s32 data;
+ u8 i;
+
+ for (i = 0; i < length; i += 2) {
+ data = i2c_smbus_read_word_data(client, command + i);
+ if (data < 0)
+ return data;
+
+ values[i] = data & 0xFF;
+ values[i+1] = data >> 8;
+ }
+ return i;
+}
+
static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
enum kxcjk1013_mode mode)
{
@@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
struct iio_poll_func *pf = p;
struct iio_dev *indio_dev = pf->indio_dev;
struct kxcjk1013_data *data = iio_priv(indio_dev);
- int bit, ret, i = 0;
+ int ret;
mutex_lock(&data->mutex);
- for (bit = 0; bit < AXIS_MAX; bit++) {
- ret = kxcjk1013_get_acc_reg(data, bit);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- goto err;
- }
- data->buffer[i++] = ret;
- }
+ ret = data->read_block_data(data->client, KXCJK1013_REG_XOUT_L,
+ AXIS_MAX * 2, (u8 *) data->buffer);
mutex_unlock(&data->mutex);
+ if (ret < 0)
+ goto err;
iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
data->timestamp);
@@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
const char *name;
int ret;
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_READ_WORD_DATA))
+ return -ENODEV;
+
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
@@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
i2c_set_clientdata(client, indio_dev);
data->client = client;
+ if (i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK))
+ data->read_block_data = i2c_smbus_read_i2c_block_data;
+ else
+ data->read_block_data = kxcjk1013_read_block_data;
+
pdata = dev_get_platdata(&client->dev);
if (pdata)
data->active_high_intr = pdata->active_high_intr;
--
1.9.1
> Adds optimization of i2c transactions in trigger handler and
> usage of available_scan_masks for kxcjk-1013.
the 'optimization' changes the capabilities of the driver, reading
individual channel is not possible anymore
> Adriana Reus (2):
> iio: accel: kxcjk-1013: use available_scan_masks
> iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>
> drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
> 1 file changed, 39 insertions(+), 11 deletions(-)
>
>
--
Peter Meerwald
+43-664-2444418 (mobile)
> Reading all axis values in one i2c transfer reduces the delays
> introduced by the i2c bus. In case i2c block read is not supported,
> fallback to reading each axis as a separate word.
see comments inline below
> Signed-off-by: Adriana Reus <[email protected]>
> Signed-off-by: Irina Tirdea <[email protected]>
> Reviewed-by: Srinivas Pandruvada <[email protected]>
> ---
> drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 5f27787..bfa2899 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> int64_t timestamp;
> enum kx_chipset chipset;
> bool is_smo8500_device;
> + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> + u8 length, u8 *values);
probably this could/should be done in the i2c layer or we end up adding a
similar function in every driver?
> };
>
> enum kxcjk1013_axis {
> @@ -216,6 +218,23 @@ static const struct {
> {800, 0, 0x06},
> {1600, 0, 0x06} };
>
> +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
> + u8 command, u8 length, u8 *values)
> +{
> + s32 data;
> + u8 i;
> +
> + for (i = 0; i < length; i += 2) {
> + data = i2c_smbus_read_word_data(client, command + i);
> + if (data < 0)
> + return data;
> +
> + values[i] = data & 0xFF;
> + values[i+1] = data >> 8;
this is incorrect; it forces the data to be little endian, however, the
endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
code breaks for big-endian CPUs
since _read_i2c_block_data() can't do endianness conversion (and the chip
does i2c endianness, i.e. little-endian), the .scan_type should become
IIO_LE and above code is correct again but still ugly :)
bottom line: change .scan_type to IIO_LE
> + }
> + return i;
> +}
> +
> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> enum kxcjk1013_mode mode)
> {
> @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct kxcjk1013_data *data = iio_priv(indio_dev);
> - int bit, ret, i = 0;
> + int ret;
>
> mutex_lock(&data->mutex);
> - for (bit = 0; bit < AXIS_MAX; bit++) {
> - ret = kxcjk1013_get_acc_reg(data, bit);
> - if (ret < 0) {
> - mutex_unlock(&data->mutex);
> - goto err;
> - }
> - data->buffer[i++] = ret;
> - }
> + ret = data->read_block_data(data->client, KXCJK1013_REG_XOUT_L,
> + AXIS_MAX * 2, (u8 *) data->buffer);
> mutex_unlock(&data->mutex);
> + if (ret < 0)
> + goto err;
>
> iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> data->timestamp);
> @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
> const char *name;
> int ret;
>
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_READ_WORD_DATA))
> + return -ENODEV;
> +
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
> @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
> i2c_set_clientdata(client, indio_dev);
> data->client = client;
>
> + if (i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> + data->read_block_data = i2c_smbus_read_i2c_block_data;
> + else
> + data->read_block_data = kxcjk1013_read_block_data;
> +
> pdata = dev_get_platdata(&client->dev);
> if (pdata)
> data->active_high_intr = pdata->active_high_intr;
>
--
Peter Meerwald
+43-664-2444418 (mobile)
> -----Original Message-----
> From: Peter Meerwald [mailto:[email protected]]
> Sent: 16 February, 2015 21:26
> To: Tirdea, Irina
> Cc: Jonathan Cameron; [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>
>
> > Reading all axis values in one i2c transfer reduces the delays
> > introduced by the i2c bus. In case i2c block read is not supported,
> > fallback to reading each axis as a separate word.
>
> see comments inline below
>
Thanks for the review, Peter!
> > Signed-off-by: Adriana Reus <[email protected]>
> > Signed-off-by: Irina Tirdea <[email protected]>
> > Reviewed-by: Srinivas Pandruvada <[email protected]>
> > ---
> > drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
> > 1 file changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> > index 5f27787..bfa2899 100644
> > --- a/drivers/iio/accel/kxcjk-1013.c
> > +++ b/drivers/iio/accel/kxcjk-1013.c
> > @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> > int64_t timestamp;
> > enum kx_chipset chipset;
> > bool is_smo8500_device;
> > + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> > + u8 length, u8 *values);
>
> probably this could/should be done in the i2c layer or we end up adding a
> similar function in every driver?
>
Actually this is exactly what I did: adding this code in a couple of drivers :)
You are right, this belongs to the i2c core. Will move it there.
> > };
> >
> > enum kxcjk1013_axis {
> > @@ -216,6 +218,23 @@ static const struct {
> > {800, 0, 0x06},
> > {1600, 0, 0x06} };
> >
> > +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
> > + u8 command, u8 length, u8 *values)
> > +{
> > + s32 data;
> > + u8 i;
> > +
> > + for (i = 0; i < length; i += 2) {
> > + data = i2c_smbus_read_word_data(client, command + i);
> > + if (data < 0)
> > + return data;
> > +
> > + values[i] = data & 0xFF;
> > + values[i+1] = data >> 8;
>
> this is incorrect; it forces the data to be little endian, however, the
> endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> code breaks for big-endian CPUs
>
> since _read_i2c_block_data() can't do endianness conversion (and the chip
> does i2c endianness, i.e. little-endian), the .scan_type should become
> IIO_LE and above code is correct again but still ugly :)
>
> bottom line: change .scan_type to IIO_LE
>
Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
Will fix this in the next version. Thanks for catching this!
> > + }
> > + return i;
> > +}
> > +
> > static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> > enum kxcjk1013_mode mode)
> > {
> > @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
> > struct iio_poll_func *pf = p;
> > struct iio_dev *indio_dev = pf->indio_dev;
> > struct kxcjk1013_data *data = iio_priv(indio_dev);
> > - int bit, ret, i = 0;
> > + int ret;
> >
> > mutex_lock(&data->mutex);
> > - for (bit = 0; bit < AXIS_MAX; bit++) {
> > - ret = kxcjk1013_get_acc_reg(data, bit);
> > - if (ret < 0) {
> > - mutex_unlock(&data->mutex);
> > - goto err;
> > - }
> > - data->buffer[i++] = ret;
> > - }
> > + ret = data->read_block_data(data->client, KXCJK1013_REG_XOUT_L,
> > + AXIS_MAX * 2, (u8 *) data->buffer);
> > mutex_unlock(&data->mutex);
> > + if (ret < 0)
> > + goto err;
> >
> > iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > data->timestamp);
> > @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
> > const char *name;
> > int ret;
> >
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_BYTE_DATA |
> > + I2C_FUNC_SMBUS_READ_WORD_DATA))
> > + return -ENODEV;
> > +
> > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > if (!indio_dev)
> > return -ENOMEM;
> > @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
> > i2c_set_clientdata(client, indio_dev);
> > data->client = client;
> >
> > + if (i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > + data->read_block_data = i2c_smbus_read_i2c_block_data;
> > + else
> > + data->read_block_data = kxcjk1013_read_block_data;
> > +
> > pdata = dev_get_platdata(&client->dev);
> > if (pdata)
> > data->active_high_intr = pdata->active_high_intr;
> >
>
> --
>
> Peter Meerwald
> +43-664-2444418 (mobile)
> -----Original Message-----
> From: Peter Meerwald [mailto:[email protected]]
> Sent: 16 February, 2015 21:14
> To: Tirdea, Irina
> Cc: Jonathan Cameron; [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
>
>
> > Adds optimization of i2c transactions in trigger handler and
> > usage of available_scan_masks for kxcjk-1013.
>
> the 'optimization' changes the capabilities of the driver, reading
> individual channel is not possible anymore
>
We can still read individual channels, just that the driver does not see that anymore and the demux is handled by the iio core.
available_scan_masks contains the bitmap superset for all channels we can enable (I will remove the 0 available_scan_mask as it does not makes sense).
When a new channel is enabled, the iio core checks if the new mask is a subset of any entry in available_scan_masks. When the buffer is pushed from the driver, the iio core will select only the enabled channel values.
> > Adriana Reus (2):
> > iio: accel: kxcjk-1013: use available_scan_masks
> > iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >
> > drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
> > 1 file changed, 39 insertions(+), 11 deletions(-)
> >
> >
>
> --
>
> Peter Meerwald
> +43-664-2444418 (mobile)
On Fri, 2015-02-20 at 12:02 +0000, Tirdea, Irina wrote:
>
> > -----Original Message-----
> > From: Peter Meerwald [mailto:[email protected]]
> > Sent: 16 February, 2015 21:26
> > To: Tirdea, Irina
> > Cc: Jonathan Cameron; [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
> > Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >
> >
> > > Reading all axis values in one i2c transfer reduces the delays
> > > introduced by the i2c bus. In case i2c block read is not supported,
> > > fallback to reading each axis as a separate word.
> >
> > see comments inline below
> >
> Thanks for the review, Peter!
>
> > > Signed-off-by: Adriana Reus <[email protected]>
> > > Signed-off-by: Irina Tirdea <[email protected]>
> > > Reviewed-by: Srinivas Pandruvada <[email protected]>
> > > ---
> > > drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 35 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> > > index 5f27787..bfa2899 100644
> > > --- a/drivers/iio/accel/kxcjk-1013.c
> > > +++ b/drivers/iio/accel/kxcjk-1013.c
> > > @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> > > int64_t timestamp;
> > > enum kx_chipset chipset;
> > > bool is_smo8500_device;
> > > + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> > > + u8 length, u8 *values);
> >
> > probably this could/should be done in the i2c layer or we end up adding a
> > similar function in every driver?
> >
> Actually this is exactly what I did: adding this code in a couple of drivers :)
> You are right, this belongs to the i2c core. Will move it there.
>
> > > };
> > >
> > > enum kxcjk1013_axis {
> > > @@ -216,6 +218,23 @@ static const struct {
> > > {800, 0, 0x06},
> > > {1600, 0, 0x06} };
> > >
> > > +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
> > > + u8 command, u8 length, u8 *values)
> > > +{
> > > + s32 data;
> > > + u8 i;
> > > +
> > > + for (i = 0; i < length; i += 2) {
> > > + data = i2c_smbus_read_word_data(client, command + i);
> > > + if (data < 0)
> > > + return data;
> > > +
> > > + values[i] = data & 0xFF;
> > > + values[i+1] = data >> 8;
> >
> > this is incorrect; it forces the data to be little endian, however, the
> > endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> > code breaks for big-endian CPUs
> >
> > since _read_i2c_block_data() can't do endianness conversion (and the chip
> > does i2c endianness, i.e. little-endian), the .scan_type should become
> > IIO_LE and above code is correct again but still ugly :)
> >
> > bottom line: change .scan_type to IIO_LE
> >
> Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> Will fix this in the next version. Thanks for catching this!
>
I don't think changing to IIO_LE is good idea as when i2c_read_bock..
then the scan type will be CPU. So better to fix endianness in this
function.
> > > + }
> > > + return i;
> > > +}
> > > +
> > > static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> > > enum kxcjk1013_mode mode)
> > > {
> > > @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
> > > struct iio_poll_func *pf = p;
> > > struct iio_dev *indio_dev = pf->indio_dev;
> > > struct kxcjk1013_data *data = iio_priv(indio_dev);
> > > - int bit, ret, i = 0;
> > > + int ret;
> > >
> > > mutex_lock(&data->mutex);
> > > - for (bit = 0; bit < AXIS_MAX; bit++) {
> > > - ret = kxcjk1013_get_acc_reg(data, bit);
> > > - if (ret < 0) {
> > > - mutex_unlock(&data->mutex);
> > > - goto err;
> > > - }
> > > - data->buffer[i++] = ret;
> > > - }
> > > + ret = data->read_block_data(data->client, KXCJK1013_REG_XOUT_L,
> > > + AXIS_MAX * 2, (u8 *) data->buffer);
> > > mutex_unlock(&data->mutex);
> > > + if (ret < 0)
> > > + goto err;
> > >
> > > iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > > data->timestamp);
> > > @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
> > > const char *name;
> > > int ret;
> > >
> > > + if (!i2c_check_functionality(client->adapter,
> > > + I2C_FUNC_SMBUS_BYTE_DATA |
> > > + I2C_FUNC_SMBUS_READ_WORD_DATA))
> > > + return -ENODEV;
> > > +
> > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > if (!indio_dev)
> > > return -ENOMEM;
> > > @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
> > > i2c_set_clientdata(client, indio_dev);
> > > data->client = client;
> > >
> > > + if (i2c_check_functionality(client->adapter,
> > > + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > > + data->read_block_data = i2c_smbus_read_i2c_block_data;
> > > + else
> > > + data->read_block_data = kxcjk1013_read_block_data;
> > > +
> > > pdata = dev_get_platdata(&client->dev);
> > > if (pdata)
> > > data->active_high_intr = pdata->active_high_intr;
> > >
> >
> > --
> >
> > Peter Meerwald
> > +43-664-2444418 (mobile)
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> > > > +
> > > > + values[i] = data & 0xFF;
> > > > + values[i+1] = data >> 8;
> > >
> > > this is incorrect; it forces the data to be little endian, however, the
> > > endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> > > code breaks for big-endian CPUs
> > >
> > > since _read_i2c_block_data() can't do endianness conversion (and the chip
> > > does i2c endianness, i.e. little-endian), the .scan_type should become
> > > IIO_LE and above code is correct again but still ugly :)
> > >
> > > bottom line: change .scan_type to IIO_LE
> > >
> > Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> > Will fix this in the next version. Thanks for catching this!
> >
> I don't think changing to IIO_LE is good idea as when i2c_read_bock..
> then the scan type will be CPU. So better to fix endianness in this
> function.
the chip has little-endian data registers; i2c_read_block() just transfers
the data (no endianness conversion), so the data will still be
little-endian
p.
--
Peter Meerwald
+43-664-2444418 (mobile)
On Fri, 2015-02-20 at 16:23 +0100, Peter Meerwald wrote:
> > > > > +
> > > > > + values[i] = data & 0xFF;
> > > > > + values[i+1] = data >> 8;
> > > >
> > > > this is incorrect; it forces the data to be little endian, however, the
> > > > endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> > > > code breaks for big-endian CPUs
> > > >
> > > > since _read_i2c_block_data() can't do endianness conversion (and the chip
> > > > does i2c endianness, i.e. little-endian), the .scan_type should become
> > > > IIO_LE and above code is correct again but still ugly :)
> > > >
> > > > bottom line: change .scan_type to IIO_LE
> > > >
> > > Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> > > Will fix this in the next version. Thanks for catching this!
> > >
> > I don't think changing to IIO_LE is good idea as when i2c_read_bock..
> > then the scan type will be CPU. So better to fix endianness in this
> > function.
>
> the chip has little-endian data registers; i2c_read_block() just transfers
> the data (no endianness conversion), so the data will still be
> little-endian
You are right.
>
> p.
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 20/02/15 12:03, Tirdea, Irina wrote:
>
>
>> -----Original Message-----
>> From: Peter Meerwald [mailto:[email protected]]
>> Sent: 16 February, 2015 21:14
>> To: Tirdea, Irina
>> Cc: Jonathan Cameron; [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
>> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
>>
>>
>>> Adds optimization of i2c transactions in trigger handler and
>>> usage of available_scan_masks for kxcjk-1013.
>>
>> the 'optimization' changes the capabilities of the driver, reading
>> individual channel is not possible anymore
>>
> We can still read individual channels, just that the driver does not see that anymore and the demux is handled by the iio core.
> available_scan_masks contains the bitmap superset for all channels we can enable (I will remove the 0 available_scan_mask as it does not makes sense).
Careful. That's there as a null terminator for this list of available scan masks.
> When a new channel is enabled, the iio core checks if the new mask is a subset of any entry in available_scan_masks.
> When the buffer is pushed from the driver, the iio core will select only the enabled channel values.
Pretty much. IIRC This originally went in as part of handling multiple clients on these
data streams, but it was rapidly pointed out that lots of drivers spun their own
versions to break out particular elements from a multichannel read.
As such, using the generic form saved on repetition in these cases as well.
A good, complex example is drivers/iio/adc/max1363 in which really weird combinations
of channels (but far from all) are available for bulk capture and reading.
>
>>> Adriana Reus (2):
>>> iio: accel: kxcjk-1013: use available_scan_masks
>>> iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>>>
>>> drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
>>> 1 file changed, 39 insertions(+), 11 deletions(-)
>>>
>>>
>>
>> --
>>
>> Peter Meerwald
>> +43-664-2444418 (mobile)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 20/02/15 12:02, Tirdea, Irina wrote:
>
>
>> -----Original Message-----
>> From: Peter Meerwald [mailto:[email protected]]
>> Sent: 16 February, 2015 21:26
>> To: Tirdea, Irina
>> Cc: Jonathan Cameron; [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
>> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>>
>>
>>> Reading all axis values in one i2c transfer reduces the delays
>>> introduced by the i2c bus. In case i2c block read is not supported,
>>> fallback to reading each axis as a separate word.
>>
>> see comments inline below
>>
> Thanks for the review, Peter!
>
>>> Signed-off-by: Adriana Reus <[email protected]>
>>> Signed-off-by: Irina Tirdea <[email protected]>
>>> Reviewed-by: Srinivas Pandruvada <[email protected]>
>>> ---
>>> drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 35 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>>> index 5f27787..bfa2899 100644
>>> --- a/drivers/iio/accel/kxcjk-1013.c
>>> +++ b/drivers/iio/accel/kxcjk-1013.c
>>> @@ -109,6 +109,8 @@ struct kxcjk1013_data {
>>> int64_t timestamp;
>>> enum kx_chipset chipset;
>>> bool is_smo8500_device;
>>> + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
>>> + u8 length, u8 *values);
>>
>> probably this could/should be done in the i2c layer or we end up adding a
>> similar function in every driver?
>>
> Actually this is exactly what I did: adding this code in a couple of drivers :)
> You are right, this belongs to the i2c core. Will move it there.
A good idea. Might be possible to make this a transparent fallback so no
special handling is needed in drivers at all (e.g. emulate the block read using
the biggest read that is available) - taking care about the endian fun and
games that results.
Propose it to Wolfram and the i2c list and see what people think of it.
Jonathan
>
>>> };
>>>
>>> enum kxcjk1013_axis {
>>> @@ -216,6 +218,23 @@ static const struct {
>>> {800, 0, 0x06},
>>> {1600, 0, 0x06} };
>>>
>>> +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
>>> + u8 command, u8 length, u8 *values)
>>> +{
>>> + s32 data;
>>> + u8 i;
>>> +
>>> + for (i = 0; i < length; i += 2) {
>>> + data = i2c_smbus_read_word_data(client, command + i);
>>> + if (data < 0)
>>> + return data;
>>> +
>>> + values[i] = data & 0xFF;
>>> + values[i+1] = data >> 8;
>>
>> this is incorrect; it forces the data to be little endian, however, the
>> endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
>> code breaks for big-endian CPUs
>>
>> since _read_i2c_block_data() can't do endianness conversion (and the chip
>> does i2c endianness, i.e. little-endian), the .scan_type should become
>> IIO_LE and above code is correct again but still ugly :)
>>
>> bottom line: change .scan_type to IIO_LE
>>
> Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> Will fix this in the next version. Thanks for catching this!
>
>>> + }
>>> + return i;
>>> +}
>>> +
>>> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
>>> enum kxcjk1013_mode mode)
>>> {
>>> @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
>>> struct iio_poll_func *pf = p;
>>> struct iio_dev *indio_dev = pf->indio_dev;
>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> - int bit, ret, i = 0;
>>> + int ret;
>>>
>>> mutex_lock(&data->mutex);
>>> - for (bit = 0; bit < AXIS_MAX; bit++) {
>>> - ret = kxcjk1013_get_acc_reg(data, bit);
>>> - if (ret < 0) {
>>> - mutex_unlock(&data->mutex);
>>> - goto err;
>>> - }
>>> - data->buffer[i++] = ret;
>>> - }
>>> + ret = data->read_block_data(data->client, KXCJK1013_REG_XOUT_L,
>>> + AXIS_MAX * 2, (u8 *) data->buffer);
>>> mutex_unlock(&data->mutex);
>>> + if (ret < 0)
>>> + goto err;
>>>
>>> iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>> data->timestamp);
>>> @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>> const char *name;
>>> int ret;
>>>
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_BYTE_DATA |
>>> + I2C_FUNC_SMBUS_READ_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>> if (!indio_dev)
>>> return -ENOMEM;
>>> @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>> i2c_set_clientdata(client, indio_dev);
>>> data->client = client;
>>>
>>> + if (i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
>>> + data->read_block_data = i2c_smbus_read_i2c_block_data;
>>> + else
>>> + data->read_block_data = kxcjk1013_read_block_data;
>>> +
>>> pdata = dev_get_platdata(&client->dev);
>>> if (pdata)
>>> data->active_high_intr = pdata->active_high_intr;
>>>
>>
>> --
>>
>> Peter Meerwald
>> +43-664-2444418 (mobile)
> -----Original Message-----
> From: Jonathan Cameron [mailto:[email protected]]
> Sent: 21 February, 2015 20:27
> To: Tirdea, Irina; Peter Meerwald
> Cc: [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
>
> On 20/02/15 12:03, Tirdea, Irina wrote:
> >
> >
> >> -----Original Message-----
> >> From: Peter Meerwald [mailto:[email protected]]
> >> Sent: 16 February, 2015 21:14
> >> To: Tirdea, Irina
> >> Cc: Jonathan Cameron; [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
> >> Subject: Re: [PATCH 0/2] kxcjk-1013 driver optimizations
> >>
> >>
> >>> Adds optimization of i2c transactions in trigger handler and
> >>> usage of available_scan_masks for kxcjk-1013.
> >>
> >> the 'optimization' changes the capabilities of the driver, reading
> >> individual channel is not possible anymore
> >>
> > We can still read individual channels, just that the driver does not see that anymore and the demux is handled by the iio core.
> > available_scan_masks contains the bitmap superset for all channels we can enable (I will remove the 0 available_scan_mask as it
> does not makes sense).
> Careful. That's there as a null terminator for this list of available scan masks.
Missed this. You are right, thanks! Seems it's good as is.
> > When a new channel is enabled, the iio core checks if the new mask is a subset of any entry in available_scan_masks.
> > When the buffer is pushed from the driver, the iio core will select only the enabled channel values.
>
> Pretty much. IIRC This originally went in as part of handling multiple clients on these
> data streams, but it was rapidly pointed out that lots of drivers spun their own
> versions to break out particular elements from a multichannel read.
> As such, using the generic form saved on repetition in these cases as well.
> A good, complex example is drivers/iio/adc/max1363 in which really weird combinations
> of channels (but far from all) are available for bulk capture and reading.
> >
> >>> Adriana Reus (2):
> >>> iio: accel: kxcjk-1013: use available_scan_masks
> >>> iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >>>
> >>> drivers/iio/accel/kxcjk-1013.c | 50 ++++++++++++++++++++++++++++++++----------
> >>> 1 file changed, 39 insertions(+), 11 deletions(-)
> >>>
> >>>
> >>
> >> --
> >>
> >> Peter Meerwald
> >> +43-664-2444418 (mobile)
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> -----Original Message-----
> From: Jonathan Cameron [mailto:[email protected]]
> Sent: 21 February, 2015 20:30
> To: Tirdea, Irina; Peter Meerwald
> Cc: [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
>
> On 20/02/15 12:02, Tirdea, Irina wrote:
> >
> >
> >> -----Original Message-----
> >> From: Peter Meerwald [mailto:[email protected]]
> >> Sent: 16 February, 2015 21:26
> >> To: Tirdea, Irina
> >> Cc: Jonathan Cameron; [email protected]; [email protected]; Pandruvada, Srinivas; Reus, Adriana
> >> Subject: Re: [PATCH 2/2] iio: accel: kxcjk-1013: optimize i2c transfers in trigger handler
> >>
> >>
> >>> Reading all axis values in one i2c transfer reduces the delays
> >>> introduced by the i2c bus. In case i2c block read is not supported,
> >>> fallback to reading each axis as a separate word.
> >>
> >> see comments inline below
> >>
> > Thanks for the review, Peter!
> >
> >>> Signed-off-by: Adriana Reus <[email protected]>
> >>> Signed-off-by: Irina Tirdea <[email protected]>
> >>> Reviewed-by: Srinivas Pandruvada <[email protected]>
> >>> ---
> >>> drivers/iio/accel/kxcjk-1013.c | 44 +++++++++++++++++++++++++++++++++---------
> >>> 1 file changed, 35 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> >>> index 5f27787..bfa2899 100644
> >>> --- a/drivers/iio/accel/kxcjk-1013.c
> >>> +++ b/drivers/iio/accel/kxcjk-1013.c
> >>> @@ -109,6 +109,8 @@ struct kxcjk1013_data {
> >>> int64_t timestamp;
> >>> enum kx_chipset chipset;
> >>> bool is_smo8500_device;
> >>> + s32 (*read_block_data)(const struct i2c_client *client, u8 command,
> >>> + u8 length, u8 *values);
> >>
> >> probably this could/should be done in the i2c layer or we end up adding a
> >> similar function in every driver?
> >>
> > Actually this is exactly what I did: adding this code in a couple of drivers :)
> > You are right, this belongs to the i2c core. Will move it there.
> A good idea. Might be possible to make this a transparent fallback so no
> special handling is needed in drivers at all (e.g. emulate the block read using
> the biggest read that is available) - taking care about the endian fun and
> games that results.
>
> Propose it to Wolfram and the i2c list and see what people think of it.
>
I'll do the block emulation code first and send it on the i2c list (still checking some endianness issues).
Once I'll have a response for the i2c part, I'll resend these patchsets with the required fixes.
Thanks,
Irina
> Jonathan
> >
> >>> };
> >>>
> >>> enum kxcjk1013_axis {
> >>> @@ -216,6 +218,23 @@ static const struct {
> >>> {800, 0, 0x06},
> >>> {1600, 0, 0x06} };
> >>>
> >>> +static s32 kxcjk1013_read_block_data(const struct i2c_client *client,
> >>> + u8 command, u8 length, u8 *values)
> >>> +{
> >>> + s32 data;
> >>> + u8 i;
> >>> +
> >>> + for (i = 0; i < length; i += 2) {
> >>> + data = i2c_smbus_read_word_data(client, command + i);
> >>> + if (data < 0)
> >>> + return data;
> >>> +
> >>> + values[i] = data & 0xFF;
> >>> + values[i+1] = data >> 8;
> >>
> >> this is incorrect; it forces the data to be little endian, however, the
> >> endianness (as specified in the driver's .scan_type) is IIO_CPU -- the
> >> code breaks for big-endian CPUs
> >>
> >> since _read_i2c_block_data() can't do endianness conversion (and the chip
> >> does i2c endianness, i.e. little-endian), the .scan_type should become
> >> IIO_LE and above code is correct again but still ugly :)
> >>
> >> bottom line: change .scan_type to IIO_LE
> >>
> > Good point. Changing the endianess to IIO_LE is correct for either kxcjk1013_read_block_data or i2c_smbus_read_i2c_block_data.
> > Will fix this in the next version. Thanks for catching this!
> >
> >>> + }
> >>> + return i;
> >>> +}
> >>> +
> >>> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> >>> enum kxcjk1013_mode mode)
> >>> {
> >>> @@ -955,18 +974,14 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
> >>> struct iio_poll_func *pf = p;
> >>> struct iio_dev *indio_dev = pf->indio_dev;
> >>> struct kxcjk1013_data *data = iio_priv(indio_dev);
> >>> - int bit, ret, i = 0;
> >>> + int ret;
> >>>
> >>> mutex_lock(&data->mutex);
> >>> - for (bit = 0; bit < AXIS_MAX; bit++) {
> >>> - ret = kxcjk1013_get_acc_reg(data, bit);
> >>> - if (ret < 0) {
> >>> - mutex_unlock(&data->mutex);
> >>> - goto err;
> >>> - }
> >>> - data->buffer[i++] = ret;
> >>> - }
> >>> + ret = data->read_block_data(data->client, KXCJK1013_REG_XOUT_L,
> >>> + AXIS_MAX * 2, (u8 *) data->buffer);
> >>> mutex_unlock(&data->mutex);
> >>> + if (ret < 0)
> >>> + goto err;
> >>>
> >>> iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >>> data->timestamp);
> >>> @@ -1196,6 +1211,11 @@ static int kxcjk1013_probe(struct i2c_client *client,
> >>> const char *name;
> >>> int ret;
> >>>
> >>> + if (!i2c_check_functionality(client->adapter,
> >>> + I2C_FUNC_SMBUS_BYTE_DATA |
> >>> + I2C_FUNC_SMBUS_READ_WORD_DATA))
> >>> + return -ENODEV;
> >>> +
> >>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >>> if (!indio_dev)
> >>> return -ENOMEM;
> >>> @@ -1204,6 +1224,12 @@ static int kxcjk1013_probe(struct i2c_client *client,
> >>> i2c_set_clientdata(client, indio_dev);
> >>> data->client = client;
> >>>
> >>> + if (i2c_check_functionality(client->adapter,
> >>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> >>> + data->read_block_data = i2c_smbus_read_i2c_block_data;
> >>> + else
> >>> + data->read_block_data = kxcjk1013_read_block_data;
> >>> +
> >>> pdata = dev_get_platdata(&client->dev);
> >>> if (pdata)
> >>> data->active_high_intr = pdata->active_high_intr;
> >>>
> >>
> >> --
> >>
> >> Peter Meerwald
> >> +43-664-2444418 (mobile)