> 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).
>
I will perform some tests on multiple devices later today and I will
let you know
Regards,
Lorenzo
> 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 */
>>>>>
>>>
>>>
>
--
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
--
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
