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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> > > > > > 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