Re: Bugs in dps310 Linux driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 06 Mar 2023 22:15:15 +0100
"Andres Heinloo" <andres@gfz-potsdam.de> wrote:

> On Sun, 05 Mar 2023 03:05:01 +0100
>   "Andres Heinloo" <andres@gfz-potsdam.de> wrote:
> > On Sat, 4 Mar 2023 17:06:20 +0000
> >  Jonathan Cameron <jic23@kernel.org> wrote:  
> >> On Fri, 03 Mar 2023 12:10:00 +0100
> >> "Andres Heinloo" <andres@gfz-potsdam.de> 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.  
> 
> Unfortunately it seems that there may be more problems with the 
> driver. I've noticed that my device has lost sample rate and precision 
> settings few times. When looking at the code, it seems the settings 
> are not restored when dps310_reset_reinit() is called at 
> https://github.com/torvalds/linux/blob/8ca09d5fa3549d142c2080a72a4c70ce389163cd/drivers/iio/pressure/dps310.c#L438
> 

Agreed. Looks like that stuff is missing.  This reset path is pretty horrible
in general, so I'm not surprised a few things got missed. Should be easy enough
to add a cache of those and and set them again if you want to try that.


> Apparently I've also ended up with data->timeout_recovery_failed == 
> true at 
> https://github.com/torvalds/linux/blob/8ca09d5fa3549d142c2080a72a4c70ce389163cd/drivers/iio/pressure/dps310.c#L443, 
> but the device worked fine after just reloading the kernel module. 
> This is difficult to reproduce, though.

Hmm. There are a few things that could cause that.  Maybe something running slow, or
an intermittent write failure (noisy environment or bad wire / track maybe?)


> Andres