Re: [RFC PATCH v1 4/9] iio:st_sensors: align on storagebits boundaries

[Gregor Boirie <gregor.boirie@parrot.com>]

On 05/01/2016 09:27 PM, Jonathan Cameron wrote:
> 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.
I'll give it a try, maybe next week as I'm quite busy right now.
>
> 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@parrot.com>
>>> ---
>>>   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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>