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

[Gregor Boirie <gregor.boirie@xxxxxxxxxx>]

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@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