* [PATCH] iio: pressure: bmp280: Fix BMP580 temperature reading
@ 2024-04-25 6:22 Adam Rizkalla
2024-04-28 13:40 ` Jonathan Cameron
2024-05-02 15:44 ` Angel Iglesias
0 siblings, 2 replies; 8+ messages in thread
From: Adam Rizkalla @ 2024-04-25 6:22 UTC (permalink / raw)
To: linux-iio
Cc: Jonathan Cameron, Lars-Peter Clausen, Angel Iglesias, Adam Rizkalla
Fix overflow issue when storing BMP580 temperature reading and
properly preserve sign of 24-bit data.
Signed-off-by: Adam Rizkalla <ajarizzo@gmail.com>
---
drivers/iio/pressure/bmp280-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index fe8734468ed3..e79c9715bb28 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1393,12 +1393,12 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2)
/*
* Temperature is returned in Celsius degrees in fractional
- * form down 2^16. We rescale by x1000 to return milli Celsius
- * to respect IIO ABI.
+ * form down 2^16. We rescale by x1000 to return millidegrees
+ * Celsius to respect IIO ABI.
*/
- *val = raw_temp * 1000;
- *val2 = 16;
- return IIO_VAL_FRACTIONAL_LOG2;
+ raw_temp = sign_extend32(raw_temp, 23);
+ *val = ((s64)raw_temp * 1000) / (1 << 16);
+ return IIO_VAL_INT;
}
static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
--
2.40.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: pressure: bmp280: Fix BMP580 temperature reading
2024-04-25 6:22 [PATCH] iio: pressure: bmp280: Fix BMP580 temperature reading Adam Rizkalla
@ 2024-04-28 13:40 ` Jonathan Cameron
2024-05-02 13:05 ` Replying to: " Vasileios Amoiridis
2024-05-02 15:44 ` Angel Iglesias
1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2024-04-28 13:40 UTC (permalink / raw)
To: Adam Rizkalla; +Cc: linux-iio, Lars-Peter Clausen, Angel Iglesias
On Thu, 25 Apr 2024 01:22:49 -0500
Adam Rizkalla <ajarizzo@gmail.com> wrote:
> Fix overflow issue when storing BMP580 temperature reading and
> properly preserve sign of 24-bit data.
>
> Signed-off-by: Adam Rizkalla <ajarizzo@gmail.com>
Hi Adam,
Looks like a correct fix to me, but leaving on list a little longer
for Angel to take a look.
Thanks,
Jonathan
> ---
> drivers/iio/pressure/bmp280-core.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index fe8734468ed3..e79c9715bb28 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -1393,12 +1393,12 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2)
>
> /*
> * Temperature is returned in Celsius degrees in fractional
> - * form down 2^16. We rescale by x1000 to return milli Celsius
> - * to respect IIO ABI.
> + * form down 2^16. We rescale by x1000 to return millidegrees
> + * Celsius to respect IIO ABI.
> */
> - *val = raw_temp * 1000;
> - *val2 = 16;
> - return IIO_VAL_FRACTIONAL_LOG2;
> + raw_temp = sign_extend32(raw_temp, 23);
> + *val = ((s64)raw_temp * 1000) / (1 << 16);
> + return IIO_VAL_INT;
> }
>
> static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Replying to: [PATCH] iio: pressure: bmp280: Fix BMP580 temperature reading
2024-04-28 13:40 ` Jonathan Cameron
@ 2024-05-02 13:05 ` Vasileios Amoiridis
0 siblings, 0 replies; 8+ messages in thread
From: Vasileios Amoiridis @ 2024-05-02 13:05 UTC (permalink / raw)
To: jic23; +Cc: ajarizzo, ang.iglesiasg, lars, linux-iio, Vasileios Amoiridis
Hi guys!
I am sending this e-mail as a reply to [1]
I know I am not included in the mailing list, but since I am working on that
driver currently [2] and I am also reading daily the iio mailing list I found
your patch quite interesting and I wanted to test it.
I managed to generate a negative temperature for the sensor and as it looks like
you are right. Before your patch, the return value was overflowing for negative
temperatures. So, good job!
I have one question though:
1) The comment that includes the "milli Celsius" is also similar in the
bmp380_read_temp() function so maybe you could change both. But since it is
a fixes patch, is that necessary to be done in this commit?
[1]: https://lore.kernel.org/linux-iio/Zin2udkXRD0+GrML@adam-asahi.lan/
[2]: https://lore.kernel.org/linux-iio/20240429190046.24252-1-vassilisamir@gmail.com/
P.S: I don't know how tags work, but if for the tested tag the testing that I
did is enough, then here is my tag:
Tested-By: Vasileios Amoiridis <vassilisamir@gmail.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: pressure: bmp280: Fix BMP580 temperature reading
2024-04-25 6:22 [PATCH] iio: pressure: bmp280: Fix BMP580 temperature reading Adam Rizkalla
2024-04-28 13:40 ` Jonathan Cameron
@ 2024-05-02 15:44 ` Angel Iglesias
2024-05-02 17:16 ` Replying to: [PATCH] iio " Vasileios Amoiridis
1 sibling, 1 reply; 8+ messages in thread
From: Angel Iglesias @ 2024-05-02 15:44 UTC (permalink / raw)
To: Adam Rizkalla, linux-iio; +Cc: Jonathan Cameron, Lars-Peter Clausen
On Thu, 2024-04-25 at 01:22 -0500, Adam Rizkalla wrote:
> Fix overflow issue when storing BMP580 temperature reading and
> properly preserve sign of 24-bit data.
>
> Signed-off-by: Adam Rizkalla <ajarizzo@gmail.com>
> ---
> drivers/iio/pressure/bmp280-core.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-
> core.c
> index fe8734468ed3..e79c9715bb28 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -1393,12 +1393,12 @@ static int bmp580_read_temp(struct bmp280_data *data,
> int *val, int *val2)
>
> /*
> * Temperature is returned in Celsius degrees in fractional
> - * form down 2^16. We rescale by x1000 to return milli Celsius
> - * to respect IIO ABI.
> + * form down 2^16. We rescale by x1000 to return millidegrees
> + * Celsius to respect IIO ABI.
> */
> - *val = raw_temp * 1000;
> - *val2 = 16;
> - return IIO_VAL_FRACTIONAL_LOG2;
> + raw_temp = sign_extend32(raw_temp, 23);
> + *val = ((s64)raw_temp * 1000) / (1 << 16);
> + return IIO_VAL_INT;
> }
>
> static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
Hi Adam,
Great catch! Reading back the device's datasheet, it also says the raw pressure
value it's a 24 bit signed integer, so we have the same problem on
bmp580_read_press. Could you make a v2 version of this series including the
pressure fix?
Kind regards,
Angel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Replying to: [PATCH] iio pressure: bmp280: Fix BMP580 temperature reading
2024-05-02 15:44 ` Angel Iglesias
@ 2024-05-02 17:16 ` Vasileios Amoiridis
2024-05-02 18:15 ` Adam Rizkalla
0 siblings, 1 reply; 8+ messages in thread
From: Vasileios Amoiridis @ 2024-05-02 17:16 UTC (permalink / raw)
To: ang.iglesiasg; +Cc: ajarizzo, jic23, lars, linux-iio
Hi Angel!
Indeed the datasheet says that the pressure is a signed value. But this comes
in contrast with 2 things:
1) The BMP58x sensor does not have a compensation function so the value from the
sensor is a pressure value just in different unit from the one reported by IIO.
And the sensor is able to report in between 30-125kPa according to the
datasheet which are both positive values so it makes more sense to be an
unsigned value.
2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is
declared as an unsigned value.
So, what should we trust?
[1]: https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Replying to: [PATCH] iio pressure: bmp280: Fix BMP580 temperature reading
2024-05-02 17:16 ` Replying to: [PATCH] iio " Vasileios Amoiridis
@ 2024-05-02 18:15 ` Adam Rizkalla
2024-05-03 7:13 ` Angel Iglesias
0 siblings, 1 reply; 8+ messages in thread
From: Adam Rizkalla @ 2024-05-02 18:15 UTC (permalink / raw)
To: Vasileios Amoiridis; +Cc: ang.iglesiasg, jic23, lars, linux-iio
On Thu, May 02, 2024 at 07:16:16PM +0200, Vasileios Amoiridis wrote:
> Hi Angel!
>
> Indeed the datasheet says that the pressure is a signed value. But this comes
> in contrast with 2 things:
>
> 1) The BMP58x sensor does not have a compensation function so the value from the
> sensor is a pressure value just in different unit from the one reported by IIO.
> And the sensor is able to report in between 30-125kPa according to the
> datasheet which are both positive values so it makes more sense to be an
> unsigned value.
>
> 2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is
> declared as an unsigned value.
>
> So, what should we trust?
>
> [1]: https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895
The pressure sensor reading cannot be negative, as the pressure range of the sensor
is 300 - 1250 hPa,so this change does not need to be applied for bmp580_read_press().
Also, the overflow issue does not happen with the pressure reading since the value
read back from the device is scaled up only by 2^6 for pressure vs 2^16 for temperature,
so multiplying by 1000 even for the maximum value would still fit in a 32-bit signed
integer. Temperature ranges above ~32.767C, however, will overflow a 32-bit signed
integer when multiplied by 2^16 * 1000 which is why this change is necessary only for
temperature readings.
Hope this helps clarify.
Best,
Adam
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Replying to: [PATCH] iio pressure: bmp280: Fix BMP580 temperature reading
2024-05-02 18:15 ` Adam Rizkalla
@ 2024-05-03 7:13 ` Angel Iglesias
2024-05-05 18:02 ` Jonathan Cameron
0 siblings, 1 reply; 8+ messages in thread
From: Angel Iglesias @ 2024-05-03 7:13 UTC (permalink / raw)
To: Adam Rizkalla, Vasileios Amoiridis; +Cc: jic23, lars, linux-iio
On Thu, 2024-05-02 at 13:15 -0500, Adam Rizkalla wrote:
> On Thu, May 02, 2024 at 07:16:16PM +0200, Vasileios Amoiridis wrote:
> > Hi Angel!
> >
> > Indeed the datasheet says that the pressure is a signed value. But this
> > comes
> > in contrast with 2 things:
> >
> > 1) The BMP58x sensor does not have a compensation function so the value from
> > the
> > sensor is a pressure value just in different unit from the one reported by
> > IIO.
> > And the sensor is able to report in between 30-125kPa according to the
> > datasheet which are both positive values so it makes more sense to be an
> > unsigned value.
> >
> > 2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is
> > declared as an unsigned value.
> >
> > So, what should we trust?
> >
> > [1]:
> > https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895
>
> The pressure sensor reading cannot be negative, as the pressure range of the
> sensor
> is 300 - 1250 hPa,so this change does not need to be applied for
> bmp580_read_press().
> Also, the overflow issue does not happen with the pressure reading since the
> value
> read back from the device is scaled up only by 2^6 for pressure vs 2^16 for
> temperature,
> so multiplying by 1000 even for the maximum value would still fit in a 32-bit
> signed
> integer. Temperature ranges above ~32.767C, however, will overflow a 32-bit
> signed
> integer when multiplied by 2^16 * 1000 which is why this change is necessary
> only for
> temperature readings.
>
> Hope this helps clarify.
>
> Best,
> Adam
Crystal clear. Thanks to both of you for the clarifications :)
Acked-by: Angel Iglesias <ang.iglesiasg@gmail.com>
Kind regards,
Angel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Replying to: [PATCH] iio pressure: bmp280: Fix BMP580 temperature reading
2024-05-03 7:13 ` Angel Iglesias
@ 2024-05-05 18:02 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2024-05-05 18:02 UTC (permalink / raw)
To: Angel Iglesias; +Cc: Adam Rizkalla, Vasileios Amoiridis, lars, linux-iio
On Fri, 03 May 2024 09:13:33 +0200
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> On Thu, 2024-05-02 at 13:15 -0500, Adam Rizkalla wrote:
> > On Thu, May 02, 2024 at 07:16:16PM +0200, Vasileios Amoiridis wrote:
> > > Hi Angel!
> > >
> > > Indeed the datasheet says that the pressure is a signed value. But this
> > > comes
> > > in contrast with 2 things:
> > >
> > > 1) The BMP58x sensor does not have a compensation function so the value from
> > > the
> > > sensor is a pressure value just in different unit from the one reported by
> > > IIO.
> > > And the sensor is able to report in between 30-125kPa according to the
> > > datasheet which are both positive values so it makes more sense to be an
> > > unsigned value.
> > >
> > > 2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is
> > > declared as an unsigned value.
> > >
> > > So, what should we trust?
> > >
> > > [1]:
> > > https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895
> >
> > The pressure sensor reading cannot be negative, as the pressure range of the
> > sensor
> > is 300 - 1250 hPa,so this change does not need to be applied for
> > bmp580_read_press().
> > Also, the overflow issue does not happen with the pressure reading since the
> > value
> > read back from the device is scaled up only by 2^6 for pressure vs 2^16 for
> > temperature,
> > so multiplying by 1000 even for the maximum value would still fit in a 32-bit
> > signed
> > integer. Temperature ranges above ~32.767C, however, will overflow a 32-bit
> > signed
> > integer when multiplied by 2^16 * 1000 which is why this change is necessary
> > only for
> > temperature readings.
> >
> > Hope this helps clarify.
> >
> > Best,
> > Adam
>
> Crystal clear. Thanks to both of you for the clarifications :)
>
> Acked-by: Angel Iglesias <ang.iglesiasg@gmail.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.
Thanks,
Jonathan
>
> Kind regards,
> Angel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-05-05 18:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-25 6:22 [PATCH] iio: pressure: bmp280: Fix BMP580 temperature reading Adam Rizkalla
2024-04-28 13:40 ` Jonathan Cameron
2024-05-02 13:05 ` Replying to: " Vasileios Amoiridis
2024-05-02 15:44 ` Angel Iglesias
2024-05-02 17:16 ` Replying to: [PATCH] iio " Vasileios Amoiridis
2024-05-02 18:15 ` Adam Rizkalla
2024-05-03 7:13 ` Angel Iglesias
2024-05-05 18:02 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.