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

[Jonathan Cameron <jic23@kernel.org>]

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.

> 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

> 
> How does that sound to you?
> 
> Cheers,
> Vasilis