On 24/04/16 10:35, Jonathan Cameron wrote:
> On 19/04/16 10:18, Gregor Boirie wrote:
>> Triggered buffering memory accesses are not aligned on per channel
>> storagebits boundaries. Fix this by reading each channel individually to
>> ensure proper alignment.
>> Note that this patch drops the earlier optimisation that packs multiple
>> channels reading into a single data block transaction when their
>> respective I2C register addresses are contiguous.
> I wonder how much effect this change has on the read out rate and whether
> repacking in software would be significantly quicker than the multiple
> transfers...
Thinking more on this I'm really not happy with this solution - may well
kill data rates on some chips, to deal with this one unaligned case.
Gregor, can you give repacking in software a go?
Won't be that hard I think... Be cynical and just memove the data a byte if
you hit a 24bit channel.
We could do this in the core demux but then we'd have to do something a bit
clever with the reported buffer element type so it looked different to the driver
and the buffer.
Lars, IIRC you've messed with that corner of the code more recently than me.
Do you think doing unaligned data smashing in the demux would be sensible?
Can see we'd have to:
* expand the 'does the demux have to do anything' case to catch
this one (not too hard).
* make the demux table builder force size constraints to power of two.
* either work out the shift to export to userspace (nasty) or make sure we packed
the data right depending on endianness so there wasn't any change to the shift.
All this stuff occurs in the slow patch (table build) - it would look no worse
than normal demux (some memcpys) in the fast path.
Good idea or not? What do people think.
I'm might mock this up in iio_dummy if I get bored and find out how bad it really is...
Jonathan
>
>>
>> Signed-off-by: Gregor Boirie <gregor.boirie@xxxxxxxxxx>
>> ---
>> drivers/iio/common/st_sensors/st_sensors_buffer.c | 38 ++++++++++-------------
>> drivers/iio/common/st_sensors/st_sensors_core.c | 2 +-
>> 2 files changed, 18 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> index c558985..a7f1ced 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
>> @@ -21,33 +21,29 @@
>>
>> #include <linux/iio/common/st_sensors.h>
>>
>> -
>> int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
>> {
>> - int i, len;
>> - int total = 0;
>> + unsigned int cid;
>> struct st_sensor_data *sdata = iio_priv(indio_dev);
>> - unsigned int num_data_channels = sdata->num_data_channels;
>> -
>> - for (i = 0; i < num_data_channels; i++) {
>> - unsigned int bytes_to_read;
>> -
>> - if (test_bit(i, indio_dev->active_scan_mask)) {
>> - bytes_to_read = indio_dev->channels[i].scan_type.storagebits >> 3;
>> - len = sdata->tf->read_multiple_byte(&sdata->tb,
>> - sdata->dev, indio_dev->channels[i].address,
>> - bytes_to_read,
>> - buf + total, sdata->multiread_bit);
>> -
>> - if (len < bytes_to_read)
>> - return -EIO;
>>
>> - /* Advance the buffer pointer */
>> - total += len;
>> - }
>> + for_each_set_bit(cid, indio_dev->active_scan_mask,
>> + sdata->num_data_channels) {
>> + const struct iio_chan_spec *chan = &indio_dev->channels[cid];
>> + int sb = chan->scan_type.storagebits / 8;
>> + int rb = chan->scan_type.realbits / 8;
>> + int err;
>> +
>> + buf = PTR_ALIGN(buf, sb);
>> + err = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
>> + chan->address, rb, buf,
>> + sdata->multiread_bit);
>> + if (err != rb)
>> + return (err < 0) ? err : -EIO;
>> +
>> + buf += sb;
>> }
>>
>> - return total;
>> + return 0;
>> }
>> EXPORT_SYMBOL(st_sensors_get_buffer_element);
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
>> index dffe006..6901c7f 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
>> @@ -463,7 +463,7 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
>> int err;
>> u8 *outdata;
>> struct st_sensor_data *sdata = iio_priv(indio_dev);
>> - unsigned int byte_for_channel = ch->scan_type.storagebits >> 3;
>> + unsigned int byte_for_channel = ch->scan_type.realbits >> 3;
>>
>> outdata = kmalloc(byte_for_channel, GFP_KERNEL);
>> if (!outdata)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
