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