Re: Bugs in dps310 Linux driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 4 Mar 2023 17:06:20 +0000
 Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
On Fri, 03 Mar 2023 12:10:00 +0100
"Andres Heinloo" <andres@xxxxxxxxxxxxxx> wrote:

Hello,

I've been struggling with the dps310 driver, which gives incorrect pressure values and in particular different values than manufacturers code (https://github.com/Infineon/RaspberryPi_DPS).

I think I've found where the problem is. Firstly, there is a mistake in bit numbering at https://github.com/torvalds/linux/blob/857f1268a591147f7be7509f249dbb3aba6fc65c/drivers/iio/pressure/dps310.c#L51

According to datasheet, correct is:

#define  DPS310_INT_HL          BIT(7)
#define  DPS310_TMP_SHIFT_EN    BIT(3)
#define  DPS310_PRS_SHIFT_EN    BIT(2)
#define  DPS310_FIFO_EN         BIT(1)
#define  DPS310_SPI_EN          BIT(0)

Eg., the current code is using wrong bit (4) for DPS310_PRS_SHIFT_EN, which means that pressure shift is never enabled.

Checking the datasheet, seems like you are right.
https://www.infineon.com/dgdl/Infineon-DPS310-DataSheet-v01_02-EN.pdf?fileId=5546d462576f34750157750826c42242
Section 7: Though that's not the only bit that is wrong. Looks like FIFO enable is as well.
So any fix should deal with that as well.

Yes, DPS310_PRS_SHIFT_EN, DPS310_FIFO_EN, DPS310_SPI_EN are all wrong, but the latter 2 are not used by the driver.


The differences between the register map and the datasheet I'm looking at make me think that perhaps the driver was developed against a prototype part. The registers are in a different order for starters with the B0, B1 and B2
sets in reverse order.  Any fix patch should tidy that up as well.

Yes, but that's just different naming. MSB is called B2 in the datasheet and B0 in the driver.


Secondly, there is a problem with overflows starting at https://github.com/torvalds/linux/blob/857f1268a591147f7be7509f249dbb3aba6fc65c/drivers/iio/pressure/dps310.c#L654

Since p is a 24-bit value,

nums[3] = p * p * p * (s64)data->c30;

can and does overflow.

Makes sense, though I can't immediately see a good solution as we need
to maintain the remainder part.

I don't have a good solution either, but there must be other IIO sensors that have something similar that could be possibly reused.


Second overflow problem is at https://github.com/torvalds/linux/blob/857f1268a591147f7be7509f249dbb3aba6fc65c/drivers/iio/pressure/dps310.c#L684

In fact, I don't understand why 1000000000LL is needed. Since only 7 values are summed, using 10LL should give the same precision.
Whilst the existing value seems large - I'm not great with precision calcs so could
you lay out why 10LL is sufficient?

Unless I overlooked something, the error of integer division (eg., discarding fractional part) is <1. In this case, the results of 7 integer divisions are summed, so the error is <7. When multiplying numerators by 10LL, the error would be <0.7. Which is OK, since we are interested only in the integer part.

Andres



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux