Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions

[Vasileios Amoiridis <vassilisamir@gmail.com>]

On Tue, Jun 25, 2024 at 09:44:39PM +0100, Jonathan Cameron wrote:
> On Sun, 23 Jun 2024 19:18:41 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > On Sun, Jun 23, 2024 at 05:23:30PM +0100, Jonathan Cameron wrote:
> > > On Sat, 22 Jun 2024 14:19:18 +0200
> > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > >   
> > > > On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:  
> > > > > On Tue, 18 Jun 2024 01:05:38 +0200
> > > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > > > >     
> > > > > > Add the coefficients for the IIO standard units and the IIO value
> > > > > > inside the chip_info structure.
> > > > > > 
> > > > > > Move the calculations for the IIO unit compatibility from inside the
> > > > > > read_{temp,press,humid}() functions and move them to the general
> > > > > > read_raw() function.
> > > > > > 
> > > > > > In this way, all the data for the calculation of the value are
> > > > > > located in the chip_info structure of the respective sensor.
> > > > > > 
> > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>    
> > > > > Does this incorporate the fix?  I'm a little confused looking at
> > > > > what is visible here, so I'd like Adam to take a look.
> > > > > 
> > > > > Btw, you missed cc'ing Adam.
> > > > >     
> > > > 
> > > > Ah, I only used the output of get_maintainer...  
> > > 
> > > always be careful to sanity check that :)
> > >   
> > > > ...
> > > >     
> > > > > > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
> > > > > >  	case IIO_CHAN_INFO_PROCESSED:
> > > > > >  		switch (chan->type) {
> > > > > >  		case IIO_HUMIDITYRELATIVE:
> > > > > > -			return data->chip_info->read_humid(data, val, val2);
> > > > > > +			ret = data->chip_info->read_humid(data, &chan_value);
> > > > > > +			if (ret)
> > > > > > +				return ret;
> > > > > > +
> > > > > > +			*val = data->chip_info->humid_coeffs[0] * chan_value;
> > > > > > +			*val2 = data->chip_info->humid_coeffs[1];
> > > > > > +			return data->chip_info->humid_coeffs_type;
> > > > > >  		case IIO_PRESSURE:
> > > > > > -			return data->chip_info->read_press(data, val, val2);
> > > > > > +			ret = data->chip_info->read_press(data, &chan_value);
> > > > > > +			if (ret)
> > > > > > +				return ret;
> > > > > > +
> > > > > > +			*val = data->chip_info->press_coeffs[0] * chan_value;
> > > > > > +			*val2 = data->chip_info->press_coeffs[1];
> > > > > > +			return data->chip_info->press_coeffs_type;
> > > > > >  		case IIO_TEMP:
> > > > > > -			return data->chip_info->read_temp(data, val, val2);
> > > > > > +			ret = data->chip_info->read_temp(data, &chan_value);
> > > > > > +			if (ret)
> > > > > > +				return ret;
> > > > > > +
> > > > > > +			*val = data->chip_info->temp_coeffs[0] * (s64)chan_value;    
> > > > 
> > > > This is the first difference with the previous version where I incorporated
> > > > the typecasting to (s64).  
> > > 
> > > On a 32 bit platform that will then get pushed into a 32 bit int and overflow
> > > I think.  Back when IIO got started everything was 32 bit so it didn't make sense
> > > to make these 64 bit or indeed to worry about forcing the size.
> > > 
> > > Jonathan
> > >   
> > 
> > Well, I use a 32-bit platform, (BeagleBone Black) and the negative values are
> > handled gracefully with this code. Also, how is that different from the previous
> > code? Instead of doing the typecasting to (s64) in the bmp580_read_temp()
> > function, we do it here. I feel that it is the same piece of code, just in
> > different places.
> 
> The problem, I think, is the storage between those two places is too small.
> It's not about negatives, but rather use that in the extremely large value
> case it might not fit in the 32 bit int behind val so casting that back up
> to 64 bits later won't recover the lost most significant bits.
> 

Well, that is tricky, I didn't think about it. Thanks for noticing.

> > What I can do though, for your peace of mind is the following:
> > 
> > Since the problem with overflow comes when we multiply by 1000 in order to have
> > milli-Celsius, what I can do, is to "force" the division with 2^16 to happen
> > first. In this way, they divided value cannot be overflowed. The division will
> > happen inside the bmp580_read_temp() function and let the multiplication happen
> > later by the IIO code. Then we can drop the (s64) from here.
> 
> See Adam's reply - that should avoid overflow and preserve accuracy which you
> tend to loose with divide then multiply.
> 
> Jonathan
> 

I saw it, and I will apply it, thanks to both of you!

Cheers,
Vasilis