* [PATCH v8 0/3] Driver cleanup and triggered buffer support
@ 2024-06-17 23:05 Vasileios Amoiridis
2024-06-17 23:05 ` [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions Vasileios Amoiridis
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Vasileios Amoiridis @ 2024-06-17 23:05 UTC (permalink / raw)
To: jic23, lars
Cc: andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan,
phil, 579lpy, linus.walleij, semen.protsenko, linux-iio,
linux-kernel, Vasileios Amoiridis
Based on current testing branch.
Changes in v8:
Patch 1/3:
- Changes in bmp580_read_temp() in order to incorporate the fix that was
introduced in [1].
Patch 3/3:
- Splitted bmp280_trigger_handler to bm(p/e)_trigger_handler as it was
mentioned in v7 comment.
- Added the sign_extend32() from the fix in [1] to the
bmp580_buffer_handler() as well.
These commits were dropped since there was a conflict with a fix for that
driver. Link to conversation [2].
Version 7: https://lore.kernel.org/linux-iio/20240512230524.53990-1-vassilisamir@gmail.com/
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0f0f6306617cb4b6231fc9d4ec68ab9a56dba7c0
[2]: https://lore.kernel.org/linux-iio/20240602201200.30418-1-ajarizzo@gmail.com/
Vasileios Amoiridis (3):
iio: pressure: bmp280: Generalize read_*() functions
iio: pressure: bmp280: Add SCALE, RAW values in channels and
refactorize them
iio: pressure: bmp280: Add triggered buffer support
drivers/iio/pressure/Kconfig | 2 +
drivers/iio/pressure/bmp280-core.c | 627 ++++++++++++++++++++++++-----
drivers/iio/pressure/bmp280-spi.c | 8 +-
drivers/iio/pressure/bmp280.h | 34 +-
4 files changed, 568 insertions(+), 103 deletions(-)
base-commit: 7db8a847f98caae68c70bdab9ba92d1af38e5656
--
2.25.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-17 23:05 [PATCH v8 0/3] Driver cleanup and triggered buffer support Vasileios Amoiridis @ 2024-06-17 23:05 ` Vasileios Amoiridis 2024-06-22 9:28 ` Jonathan Cameron 2024-06-17 23:05 ` [PATCH v8 2/3] iio: pressure: bmp280: Add SCALE, RAW values in channels and refactorize them Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support Vasileios Amoiridis 2 siblings, 1 reply; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-17 23:05 UTC (permalink / raw) To: jic23, lars Cc: andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Vasileios Amoiridis 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> --- drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------ drivers/iio/pressure/bmp280.h | 13 ++- 2 files changed, 107 insertions(+), 73 deletions(-) diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 50d71ad83f37..27c00af060fa 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -445,10 +445,8 @@ static u32 bmp280_compensate_press(struct bmp280_data *data, return (u32)p; } -static int bmp280_read_temp(struct bmp280_data *data, - int *val, int *val2) +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) { - s32 comp_temp; u32 adc_temp; int ret; @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data, if (ret) return ret; - comp_temp = bmp280_compensate_temp(data, adc_temp); + *comp_temp = bmp280_compensate_temp(data, adc_temp); - *val = comp_temp * 10; - return IIO_VAL_INT; + return 0; } -static int bmp280_read_press(struct bmp280_data *data, - int *val, int *val2) +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) { - u32 comp_press, adc_press, t_fine; + u32 adc_press; + s32 t_fine; int ret; ret = bmp280_get_t_fine(data, &t_fine); @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data, if (ret) return ret; - comp_press = bmp280_compensate_press(data, adc_press, t_fine); - - *val = comp_press; - *val2 = 256000; + *comp_press = bmp280_compensate_press(data, adc_press, t_fine); - return IIO_VAL_FRACTIONAL; + return 0; } -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity) { - u32 comp_humidity; u16 adc_humidity; s32 t_fine; int ret; @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) if (ret) return ret; - comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); - - *val = comp_humidity * 1000 / 1024; + *comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); - return IIO_VAL_INT; + return 0; } static int bmp280_read_raw_impl(struct iio_dev *indio_dev, @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev, int *val, int *val2, long mask) { struct bmp280_data *data = iio_priv(indio_dev); + int chan_value; + int ret; guard(mutex)(&data->lock); @@ -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; + *val2 = data->chip_info->temp_coeffs[1]; + return data->chip_info->temp_coeffs_type; default: return -EINVAL; } @@ -822,6 +833,8 @@ static int bmp280_chip_config(struct bmp280_data *data) static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID }; +static const int bmp280_temp_coeffs[] = { 10, 1 }; +static const int bmp280_press_coeffs[] = { 1, 256000 }; const struct bmp280_chip_info bmp280_chip_info = { .id_reg = BMP280_REG_ID, @@ -850,6 +863,11 @@ const struct bmp280_chip_info bmp280_chip_info = { .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail), .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1, + .temp_coeffs = bmp280_temp_coeffs, + .temp_coeffs_type = IIO_VAL_FRACTIONAL, + .press_coeffs = bmp280_press_coeffs, + .press_coeffs_type = IIO_VAL_FRACTIONAL, + .chip_config = bmp280_chip_config, .read_temp = bmp280_read_temp, .read_press = bmp280_read_press, @@ -877,6 +895,7 @@ static int bme280_chip_config(struct bmp280_data *data) } static const u8 bme280_chip_ids[] = { BME280_CHIP_ID }; +static const int bme280_humid_coeffs[] = { 1000, 1024 }; const struct bmp280_chip_info bme280_chip_info = { .id_reg = BMP280_REG_ID, @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = { .num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail), .oversampling_humid_default = BME280_OSRS_HUMIDITY_16X - 1, + .temp_coeffs = bmp280_temp_coeffs, + .temp_coeffs_type = IIO_VAL_FRACTIONAL, + .press_coeffs = bmp280_press_coeffs, + .press_coeffs_type = IIO_VAL_FRACTIONAL, + .humid_coeffs = bme280_humid_coeffs, + .humid_coeffs_type = IIO_VAL_FRACTIONAL, + .chip_config = bme280_chip_config, .read_temp = bmp280_read_temp, .read_press = bmp280_read_press, @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, return comp_press; } -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp) { - s32 comp_temp; u32 adc_temp; int ret; @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) if (ret) return ret; - comp_temp = bmp380_compensate_temp(data, adc_temp); + *comp_temp = bmp380_compensate_temp(data, adc_temp); - *val = comp_temp * 10; - return IIO_VAL_INT; + return 0; } -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press) { - u32 adc_press, comp_press, t_fine; + u32 adc_press, t_fine; int ret; ret = bmp380_get_t_fine(data, &t_fine); @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) if (ret) return ret; - comp_press = bmp380_compensate_press(data, adc_press, t_fine); - - *val = comp_press; - *val2 = 100000; + *comp_press = bmp380_compensate_press(data, adc_press, t_fine); - return IIO_VAL_FRACTIONAL; + return 0; } static int bmp380_read_calib(struct bmp280_data *data) @@ -1296,6 +1317,8 @@ static int bmp380_chip_config(struct bmp280_data *data) static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128}; static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID }; +static const int bmp380_temp_coeffs[] = { 10, 1 }; +static const int bmp380_press_coeffs[] = { 1, 100000 }; const struct bmp280_chip_info bmp380_chip_info = { .id_reg = BMP380_REG_ID, @@ -1323,6 +1346,11 @@ const struct bmp280_chip_info bmp380_chip_info = { .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), .iir_filter_coeff_default = 2, + .temp_coeffs = bmp380_temp_coeffs, + .temp_coeffs_type = IIO_VAL_FRACTIONAL, + .press_coeffs = bmp380_press_coeffs, + .press_coeffs_type = IIO_VAL_FRACTIONAL, + .chip_config = bmp380_chip_config, .read_temp = bmp380_read_temp, .read_press = bmp380_read_press, @@ -1443,9 +1471,9 @@ static int bmp580_nvm_operation(struct bmp280_data *data, bool is_write) * for what is expected on IIO ABI. */ -static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) +static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp) { - s32 raw_temp; + s32 value_temp; int ret; ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, @@ -1455,25 +1483,19 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) return ret; } - raw_temp = get_unaligned_le24(data->buf); - if (raw_temp == BMP580_TEMP_SKIPPED) { + value_temp = get_unaligned_le24(data->buf); + if (value_temp == BMP580_TEMP_SKIPPED) { dev_err(data->dev, "reading temperature skipped\n"); return -EIO; } + *raw_temp = sign_extend32(value_temp, 23); - /* - * Temperature is returned in Celsius degrees in fractional - * form down 2^16. We rescale by x1000 to return millidegrees - * Celsius to respect IIO ABI. - */ - raw_temp = sign_extend32(raw_temp, 23); - *val = ((s64)raw_temp * 1000) / (1 << 16); - return IIO_VAL_INT; + return 0; } -static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) +static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press) { - u32 raw_press; + u32 value_press; int ret; ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, @@ -1483,18 +1505,14 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) return ret; } - raw_press = get_unaligned_le24(data->buf); - if (raw_press == BMP580_PRESS_SKIPPED) { + value_press = get_unaligned_le24(data->buf); + if (value_press == BMP580_PRESS_SKIPPED) { dev_err(data->dev, "reading pressure skipped\n"); return -EIO; } - /* - * Pressure is returned in Pascals in fractional form down 2^16. - * We rescale /1000 to convert to kilopascal to respect IIO ABI. - */ - *val = raw_press; - *val2 = 64000; /* 2^6 * 1000 */ - return IIO_VAL_FRACTIONAL; + *raw_press = value_press; + + return 0; } static const int bmp580_odr_table[][2] = { @@ -1830,6 +1848,8 @@ static int bmp580_chip_config(struct bmp280_data *data) static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; +static const int bmp580_temp_coeffs[] = { 1000, 16 }; +static const int bmp580_press_coeffs[] = { 1, 64000}; const struct bmp280_chip_info bmp580_chip_info = { .id_reg = BMP580_REG_CHIP_ID, @@ -1856,6 +1876,11 @@ const struct bmp280_chip_info bmp580_chip_info = { .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), .iir_filter_coeff_default = 2, + .temp_coeffs = bmp580_temp_coeffs, + .temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2, + .press_coeffs = bmp580_press_coeffs, + .press_coeffs_type = IIO_VAL_FRACTIONAL, + .chip_config = bmp580_chip_config, .read_temp = bmp580_read_temp, .read_press = bmp580_read_press, @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp) return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16; } -static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) +static int bmp180_read_temp(struct bmp280_data *data, s32 *comp_temp) { - s32 comp_temp; u32 adc_temp; int ret; @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) if (ret) return ret; - comp_temp = bmp180_compensate_temp(data, adc_temp); + *comp_temp = bmp180_compensate_temp(data, adc_temp); - *val = comp_temp * 100; - return IIO_VAL_INT; + return 0; } static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press, return p + ((x1 + x2 + 3791) >> 4); } -static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) +static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press) { - u32 comp_press, adc_press; + u32 adc_press; s32 t_fine; int ret; @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) if (ret) return ret; - comp_press = bmp180_compensate_press(data, adc_press, t_fine); - - *val = comp_press; - *val2 = 1000; + *comp_press = bmp180_compensate_press(data, adc_press, t_fine); - return IIO_VAL_FRACTIONAL; + return 0; } static int bmp180_chip_config(struct bmp280_data *data) @@ -2117,6 +2137,8 @@ static int bmp180_chip_config(struct bmp280_data *data) static const int bmp180_oversampling_temp_avail[] = { 1 }; static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; +static const int bmp180_temp_coeffs[] = { 100, 1 }; +static const int bmp180_press_coeffs[] = { 1, 1000 }; const struct bmp280_chip_info bmp180_chip_info = { .id_reg = BMP280_REG_ID, @@ -2137,6 +2159,11 @@ const struct bmp280_chip_info bmp180_chip_info = { ARRAY_SIZE(bmp180_oversampling_press_avail), .oversampling_press_default = BMP180_MEAS_PRESS_8X, + .temp_coeffs = bmp180_temp_coeffs, + .temp_coeffs_type = IIO_VAL_FRACTIONAL, + .press_coeffs = bmp180_press_coeffs, + .press_coeffs_type = IIO_VAL_FRACTIONAL, + .chip_config = bmp180_chip_config, .read_temp = bmp180_read_temp, .read_press = bmp180_read_press, diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index 7c30e4d523be..a3d2cd722760 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -446,10 +446,17 @@ struct bmp280_chip_info { int num_sampling_freq_avail; int sampling_freq_default; + const int *temp_coeffs; + const int temp_coeffs_type; + const int *press_coeffs; + const int press_coeffs_type; + const int *humid_coeffs; + const int humid_coeffs_type; + int (*chip_config)(struct bmp280_data *data); - int (*read_temp)(struct bmp280_data *data, int *val, int *val2); - int (*read_press)(struct bmp280_data *data, int *val, int *val2); - int (*read_humid)(struct bmp280_data *data, int *val, int *val2); + int (*read_temp)(struct bmp280_data *data, s32 *adc_temp); + int (*read_press)(struct bmp280_data *data, u32 *adc_press); + int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); int (*read_calib)(struct bmp280_data *data); int (*preinit)(struct bmp280_data *data); }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-17 23:05 ` [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions Vasileios Amoiridis @ 2024-06-22 9:28 ` Jonathan Cameron 2024-06-22 12:19 ` Vasileios Amoiridis 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2024-06-22 9:28 UTC (permalink / raw) To: Vasileios Amoiridis Cc: lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla 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. > --- > drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------ > drivers/iio/pressure/bmp280.h | 13 ++- > 2 files changed, 107 insertions(+), 73 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index 50d71ad83f37..27c00af060fa 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -445,10 +445,8 @@ static u32 bmp280_compensate_press(struct bmp280_data *data, > return (u32)p; > } > > -static int bmp280_read_temp(struct bmp280_data *data, > - int *val, int *val2) > +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) > { > - s32 comp_temp; > u32 adc_temp; > int ret; > > @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data, > if (ret) > return ret; > > - comp_temp = bmp280_compensate_temp(data, adc_temp); > + *comp_temp = bmp280_compensate_temp(data, adc_temp); > > - *val = comp_temp * 10; > - return IIO_VAL_INT; > + return 0; > } > > -static int bmp280_read_press(struct bmp280_data *data, > - int *val, int *val2) > +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) > { > - u32 comp_press, adc_press, t_fine; > + u32 adc_press; > + s32 t_fine; > int ret; > > ret = bmp280_get_t_fine(data, &t_fine); > @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data, > if (ret) > return ret; > > - comp_press = bmp280_compensate_press(data, adc_press, t_fine); > - > - *val = comp_press; > - *val2 = 256000; > + *comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > - return IIO_VAL_FRACTIONAL; > + return 0; > } > > -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity) > { > - u32 comp_humidity; > u16 adc_humidity; > s32 t_fine; > int ret; > @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > if (ret) > return ret; > > - comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > - > - *val = comp_humidity * 1000 / 1024; > + *comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > - return IIO_VAL_INT; > + return 0; > } > > static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > int *val, int *val2, long mask) > { > struct bmp280_data *data = iio_priv(indio_dev); > + int chan_value; > + int ret; > > guard(mutex)(&data->lock); > > @@ -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; > + *val2 = data->chip_info->temp_coeffs[1]; > + return data->chip_info->temp_coeffs_type; > default: > return -EINVAL; > } > @@ -822,6 +833,8 @@ static int bmp280_chip_config(struct bmp280_data *data) > > static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; > static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID }; > +static const int bmp280_temp_coeffs[] = { 10, 1 }; > +static const int bmp280_press_coeffs[] = { 1, 256000 }; > > const struct bmp280_chip_info bmp280_chip_info = { > .id_reg = BMP280_REG_ID, > @@ -850,6 +863,11 @@ const struct bmp280_chip_info bmp280_chip_info = { > .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail), > .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1, > > + .temp_coeffs = bmp280_temp_coeffs, > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > + .press_coeffs = bmp280_press_coeffs, > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > + > .chip_config = bmp280_chip_config, > .read_temp = bmp280_read_temp, > .read_press = bmp280_read_press, > @@ -877,6 +895,7 @@ static int bme280_chip_config(struct bmp280_data *data) > } > > static const u8 bme280_chip_ids[] = { BME280_CHIP_ID }; > +static const int bme280_humid_coeffs[] = { 1000, 1024 }; > > const struct bmp280_chip_info bme280_chip_info = { > .id_reg = BMP280_REG_ID, > @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = { > .num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail), > .oversampling_humid_default = BME280_OSRS_HUMIDITY_16X - 1, > > + .temp_coeffs = bmp280_temp_coeffs, > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > + .press_coeffs = bmp280_press_coeffs, > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > + .humid_coeffs = bme280_humid_coeffs, > + .humid_coeffs_type = IIO_VAL_FRACTIONAL, > + > .chip_config = bme280_chip_config, > .read_temp = bmp280_read_temp, > .read_press = bmp280_read_press, > @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, > return comp_press; > } > > -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp) > { > - s32 comp_temp; > u32 adc_temp; > int ret; > > @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > if (ret) > return ret; > > - comp_temp = bmp380_compensate_temp(data, adc_temp); > + *comp_temp = bmp380_compensate_temp(data, adc_temp); > > - *val = comp_temp * 10; > - return IIO_VAL_INT; > + return 0; > } > > -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press) > { > - u32 adc_press, comp_press, t_fine; > + u32 adc_press, t_fine; > int ret; > > ret = bmp380_get_t_fine(data, &t_fine); > @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > if (ret) > return ret; > > - comp_press = bmp380_compensate_press(data, adc_press, t_fine); > - > - *val = comp_press; > - *val2 = 100000; > + *comp_press = bmp380_compensate_press(data, adc_press, t_fine); > > - return IIO_VAL_FRACTIONAL; > + return 0; > } > > static int bmp380_read_calib(struct bmp280_data *data) > @@ -1296,6 +1317,8 @@ static int bmp380_chip_config(struct bmp280_data *data) > static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; > static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128}; > static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID }; > +static const int bmp380_temp_coeffs[] = { 10, 1 }; > +static const int bmp380_press_coeffs[] = { 1, 100000 }; > > const struct bmp280_chip_info bmp380_chip_info = { > .id_reg = BMP380_REG_ID, > @@ -1323,6 +1346,11 @@ const struct bmp280_chip_info bmp380_chip_info = { > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > .iir_filter_coeff_default = 2, > > + .temp_coeffs = bmp380_temp_coeffs, > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > + .press_coeffs = bmp380_press_coeffs, > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > + > .chip_config = bmp380_chip_config, > .read_temp = bmp380_read_temp, > .read_press = bmp380_read_press, > @@ -1443,9 +1471,9 @@ static int bmp580_nvm_operation(struct bmp280_data *data, bool is_write) > * for what is expected on IIO ABI. > */ > > -static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > +static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp) > { > - s32 raw_temp; > + s32 value_temp; > int ret; > > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > @@ -1455,25 +1483,19 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > return ret; > } > > - raw_temp = get_unaligned_le24(data->buf); > - if (raw_temp == BMP580_TEMP_SKIPPED) { > + value_temp = get_unaligned_le24(data->buf); > + if (value_temp == BMP580_TEMP_SKIPPED) { > dev_err(data->dev, "reading temperature skipped\n"); > return -EIO; > } > + *raw_temp = sign_extend32(value_temp, 23); > > - /* > - * Temperature is returned in Celsius degrees in fractional > - * form down 2^16. We rescale by x1000 to return millidegrees > - * Celsius to respect IIO ABI. > - */ > - raw_temp = sign_extend32(raw_temp, 23); > - *val = ((s64)raw_temp * 1000) / (1 << 16); > - return IIO_VAL_INT; > + return 0; > } > > -static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > +static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press) > { > - u32 raw_press; > + u32 value_press; > int ret; > > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > @@ -1483,18 +1505,14 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > return ret; > } > > - raw_press = get_unaligned_le24(data->buf); > - if (raw_press == BMP580_PRESS_SKIPPED) { > + value_press = get_unaligned_le24(data->buf); > + if (value_press == BMP580_PRESS_SKIPPED) { > dev_err(data->dev, "reading pressure skipped\n"); > return -EIO; > } > - /* > - * Pressure is returned in Pascals in fractional form down 2^16. > - * We rescale /1000 to convert to kilopascal to respect IIO ABI. > - */ > - *val = raw_press; > - *val2 = 64000; /* 2^6 * 1000 */ > - return IIO_VAL_FRACTIONAL; > + *raw_press = value_press; > + > + return 0; > } > > static const int bmp580_odr_table[][2] = { > @@ -1830,6 +1848,8 @@ static int bmp580_chip_config(struct bmp280_data *data) > > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > +static const int bmp580_temp_coeffs[] = { 1000, 16 }; > +static const int bmp580_press_coeffs[] = { 1, 64000}; > > const struct bmp280_chip_info bmp580_chip_info = { > .id_reg = BMP580_REG_CHIP_ID, > @@ -1856,6 +1876,11 @@ const struct bmp280_chip_info bmp580_chip_info = { > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > .iir_filter_coeff_default = 2, > > + .temp_coeffs = bmp580_temp_coeffs, > + .temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2, > + .press_coeffs = bmp580_press_coeffs, > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > + > .chip_config = bmp580_chip_config, > .read_temp = bmp580_read_temp, > .read_press = bmp580_read_press, > @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp) > return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16; > } > > -static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > +static int bmp180_read_temp(struct bmp280_data *data, s32 *comp_temp) > { > - s32 comp_temp; > u32 adc_temp; > int ret; > > @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > if (ret) > return ret; > > - comp_temp = bmp180_compensate_temp(data, adc_temp); > + *comp_temp = bmp180_compensate_temp(data, adc_temp); > > - *val = comp_temp * 100; > - return IIO_VAL_INT; > + return 0; > } > > static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) > @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press, > return p + ((x1 + x2 + 3791) >> 4); > } > > -static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > +static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press) > { > - u32 comp_press, adc_press; > + u32 adc_press; > s32 t_fine; > int ret; > > @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > if (ret) > return ret; > > - comp_press = bmp180_compensate_press(data, adc_press, t_fine); > - > - *val = comp_press; > - *val2 = 1000; > + *comp_press = bmp180_compensate_press(data, adc_press, t_fine); > > - return IIO_VAL_FRACTIONAL; > + return 0; > } > > static int bmp180_chip_config(struct bmp280_data *data) > @@ -2117,6 +2137,8 @@ static int bmp180_chip_config(struct bmp280_data *data) > static const int bmp180_oversampling_temp_avail[] = { 1 }; > static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; > static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; > +static const int bmp180_temp_coeffs[] = { 100, 1 }; > +static const int bmp180_press_coeffs[] = { 1, 1000 }; > > const struct bmp280_chip_info bmp180_chip_info = { > .id_reg = BMP280_REG_ID, > @@ -2137,6 +2159,11 @@ const struct bmp280_chip_info bmp180_chip_info = { > ARRAY_SIZE(bmp180_oversampling_press_avail), > .oversampling_press_default = BMP180_MEAS_PRESS_8X, > > + .temp_coeffs = bmp180_temp_coeffs, > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > + .press_coeffs = bmp180_press_coeffs, > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > + > .chip_config = bmp180_chip_config, > .read_temp = bmp180_read_temp, > .read_press = bmp180_read_press, > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > index 7c30e4d523be..a3d2cd722760 100644 > --- a/drivers/iio/pressure/bmp280.h > +++ b/drivers/iio/pressure/bmp280.h > @@ -446,10 +446,17 @@ struct bmp280_chip_info { > int num_sampling_freq_avail; > int sampling_freq_default; > > + const int *temp_coeffs; > + const int temp_coeffs_type; > + const int *press_coeffs; > + const int press_coeffs_type; > + const int *humid_coeffs; > + const int humid_coeffs_type; > + > int (*chip_config)(struct bmp280_data *data); > - int (*read_temp)(struct bmp280_data *data, int *val, int *val2); > - int (*read_press)(struct bmp280_data *data, int *val, int *val2); > - int (*read_humid)(struct bmp280_data *data, int *val, int *val2); > + int (*read_temp)(struct bmp280_data *data, s32 *adc_temp); > + int (*read_press)(struct bmp280_data *data, u32 *adc_press); > + int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); > int (*read_calib)(struct bmp280_data *data); > int (*preinit)(struct bmp280_data *data); > }; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-22 9:28 ` Jonathan Cameron @ 2024-06-22 12:19 ` Vasileios Amoiridis 2024-06-23 16:23 ` Jonathan Cameron 2024-06-24 4:26 ` Adam Rizkalla 0 siblings, 2 replies; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-22 12:19 UTC (permalink / raw) To: Jonathan Cameron Cc: Vasileios Amoiridis, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla 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... > > --- > > drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------ > > drivers/iio/pressure/bmp280.h | 13 ++- > > 2 files changed, 107 insertions(+), 73 deletions(-) > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > index 50d71ad83f37..27c00af060fa 100644 > > --- a/drivers/iio/pressure/bmp280-core.c > > +++ b/drivers/iio/pressure/bmp280-core.c > > @@ -445,10 +445,8 @@ static u32 bmp280_compensate_press(struct bmp280_data *data, > > return (u32)p; > > } > > > > -static int bmp280_read_temp(struct bmp280_data *data, > > - int *val, int *val2) > > +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) > > { > > - s32 comp_temp; > > u32 adc_temp; > > int ret; > > > > @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data, > > if (ret) > > return ret; > > > > - comp_temp = bmp280_compensate_temp(data, adc_temp); > > + *comp_temp = bmp280_compensate_temp(data, adc_temp); > > > > - *val = comp_temp * 10; > > - return IIO_VAL_INT; > > + return 0; > > } > > > > -static int bmp280_read_press(struct bmp280_data *data, > > - int *val, int *val2) > > +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) > > { > > - u32 comp_press, adc_press, t_fine; > > + u32 adc_press; > > + s32 t_fine; > > int ret; > > > > ret = bmp280_get_t_fine(data, &t_fine); > > @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data, > > if (ret) > > return ret; > > > > - comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > - > > - *val = comp_press; > > - *val2 = 256000; > > + *comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > > > - return IIO_VAL_FRACTIONAL; > > + return 0; > > } > > > > -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity) > > { > > - u32 comp_humidity; > > u16 adc_humidity; > > s32 t_fine; > > int ret; > > @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > if (ret) > > return ret; > > > > - comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > - > > - *val = comp_humidity * 1000 / 1024; > > + *comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > > - return IIO_VAL_INT; > > + return 0; > > } > > > > static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > > @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > > int *val, int *val2, long mask) > > { > > struct bmp280_data *data = iio_priv(indio_dev); > > + int chan_value; > > + int ret; > > > > guard(mutex)(&data->lock); > > ... > > @@ -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). > > + *val2 = data->chip_info->temp_coeffs[1]; > > + return data->chip_info->temp_coeffs_type; > > default: > > return -EINVAL; > > } > > @@ -822,6 +833,8 @@ static int bmp280_chip_config(struct bmp280_data *data) > > > > static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; > > static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID }; > > +static const int bmp280_temp_coeffs[] = { 10, 1 }; > > +static const int bmp280_press_coeffs[] = { 1, 256000 }; > > > > const struct bmp280_chip_info bmp280_chip_info = { > > .id_reg = BMP280_REG_ID, > > @@ -850,6 +863,11 @@ const struct bmp280_chip_info bmp280_chip_info = { > > .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail), > > .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1, > > > > + .temp_coeffs = bmp280_temp_coeffs, > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > + .press_coeffs = bmp280_press_coeffs, > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > + > > .chip_config = bmp280_chip_config, > > .read_temp = bmp280_read_temp, > > .read_press = bmp280_read_press, > > @@ -877,6 +895,7 @@ static int bme280_chip_config(struct bmp280_data *data) > > } > > > > static const u8 bme280_chip_ids[] = { BME280_CHIP_ID }; > > +static const int bme280_humid_coeffs[] = { 1000, 1024 }; > > > > const struct bmp280_chip_info bme280_chip_info = { > > .id_reg = BMP280_REG_ID, > > @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = { > > .num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail), > > .oversampling_humid_default = BME280_OSRS_HUMIDITY_16X - 1, > > > > + .temp_coeffs = bmp280_temp_coeffs, > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > + .press_coeffs = bmp280_press_coeffs, > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > + .humid_coeffs = bme280_humid_coeffs, > > + .humid_coeffs_type = IIO_VAL_FRACTIONAL, > > + > > .chip_config = bme280_chip_config, > > .read_temp = bmp280_read_temp, > > .read_press = bmp280_read_press, > > @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, > > return comp_press; > > } > > > > -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > > +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp) > > { > > - s32 comp_temp; > > u32 adc_temp; > > int ret; > > > > @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > > if (ret) > > return ret; > > > > - comp_temp = bmp380_compensate_temp(data, adc_temp); > > + *comp_temp = bmp380_compensate_temp(data, adc_temp); > > > > - *val = comp_temp * 10; > > - return IIO_VAL_INT; > > + return 0; > > } > > > > -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > > +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press) > > { > > - u32 adc_press, comp_press, t_fine; > > + u32 adc_press, t_fine; > > int ret; > > > > ret = bmp380_get_t_fine(data, &t_fine); > > @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > > if (ret) > > return ret; > > > > - comp_press = bmp380_compensate_press(data, adc_press, t_fine); > > - > > - *val = comp_press; > > - *val2 = 100000; > > + *comp_press = bmp380_compensate_press(data, adc_press, t_fine); > > > > - return IIO_VAL_FRACTIONAL; > > + return 0; > > } > > > > static int bmp380_read_calib(struct bmp280_data *data) > > @@ -1296,6 +1317,8 @@ static int bmp380_chip_config(struct bmp280_data *data) > > static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; > > static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128}; > > static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID }; > > +static const int bmp380_temp_coeffs[] = { 10, 1 }; > > +static const int bmp380_press_coeffs[] = { 1, 100000 }; > > > > const struct bmp280_chip_info bmp380_chip_info = { > > .id_reg = BMP380_REG_ID, > > @@ -1323,6 +1346,11 @@ const struct bmp280_chip_info bmp380_chip_info = { > > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > > .iir_filter_coeff_default = 2, > > > > + .temp_coeffs = bmp380_temp_coeffs, > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > + .press_coeffs = bmp380_press_coeffs, > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > + > > .chip_config = bmp380_chip_config, > > .read_temp = bmp380_read_temp, > > .read_press = bmp380_read_press, > > @@ -1443,9 +1471,9 @@ static int bmp580_nvm_operation(struct bmp280_data *data, bool is_write) > > * for what is expected on IIO ABI. > > */ > > ... > > -static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > > +static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp) > > { > > - s32 raw_temp; > > + s32 value_temp; > > int ret; > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > > @@ -1455,25 +1483,19 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > > return ret; > > } > > > > - raw_temp = get_unaligned_le24(data->buf); > > - if (raw_temp == BMP580_TEMP_SKIPPED) { > > + value_temp = get_unaligned_le24(data->buf); > > + if (value_temp == BMP580_TEMP_SKIPPED) { > > dev_err(data->dev, "reading temperature skipped\n"); > > return -EIO; > > } > > + *raw_temp = sign_extend32(value_temp, 23); > > Here I use Adam's correction with sign_extend32() > > - /* > > - * Temperature is returned in Celsius degrees in fractional > > - * form down 2^16. We rescale by x1000 to return millidegrees > > - * Celsius to respect IIO ABI. > > - */ > > - raw_temp = sign_extend32(raw_temp, 23); > > - *val = ((s64)raw_temp * 1000) / (1 << 16); > > - return IIO_VAL_INT; > > + return 0; > > } > > > > -static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > > +static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press) > > { > > - u32 raw_press; > > + u32 value_press; > > int ret; > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > > @@ -1483,18 +1505,14 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > > return ret; > > } > > > > - raw_press = get_unaligned_le24(data->buf); > > - if (raw_press == BMP580_PRESS_SKIPPED) { > > + value_press = get_unaligned_le24(data->buf); > > + if (value_press == BMP580_PRESS_SKIPPED) { > > dev_err(data->dev, "reading pressure skipped\n"); > > return -EIO; > > } > > - /* > > - * Pressure is returned in Pascals in fractional form down 2^16. > > - * We rescale /1000 to convert to kilopascal to respect IIO ABI. > > - */ > > - *val = raw_press; > > - *val2 = 64000; /* 2^6 * 1000 */ > > - return IIO_VAL_FRACTIONAL; > > + *raw_press = value_press; > > + > > + return 0; > > } > > > > static const int bmp580_odr_table[][2] = { > > @@ -1830,6 +1848,8 @@ static int bmp580_chip_config(struct bmp280_data *data) > > > > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > > +static const int bmp580_temp_coeffs[] = { 1000, 16 }; > > +static const int bmp580_press_coeffs[] = { 1, 64000}; > > > > const struct bmp280_chip_info bmp580_chip_info = { > > .id_reg = BMP580_REG_CHIP_ID, > > @@ -1856,6 +1876,11 @@ const struct bmp280_chip_info bmp580_chip_info = { > > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > > .iir_filter_coeff_default = 2, > > > > + .temp_coeffs = bmp580_temp_coeffs, > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2, > > + .press_coeffs = bmp580_press_coeffs, > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > + > > .chip_config = bmp580_chip_config, > > .read_temp = bmp580_read_temp, > > .read_press = bmp580_read_press, > > @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16; > > } > > > > -static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > > +static int bmp180_read_temp(struct bmp280_data *data, s32 *comp_temp) > > { > > - s32 comp_temp; > > u32 adc_temp; > > int ret; > > > > @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > > if (ret) > > return ret; > > > > - comp_temp = bmp180_compensate_temp(data, adc_temp); > > + *comp_temp = bmp180_compensate_temp(data, adc_temp); > > > > - *val = comp_temp * 100; > > - return IIO_VAL_INT; > > + return 0; > > } > > > > static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) > > @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press, > > return p + ((x1 + x2 + 3791) >> 4); > > } > > > > -static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > > +static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press) > > { > > - u32 comp_press, adc_press; > > + u32 adc_press; > > s32 t_fine; > > int ret; > > > > @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > > if (ret) > > return ret; > > > > - comp_press = bmp180_compensate_press(data, adc_press, t_fine); > > - > > - *val = comp_press; > > - *val2 = 1000; > > + *comp_press = bmp180_compensate_press(data, adc_press, t_fine); > > > > - return IIO_VAL_FRACTIONAL; > > + return 0; > > } > > > > static int bmp180_chip_config(struct bmp280_data *data) > > @@ -2117,6 +2137,8 @@ static int bmp180_chip_config(struct bmp280_data *data) > > static const int bmp180_oversampling_temp_avail[] = { 1 }; > > static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; > > static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; > > +static const int bmp180_temp_coeffs[] = { 100, 1 }; > > +static const int bmp180_press_coeffs[] = { 1, 1000 }; > > > > const struct bmp280_chip_info bmp180_chip_info = { > > .id_reg = BMP280_REG_ID, > > @@ -2137,6 +2159,11 @@ const struct bmp280_chip_info bmp180_chip_info = { > > ARRAY_SIZE(bmp180_oversampling_press_avail), > > .oversampling_press_default = BMP180_MEAS_PRESS_8X, > > > > + .temp_coeffs = bmp180_temp_coeffs, > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > + .press_coeffs = bmp180_press_coeffs, > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > + > > .chip_config = bmp180_chip_config, > > .read_temp = bmp180_read_temp, > > .read_press = bmp180_read_press, > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > index 7c30e4d523be..a3d2cd722760 100644 > > --- a/drivers/iio/pressure/bmp280.h > > +++ b/drivers/iio/pressure/bmp280.h > > @@ -446,10 +446,17 @@ struct bmp280_chip_info { > > int num_sampling_freq_avail; > > int sampling_freq_default; > > > > + const int *temp_coeffs; > > + const int temp_coeffs_type; > > + const int *press_coeffs; > > + const int press_coeffs_type; > > + const int *humid_coeffs; > > + const int humid_coeffs_type; > > + > > int (*chip_config)(struct bmp280_data *data); > > - int (*read_temp)(struct bmp280_data *data, int *val, int *val2); > > - int (*read_press)(struct bmp280_data *data, int *val, int *val2); > > - int (*read_humid)(struct bmp280_data *data, int *val, int *val2); > > + int (*read_temp)(struct bmp280_data *data, s32 *adc_temp); > > + int (*read_press)(struct bmp280_data *data, u32 *adc_press); > > + int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); > > int (*read_calib)(struct bmp280_data *data); > > int (*preinit)(struct bmp280_data *data); > > }; > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-22 12:19 ` Vasileios Amoiridis @ 2024-06-23 16:23 ` Jonathan Cameron 2024-06-23 17:18 ` Vasileios Amoiridis 2024-06-24 4:26 ` Adam Rizkalla 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2024-06-23 16:23 UTC (permalink / raw) To: Vasileios Amoiridis, ak, phil Cc: lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, petre.rodan, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-23 16:23 ` Jonathan Cameron @ 2024-06-23 17:18 ` Vasileios Amoiridis 2024-06-25 20:44 ` Jonathan Cameron 0 siblings, 1 reply; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-23 17:18 UTC (permalink / raw) To: Jonathan Cameron Cc: Vasileios Amoiridis, ak, phil, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, petre.rodan, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla 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. 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-23 17:18 ` Vasileios Amoiridis @ 2024-06-25 20:44 ` Jonathan Cameron 2024-06-26 13:41 ` Vasileios Amoiridis 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2024-06-25 20:44 UTC (permalink / raw) To: Vasileios Amoiridis Cc: ak, phil, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, petre.rodan, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-25 20:44 ` Jonathan Cameron @ 2024-06-26 13:41 ` Vasileios Amoiridis 0 siblings, 0 replies; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-26 13:41 UTC (permalink / raw) To: Jonathan Cameron Cc: Vasileios Amoiridis, ak, phil, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, petre.rodan, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-22 12:19 ` Vasileios Amoiridis 2024-06-23 16:23 ` Jonathan Cameron @ 2024-06-24 4:26 ` Adam Rizkalla 2024-06-26 13:38 ` Vasileios Amoiridis 1 sibling, 1 reply; 17+ messages in thread From: Adam Rizkalla @ 2024-06-24 4:26 UTC (permalink / raw) To: Vasileios Amoiridis Cc: Jonathan Cameron, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel On Sat, Jun 22, 2024 at 02:19:18PM +0200, Vasileios Amoiridis 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... > > > > --- > > > drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------ > > > drivers/iio/pressure/bmp280.h | 13 ++- > > > 2 files changed, 107 insertions(+), 73 deletions(-) > > > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > > index 50d71ad83f37..27c00af060fa 100644 > > > --- a/drivers/iio/pressure/bmp280-core.c > > > +++ b/drivers/iio/pressure/bmp280-core.c > > > @@ -445,10 +445,8 @@ static u32 bmp280_compensate_press(struct bmp280_data *data, > > > return (u32)p; > > > } > > > > > > -static int bmp280_read_temp(struct bmp280_data *data, > > > - int *val, int *val2) > > > +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) > > > { > > > - s32 comp_temp; > > > u32 adc_temp; > > > int ret; > > > > > > @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data, > > > if (ret) > > > return ret; > > > > > > - comp_temp = bmp280_compensate_temp(data, adc_temp); > > > + *comp_temp = bmp280_compensate_temp(data, adc_temp); > > > > > > - *val = comp_temp * 10; > > > - return IIO_VAL_INT; > > > + return 0; > > > } > > > > > > -static int bmp280_read_press(struct bmp280_data *data, > > > - int *val, int *val2) > > > +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) > > > { > > > - u32 comp_press, adc_press, t_fine; > > > + u32 adc_press; > > > + s32 t_fine; > > > int ret; > > > > > > ret = bmp280_get_t_fine(data, &t_fine); > > > @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data, > > > if (ret) > > > return ret; > > > > > > - comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > > - > > > - *val = comp_press; > > > - *val2 = 256000; > > > + *comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > > > > > - return IIO_VAL_FRACTIONAL; > > > + return 0; > > > } > > > > > > -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > > +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity) > > > { > > > - u32 comp_humidity; > > > u16 adc_humidity; > > > s32 t_fine; > > > int ret; > > > @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > > if (ret) > > > return ret; > > > > > > - comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > - > > > - *val = comp_humidity * 1000 / 1024; > > > + *comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > > > > - return IIO_VAL_INT; > > > + return 0; > > > } > > > > > > static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > > > @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > > > int *val, int *val2, long mask) > > > { > > > struct bmp280_data *data = iio_priv(indio_dev); > > > + int chan_value; > > > + int ret; > > > > > > guard(mutex)(&data->lock); > > > > > ... > > > > @@ -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). > > > > + *val2 = data->chip_info->temp_coeffs[1]; > > > + return data->chip_info->temp_coeffs_type; > > > default: > > > return -EINVAL; > > > } > > > @@ -822,6 +833,8 @@ static int bmp280_chip_config(struct bmp280_data *data) > > > > > > static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; > > > static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID }; > > > +static const int bmp280_temp_coeffs[] = { 10, 1 }; > > > +static const int bmp280_press_coeffs[] = { 1, 256000 }; > > > > > > const struct bmp280_chip_info bmp280_chip_info = { > > > .id_reg = BMP280_REG_ID, > > > @@ -850,6 +863,11 @@ const struct bmp280_chip_info bmp280_chip_info = { > > > .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail), > > > .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1, > > > > > > + .temp_coeffs = bmp280_temp_coeffs, > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > + .press_coeffs = bmp280_press_coeffs, > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > + > > > .chip_config = bmp280_chip_config, > > > .read_temp = bmp280_read_temp, > > > .read_press = bmp280_read_press, > > > @@ -877,6 +895,7 @@ static int bme280_chip_config(struct bmp280_data *data) > > > } > > > > > > static const u8 bme280_chip_ids[] = { BME280_CHIP_ID }; > > > +static const int bme280_humid_coeffs[] = { 1000, 1024 }; > > > > > > const struct bmp280_chip_info bme280_chip_info = { > > > .id_reg = BMP280_REG_ID, > > > @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = { > > > .num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail), > > > .oversampling_humid_default = BME280_OSRS_HUMIDITY_16X - 1, > > > > > > + .temp_coeffs = bmp280_temp_coeffs, > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > + .press_coeffs = bmp280_press_coeffs, > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > + .humid_coeffs = bme280_humid_coeffs, > > > + .humid_coeffs_type = IIO_VAL_FRACTIONAL, > > > + > > > .chip_config = bme280_chip_config, > > > .read_temp = bmp280_read_temp, > > > .read_press = bmp280_read_press, > > > @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, > > > return comp_press; > > > } > > > > > > -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > > > +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp) > > > { > > > - s32 comp_temp; > > > u32 adc_temp; > > > int ret; > > > > > > @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > > > if (ret) > > > return ret; > > > > > > - comp_temp = bmp380_compensate_temp(data, adc_temp); > > > + *comp_temp = bmp380_compensate_temp(data, adc_temp); > > > > > > - *val = comp_temp * 10; > > > - return IIO_VAL_INT; > > > + return 0; > > > } > > > > > > -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > > > +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press) > > > { > > > - u32 adc_press, comp_press, t_fine; > > > + u32 adc_press, t_fine; > > > int ret; > > > > > > ret = bmp380_get_t_fine(data, &t_fine); > > > @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > > > if (ret) > > > return ret; > > > > > > - comp_press = bmp380_compensate_press(data, adc_press, t_fine); > > > - > > > - *val = comp_press; > > > - *val2 = 100000; > > > + *comp_press = bmp380_compensate_press(data, adc_press, t_fine); > > > > > > - return IIO_VAL_FRACTIONAL; > > > + return 0; > > > } > > > > > > static int bmp380_read_calib(struct bmp280_data *data) > > > @@ -1296,6 +1317,8 @@ static int bmp380_chip_config(struct bmp280_data *data) > > > static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; > > > static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128}; > > > static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID }; > > > +static const int bmp380_temp_coeffs[] = { 10, 1 }; > > > +static const int bmp380_press_coeffs[] = { 1, 100000 }; > > > > > > const struct bmp280_chip_info bmp380_chip_info = { > > > .id_reg = BMP380_REG_ID, > > > @@ -1323,6 +1346,11 @@ const struct bmp280_chip_info bmp380_chip_info = { > > > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > > > .iir_filter_coeff_default = 2, > > > > > > + .temp_coeffs = bmp380_temp_coeffs, > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > + .press_coeffs = bmp380_press_coeffs, > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > + > > > .chip_config = bmp380_chip_config, > > > .read_temp = bmp380_read_temp, > > > .read_press = bmp380_read_press, > > > @@ -1443,9 +1471,9 @@ static int bmp580_nvm_operation(struct bmp280_data *data, bool is_write) > > > * for what is expected on IIO ABI. > > > */ > > > > > ... > > > > -static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > > > +static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp) > > > { > > > - s32 raw_temp; > > > + s32 value_temp; > > > int ret; > > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > > > @@ -1455,25 +1483,19 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > > > return ret; > > > } > > > > > > - raw_temp = get_unaligned_le24(data->buf); > > > - if (raw_temp == BMP580_TEMP_SKIPPED) { > > > + value_temp = get_unaligned_le24(data->buf); > > > + if (value_temp == BMP580_TEMP_SKIPPED) { > > > dev_err(data->dev, "reading temperature skipped\n"); > > > return -EIO; > > > } > > > + *raw_temp = sign_extend32(value_temp, 23); > > > > > Here I use Adam's correction with sign_extend32() > > > > - /* > > > - * Temperature is returned in Celsius degrees in fractional > > > - * form down 2^16. We rescale by x1000 to return millidegrees > > > - * Celsius to respect IIO ABI. > > > - */ > > > - raw_temp = sign_extend32(raw_temp, 23); > > > - *val = ((s64)raw_temp * 1000) / (1 << 16); > > > - return IIO_VAL_INT; > > > + return 0; > > > } > > > > > > -static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > > > +static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press) > > > { > > > - u32 raw_press; > > > + u32 value_press; > > > int ret; > > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > > > @@ -1483,18 +1505,14 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > > > return ret; > > > } > > > > > > - raw_press = get_unaligned_le24(data->buf); > > > - if (raw_press == BMP580_PRESS_SKIPPED) { > > > + value_press = get_unaligned_le24(data->buf); > > > + if (value_press == BMP580_PRESS_SKIPPED) { > > > dev_err(data->dev, "reading pressure skipped\n"); > > > return -EIO; > > > } > > > - /* > > > - * Pressure is returned in Pascals in fractional form down 2^16. > > > - * We rescale /1000 to convert to kilopascal to respect IIO ABI. > > > - */ > > > - *val = raw_press; > > > - *val2 = 64000; /* 2^6 * 1000 */ > > > - return IIO_VAL_FRACTIONAL; > > > + *raw_press = value_press; > > > + > > > + return 0; > > > } > > > > > > static const int bmp580_odr_table[][2] = { > > > @@ -1830,6 +1848,8 @@ static int bmp580_chip_config(struct bmp280_data *data) > > > > > > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > > > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > > > +static const int bmp580_temp_coeffs[] = { 1000, 16 }; I believe we can do something similar to what Jonathan mentioned in [1] to avoid using a 64-bit signed int by changing these coefficients to something like: static const int bmp580_temp_coeffs[] = { 250, 14 }; This would allow the result for the max temperature value (85) to still fit in a 32-bit signed integer and would give a bit more resolution in the result. The bit shift of 14 would achieve the same effect with the new scaling. 85 * 2^16 * 250 = 1,392,640,000 (can be stored as a s32) Then, 1,392,640,000 / 2^14 = 85000 [1] https://lore.kernel.org/all/20240602201200.30418-1-ajarizzo@gmail.com/ > > > +static const int bmp580_press_coeffs[] = { 1, 64000}; > > > > > > const struct bmp280_chip_info bmp580_chip_info = { > > > .id_reg = BMP580_REG_CHIP_ID, > > > @@ -1856,6 +1876,11 @@ const struct bmp280_chip_info bmp580_chip_info = { > > > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > > > .iir_filter_coeff_default = 2, > > > > > > + .temp_coeffs = bmp580_temp_coeffs, > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2, > > > + .press_coeffs = bmp580_press_coeffs, > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > + > > > .chip_config = bmp580_chip_config, > > > .read_temp = bmp580_read_temp, > > > .read_press = bmp580_read_press, > > > @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > > return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16; > > > } > > > > > > -static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > > > +static int bmp180_read_temp(struct bmp280_data *data, s32 *comp_temp) > > > { > > > - s32 comp_temp; > > > u32 adc_temp; > > > int ret; > > > > > > @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > > > if (ret) > > > return ret; > > > > > > - comp_temp = bmp180_compensate_temp(data, adc_temp); > > > + *comp_temp = bmp180_compensate_temp(data, adc_temp); > > > > > > - *val = comp_temp * 100; > > > - return IIO_VAL_INT; > > > + return 0; > > > } > > > > > > static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) > > > @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press, > > > return p + ((x1 + x2 + 3791) >> 4); > > > } > > > > > > -static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > > > +static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press) > > > { > > > - u32 comp_press, adc_press; > > > + u32 adc_press; > > > s32 t_fine; > > > int ret; > > > > > > @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > > > if (ret) > > > return ret; > > > > > > - comp_press = bmp180_compensate_press(data, adc_press, t_fine); > > > - > > > - *val = comp_press; > > > - *val2 = 1000; > > > + *comp_press = bmp180_compensate_press(data, adc_press, t_fine); > > > > > > - return IIO_VAL_FRACTIONAL; > > > + return 0; > > > } > > > > > > static int bmp180_chip_config(struct bmp280_data *data) > > > @@ -2117,6 +2137,8 @@ static int bmp180_chip_config(struct bmp280_data *data) > > > static const int bmp180_oversampling_temp_avail[] = { 1 }; > > > static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; > > > static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; > > > +static const int bmp180_temp_coeffs[] = { 100, 1 }; > > > +static const int bmp180_press_coeffs[] = { 1, 1000 }; > > > > > > const struct bmp280_chip_info bmp180_chip_info = { > > > .id_reg = BMP280_REG_ID, > > > @@ -2137,6 +2159,11 @@ const struct bmp280_chip_info bmp180_chip_info = { > > > ARRAY_SIZE(bmp180_oversampling_press_avail), > > > .oversampling_press_default = BMP180_MEAS_PRESS_8X, > > > > > > + .temp_coeffs = bmp180_temp_coeffs, > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > + .press_coeffs = bmp180_press_coeffs, > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > + > > > .chip_config = bmp180_chip_config, > > > .read_temp = bmp180_read_temp, > > > .read_press = bmp180_read_press, > > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > > index 7c30e4d523be..a3d2cd722760 100644 > > > --- a/drivers/iio/pressure/bmp280.h > > > +++ b/drivers/iio/pressure/bmp280.h > > > @@ -446,10 +446,17 @@ struct bmp280_chip_info { > > > int num_sampling_freq_avail; > > > int sampling_freq_default; > > > > > > + const int *temp_coeffs; > > > + const int temp_coeffs_type; > > > + const int *press_coeffs; > > > + const int press_coeffs_type; > > > + const int *humid_coeffs; > > > + const int humid_coeffs_type; > > > + > > > int (*chip_config)(struct bmp280_data *data); > > > - int (*read_temp)(struct bmp280_data *data, int *val, int *val2); > > > - int (*read_press)(struct bmp280_data *data, int *val, int *val2); > > > - int (*read_humid)(struct bmp280_data *data, int *val, int *val2); > > > + int (*read_temp)(struct bmp280_data *data, s32 *adc_temp); > > > + int (*read_press)(struct bmp280_data *data, u32 *adc_press); > > > + int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); > > > int (*read_calib)(struct bmp280_data *data); > > > int (*preinit)(struct bmp280_data *data); > > > }; > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions 2024-06-24 4:26 ` Adam Rizkalla @ 2024-06-26 13:38 ` Vasileios Amoiridis 0 siblings, 0 replies; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-26 13:38 UTC (permalink / raw) To: Adam Rizkalla Cc: Vasileios Amoiridis, Jonathan Cameron, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel On Sun, Jun 23, 2024 at 11:26:23PM -0500, Adam Rizkalla wrote: > On Sat, Jun 22, 2024 at 02:19:18PM +0200, Vasileios Amoiridis 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... > > > > > > --- > > > > drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------ > > > > drivers/iio/pressure/bmp280.h | 13 ++- > > > > 2 files changed, 107 insertions(+), 73 deletions(-) > > > > > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > > > index 50d71ad83f37..27c00af060fa 100644 > > > > --- a/drivers/iio/pressure/bmp280-core.c > > > > +++ b/drivers/iio/pressure/bmp280-core.c > > > > @@ -445,10 +445,8 @@ static u32 bmp280_compensate_press(struct bmp280_data *data, > > > > return (u32)p; > > > > } > > > > > > > > -static int bmp280_read_temp(struct bmp280_data *data, > > > > - int *val, int *val2) > > > > +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) > > > > { > > > > - s32 comp_temp; > > > > u32 adc_temp; > > > > int ret; > > > > > > > > @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data, > > > > if (ret) > > > > return ret; > > > > > > > > - comp_temp = bmp280_compensate_temp(data, adc_temp); > > > > + *comp_temp = bmp280_compensate_temp(data, adc_temp); > > > > > > > > - *val = comp_temp * 10; > > > > - return IIO_VAL_INT; > > > > + return 0; > > > > } > > > > > > > > -static int bmp280_read_press(struct bmp280_data *data, > > > > - int *val, int *val2) > > > > +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) > > > > { > > > > - u32 comp_press, adc_press, t_fine; > > > > + u32 adc_press; > > > > + s32 t_fine; > > > > int ret; > > > > > > > > ret = bmp280_get_t_fine(data, &t_fine); > > > > @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data, > > > > if (ret) > > > > return ret; > > > > > > > > - comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > > > - > > > > - *val = comp_press; > > > > - *val2 = 256000; > > > > + *comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > > > > > > > - return IIO_VAL_FRACTIONAL; > > > > + return 0; > > > > } > > > > > > > > -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > > > +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity) > > > > { > > > > - u32 comp_humidity; > > > > u16 adc_humidity; > > > > s32 t_fine; > > > > int ret; > > > > @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > > > if (ret) > > > > return ret; > > > > > > > > - comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > > - > > > > - *val = comp_humidity * 1000 / 1024; > > > > + *comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > > > > > > - return IIO_VAL_INT; > > > > + return 0; > > > > } > > > > > > > > static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > > > > @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev, > > > > int *val, int *val2, long mask) > > > > { > > > > struct bmp280_data *data = iio_priv(indio_dev); > > > > + int chan_value; > > > > + int ret; > > > > > > > > guard(mutex)(&data->lock); > > > > > > > > ... > > > > > > @@ -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). > > > > > > + *val2 = data->chip_info->temp_coeffs[1]; > > > > + return data->chip_info->temp_coeffs_type; > > > > default: > > > > return -EINVAL; > > > > } > > > > @@ -822,6 +833,8 @@ static int bmp280_chip_config(struct bmp280_data *data) > > > > > > > > static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; > > > > static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID }; > > > > +static const int bmp280_temp_coeffs[] = { 10, 1 }; > > > > +static const int bmp280_press_coeffs[] = { 1, 256000 }; > > > > > > > > const struct bmp280_chip_info bmp280_chip_info = { > > > > .id_reg = BMP280_REG_ID, > > > > @@ -850,6 +863,11 @@ const struct bmp280_chip_info bmp280_chip_info = { > > > > .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail), > > > > .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1, > > > > > > > > + .temp_coeffs = bmp280_temp_coeffs, > > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + .press_coeffs = bmp280_press_coeffs, > > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + > > > > .chip_config = bmp280_chip_config, > > > > .read_temp = bmp280_read_temp, > > > > .read_press = bmp280_read_press, > > > > @@ -877,6 +895,7 @@ static int bme280_chip_config(struct bmp280_data *data) > > > > } > > > > > > > > static const u8 bme280_chip_ids[] = { BME280_CHIP_ID }; > > > > +static const int bme280_humid_coeffs[] = { 1000, 1024 }; > > > > > > > > const struct bmp280_chip_info bme280_chip_info = { > > > > .id_reg = BMP280_REG_ID, > > > > @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = { > > > > .num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail), > > > > .oversampling_humid_default = BME280_OSRS_HUMIDITY_16X - 1, > > > > > > > > + .temp_coeffs = bmp280_temp_coeffs, > > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + .press_coeffs = bmp280_press_coeffs, > > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + .humid_coeffs = bme280_humid_coeffs, > > > > + .humid_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + > > > > .chip_config = bme280_chip_config, > > > > .read_temp = bmp280_read_temp, > > > > .read_press = bmp280_read_press, > > > > @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, > > > > return comp_press; > > > > } > > > > > > > > -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > > > > +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp) > > > > { > > > > - s32 comp_temp; > > > > u32 adc_temp; > > > > int ret; > > > > > > > > @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) > > > > if (ret) > > > > return ret; > > > > > > > > - comp_temp = bmp380_compensate_temp(data, adc_temp); > > > > + *comp_temp = bmp380_compensate_temp(data, adc_temp); > > > > > > > > - *val = comp_temp * 10; > > > > - return IIO_VAL_INT; > > > > + return 0; > > > > } > > > > > > > > -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > > > > +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press) > > > > { > > > > - u32 adc_press, comp_press, t_fine; > > > > + u32 adc_press, t_fine; > > > > int ret; > > > > > > > > ret = bmp380_get_t_fine(data, &t_fine); > > > > @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) > > > > if (ret) > > > > return ret; > > > > > > > > - comp_press = bmp380_compensate_press(data, adc_press, t_fine); > > > > - > > > > - *val = comp_press; > > > > - *val2 = 100000; > > > > + *comp_press = bmp380_compensate_press(data, adc_press, t_fine); > > > > > > > > - return IIO_VAL_FRACTIONAL; > > > > + return 0; > > > > } > > > > > > > > static int bmp380_read_calib(struct bmp280_data *data) > > > > @@ -1296,6 +1317,8 @@ static int bmp380_chip_config(struct bmp280_data *data) > > > > static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; > > > > static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128}; > > > > static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID }; > > > > +static const int bmp380_temp_coeffs[] = { 10, 1 }; > > > > +static const int bmp380_press_coeffs[] = { 1, 100000 }; > > > > > > > > const struct bmp280_chip_info bmp380_chip_info = { > > > > .id_reg = BMP380_REG_ID, > > > > @@ -1323,6 +1346,11 @@ const struct bmp280_chip_info bmp380_chip_info = { > > > > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > > > > .iir_filter_coeff_default = 2, > > > > > > > > + .temp_coeffs = bmp380_temp_coeffs, > > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + .press_coeffs = bmp380_press_coeffs, > > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + > > > > .chip_config = bmp380_chip_config, > > > > .read_temp = bmp380_read_temp, > > > > .read_press = bmp380_read_press, > > > > @@ -1443,9 +1471,9 @@ static int bmp580_nvm_operation(struct bmp280_data *data, bool is_write) > > > > * for what is expected on IIO ABI. > > > > */ > > > > > > > > ... > > > > > > -static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > > > > +static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp) > > > > { > > > > - s32 raw_temp; > > > > + s32 value_temp; > > > > int ret; > > > > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > > > > @@ -1455,25 +1483,19 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > > > > return ret; > > > > } > > > > > > > > - raw_temp = get_unaligned_le24(data->buf); > > > > - if (raw_temp == BMP580_TEMP_SKIPPED) { > > > > + value_temp = get_unaligned_le24(data->buf); > > > > + if (value_temp == BMP580_TEMP_SKIPPED) { > > > > dev_err(data->dev, "reading temperature skipped\n"); > > > > return -EIO; > > > > } > > > > + *raw_temp = sign_extend32(value_temp, 23); > > > > > > > > Here I use Adam's correction with sign_extend32() > > > > > > - /* > > > > - * Temperature is returned in Celsius degrees in fractional > > > > - * form down 2^16. We rescale by x1000 to return millidegrees > > > > - * Celsius to respect IIO ABI. > > > > - */ > > > > - raw_temp = sign_extend32(raw_temp, 23); > > > > - *val = ((s64)raw_temp * 1000) / (1 << 16); > > > > - return IIO_VAL_INT; > > > > + return 0; > > > > } > > > > > > > > -static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > > > > +static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press) > > > > { > > > > - u32 raw_press; > > > > + u32 value_press; > > > > int ret; > > > > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > > > > @@ -1483,18 +1505,14 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) > > > > return ret; > > > > } > > > > > > > > - raw_press = get_unaligned_le24(data->buf); > > > > - if (raw_press == BMP580_PRESS_SKIPPED) { > > > > + value_press = get_unaligned_le24(data->buf); > > > > + if (value_press == BMP580_PRESS_SKIPPED) { > > > > dev_err(data->dev, "reading pressure skipped\n"); > > > > return -EIO; > > > > } > > > > - /* > > > > - * Pressure is returned in Pascals in fractional form down 2^16. > > > > - * We rescale /1000 to convert to kilopascal to respect IIO ABI. > > > > - */ > > > > - *val = raw_press; > > > > - *val2 = 64000; /* 2^6 * 1000 */ > > > > - return IIO_VAL_FRACTIONAL; > > > > + *raw_press = value_press; > > > > + > > > > + return 0; > > > > } > > > > > > > > static const int bmp580_odr_table[][2] = { > > > > @@ -1830,6 +1848,8 @@ static int bmp580_chip_config(struct bmp280_data *data) > > > > > > > > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > > > > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > > > > +static const int bmp580_temp_coeffs[] = { 1000, 16 }; > > I believe we can do something similar to what Jonathan mentioned in [1] > to avoid using a 64-bit signed int by changing these coefficients to > something like: > > static const int bmp580_temp_coeffs[] = { 250, 14 }; > > This would allow the result for the max temperature value (85) to still > fit in a 32-bit signed integer and would give a bit more resolution in > the result. The bit shift of 14 would achieve the same effect with the > new scaling. > > 85 * 2^16 * 250 = 1,392,640,000 (can be stored as a s32) > > Then, 1,392,640,000 / 2^14 = 85000 > > > [1] https://lore.kernel.org/all/20240602201200.30418-1-ajarizzo@gmail.com/ > Hi Adam, This is such a good and simple idea. I don't know how it didn't even came up to my mind. I could even push a bit further to go to 125 and 19 maybe. Thanks a lot for the feedback. Cheers, Vasilis > > > > +static const int bmp580_press_coeffs[] = { 1, 64000}; > > > > > > > > const struct bmp280_chip_info bmp580_chip_info = { > > > > .id_reg = BMP580_REG_CHIP_ID, > > > > @@ -1856,6 +1876,11 @@ const struct bmp280_chip_info bmp580_chip_info = { > > > > .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), > > > > .iir_filter_coeff_default = 2, > > > > > > > > + .temp_coeffs = bmp580_temp_coeffs, > > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2, > > > > + .press_coeffs = bmp580_press_coeffs, > > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + > > > > .chip_config = bmp580_chip_config, > > > > .read_temp = bmp580_read_temp, > > > > .read_press = bmp580_read_press, > > > > @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > > > return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16; > > > > } > > > > > > > > -static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > > > > +static int bmp180_read_temp(struct bmp280_data *data, s32 *comp_temp) > > > > { > > > > - s32 comp_temp; > > > > u32 adc_temp; > > > > int ret; > > > > > > > > @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) > > > > if (ret) > > > > return ret; > > > > > > > > - comp_temp = bmp180_compensate_temp(data, adc_temp); > > > > + *comp_temp = bmp180_compensate_temp(data, adc_temp); > > > > > > > > - *val = comp_temp * 100; > > > > - return IIO_VAL_INT; > > > > + return 0; > > > > } > > > > > > > > static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) > > > > @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press, > > > > return p + ((x1 + x2 + 3791) >> 4); > > > > } > > > > > > > > -static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > > > > +static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press) > > > > { > > > > - u32 comp_press, adc_press; > > > > + u32 adc_press; > > > > s32 t_fine; > > > > int ret; > > > > > > > > @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) > > > > if (ret) > > > > return ret; > > > > > > > > - comp_press = bmp180_compensate_press(data, adc_press, t_fine); > > > > - > > > > - *val = comp_press; > > > > - *val2 = 1000; > > > > + *comp_press = bmp180_compensate_press(data, adc_press, t_fine); > > > > > > > > - return IIO_VAL_FRACTIONAL; > > > > + return 0; > > > > } > > > > > > > > static int bmp180_chip_config(struct bmp280_data *data) > > > > @@ -2117,6 +2137,8 @@ static int bmp180_chip_config(struct bmp280_data *data) > > > > static const int bmp180_oversampling_temp_avail[] = { 1 }; > > > > static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; > > > > static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; > > > > +static const int bmp180_temp_coeffs[] = { 100, 1 }; > > > > +static const int bmp180_press_coeffs[] = { 1, 1000 }; > > > > > > > > const struct bmp280_chip_info bmp180_chip_info = { > > > > .id_reg = BMP280_REG_ID, > > > > @@ -2137,6 +2159,11 @@ const struct bmp280_chip_info bmp180_chip_info = { > > > > ARRAY_SIZE(bmp180_oversampling_press_avail), > > > > .oversampling_press_default = BMP180_MEAS_PRESS_8X, > > > > > > > > + .temp_coeffs = bmp180_temp_coeffs, > > > > + .temp_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + .press_coeffs = bmp180_press_coeffs, > > > > + .press_coeffs_type = IIO_VAL_FRACTIONAL, > > > > + > > > > .chip_config = bmp180_chip_config, > > > > .read_temp = bmp180_read_temp, > > > > .read_press = bmp180_read_press, > > > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > > > index 7c30e4d523be..a3d2cd722760 100644 > > > > --- a/drivers/iio/pressure/bmp280.h > > > > +++ b/drivers/iio/pressure/bmp280.h > > > > @@ -446,10 +446,17 @@ struct bmp280_chip_info { > > > > int num_sampling_freq_avail; > > > > int sampling_freq_default; > > > > > > > > + const int *temp_coeffs; > > > > + const int temp_coeffs_type; > > > > + const int *press_coeffs; > > > > + const int press_coeffs_type; > > > > + const int *humid_coeffs; > > > > + const int humid_coeffs_type; > > > > + > > > > int (*chip_config)(struct bmp280_data *data); > > > > - int (*read_temp)(struct bmp280_data *data, int *val, int *val2); > > > > - int (*read_press)(struct bmp280_data *data, int *val, int *val2); > > > > - int (*read_humid)(struct bmp280_data *data, int *val, int *val2); > > > > + int (*read_temp)(struct bmp280_data *data, s32 *adc_temp); > > > > + int (*read_press)(struct bmp280_data *data, u32 *adc_press); > > > > + int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); > > > > int (*read_calib)(struct bmp280_data *data); > > > > int (*preinit)(struct bmp280_data *data); > > > > }; > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 2/3] iio: pressure: bmp280: Add SCALE, RAW values in channels and refactorize them 2024-06-17 23:05 [PATCH v8 0/3] Driver cleanup and triggered buffer support Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions Vasileios Amoiridis @ 2024-06-17 23:05 ` Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support Vasileios Amoiridis 2 siblings, 0 replies; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-17 23:05 UTC (permalink / raw) To: jic23, lars Cc: andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Vasileios Amoiridis Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW channels in order to be able to calculate the processed value with standard userspace IIO tools. Can be used for triggered buffers as well. Even though it is not a good design choice to have SCALE, RAW and PROCESSED together, the PROCESSED channel is kept for ABI compatibility. While at it, separate BMPxxx and BMExxx device channels since BME supports also humidity measurements. Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> Link: https://lore.kernel.org/r/20240512230524.53990-5-vassilisamir@gmail.com --- drivers/iio/pressure/bmp280-core.c | 96 ++++++++++++++++++++++++++---- 1 file changed, 83 insertions(+), 13 deletions(-) diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 27c00af060fa..5f64d9951f37 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -137,17 +137,45 @@ enum { static const struct iio_chan_spec bmp280_channels[] = { { .type = IIO_PRESSURE, + /* PROCESSED maintained for ABI backwards compatibility */ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), }, { .type = IIO_TEMP, + /* PROCESSED maintained for ABI backwards compatibility */ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + }, +}; + +static const struct iio_chan_spec bme280_channels[] = { + { + .type = IIO_PRESSURE, + /* PROCESSED maintained for ABI backwards compatibility */ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + }, + { + .type = IIO_TEMP, + /* PROCESSED maintained for ABI backwards compatibility */ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), }, { .type = IIO_HUMIDITYRELATIVE, + /* PROCESSED maintained for ABI backwards compatibility */ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), }, }; @@ -155,21 +183,20 @@ static const struct iio_chan_spec bmp280_channels[] = { static const struct iio_chan_spec bmp380_channels[] = { { .type = IIO_PRESSURE, + /* PROCESSED maintained for ABI backwards compatibility */ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), }, { .type = IIO_TEMP, + /* PROCESSED maintained for ABI backwards compatibility */ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), - .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | - BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), - }, - { - .type = IIO_HUMIDITYRELATIVE, - .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), @@ -537,6 +564,49 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_CHAN_INFO_RAW: + switch (chan->type) { + case IIO_HUMIDITYRELATIVE: + ret = data->chip_info->read_humid(data, &chan_value); + if (ret) + return ret; + + *val = chan_value; + return IIO_VAL_INT; + case IIO_PRESSURE: + ret = data->chip_info->read_press(data, &chan_value); + if (ret) + return ret; + + *val = chan_value; + return IIO_VAL_INT; + case IIO_TEMP: + ret = data->chip_info->read_temp(data, &chan_value); + if (ret) + return ret; + + *val = chan_value; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_SCALE: + switch (chan->type) { + case IIO_HUMIDITYRELATIVE: + *val = data->chip_info->humid_coeffs[0]; + *val2 = data->chip_info->humid_coeffs[1]; + return data->chip_info->humid_coeffs_type; + case IIO_PRESSURE: + *val = data->chip_info->press_coeffs[0]; + *val2 = data->chip_info->press_coeffs[1]; + return data->chip_info->press_coeffs_type; + case IIO_TEMP: + *val = data->chip_info->temp_coeffs[0]; + *val2 = data->chip_info->temp_coeffs[1]; + return data->chip_info->temp_coeffs_type; + default: + return -EINVAL; + } case IIO_CHAN_INFO_OVERSAMPLING_RATIO: switch (chan->type) { case IIO_HUMIDITYRELATIVE: @@ -843,7 +913,7 @@ const struct bmp280_chip_info bmp280_chip_info = { .regmap_config = &bmp280_regmap_config, .start_up_time = 2000, .channels = bmp280_channels, - .num_channels = 2, + .num_channels = ARRAY_SIZE(bmp280_channels), .oversampling_temp_avail = bmp280_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail), @@ -903,8 +973,8 @@ const struct bmp280_chip_info bme280_chip_info = { .num_chip_id = ARRAY_SIZE(bme280_chip_ids), .regmap_config = &bmp280_regmap_config, .start_up_time = 2000, - .channels = bmp280_channels, - .num_channels = 3, + .channels = bme280_channels, + .num_channels = ARRAY_SIZE(bme280_channels), .oversampling_temp_avail = bmp280_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail), @@ -1328,7 +1398,7 @@ const struct bmp280_chip_info bmp380_chip_info = { .spi_read_extra_byte = true, .start_up_time = 2000, .channels = bmp380_channels, - .num_channels = 2, + .num_channels = ARRAY_SIZE(bmp380_channels), .oversampling_temp_avail = bmp380_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp380_oversampling_avail), @@ -1858,7 +1928,7 @@ const struct bmp280_chip_info bmp580_chip_info = { .regmap_config = &bmp580_regmap_config, .start_up_time = 2000, .channels = bmp380_channels, - .num_channels = 2, + .num_channels = ARRAY_SIZE(bmp380_channels), .oversampling_temp_avail = bmp580_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail), @@ -2147,7 +2217,7 @@ const struct bmp280_chip_info bmp180_chip_info = { .regmap_config = &bmp180_regmap_config, .start_up_time = 2000, .channels = bmp280_channels, - .num_channels = 2, + .num_channels = ARRAY_SIZE(bmp280_channels), .oversampling_temp_avail = bmp180_oversampling_temp_avail, .num_oversampling_temp_avail = -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support 2024-06-17 23:05 [PATCH v8 0/3] Driver cleanup and triggered buffer support Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 2/3] iio: pressure: bmp280: Add SCALE, RAW values in channels and refactorize them Vasileios Amoiridis @ 2024-06-17 23:05 ` Vasileios Amoiridis 2024-06-22 9:40 ` Jonathan Cameron 2 siblings, 1 reply; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-17 23:05 UTC (permalink / raw) To: jic23, lars Cc: andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Vasileios Amoiridis BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their temperature, pressure and humidity readings. This facilitates the use of burst/bulk reads in order to acquire data faster. The approach is different from the one used in oneshot captures. BMP085 & BMP1xx devices use a completely different measurement process that is well defined and is used in their buffer_handler(). Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com --- drivers/iio/pressure/Kconfig | 2 + drivers/iio/pressure/bmp280-core.c | 366 ++++++++++++++++++++++++++++- drivers/iio/pressure/bmp280-spi.c | 8 +- drivers/iio/pressure/bmp280.h | 21 +- 4 files changed, 379 insertions(+), 18 deletions(-) diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig index 3ad38506028e..0b5406a3f85d 100644 --- a/drivers/iio/pressure/Kconfig +++ b/drivers/iio/pressure/Kconfig @@ -31,6 +31,8 @@ config BMP280 select REGMAP select BMP280_I2C if (I2C) select BMP280_SPI if (SPI_MASTER) + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER help Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380 and BMP580 pressure and temperature sensors. Also supports the BME280 with diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 5f64d9951f37..05320a2e990e 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -41,7 +41,10 @@ #include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <linux/iio/buffer.h> #include <linux/iio/iio.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> #include <asm/unaligned.h> @@ -134,6 +137,12 @@ enum { BMP380_P11 = 20, }; +enum bmp280_scan { + BMP280_PRESS, + BMP280_TEMP, + BME280_HUMID, +}; + static const struct iio_chan_spec bmp280_channels[] = { { .type = IIO_PRESSURE, @@ -142,6 +151,13 @@ static const struct iio_chan_spec bmp280_channels[] = { BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .scan_index = 0, + .scan_type = { + .sign = 'u', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, }, { .type = IIO_TEMP, @@ -150,7 +166,15 @@ static const struct iio_chan_spec bmp280_channels[] = { BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .scan_index = 1, + .scan_type = { + .sign = 's', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, }, + IIO_CHAN_SOFT_TIMESTAMP(2), }; static const struct iio_chan_spec bme280_channels[] = { @@ -161,6 +185,13 @@ static const struct iio_chan_spec bme280_channels[] = { BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .scan_index = 0, + .scan_type = { + .sign = 'u', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, }, { .type = IIO_TEMP, @@ -169,6 +200,13 @@ static const struct iio_chan_spec bme280_channels[] = { BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .scan_index = 1, + .scan_type = { + .sign = 's', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, }, { .type = IIO_HUMIDITYRELATIVE, @@ -177,7 +215,15 @@ static const struct iio_chan_spec bme280_channels[] = { BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .scan_index = 2, + .scan_type = { + .sign = 'u', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, }, + IIO_CHAN_SOFT_TIMESTAMP(3), }; static const struct iio_chan_spec bmp380_channels[] = { @@ -190,6 +236,13 @@ static const struct iio_chan_spec bmp380_channels[] = { BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), + .scan_index = 0, + .scan_type = { + .sign = 'u', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, }, { .type = IIO_TEMP, @@ -200,7 +253,15 @@ static const struct iio_chan_spec bmp380_channels[] = { BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), + .scan_index = 1, + .scan_type = { + .sign = 's', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, }, + IIO_CHAN_SOFT_TIMESTAMP(2), }; static int bmp280_read_calib(struct bmp280_data *data) @@ -316,7 +377,7 @@ static int bme280_read_humid_adc(struct bmp280_data *data, u16 *adc_humidity) int ret; ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, - &data->be16, sizeof(data->be16)); + &data->be16, BME280_NUM_HUMIDITY_BYTES); if (ret) { dev_err(data->dev, "failed to read humidity\n"); return ret; @@ -362,7 +423,7 @@ static int bmp280_read_temp_adc(struct bmp280_data *data, u32 *adc_temp) int ret; ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_TEMP_BYTES); if (ret) { dev_err(data->dev, "failed to read temperature\n"); return ret; @@ -423,7 +484,7 @@ static int bmp280_read_press_adc(struct bmp280_data *data, u32 *adc_press) int ret; ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_PRESS_BYTES); if (ret) { dev_err(data->dev, "failed to read pressure\n"); return ret; @@ -558,7 +619,7 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev, if (ret) return ret; - *val = data->chip_info->temp_coeffs[0] * (s64)chan_value; + *val = data->chip_info->temp_coeffs[0] * chan_value; *val2 = data->chip_info->temp_coeffs[1]; return data->chip_info->temp_coeffs_type; default: @@ -874,6 +935,16 @@ static const struct iio_info bmp280_info = { .write_raw = &bmp280_write_raw, }; +static const unsigned long bmp280_avail_scan_masks[] = { + BIT(BMP280_TEMP) | BIT(BMP280_PRESS), + 0 +}; + +static const unsigned long bme280_avail_scan_masks[] = { + BIT(BME280_HUMID) | BIT(BMP280_TEMP) | BIT(BMP280_PRESS), + 0 +}; + static int bmp280_chip_config(struct bmp280_data *data) { u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) | @@ -901,6 +972,53 @@ static int bmp280_chip_config(struct bmp280_data *data) return ret; } +static irqreturn_t bmp280_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + s32 adc_temp, adc_press, t_fine; + int ret; + + guard(mutex)(&data->lock); + + /* Burst read data registers */ + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, + data->buf, BMP280_BURST_READ_BYTES); + if (ret) { + dev_err(data->dev, "failed to burst read sensor data\n"); + goto out; + } + + /* Temperature calculations */ + adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[3])); + if (adc_temp == BMP280_TEMP_SKIPPED) { + dev_err(data->dev, "reading temperature skipped\n"); + goto out; + } + + data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); + + /* Pressure calculations */ + adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); + if (adc_press == BMP280_PRESS_SKIPPED) { + dev_err(data->dev, "reading pressure skipped\n"); + goto out; + } + + t_fine = bmp280_calc_t_fine(data, adc_temp); + + data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); + + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_get_time_ns(indio_dev)); + +out: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID }; static const int bmp280_temp_coeffs[] = { 10, 1 }; @@ -914,6 +1032,7 @@ const struct bmp280_chip_info bmp280_chip_info = { .start_up_time = 2000, .channels = bmp280_channels, .num_channels = ARRAY_SIZE(bmp280_channels), + .avail_scan_masks = bmp280_avail_scan_masks, .oversampling_temp_avail = bmp280_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail), @@ -942,6 +1061,8 @@ const struct bmp280_chip_info bmp280_chip_info = { .read_temp = bmp280_read_temp, .read_press = bmp280_read_press, .read_calib = bmp280_read_calib, + + .buffer_handler = bmp280_buffer_handler, }; EXPORT_SYMBOL_NS(bmp280_chip_info, IIO_BMP280); @@ -964,6 +1085,62 @@ static int bme280_chip_config(struct bmp280_data *data) return bmp280_chip_config(data); } +static irqreturn_t bme280_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + s32 adc_temp, adc_press, adc_humidity, t_fine; + int ret; + + guard(mutex)(&data->lock); + + /* Burst read data registers */ + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, + data->buf, BME280_BURST_READ_BYTES); + if (ret) { + dev_err(data->dev, "failed to burst read sensor data\n"); + goto out; + } + + /* Temperature calculations */ + adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[3])); + if (adc_temp == BMP280_TEMP_SKIPPED) { + dev_err(data->dev, "reading temperature skipped\n"); + goto out; + } + + data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); + + /* Pressure calculations */ + adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); + if (adc_press == BMP280_PRESS_SKIPPED) { + dev_err(data->dev, "reading pressure skipped\n"); + goto out; + } + + t_fine = bmp280_calc_t_fine(data, adc_temp); + + data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); + + /* Humidity calculations */ + adc_humidity = get_unaligned_be16(&data->buf[6]); + + if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { + dev_err(data->dev, "reading humidity skipped\n"); + goto out; + } + data->sensor_data[2] = bme280_compensate_humidity(data, adc_humidity, t_fine); + + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_get_time_ns(indio_dev)); + +out: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const u8 bme280_chip_ids[] = { BME280_CHIP_ID }; static const int bme280_humid_coeffs[] = { 1000, 1024 }; @@ -975,6 +1152,7 @@ const struct bmp280_chip_info bme280_chip_info = { .start_up_time = 2000, .channels = bme280_channels, .num_channels = ARRAY_SIZE(bme280_channels), + .avail_scan_masks = bme280_avail_scan_masks, .oversampling_temp_avail = bmp280_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail), @@ -1000,6 +1178,8 @@ const struct bmp280_chip_info bme280_chip_info = { .read_press = bmp280_read_press, .read_humid = bme280_read_humid, .read_calib = bme280_read_calib, + + .buffer_handler = bme280_buffer_handler, }; EXPORT_SYMBOL_NS(bme280_chip_info, IIO_BMP280); @@ -1054,7 +1234,7 @@ static int bmp380_read_temp_adc(struct bmp280_data *data, u32 *adc_temp) int ret; ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_TEMP_BYTES); if (ret) { dev_err(data->dev, "failed to read temperature\n"); return ret; @@ -1123,7 +1303,7 @@ static int bmp380_read_press_adc(struct bmp280_data *data, u32 *adc_press) int ret; ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_PRESS_BYTES); if (ret) { dev_err(data->dev, "failed to read pressure\n"); return ret; @@ -1384,6 +1564,53 @@ static int bmp380_chip_config(struct bmp280_data *data) return 0; } +static irqreturn_t bmp380_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + s32 adc_temp, adc_press, t_fine; + int ret; + + guard(mutex)(&data->lock); + + /* Burst read data registers */ + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, + data->buf, BMP280_BURST_READ_BYTES); + if (ret) { + dev_err(data->dev, "failed to burst read sensor data\n"); + goto out; + } + + /* Temperature calculations */ + adc_temp = get_unaligned_le24(&data->buf[3]); + if (adc_temp == BMP380_TEMP_SKIPPED) { + dev_err(data->dev, "reading temperature skipped\n"); + goto out; + } + + data->sensor_data[1] = bmp380_compensate_temp(data, adc_temp); + + /* Pressure calculations */ + adc_press = get_unaligned_le24(&data->buf[0]); + if (adc_press == BMP380_PRESS_SKIPPED) { + dev_err(data->dev, "reading pressure skipped\n"); + goto out; + } + + t_fine = bmp380_calc_t_fine(data, adc_temp); + + data->sensor_data[0] = bmp380_compensate_press(data, adc_press, t_fine); + + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_get_time_ns(indio_dev)); + +out: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128}; static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID }; @@ -1399,6 +1626,7 @@ const struct bmp280_chip_info bmp380_chip_info = { .start_up_time = 2000, .channels = bmp380_channels, .num_channels = ARRAY_SIZE(bmp380_channels), + .avail_scan_masks = bmp280_avail_scan_masks, .oversampling_temp_avail = bmp380_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp380_oversampling_avail), @@ -1426,6 +1654,8 @@ const struct bmp280_chip_info bmp380_chip_info = { .read_press = bmp380_read_press, .read_calib = bmp380_read_calib, .preinit = bmp380_preinit, + + .buffer_handler = bmp380_buffer_handler, }; EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280); @@ -1546,8 +1776,8 @@ static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp) s32 value_temp; int ret; - ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, - sizeof(data->buf)); + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, + data->buf, BMP280_NUM_TEMP_BYTES); if (ret) { dev_err(data->dev, "failed to read temperature\n"); return ret; @@ -1568,8 +1798,8 @@ static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press) u32 value_press; int ret; - ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, - sizeof(data->buf)); + ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, + data->buf, BMP280_NUM_PRESS_BYTES); if (ret) { dev_err(data->dev, "failed to read pressure\n"); return ret; @@ -1916,6 +2146,51 @@ static int bmp580_chip_config(struct bmp280_data *data) return 0; } +static irqreturn_t bmp580_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + s32 adc_temp, adc_press; + int ret; + + guard(mutex)(&data->lock); + + /* Burst read data registers */ + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, + data->buf, BMP280_BURST_READ_BYTES); + if (ret) { + dev_err(data->dev, "failed to burst read sensor data\n"); + goto out; + } + + /* Temperature calculations */ + adc_temp = get_unaligned_le24(&data->buf[0]); + if (adc_temp == BMP580_TEMP_SKIPPED) { + dev_err(data->dev, "reading temperature skipped\n"); + goto out; + } + + data->sensor_data[1] = sign_extend32(adc_temp, 23); + + /* Pressure calculations */ + adc_press = get_unaligned_le24(&data->buf[3]); + if (adc_press == BMP380_PRESS_SKIPPED) { + dev_err(data->dev, "reading pressure skipped\n"); + goto out; + } + + data->sensor_data[0] = adc_press; + + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_get_time_ns(indio_dev)); + +out: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; static const int bmp580_temp_coeffs[] = { 1000, 16 }; @@ -1929,6 +2204,7 @@ const struct bmp280_chip_info bmp580_chip_info = { .start_up_time = 2000, .channels = bmp380_channels, .num_channels = ARRAY_SIZE(bmp380_channels), + .avail_scan_masks = bmp280_avail_scan_masks, .oversampling_temp_avail = bmp580_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail), @@ -1955,6 +2231,8 @@ const struct bmp280_chip_info bmp580_chip_info = { .read_temp = bmp580_read_temp, .read_press = bmp580_read_press, .preinit = bmp580_preinit, + + .buffer_handler = bmp580_buffer_handler, }; EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); @@ -2133,7 +2411,7 @@ static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) return ret; ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_PRESS_BYTES); if (ret) { dev_err(data->dev, "failed to read pressure\n"); return ret; @@ -2204,6 +2482,36 @@ static int bmp180_chip_config(struct bmp280_data *data) return 0; } +static irqreturn_t bmp180_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + int ret, chan_value; + + guard(mutex)(&data->lock); + + ret = bmp180_read_temp(data, &chan_value); + if (ret) + goto out; + + data->sensor_data[1] = chan_value; + + ret = bmp180_read_press(data, &chan_value); + if (ret) + goto out; + + data->sensor_data[0] = chan_value; + + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_get_time_ns(indio_dev)); + +out: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp180_oversampling_temp_avail[] = { 1 }; static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; @@ -2218,6 +2526,7 @@ const struct bmp280_chip_info bmp180_chip_info = { .start_up_time = 2000, .channels = bmp280_channels, .num_channels = ARRAY_SIZE(bmp280_channels), + .avail_scan_masks = bmp280_avail_scan_masks, .oversampling_temp_avail = bmp180_oversampling_temp_avail, .num_oversampling_temp_avail = @@ -2238,6 +2547,8 @@ const struct bmp280_chip_info bmp180_chip_info = { .read_temp = bmp180_read_temp, .read_press = bmp180_read_press, .read_calib = bmp180_read_calib, + + .buffer_handler = bmp180_buffer_handler, }; EXPORT_SYMBOL_NS(bmp180_chip_info, IIO_BMP280); @@ -2283,6 +2594,30 @@ static int bmp085_fetch_eoc_irq(struct device *dev, return 0; } +static int bmp280_buffer_preenable(struct iio_dev *indio_dev) +{ + struct bmp280_data *data = iio_priv(indio_dev); + + pm_runtime_get_sync(data->dev); + + return 0; +} + +static int bmp280_buffer_postdisable(struct iio_dev *indio_dev) +{ + struct bmp280_data *data = iio_priv(indio_dev); + + pm_runtime_mark_last_busy(data->dev); + pm_runtime_put_autosuspend(data->dev); + + return 0; +} + +static const struct iio_buffer_setup_ops bmp280_buffer_setup_ops = { + .preenable = bmp280_buffer_preenable, + .postdisable = bmp280_buffer_postdisable, +}; + static void bmp280_pm_disable(void *data) { struct device *dev = data; @@ -2329,6 +2664,7 @@ int bmp280_common_probe(struct device *dev, /* Apply initial values from chip info structure */ indio_dev->channels = chip_info->channels; indio_dev->num_channels = chip_info->num_channels; + indio_dev->available_scan_masks = chip_info->avail_scan_masks; data->oversampling_press = chip_info->oversampling_press_default; data->oversampling_humid = chip_info->oversampling_humid_default; data->oversampling_temp = chip_info->oversampling_temp_default; @@ -2414,6 +2750,14 @@ int bmp280_common_probe(struct device *dev, "failed to read calibration coefficients\n"); } + ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev, + iio_pollfunc_store_time, + data->chip_info->buffer_handler, + &bmp280_buffer_setup_ops); + if (ret) + return dev_err_probe(data->dev, ret, + "iio triggered buffer setup failed\n"); + /* * Attempt to grab an optional EOC IRQ - only the BMP085 has this * however as it happens, the BMP085 shares the chip ID of BMP180 diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c index 62b4e58104cf..e5abee15950e 100644 --- a/drivers/iio/pressure/bmp280-spi.c +++ b/drivers/iio/pressure/bmp280-spi.c @@ -40,14 +40,10 @@ static int bmp380_regmap_spi_read(void *context, const void *reg, size_t reg_size, void *val, size_t val_size) { struct spi_device *spi = to_spi_device(context); - u8 rx_buf[4]; + u8 rx_buf[BME280_BURST_READ_BYTES + 1]; ssize_t status; - /* - * Maximum number of consecutive bytes read for a temperature or - * pressure measurement is 3. - */ - if (val_size > 3) + if (val_size > BME280_BURST_READ_BYTES) return -EINVAL; /* diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index a3d2cd722760..756c644354c2 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -304,6 +304,16 @@ #define BMP280_PRESS_SKIPPED 0x80000 #define BMP280_HUMIDITY_SKIPPED 0x8000 +/* Number of bytes for each value */ +#define BMP280_NUM_PRESS_BYTES 3 +#define BMP280_NUM_TEMP_BYTES 3 +#define BME280_NUM_HUMIDITY_BYTES 2 +#define BMP280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ + BMP280_NUM_TEMP_BYTES) +#define BME280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ + BMP280_NUM_TEMP_BYTES + \ + BME280_NUM_HUMIDITY_BYTES) + /* Core exported structs */ static const char *const bmp280_supply_names[] = { @@ -397,13 +407,19 @@ struct bmp280_data { */ int sampling_freq; + /* + * Data to push to userspace triggered buffer. Up to 3 channels and + * s64 timestamp, aligned. + */ + s32 sensor_data[6] __aligned(8); + /* * DMA (thus cache coherency maintenance) may require the * transfer buffers to live in their own cache lines. */ union { /* Sensor data buffer */ - u8 buf[3]; + u8 buf[BME280_BURST_READ_BYTES]; /* Calibration data buffers */ __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; @@ -425,6 +441,7 @@ struct bmp280_chip_info { const struct iio_chan_spec *channels; int num_channels; unsigned int start_up_time; + const unsigned long *avail_scan_masks; const int *oversampling_temp_avail; int num_oversampling_temp_avail; @@ -459,6 +476,8 @@ struct bmp280_chip_info { int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); int (*read_calib)(struct bmp280_data *data); int (*preinit)(struct bmp280_data *data); + + irqreturn_t (*buffer_handler)(int irq, void *p); }; /* Chip infos for each variant */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support 2024-06-17 23:05 ` [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support Vasileios Amoiridis @ 2024-06-22 9:40 ` Jonathan Cameron 2024-06-22 12:32 ` Vasileios Amoiridis 2024-06-22 14:09 ` Vasileios Amoiridis 0 siblings, 2 replies; 17+ messages in thread From: Jonathan Cameron @ 2024-06-22 9:40 UTC (permalink / raw) To: Vasileios Amoiridis Cc: lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla On Tue, 18 Jun 2024 01:05:40 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their > temperature, pressure and humidity readings. This facilitates the > use of burst/bulk reads in order to acquire data faster. The > approach is different from the one used in oneshot captures. > > BMP085 & BMP1xx devices use a completely different measurement > process that is well defined and is used in their buffer_handler(). > > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com > --- The sign extend in buffered path doesn't make much sense as we should be advertising the correct bit depth for the channel and making that a userspace problem. I'd failed to notice you are doing endian conversions just to check the skipped values. Ideally we'd leave the channels little endian and include that in the channel spec. Hmm. I guess this works and if we have to do the endian conversion anyway isn't too bad. It does provide slightly wrong information to userspace though. So even with this in place I think these channels should be real_bits 24. > +static irqreturn_t bmp580_buffer_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bmp280_data *data = iio_priv(indio_dev); > + s32 adc_temp, adc_press; > + int ret; > + > + guard(mutex)(&data->lock); > + > + /* Burst read data registers */ > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > + data->buf, BMP280_BURST_READ_BYTES); > + if (ret) { > + dev_err(data->dev, "failed to burst read sensor data\n"); > + goto out; > + } > + > + /* Temperature calculations */ > + adc_temp = get_unaligned_le24(&data->buf[0]); > + if (adc_temp == BMP580_TEMP_SKIPPED) { > + dev_err(data->dev, "reading temperature skipped\n"); > + goto out; > + } > + > + data->sensor_data[1] = sign_extend32(adc_temp, 23); the channel type should indicate that it's a 24 bit value. Not our problem to sign extend. Leave that to userspace. > + > + /* Pressure calculations */ > + adc_press = get_unaligned_le24(&data->buf[3]); > + if (adc_press == BMP380_PRESS_SKIPPED) { > + dev_err(data->dev, "reading pressure skipped\n"); > + goto out; > + } > + > + data->sensor_data[0] = adc_press; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > + iio_get_time_ns(indio_dev)); > + > +out: > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > static const int bmp580_temp_coeffs[] = { 1000, 16 }; > @@ -1929,6 +2204,7 @@ const struct bmp280_chip_info bmp580_chip_info = { > .start_up_time = 2000, > .channels = bmp380_channels, > .num_channels = ARRAY_SIZE(bmp380_channels), > + .avail_scan_masks = bmp280_avail_scan_masks, > > .oversampling_temp_avail = bmp580_oversampling_avail, > .num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail), > @@ -1955,6 +2231,8 @@ const struct bmp280_chip_info bmp580_chip_info = { > .read_temp = bmp580_read_temp, > .read_press = bmp580_read_press, > .preinit = bmp580_preinit, > + > + .buffer_handler = bmp580_buffer_handler, > }; > EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); > > @@ -2133,7 +2411,7 @@ static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) > return ret; > > ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_PRESS_BYTES); > if (ret) { > dev_err(data->dev, "failed to read pressure\n"); > return ret; > @@ -2204,6 +2482,36 @@ static int bmp180_chip_config(struct bmp280_data *data) > return 0; > } > > +static irqreturn_t bmp180_buffer_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bmp280_data *data = iio_priv(indio_dev); > + int ret, chan_value; > + > + guard(mutex)(&data->lock); > + > + ret = bmp180_read_temp(data, &chan_value); > + if (ret) > + goto out; > + > + data->sensor_data[1] = chan_value; > + > + ret = bmp180_read_press(data, &chan_value); > + if (ret) > + goto out; > + > + data->sensor_data[0] = chan_value; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > + iio_get_time_ns(indio_dev)); > + > +out: > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > static const int bmp180_oversampling_temp_avail[] = { 1 }; > static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; > static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; > @@ -2218,6 +2526,7 @@ const struct bmp280_chip_info bmp180_chip_info = { > .start_up_time = 2000, > .channels = bmp280_channels, > .num_channels = ARRAY_SIZE(bmp280_channels), > + .avail_scan_masks = bmp280_avail_scan_masks, > > .oversampling_temp_avail = bmp180_oversampling_temp_avail, > .num_oversampling_temp_avail = > @@ -2238,6 +2547,8 @@ const struct bmp280_chip_info bmp180_chip_info = { > .read_temp = bmp180_read_temp, > .read_press = bmp180_read_press, > .read_calib = bmp180_read_calib, > + > + .buffer_handler = bmp180_buffer_handler, > }; > EXPORT_SYMBOL_NS(bmp180_chip_info, IIO_BMP280); > > @@ -2283,6 +2594,30 @@ static int bmp085_fetch_eoc_irq(struct device *dev, > return 0; > } > > +static int bmp280_buffer_preenable(struct iio_dev *indio_dev) > +{ > + struct bmp280_data *data = iio_priv(indio_dev); > + > + pm_runtime_get_sync(data->dev); > + > + return 0; > +} > + > +static int bmp280_buffer_postdisable(struct iio_dev *indio_dev) > +{ > + struct bmp280_data *data = iio_priv(indio_dev); > + > + pm_runtime_mark_last_busy(data->dev); > + pm_runtime_put_autosuspend(data->dev); > + > + return 0; > +} > + > +static const struct iio_buffer_setup_ops bmp280_buffer_setup_ops = { > + .preenable = bmp280_buffer_preenable, > + .postdisable = bmp280_buffer_postdisable, > +}; > + > static void bmp280_pm_disable(void *data) > { > struct device *dev = data; > @@ -2329,6 +2664,7 @@ int bmp280_common_probe(struct device *dev, > /* Apply initial values from chip info structure */ > indio_dev->channels = chip_info->channels; > indio_dev->num_channels = chip_info->num_channels; > + indio_dev->available_scan_masks = chip_info->avail_scan_masks; > data->oversampling_press = chip_info->oversampling_press_default; > data->oversampling_humid = chip_info->oversampling_humid_default; > data->oversampling_temp = chip_info->oversampling_temp_default; > @@ -2414,6 +2750,14 @@ int bmp280_common_probe(struct device *dev, > "failed to read calibration coefficients\n"); > } > > + ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev, > + iio_pollfunc_store_time, > + data->chip_info->buffer_handler, > + &bmp280_buffer_setup_ops); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "iio triggered buffer setup failed\n"); > + > /* > * Attempt to grab an optional EOC IRQ - only the BMP085 has this > * however as it happens, the BMP085 shares the chip ID of BMP180 > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c > index 62b4e58104cf..e5abee15950e 100644 > --- a/drivers/iio/pressure/bmp280-spi.c > +++ b/drivers/iio/pressure/bmp280-spi.c > @@ -40,14 +40,10 @@ static int bmp380_regmap_spi_read(void *context, const void *reg, > size_t reg_size, void *val, size_t val_size) > { > struct spi_device *spi = to_spi_device(context); > - u8 rx_buf[4]; > + u8 rx_buf[BME280_BURST_READ_BYTES + 1]; > ssize_t status; > > - /* > - * Maximum number of consecutive bytes read for a temperature or > - * pressure measurement is 3. > - */ > - if (val_size > 3) > + if (val_size > BME280_BURST_READ_BYTES) > return -EINVAL; > > /* > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > index a3d2cd722760..756c644354c2 100644 > --- a/drivers/iio/pressure/bmp280.h > +++ b/drivers/iio/pressure/bmp280.h > @@ -304,6 +304,16 @@ > #define BMP280_PRESS_SKIPPED 0x80000 > #define BMP280_HUMIDITY_SKIPPED 0x8000 > > +/* Number of bytes for each value */ > +#define BMP280_NUM_PRESS_BYTES 3 > +#define BMP280_NUM_TEMP_BYTES 3 > +#define BME280_NUM_HUMIDITY_BYTES 2 > +#define BMP280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ > + BMP280_NUM_TEMP_BYTES) > +#define BME280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ > + BMP280_NUM_TEMP_BYTES + \ > + BME280_NUM_HUMIDITY_BYTES) > + > /* Core exported structs */ > > static const char *const bmp280_supply_names[] = { > @@ -397,13 +407,19 @@ struct bmp280_data { > */ > int sampling_freq; > > + /* > + * Data to push to userspace triggered buffer. Up to 3 channels and > + * s64 timestamp, aligned. > + */ > + s32 sensor_data[6] __aligned(8); > + > /* > * DMA (thus cache coherency maintenance) may require the > * transfer buffers to live in their own cache lines. > */ > union { > /* Sensor data buffer */ > - u8 buf[3]; > + u8 buf[BME280_BURST_READ_BYTES]; > /* Calibration data buffers */ > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; > @@ -425,6 +441,7 @@ struct bmp280_chip_info { > const struct iio_chan_spec *channels; > int num_channels; > unsigned int start_up_time; > + const unsigned long *avail_scan_masks; > > const int *oversampling_temp_avail; > int num_oversampling_temp_avail; > @@ -459,6 +476,8 @@ struct bmp280_chip_info { > int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); > int (*read_calib)(struct bmp280_data *data); > int (*preinit)(struct bmp280_data *data); > + > + irqreturn_t (*buffer_handler)(int irq, void *p); > }; > > /* Chip infos for each variant */ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support 2024-06-22 9:40 ` Jonathan Cameron @ 2024-06-22 12:32 ` Vasileios Amoiridis 2024-06-22 14:09 ` Vasileios Amoiridis 1 sibling, 0 replies; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-22 12:32 UTC (permalink / raw) To: Jonathan Cameron Cc: Vasileios Amoiridis, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla On Sat, Jun 22, 2024 at 10:40:39AM +0100, Jonathan Cameron wrote: > On Tue, 18 Jun 2024 01:05:40 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their > > temperature, pressure and humidity readings. This facilitates the > > use of burst/bulk reads in order to acquire data faster. The > > approach is different from the one used in oneshot captures. > > > > BMP085 & BMP1xx devices use a completely different measurement > > process that is well defined and is used in their buffer_handler(). > > > > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com > > --- > The sign extend in buffered path doesn't make much sense as we should be > advertising the correct bit depth for the channel and making that a userspace > problem. > > I'd failed to notice you are doing endian conversions just to check > the skipped values. Ideally we'd leave the channels little endian > and include that in the channel spec. > > Hmm. I guess this works and if we have to do the endian conversion > anyway isn't too bad. It does provide slightly wrong information > to userspace though. > > So even with this in place I think these channels should be real_bits 24. > Well, I totally get your point. Actually, I think that it makes much more sense, to check the skipped values in userspace. These are information that come from the datasheet, and I think that if it is important to someone to check those values, they can do it. The point is to get the data to userspace as soon as possible and then it is on the hands of user to do what they want with that. So I agree that the implementation can be simplified a lot. As for the real_bits 24, I kind of get what you mean. I will fix this as well. I will wait for Adam's comment on the first patch as well, and then I will send a v9. > > > > +static irqreturn_t bmp580_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + s32 adc_temp, adc_press; > > + int ret; > > + > > + guard(mutex)(&data->lock); > > + > > + /* Burst read data registers */ > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > + data->buf, BMP280_BURST_READ_BYTES); > > + if (ret) { > > + dev_err(data->dev, "failed to burst read sensor data\n"); > > + goto out; > > + } > > + > > + /* Temperature calculations */ > > + adc_temp = get_unaligned_le24(&data->buf[0]); > > + if (adc_temp == BMP580_TEMP_SKIPPED) { > > + dev_err(data->dev, "reading temperature skipped\n"); > > + goto out; > > + } > > + > > + data->sensor_data[1] = sign_extend32(adc_temp, 23); > > the channel type should indicate that it's a 24 bit value. Not our > problem to sign extend. Leave that to userspace. > Ok, I understand. > > + > > + /* Pressure calculations */ > > + adc_press = get_unaligned_le24(&data->buf[3]); > > + if (adc_press == BMP380_PRESS_SKIPPED) { > > + dev_err(data->dev, "reading pressure skipped\n"); > > + goto out; > > + } > > + > > + data->sensor_data[0] = adc_press; > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_get_time_ns(indio_dev)); > > + > > +out: > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > > static const int bmp580_temp_coeffs[] = { 1000, 16 }; > > @@ -1929,6 +2204,7 @@ const struct bmp280_chip_info bmp580_chip_info = { > > .start_up_time = 2000, > > .channels = bmp380_channels, > > .num_channels = ARRAY_SIZE(bmp380_channels), > > + .avail_scan_masks = bmp280_avail_scan_masks, > > > > .oversampling_temp_avail = bmp580_oversampling_avail, > > .num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail), > > @@ -1955,6 +2231,8 @@ const struct bmp280_chip_info bmp580_chip_info = { > > .read_temp = bmp580_read_temp, > > .read_press = bmp580_read_press, > > .preinit = bmp580_preinit, > > + > > + .buffer_handler = bmp580_buffer_handler, > > }; > > EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); > > > > @@ -2133,7 +2411,7 @@ static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) > > return ret; > > > > ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_PRESS_BYTES); > > if (ret) { > > dev_err(data->dev, "failed to read pressure\n"); > > return ret; > > @@ -2204,6 +2482,36 @@ static int bmp180_chip_config(struct bmp280_data *data) > > return 0; > > } > > > > +static irqreturn_t bmp180_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + int ret, chan_value; > > + > > + guard(mutex)(&data->lock); > > + > > + ret = bmp180_read_temp(data, &chan_value); > > + if (ret) > > + goto out; > > + > > + data->sensor_data[1] = chan_value; > > + > > + ret = bmp180_read_press(data, &chan_value); > > + if (ret) > > + goto out; > > + > > + data->sensor_data[0] = chan_value; > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_get_time_ns(indio_dev)); > > + > > +out: > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > static const int bmp180_oversampling_temp_avail[] = { 1 }; > > static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; > > static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; > > @@ -2218,6 +2526,7 @@ const struct bmp280_chip_info bmp180_chip_info = { > > .start_up_time = 2000, > > .channels = bmp280_channels, > > .num_channels = ARRAY_SIZE(bmp280_channels), > > + .avail_scan_masks = bmp280_avail_scan_masks, > > > > .oversampling_temp_avail = bmp180_oversampling_temp_avail, > > .num_oversampling_temp_avail = > > @@ -2238,6 +2547,8 @@ const struct bmp280_chip_info bmp180_chip_info = { > > .read_temp = bmp180_read_temp, > > .read_press = bmp180_read_press, > > .read_calib = bmp180_read_calib, > > + > > + .buffer_handler = bmp180_buffer_handler, > > }; > > EXPORT_SYMBOL_NS(bmp180_chip_info, IIO_BMP280); > > > > @@ -2283,6 +2594,30 @@ static int bmp085_fetch_eoc_irq(struct device *dev, > > return 0; > > } > > > > +static int bmp280_buffer_preenable(struct iio_dev *indio_dev) > > +{ > > + struct bmp280_data *data = iio_priv(indio_dev); > > + > > + pm_runtime_get_sync(data->dev); > > + > > + return 0; > > +} > > + > > +static int bmp280_buffer_postdisable(struct iio_dev *indio_dev) > > +{ > > + struct bmp280_data *data = iio_priv(indio_dev); > > + > > + pm_runtime_mark_last_busy(data->dev); > > + pm_runtime_put_autosuspend(data->dev); > > + > > + return 0; > > +} > > + > > +static const struct iio_buffer_setup_ops bmp280_buffer_setup_ops = { > > + .preenable = bmp280_buffer_preenable, > > + .postdisable = bmp280_buffer_postdisable, > > +}; > > + > > static void bmp280_pm_disable(void *data) > > { > > struct device *dev = data; > > @@ -2329,6 +2664,7 @@ int bmp280_common_probe(struct device *dev, > > /* Apply initial values from chip info structure */ > > indio_dev->channels = chip_info->channels; > > indio_dev->num_channels = chip_info->num_channels; > > + indio_dev->available_scan_masks = chip_info->avail_scan_masks; > > data->oversampling_press = chip_info->oversampling_press_default; > > data->oversampling_humid = chip_info->oversampling_humid_default; > > data->oversampling_temp = chip_info->oversampling_temp_default; > > @@ -2414,6 +2750,14 @@ int bmp280_common_probe(struct device *dev, > > "failed to read calibration coefficients\n"); > > } > > > > + ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev, > > + iio_pollfunc_store_time, > > + data->chip_info->buffer_handler, > > + &bmp280_buffer_setup_ops); > > + if (ret) > > + return dev_err_probe(data->dev, ret, > > + "iio triggered buffer setup failed\n"); > > + > > /* > > * Attempt to grab an optional EOC IRQ - only the BMP085 has this > > * however as it happens, the BMP085 shares the chip ID of BMP180 > > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c > > index 62b4e58104cf..e5abee15950e 100644 > > --- a/drivers/iio/pressure/bmp280-spi.c > > +++ b/drivers/iio/pressure/bmp280-spi.c > > @@ -40,14 +40,10 @@ static int bmp380_regmap_spi_read(void *context, const void *reg, > > size_t reg_size, void *val, size_t val_size) > > { > > struct spi_device *spi = to_spi_device(context); > > - u8 rx_buf[4]; > > + u8 rx_buf[BME280_BURST_READ_BYTES + 1]; > > ssize_t status; > > > > - /* > > - * Maximum number of consecutive bytes read for a temperature or > > - * pressure measurement is 3. > > - */ > > - if (val_size > 3) > > + if (val_size > BME280_BURST_READ_BYTES) > > return -EINVAL; > > > > /* > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > index a3d2cd722760..756c644354c2 100644 > > --- a/drivers/iio/pressure/bmp280.h > > +++ b/drivers/iio/pressure/bmp280.h > > @@ -304,6 +304,16 @@ > > #define BMP280_PRESS_SKIPPED 0x80000 > > #define BMP280_HUMIDITY_SKIPPED 0x8000 > > > > +/* Number of bytes for each value */ > > +#define BMP280_NUM_PRESS_BYTES 3 > > +#define BMP280_NUM_TEMP_BYTES 3 > > +#define BME280_NUM_HUMIDITY_BYTES 2 > > +#define BMP280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ > > + BMP280_NUM_TEMP_BYTES) > > +#define BME280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ > > + BMP280_NUM_TEMP_BYTES + \ > > + BME280_NUM_HUMIDITY_BYTES) > > + > > /* Core exported structs */ > > > > static const char *const bmp280_supply_names[] = { > > @@ -397,13 +407,19 @@ struct bmp280_data { > > */ > > int sampling_freq; > > > > + /* > > + * Data to push to userspace triggered buffer. Up to 3 channels and > > + * s64 timestamp, aligned. > > + */ > > + s32 sensor_data[6] __aligned(8); > > + > > /* > > * DMA (thus cache coherency maintenance) may require the > > * transfer buffers to live in their own cache lines. > > */ > > union { > > /* Sensor data buffer */ > > - u8 buf[3]; > > + u8 buf[BME280_BURST_READ_BYTES]; > > /* Calibration data buffers */ > > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; > > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; > > @@ -425,6 +441,7 @@ struct bmp280_chip_info { > > const struct iio_chan_spec *channels; > > int num_channels; > > unsigned int start_up_time; > > + const unsigned long *avail_scan_masks; > > > > const int *oversampling_temp_avail; > > int num_oversampling_temp_avail; > > @@ -459,6 +476,8 @@ struct bmp280_chip_info { > > int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); > > int (*read_calib)(struct bmp280_data *data); > > int (*preinit)(struct bmp280_data *data); > > + > > + irqreturn_t (*buffer_handler)(int irq, void *p); > > }; > > > > /* Chip infos for each variant */ > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support 2024-06-22 9:40 ` Jonathan Cameron 2024-06-22 12:32 ` Vasileios Amoiridis @ 2024-06-22 14:09 ` Vasileios Amoiridis 2024-06-23 16:26 ` Jonathan Cameron 1 sibling, 1 reply; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-22 14:09 UTC (permalink / raw) To: Jonathan Cameron Cc: Vasileios Amoiridis, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla On Sat, Jun 22, 2024 at 10:40:39AM +0100, Jonathan Cameron wrote: > On Tue, 18 Jun 2024 01:05:40 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their > > temperature, pressure and humidity readings. This facilitates the > > use of burst/bulk reads in order to acquire data faster. The > > approach is different from the one used in oneshot captures. > > > > BMP085 & BMP1xx devices use a completely different measurement > > process that is well defined and is used in their buffer_handler(). > > > > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com > > --- > The sign extend in buffered path doesn't make much sense as we should be > advertising the correct bit depth for the channel and making that a userspace > problem. > > I'd failed to notice you are doing endian conversions just to check > the skipped values. Ideally we'd leave the channels little endian > and include that in the channel spec. > > Hmm. I guess this works and if we have to do the endian conversion > anyway isn't too bad. It does provide slightly wrong information > to userspace though. > > So even with this in place I think these channels should be real_bits 24. > > > > > +static irqreturn_t bmp580_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + s32 adc_temp, adc_press; > > + int ret; > > + > > + guard(mutex)(&data->lock); > > + > > + /* Burst read data registers */ > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > + data->buf, BMP280_BURST_READ_BYTES); With the bulk read here, we have 24 bits of temperature and 24 bits of pressure, so in total 6 bytes. The only way I can see to not do the endian conversion is that I memcpy the first 3 bytes to the data->sensor_data[1], and the last 3 bytes to the data->sensor_data[0]. If you can see any other better way please let me know, otherwise the other solution is to use get_unaligned_24(). Still, we won't do the skipping part. > > + if (ret) { > > + dev_err(data->dev, "failed to burst read sensor data\n"); > > + goto out; > > + } > > + > > + /* Temperature calculations */ > > + adc_temp = get_unaligned_le24(&data->buf[0]); > > + if (adc_temp == BMP580_TEMP_SKIPPED) { > > + dev_err(data->dev, "reading temperature skipped\n"); > > + goto out; > > + } > > + > > + data->sensor_data[1] = sign_extend32(adc_temp, 23); > > the channel type should indicate that it's a 24 bit value. Not our > problem to sign extend. Leave that to userspace. > > > + > > + /* Pressure calculations */ > > + adc_press = get_unaligned_le24(&data->buf[3]); > > + if (adc_press == BMP380_PRESS_SKIPPED) { > > + dev_err(data->dev, "reading pressure skipped\n"); > > + goto out; > > + } > > + > > + data->sensor_data[0] = adc_press; > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_get_time_ns(indio_dev)); > > + > > +out: > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > > static const int bmp580_temp_coeffs[] = { 1000, 16 }; > > @@ -1929,6 +2204,7 @@ const struct bmp280_chip_info bmp580_chip_info = { > > .start_up_time = 2000, > > .channels = bmp380_channels, > > .num_channels = ARRAY_SIZE(bmp380_channels), > > + .avail_scan_masks = bmp280_avail_scan_masks, > > > > .oversampling_temp_avail = bmp580_oversampling_avail, > > .num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail), > > @@ -1955,6 +2231,8 @@ const struct bmp280_chip_info bmp580_chip_info = { > > .read_temp = bmp580_read_temp, > > .read_press = bmp580_read_press, > > .preinit = bmp580_preinit, > > + > > + .buffer_handler = bmp580_buffer_handler, > > }; > > EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); > > > > @@ -2133,7 +2411,7 @@ static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press) > > return ret; > > > > ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_PRESS_BYTES); > > if (ret) { > > dev_err(data->dev, "failed to read pressure\n"); > > return ret; > > @@ -2204,6 +2482,36 @@ static int bmp180_chip_config(struct bmp280_data *data) > > return 0; > > } > > > > +static irqreturn_t bmp180_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + int ret, chan_value; > > + > > + guard(mutex)(&data->lock); > > + > > + ret = bmp180_read_temp(data, &chan_value); > > + if (ret) > > + goto out; > > + > > + data->sensor_data[1] = chan_value; > > + > > + ret = bmp180_read_press(data, &chan_value); > > + if (ret) > > + goto out; > > + > > + data->sensor_data[0] = chan_value; > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_get_time_ns(indio_dev)); > > + > > +out: > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > static const int bmp180_oversampling_temp_avail[] = { 1 }; > > static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; > > static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; > > @@ -2218,6 +2526,7 @@ const struct bmp280_chip_info bmp180_chip_info = { > > .start_up_time = 2000, > > .channels = bmp280_channels, > > .num_channels = ARRAY_SIZE(bmp280_channels), > > + .avail_scan_masks = bmp280_avail_scan_masks, > > > > .oversampling_temp_avail = bmp180_oversampling_temp_avail, > > .num_oversampling_temp_avail = > > @@ -2238,6 +2547,8 @@ const struct bmp280_chip_info bmp180_chip_info = { > > .read_temp = bmp180_read_temp, > > .read_press = bmp180_read_press, > > .read_calib = bmp180_read_calib, > > + > > + .buffer_handler = bmp180_buffer_handler, > > }; > > EXPORT_SYMBOL_NS(bmp180_chip_info, IIO_BMP280); > > > > @@ -2283,6 +2594,30 @@ static int bmp085_fetch_eoc_irq(struct device *dev, > > return 0; > > } > > > > +static int bmp280_buffer_preenable(struct iio_dev *indio_dev) > > +{ > > + struct bmp280_data *data = iio_priv(indio_dev); > > + > > + pm_runtime_get_sync(data->dev); > > + > > + return 0; > > +} > > + > > +static int bmp280_buffer_postdisable(struct iio_dev *indio_dev) > > +{ > > + struct bmp280_data *data = iio_priv(indio_dev); > > + > > + pm_runtime_mark_last_busy(data->dev); > > + pm_runtime_put_autosuspend(data->dev); > > + > > + return 0; > > +} > > + > > +static const struct iio_buffer_setup_ops bmp280_buffer_setup_ops = { > > + .preenable = bmp280_buffer_preenable, > > + .postdisable = bmp280_buffer_postdisable, > > +}; > > + > > static void bmp280_pm_disable(void *data) > > { > > struct device *dev = data; > > @@ -2329,6 +2664,7 @@ int bmp280_common_probe(struct device *dev, > > /* Apply initial values from chip info structure */ > > indio_dev->channels = chip_info->channels; > > indio_dev->num_channels = chip_info->num_channels; > > + indio_dev->available_scan_masks = chip_info->avail_scan_masks; > > data->oversampling_press = chip_info->oversampling_press_default; > > data->oversampling_humid = chip_info->oversampling_humid_default; > > data->oversampling_temp = chip_info->oversampling_temp_default; > > @@ -2414,6 +2750,14 @@ int bmp280_common_probe(struct device *dev, > > "failed to read calibration coefficients\n"); > > } > > > > + ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev, > > + iio_pollfunc_store_time, > > + data->chip_info->buffer_handler, > > + &bmp280_buffer_setup_ops); > > + if (ret) > > + return dev_err_probe(data->dev, ret, > > + "iio triggered buffer setup failed\n"); > > + > > /* > > * Attempt to grab an optional EOC IRQ - only the BMP085 has this > > * however as it happens, the BMP085 shares the chip ID of BMP180 > > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c > > index 62b4e58104cf..e5abee15950e 100644 > > --- a/drivers/iio/pressure/bmp280-spi.c > > +++ b/drivers/iio/pressure/bmp280-spi.c > > @@ -40,14 +40,10 @@ static int bmp380_regmap_spi_read(void *context, const void *reg, > > size_t reg_size, void *val, size_t val_size) > > { > > struct spi_device *spi = to_spi_device(context); > > - u8 rx_buf[4]; > > + u8 rx_buf[BME280_BURST_READ_BYTES + 1]; > > ssize_t status; > > > > - /* > > - * Maximum number of consecutive bytes read for a temperature or > > - * pressure measurement is 3. > > - */ > > - if (val_size > 3) > > + if (val_size > BME280_BURST_READ_BYTES) > > return -EINVAL; > > > > /* > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > index a3d2cd722760..756c644354c2 100644 > > --- a/drivers/iio/pressure/bmp280.h > > +++ b/drivers/iio/pressure/bmp280.h > > @@ -304,6 +304,16 @@ > > #define BMP280_PRESS_SKIPPED 0x80000 > > #define BMP280_HUMIDITY_SKIPPED 0x8000 > > > > +/* Number of bytes for each value */ > > +#define BMP280_NUM_PRESS_BYTES 3 > > +#define BMP280_NUM_TEMP_BYTES 3 > > +#define BME280_NUM_HUMIDITY_BYTES 2 > > +#define BMP280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ > > + BMP280_NUM_TEMP_BYTES) > > +#define BME280_BURST_READ_BYTES (BMP280_NUM_PRESS_BYTES + \ > > + BMP280_NUM_TEMP_BYTES + \ > > + BME280_NUM_HUMIDITY_BYTES) > > + > > /* Core exported structs */ > > > > static const char *const bmp280_supply_names[] = { > > @@ -397,13 +407,19 @@ struct bmp280_data { > > */ > > int sampling_freq; > > > > + /* > > + * Data to push to userspace triggered buffer. Up to 3 channels and > > + * s64 timestamp, aligned. > > + */ > > + s32 sensor_data[6] __aligned(8); > > + > > /* > > * DMA (thus cache coherency maintenance) may require the > > * transfer buffers to live in their own cache lines. > > */ > > union { > > /* Sensor data buffer */ > > - u8 buf[3]; > > + u8 buf[BME280_BURST_READ_BYTES]; > > /* Calibration data buffers */ > > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; > > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; > > @@ -425,6 +441,7 @@ struct bmp280_chip_info { > > const struct iio_chan_spec *channels; > > int num_channels; > > unsigned int start_up_time; > > + const unsigned long *avail_scan_masks; > > > > const int *oversampling_temp_avail; > > int num_oversampling_temp_avail; > > @@ -459,6 +476,8 @@ struct bmp280_chip_info { > > int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity); > > int (*read_calib)(struct bmp280_data *data); > > int (*preinit)(struct bmp280_data *data); > > + > > + irqreturn_t (*buffer_handler)(int irq, void *p); > > }; > > > > /* Chip infos for each variant */ > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support 2024-06-22 14:09 ` Vasileios Amoiridis @ 2024-06-23 16:26 ` Jonathan Cameron 2024-06-23 17:19 ` Vasileios Amoiridis 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2024-06-23 16:26 UTC (permalink / raw) To: Vasileios Amoiridis Cc: lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla On Sat, 22 Jun 2024 16:09:11 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > On Sat, Jun 22, 2024 at 10:40:39AM +0100, Jonathan Cameron wrote: > > On Tue, 18 Jun 2024 01:05:40 +0200 > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their > > > temperature, pressure and humidity readings. This facilitates the > > > use of burst/bulk reads in order to acquire data faster. The > > > approach is different from the one used in oneshot captures. > > > > > > BMP085 & BMP1xx devices use a completely different measurement > > > process that is well defined and is used in their buffer_handler(). > > > > > > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com > > > --- > > The sign extend in buffered path doesn't make much sense as we should be > > advertising the correct bit depth for the channel and making that a userspace > > problem. > > > > I'd failed to notice you are doing endian conversions just to check > > the skipped values. Ideally we'd leave the channels little endian > > and include that in the channel spec. > > > > Hmm. I guess this works and if we have to do the endian conversion > > anyway isn't too bad. It does provide slightly wrong information > > to userspace though. > > > > So even with this in place I think these channels should be real_bits 24. > > > > > > > > > +static irqreturn_t bmp580_buffer_handler(int irq, void *p) > > > +{ > > > + struct iio_poll_func *pf = p; > > > + struct iio_dev *indio_dev = pf->indio_dev; > > > + struct bmp280_data *data = iio_priv(indio_dev); > > > + s32 adc_temp, adc_press; > > > + int ret; > > > + > > > + guard(mutex)(&data->lock); > > > + > > > + /* Burst read data registers */ > > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > > + data->buf, BMP280_BURST_READ_BYTES); > > With the bulk read here, we have 24 bits of temperature and 24 bits > of pressure, so in total 6 bytes. The only way I can see to not do > the endian conversion is that I memcpy the first 3 bytes to the > data->sensor_data[1], and the last 3 bytes to the data->sensor_data[0]. > > If you can see any other better way please let me know, otherwise the > other solution is to use get_unaligned_24(). Still, we won't do the > skipping part. If you return in cpu endian because it's convenient that is fine, but still set the number of realbits to 24 or whatever is appropriate. Or as you say memcpy the 3 bytes. There is probably an arch out there in which that is much more efficient than the endian fun, but I can't be bothered to figure out how ;) Jonathan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support 2024-06-23 16:26 ` Jonathan Cameron @ 2024-06-23 17:19 ` Vasileios Amoiridis 0 siblings, 0 replies; 17+ messages in thread From: Vasileios Amoiridis @ 2024-06-23 17:19 UTC (permalink / raw) To: Jonathan Cameron Cc: Vasileios Amoiridis, lars, andriy.shevchenko, ang.iglesiasg, mazziesaccount, ak, petre.rodan, phil, 579lpy, linus.walleij, semen.protsenko, linux-iio, linux-kernel, Adam Rizkalla On Sun, Jun 23, 2024 at 05:26:15PM +0100, Jonathan Cameron wrote: > On Sat, 22 Jun 2024 16:09:11 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > On Sat, Jun 22, 2024 at 10:40:39AM +0100, Jonathan Cameron wrote: > > > On Tue, 18 Jun 2024 01:05:40 +0200 > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > > > BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their > > > > temperature, pressure and humidity readings. This facilitates the > > > > use of burst/bulk reads in order to acquire data faster. The > > > > approach is different from the one used in oneshot captures. > > > > > > > > BMP085 & BMP1xx devices use a completely different measurement > > > > process that is well defined and is used in their buffer_handler(). > > > > > > > > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com > > > > --- > > > The sign extend in buffered path doesn't make much sense as we should be > > > advertising the correct bit depth for the channel and making that a userspace > > > problem. > > > > > > I'd failed to notice you are doing endian conversions just to check > > > the skipped values. Ideally we'd leave the channels little endian > > > and include that in the channel spec. > > > > > > Hmm. I guess this works and if we have to do the endian conversion > > > anyway isn't too bad. It does provide slightly wrong information > > > to userspace though. > > > > > > So even with this in place I think these channels should be real_bits 24. > > > > > > > > > > > > > +static irqreturn_t bmp580_buffer_handler(int irq, void *p) > > > > +{ > > > > + struct iio_poll_func *pf = p; > > > > + struct iio_dev *indio_dev = pf->indio_dev; > > > > + struct bmp280_data *data = iio_priv(indio_dev); > > > > + s32 adc_temp, adc_press; > > > > + int ret; > > > > + > > > > + guard(mutex)(&data->lock); > > > > + > > > > + /* Burst read data registers */ > > > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > > > + data->buf, BMP280_BURST_READ_BYTES); > > > > With the bulk read here, we have 24 bits of temperature and 24 bits > > of pressure, so in total 6 bytes. The only way I can see to not do > > the endian conversion is that I memcpy the first 3 bytes to the > > data->sensor_data[1], and the last 3 bytes to the data->sensor_data[0]. > > > > If you can see any other better way please let me know, otherwise the > > other solution is to use get_unaligned_24(). Still, we won't do the > > skipping part. > > If you return in cpu endian because it's convenient that is fine, but > still set the number of realbits to 24 or whatever is appropriate. > > Or as you say memcpy the 3 bytes. There is probably an arch out there > in which that is much more efficient than the endian fun, but I > can't be bothered to figure out how ;) > > Jonathan Well, thanks for the advice :) Cheers, Vasilis ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-06-26 13:41 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-17 23:05 [PATCH v8 0/3] Driver cleanup and triggered buffer support Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 1/3] iio: pressure: bmp280: Generalize read_*() functions Vasileios Amoiridis 2024-06-22 9:28 ` Jonathan Cameron 2024-06-22 12:19 ` Vasileios Amoiridis 2024-06-23 16:23 ` Jonathan Cameron 2024-06-23 17:18 ` Vasileios Amoiridis 2024-06-25 20:44 ` Jonathan Cameron 2024-06-26 13:41 ` Vasileios Amoiridis 2024-06-24 4:26 ` Adam Rizkalla 2024-06-26 13:38 ` Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 2/3] iio: pressure: bmp280: Add SCALE, RAW values in channels and refactorize them Vasileios Amoiridis 2024-06-17 23:05 ` [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support Vasileios Amoiridis 2024-06-22 9:40 ` Jonathan Cameron 2024-06-22 12:32 ` Vasileios Amoiridis 2024-06-22 14:09 ` Vasileios Amoiridis 2024-06-23 16:26 ` Jonathan Cameron 2024-06-23 17:19 ` Vasileios Amoiridis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox