Sorry for the delay (vacations). Answers inline...
On 10/01/2016 04:58 PM, Jonathan Cameron wrote:
On 01/10/16 15:18, Lorenzo Bianconi wrote:
On 28/09/16 21:06, Lorenzo Bianconi wrote:
Using realbits as i2c/spi read len, when that value is not byte aligned
(e.g 12 bits), lead to skip msb part of out data registers.
Fix this using storagebits as read length
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@xxxxxx>
Hi Lorenzo,
Hi Jonathan,
This would ideally have had a fixes tag.
I think it was
Fixes: e7385de5 ("iio:st_sensors: align on storagebits boundaries")
Doing that would also have lead to you to the logic that lead to
this buggy change in the first place. Would also have shown
you that there is another place that probably suffers from the
same sort of issue.
Right. I missed the same issue in st_sensors_read_axis_data()
Gregor can you take a look at this please.
If I recall the issue that lead to the original patch it was
that we were miss handling 24 bit values on pressure sensors and
this was intended to pad them?
I think the 'right' fix will be something along the lines of:
unsigned int bytes_to_read = (channel->scan_type.realbits + 7) >> 3;
This should not be correct if you consider 8bit accel like LIS331DL.
8 + 7 = 15
15 >> 3 = 1 byte.
Would get interest if the data was shifted to run across two bytes, but
it's not. Could handle that case as well by doing
(channel->scan_type.realbits + channel->scan_type.shift + 7) >> 3
Agreed.Maybe using DIV_ROUND_UP() would make ita bit clearer ? Something
like :
DIV_ROUND_UP(channel->scan_type.realbits + channel->scan_type.shift, 8)
Should alsobe applied to st_sensors_get_buffer_element() and
st_sensors_read_axis_data() functions as told earlier.
Taking a look to lps22hb code I think storagebits should be 24 and not
32 for pressure channel. What do you think?
No. Storage bytes must always be a power of 2. It's assumed at various
points in the buffer handling code in the core and userspace code.
This is why Gregor's fix was needed.
That was it.
I'll be happy to perform some testing with my setup (LPS22HB).
Regards,
Grégor.
These 24 bit packed readings are a pain, but it was much worse to try
and remove the aligned data assumptions that need them to be powers
of 2. Just think about 3 bytes floating in a little endian integer
field vs the big endian version. It's a real pain to unwind.
Should give 2 bytes for a 12 bit and still the 3 bytes needed for a
24 bit read.
Thanks,
Thanks,
Lorenzo
Jonathan
---
drivers/iio/common/st_sensors/st_sensors_buffer.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index fe7775b..d01aa34 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -30,16 +30,15 @@ static int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
for_each_set_bit(i, indio_dev->active_scan_mask, num_data_channels) {q
const struct iio_chan_spec *channel = &indio_dev->channels[i];
- unsigned int bytes_to_read = channel->scan_type.realbits >> 3;
unsigned int storage_bytes =
channel->scan_type.storagebits >> 3;
buf = PTR_ALIGN(buf, storage_bytes);
if (sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
channel->address,
- bytes_to_read, buf,
+ storage_bytes, buf,
sdata->multiread_bit) <
- bytes_to_read)
+ storage_bytes)
return -EIO;
/* Advance the buffer pointer */
--
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
