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

[Jonathan Cameron]

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

Jonathan
> 
> Thanks,
> Irina
> 
>>>
>>>> Again, fingers crossed this doesn't break anything significant.
>>>>
>>>> Applied,
>>>>
>>>> Thanks,
>>>>
>>>> Jonathan
>>>>> ---
>>>>>  drivers/iio/gyro/bmg160_core.c | 17 ++---
>>>>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/gyro/bmg160_core.c 
>>>>> b/drivers/iio/gyro/bmg160_core.c
>>>>> index 8d6e5b1..43570b8 100644
>>>>> --- a/drivers/iio/gyro/bmg160_core.c
>>>>> +++ b/drivers/iio/gy

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

[Jonathan Cameron]

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

Jonathan
> 
> Thanks,
> Irina
> 
>>>
>>>> Again, fingers crossed this doesn't break anything significant.
>>>>
>>>> Applied,
>>>>
>>>> Thanks,
>>>>
>>>> Jonathan
>>>>> ---
>>>>>  drivers/iio/gyro/bmg160_core.c | 17 ++---
>>>>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/gyro/bmg160_core.c 
>>>>> b/drivers/iio/gyro/bmg160_core.c
>>>>> index 8d6e5b1..43570b8 100644
>>>>> --- a/drivers/iio/gyro/bmg160_core.c
>>>>> +++ b/drivers/iio/gyro/bmg160_

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

[Tirdea, Irina]

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

Thanks for the review, Peter!

> > > Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> > > enable/disable the bus at each i2c transfer and must wait for
> > > the enable/disable to happen before sending the data.
> > >
> > > When reading data in the trigger handler, the bmc150 accel driver does
> 
> should refer to bmg160
> 
> > > one bus transfer for each axis. This has an impact on the frequency
> > > of the accelerometer at high sample rates due to additional delays
> > > introduced by the bus at each transfer.
> > >
> > > Reading all axis values in one bus transfer reduces the delays
> > > introduced by the bus.
> > >
> > > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > I forgot to highlight on the earlier driver that there is also 'technically'
> > a bit of an ABI change here because we are now exporting as LE rather than 
> > CPU
> > order.  However, I 'hope' anyone actually accessing the buffered data is 
> > either
> > doing it through a nice library or hasn't hacked the endian unwinding out of
> > the generic_buffer example!
> 
> the patch takes away the possibility to do buffered reads on individual
> channels (not sure if this is useful per se)

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

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

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

Thanks,
Irina 

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

[Tirdea, Irina]

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

Thanks for the review, Peter!

> > > Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> > > enable/disable the bus at each i2c transfer and must wait for
> > > the enable/disable to happen before sending the data.
> > >
> > > When reading data in the trigger handler, the bmc150 accel driver does
> 
> should refer to bmg160
> 
> > > one bus transfer for each axis. This has an impact on the frequency
> > > of the accelerometer at high sample rates due to additional delays
> > > introduced by the bus at each transfer.
> > >
> > > Reading all axis values in one bus transfer reduces the delays
> > > introduced by the bus.
> > >
> > > Signed-off-by: Irina Tirdea 
> > I forgot to highlight on the earlier driver that there is also 'technically'
> > a bit of an ABI change here because we are now exporting as LE rather than 
> > CPU
> > order.  However, I 'hope' anyone actually accessing the buffered data is 
> > either
> > doing it through a nice library or hasn't hacked the endian unwinding out of
> > the generic_buffer example!
> 
> the patch takes away the possibility to do buffered reads on individual
> channels (not sure if this is useful per se)

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

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

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

Thanks,
Irina 

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

[Tirdea, Irina]

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

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

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

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

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

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

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

Thanks,
Irina

> >
> >> Again, fingers crossed this doesn't break anything significant.
> >>
> >> Applied,
> >>
> >> Thanks,
> >>
> >> Jonathan
> >>> ---
> >>>  drivers/iio/gyro/bmg160_core.c | 17 ++---
> >>>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/gyro/bmg160_core.c 
> >>> b/drivers/iio/gyro/bmg160_core.c
> >>> index 8d6e5b1..43570b8 100644
> >>> --- a/drivers/iio/gyro/bmg160_core.c
> >>> +++ b/drivers/iio/gyro/bmg160_core.c
> >>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
> >>>   .sign = 's',\
> >>>   .realbits = 16, \
> >>>   .storagebits = 16,  \
> >>> + .endianness = IIO_LE,   \
> >>>   },  \
> >>>   .event_spec = _event,\
> >>>   .num_event_specs = 1\
> >>&g

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

[Tirdea, Irina]

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

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

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

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

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

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

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

Thanks,
Irina

> >
> >> Again, fingers crossed this doesn't break anything significant.
> >>
> >> Applied,
> >>
> >> Thanks,
> >>
> >> Jonathan
> >>> ---
> >>>  drivers/iio/gyro/bmg160_core.c | 17 ++---
> >>>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/gyro/bmg160_core.c 
> >>> b/drivers/iio/gyro/bmg160_core.c
> >>> index 8d6e5b1..43570b8 100644
> >>> --- a/drivers/iio/gyro/bmg160_core.c
> >>> +++ b/drivers/iio/gyro/bmg160_core.c
> >>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
> >>>   .sign = 's',\
> >>>   .realbits = 16, \
> >>>   .storagebits = 16,  \
> >>> + .endianness = IIO_LE,   \
> >>>   },  \
> >>>   .event_spec = _event,\
> >>>   .num_event_specs = 1\
> >>> @@ -773,20 +774,14 @@ stat

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

[Jonathan Cameron]

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

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

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

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

[Jonathan Cameron]

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

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

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

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

[Peter Meerwald-Stadler]

> > Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> > enable/disable the bus at each i2c transfer and must wait for
> > the enable/disable to happen before sending the data.
> > 
> > When reading data in the trigger handler, the bmc150 accel driver does

should refer to bmg160

> > one bus transfer for each axis. This has an impact on the frequency
> > of the accelerometer at high sample rates due to additional delays
> > introduced by the bus at each transfer.
> > 
> > Reading all axis values in one bus transfer reduces the delays
> > introduced by the bus.
> > 
> > Signed-off-by: Irina Tirdea 
> I forgot to highlight on the earlier driver that there is also 'technically'
> a bit of an ABI change here because we are now exporting as LE rather than CPU
> order.  However, I 'hope' anyone actually accessing the buffered data is 
> either
> doing it through a nice library or hasn't hacked the endian unwinding out of
> the generic_buffer example!

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

this optimizes for the common case, ok;

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

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

-- 

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

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

[Peter Meerwald-Stadler]

> > Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
> > enable/disable the bus at each i2c transfer and must wait for
> > the enable/disable to happen before sending the data.
> > 
> > When reading data in the trigger handler, the bmc150 accel driver does

should refer to bmg160

> > one bus transfer for each axis. This has an impact on the frequency
> > of the accelerometer at high sample rates due to additional delays
> > introduced by the bus at each transfer.
> > 
> > Reading all axis values in one bus transfer reduces the delays
> > introduced by the bus.
> > 
> > Signed-off-by: Irina Tirdea 
> I forgot to highlight on the earlier driver that there is also 'technically'
> a bit of an ABI change here because we are now exporting as LE rather than CPU
> order.  However, I 'hope' anyone actually accessing the buffered data is 
> either
> doing it through a nice library or hasn't hacked the endian unwinding out of
> the generic_buffer example!

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

this optimizes for the common case, ok;

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

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

-- 

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

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

[Jonathan Cameron]

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

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

Applied,

Thanks,

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

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

[Jonathan Cameron]

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

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

Applied,

Thanks,

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