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. 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.
How does that sound to you?
Cheers,
Vasilis
