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/gyro/bmg160_core.c
>>>>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
>>>>> .sign = 's', \
>>>>> .realbits = 16, \
>>>>> .storagebits = 16, \
>>>>> + .endianness = IIO_LE, \
>>>>> }, \
>>>>> .event_spec = &bmg160_event, \
>>>>> .num_event_specs = 1 \
>>>>> @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq,
>>>>> void *p)
>>>>> struct iio_poll_func *pf = p;
>>>>> struct iio_dev *indio_dev = pf->indio_dev;
>>>>> struct bmg160_data *data = iio_priv(indio_dev);
>>>>> - int bit, ret, i = 0;
>>>>> - unsigned int val;
>>>>> + int ret;
>>>>>
>>>>> mutex_lock(&data->mutex);
>>>>> - for (bit = 0; bit < AXIS_MAX; bit++) {
>>>>> - ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
>>>>> - &val, 2);
>>>>> - if (ret < 0) {
>>>>> - mutex_unlock(&data->mutex);
>>>>> - goto err;
>>>>> - }
>>>>> - data->buffer[i++] = ret;
>>>>> - }
>>>>> + ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
>>>>> + data->buffer, AXIS_MAX * 2);
>>>>> mutex_unlock(&data->mutex);
>>>>> + if (ret < 0)
>>>>> + goto err;
>>>>>
>>>>> iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>> pf->timestamp);
>>>>>
>>>>
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
