All the proposals were implemented, and 2 extra patches were added (Patch 2 and
patch 4) in order to have better logical split between patches.
Changes in v2:
Patch 1: Sorted and removed headers as per request.
Patch 2: *NEW* Patch, adds coefficients for IIO units in the chip_info
structure as per request, remove them from the read_* functions.
Patch 3: Patch 2 of v1. Added RAW values as well, as per request.
Patch 4: *NEW* Remove the temperature reading from inside read_* functions so
the addition of a buffer for userspace is facilitated + make the code much more
intuitive.
Patch 5: Patch 3 of v1. No logical change, only minor typos as per request.
Patch 6: Patch 4 of v1. Previous commits allowed for much cleaner approach as
per request. Dropped filling of extra buffer in the read_* functions.
Patch 4 allows to put extra buffer in the union of the chip_info and fill
the buffer in the buffer_handler function.
[1] https://lore.kernel.org/linux-iio/[email protected]
Vasileios Amoiridis (6):
iio: pressure: BMP280 core driver headers sorting
iio: pressure: Simplify read_* functions
iio: pressure: add SCALE and RAW values for channels
iio: pressure: Simplify and make more clear temperature readings
iio: pressure: Add timestamp and scan_masks for BM280 driver
iio: pressure: Add triggered buffer support for BMP280 driver
drivers/iio/pressure/Kconfig | 2 +
drivers/iio/pressure/bmp280-core.c | 431 +++++++++++++++++++++--------
drivers/iio/pressure/bmp280.h | 18 +-
3 files changed, 326 insertions(+), 125 deletions(-)
--
2.25.1
Add the coefficients for the IIO standard units inside the chip_info
structure.
Remove the calculations with the coefficients for the IIO compatibility
from inside the read_(temp/press/humid) functions and move it to the
read_raw function.
Execute the calculations with the coefficients inside the read_raw
oneshot capture functions.
Also fix raw_* and comp_* values signs.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 150 +++++++++++++----------------
drivers/iio/pressure/bmp280.h | 10 +-
2 files changed, 74 insertions(+), 86 deletions(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 871b2214121b..dfd845acfa22 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -363,8 +363,7 @@ 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 s32 bmp280_read_temp(struct bmp280_data *data)
{
s32 adc_temp, comp_temp;
int ret;
@@ -384,27 +383,17 @@ static int bmp280_read_temp(struct bmp280_data *data,
}
comp_temp = bmp280_compensate_temp(data, adc_temp);
- /*
- * val might be NULL if we're called by the read_press routine,
- * who only cares about the carry over t_fine value.
- */
- if (val) {
- *val = comp_temp * 10;
- return IIO_VAL_INT;
- }
-
- return 0;
+ return comp_temp;
}
-static int bmp280_read_press(struct bmp280_data *data,
- int *val, int *val2)
+static u32 bmp280_read_press(struct bmp280_data *data)
{
u32 comp_press;
s32 adc_press;
int ret;
/* Read and compensate temperature so we get a reading of t_fine. */
- ret = bmp280_read_temp(data, NULL, NULL);
+ ret = bmp280_read_temp(data);
if (ret < 0)
return ret;
@@ -423,20 +412,17 @@ static int bmp280_read_press(struct bmp280_data *data,
}
comp_press = bmp280_compensate_press(data, adc_press);
- *val = comp_press;
- *val2 = 256000;
-
- return IIO_VAL_FRACTIONAL;
+ return comp_press;
}
-static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
+static u32 bmp280_read_humid(struct bmp280_data *data)
{
u32 comp_humidity;
s32 adc_humidity;
int ret;
/* Read and compensate temperature so we get a reading of t_fine. */
- ret = bmp280_read_temp(data, NULL, NULL);
+ ret = bmp280_read_temp(data);
if (ret < 0)
return ret;
@@ -455,9 +441,7 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
}
comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
- *val = comp_humidity * 1000 / 1024;
-
- return IIO_VAL_INT;
+ return comp_humidity;
}
static int bmp280_read_raw(struct iio_dev *indio_dev,
@@ -474,13 +458,27 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
switch (chan->type) {
case IIO_HUMIDITYRELATIVE:
- ret = data->chip_info->read_humid(data, val, val2);
+ ret = data->chip_info->read_humid(data);
+ *val = data->chip_info->humid_coeffs[0] * ret;
+ *val2 = data->chip_info->humid_coeffs[1];
+ ret = IIO_VAL_FRACTIONAL;
break;
case IIO_PRESSURE:
- ret = data->chip_info->read_press(data, val, val2);
+ ret = data->chip_info->read_press(data);
+ *val = data->chip_info->press_coeffs[0] * ret;
+ *val2 = data->chip_info->press_coeffs[1];
+ ret = IIO_VAL_FRACTIONAL;
break;
case IIO_TEMP:
- ret = data->chip_info->read_temp(data, val, val2);
+ ret = data->chip_info->read_temp(data);
+ *val = data->chip_info->temp_coeffs[0] * ret;
+ *val2 = data->chip_info->temp_coeffs[1];
+
+ if (!strcmp(indio_dev->name, "bmp580"))
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ else
+ ret = IIO_VAL_FRACTIONAL;
+
break;
default:
ret = -EINVAL;
@@ -796,6 +794,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,
@@ -824,6 +824,9 @@ 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,
+ .press_coeffs = bmp280_press_coeffs,
+
.chip_config = bmp280_chip_config,
.read_temp = bmp280_read_temp,
.read_press = bmp280_read_press,
@@ -850,6 +853,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,
@@ -872,6 +876,10 @@ const struct bmp280_chip_info bme280_chip_info = {
.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
.oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
+ .temp_coeffs = bmp280_temp_coeffs,
+ .press_coeffs = bmp280_press_coeffs,
+ .humid_coeffs = bme280_humid_coeffs,
+
.chip_config = bme280_chip_config,
.read_temp = bmp280_read_temp,
.read_press = bmp280_read_press,
@@ -997,7 +1005,7 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
return comp_press;
}
-static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
+static s32 bmp380_read_temp(struct bmp280_data *data)
{
s32 comp_temp;
u32 adc_temp;
@@ -1017,27 +1025,17 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
}
comp_temp = bmp380_compensate_temp(data, adc_temp);
- /*
- * Val might be NULL if we're called by the read_press routine,
- * who only cares about the carry over t_fine value.
- */
- if (val) {
- /* IIO reports temperatures in milli Celsius */
- *val = comp_temp * 10;
- return IIO_VAL_INT;
- }
-
- return 0;
+ return comp_temp;
}
-static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
+static u32 bmp380_read_press(struct bmp280_data *data)
{
- s32 comp_press;
- u32 adc_press;
+ u32 comp_press;
+ s32 adc_press;
int ret;
/* Read and compensate for temperature so we get a reading of t_fine */
- ret = bmp380_read_temp(data, NULL, NULL);
+ ret = bmp380_read_temp(data);
if (ret)
return ret;
@@ -1055,11 +1053,7 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
}
comp_press = bmp380_compensate_press(data, adc_press);
- *val = comp_press;
- /* Compensated pressure is in cPa (centipascals) */
- *val2 = 100000;
-
- return IIO_VAL_FRACTIONAL;
+ return comp_press;
}
static int bmp380_read_calib(struct bmp280_data *data)
@@ -1227,6 +1221,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,
@@ -1253,6 +1249,9 @@ 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,
+ .press_coeffs = bmp380_press_coeffs,
+
.chip_config = bmp380_chip_config,
.read_temp = bmp380_read_temp,
.read_press = bmp380_read_press,
@@ -1373,7 +1372,7 @@ 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 s32 bmp580_read_temp(struct bmp280_data *data)
{
s32 raw_temp;
int ret;
@@ -1391,17 +1390,10 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2)
return -EIO;
}
- /*
- * Temperature is returned in Celsius degrees in fractional
- * form down 2^16. We rescale by x1000 to return milli Celsius
- * to respect IIO ABI.
- */
- *val = raw_temp * 1000;
- *val2 = 16;
- return IIO_VAL_FRACTIONAL_LOG2;
+ return raw_temp;
}
-static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
+static u32 bmp580_read_press(struct bmp280_data *data)
{
u32 raw_press;
int ret;
@@ -1418,13 +1410,8 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
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;
+
+ return raw_press;
}
static const int bmp580_odr_table[][2] = {
@@ -1729,6 +1716,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,
@@ -1755,6 +1744,9 @@ 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,
+ .press_coeffs = bmp580_press_coeffs,
+
.chip_config = bmp580_chip_config,
.read_temp = bmp580_read_temp,
.read_press = bmp580_read_press,
@@ -1882,7 +1874,7 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, s32 adc_temp)
return (data->t_fine + 8) >> 4;
}
-static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
+static s32 bmp180_read_temp(struct bmp280_data *data)
{
s32 adc_temp, comp_temp;
int ret;
@@ -1893,16 +1885,7 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
comp_temp = bmp180_compensate_temp(data, adc_temp);
- /*
- * val might be NULL if we're called by the read_press routine,
- * who only cares about the carry over t_fine value.
- */
- if (val) {
- *val = comp_temp * 100;
- return IIO_VAL_INT;
- }
-
- return 0;
+ return comp_temp;
}
static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
@@ -1962,15 +1945,14 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press)
return p + ((x1 + x2 + 3791) >> 4);
}
-static int bmp180_read_press(struct bmp280_data *data,
- int *val, int *val2)
+static u32 bmp180_read_press(struct bmp280_data *data)
{
u32 comp_press;
s32 adc_press;
int ret;
/* Read and compensate temperature so we get a reading of t_fine. */
- ret = bmp180_read_temp(data, NULL, NULL);
+ ret = bmp180_read_temp(data);
if (ret)
return ret;
@@ -1980,10 +1962,7 @@ static int bmp180_read_press(struct bmp280_data *data,
comp_press = bmp180_compensate_press(data, adc_press);
- *val = comp_press;
- *val2 = 1000;
-
- return IIO_VAL_FRACTIONAL;
+ return comp_press;
}
static int bmp180_chip_config(struct bmp280_data *data)
@@ -1994,6 +1973,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,
@@ -2014,6 +1995,9 @@ 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,
+ .press_coeffs = bmp180_press_coeffs,
+
.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 4012387d7956..dde55b556ea2 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -448,10 +448,14 @@ struct bmp280_chip_info {
int num_sampling_freq_avail;
int sampling_freq_default;
+ const int *temp_coeffs;
+ const int *press_coeffs;
+ const int *humid_coeffs;
+
int (*chip_config)(struct bmp280_data *);
- int (*read_temp)(struct bmp280_data *, int *, int *);
- int (*read_press)(struct bmp280_data *, int *, int *);
- int (*read_humid)(struct bmp280_data *, int *, int *);
+ s32 (*read_temp)(struct bmp280_data *);
+ u32 (*read_press)(struct bmp280_data *);
+ u32 (*read_humid)(struct bmp280_data *);
int (*read_calib)(struct bmp280_data *);
int (*preinit)(struct bmp280_data *);
};
--
2.25.1
The scan mask for the BME280 supports humidity measurement needs
to be distinguished from the rest in order for the timestamp to
be able to work. Scan masks are added for different combinations
of measurements. The temperature measurement is always needed for
pressure and humidity measurements.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 116 +++++++++++++++++++++++++----
drivers/iio/pressure/bmp280.h | 1 +
2 files changed, 102 insertions(+), 15 deletions(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 377e90d9e5a2..f2cf9bef522c 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -134,40 +134,91 @@ enum {
BMP380_P11 = 20,
};
+enum bmp280_scan {
+ BMP280_TEMP,
+ BMP280_PRESS,
+ BME280_HUMID,
+};
+
static const struct iio_chan_spec bmp280_channels[] = {
+ {
+ .type = IIO_TEMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 32,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
+ },
{
.type = IIO_PRESSURE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
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[] = {
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 32,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
},
{
- .type = IIO_HUMIDITYRELATIVE,
+ .type = IIO_PRESSURE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 32,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
},
-};
-
-static const struct iio_chan_spec bmp380_channels[] = {
{
- .type = IIO_PRESSURE,
+ .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),
+ .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[] = {
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
@@ -176,16 +227,31 @@ 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 = 's',
+ .realbits = 32,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
},
{
- .type = IIO_HUMIDITYRELATIVE,
+ .type = IIO_PRESSURE,
.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) |
+ .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 = 'u',
+ .realbits = 32,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
},
+ IIO_CHAN_SOFT_TIMESTAMP(2),
};
static int bmp280_read_calib(struct bmp280_data *data)
@@ -829,6 +895,20 @@ 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) | BIT(BMP280_TEMP),
+ 0
+};
+
+static const unsigned long bme280_avail_scan_masks[] = {
+ BIT(BMP280_TEMP),
+ BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+ BIT(BME280_HUMID) | BIT(BMP280_TEMP),
+ BIT(BME280_HUMID) | BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+ 0
+};
+
static int bmp280_chip_config(struct bmp280_data *data)
{
u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
@@ -870,7 +950,8 @@ 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 = 3,
+ .avail_scan_masks = bmp280_avail_scan_masks,
.oversampling_temp_avail = bmp280_oversampling_avail,
.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
@@ -927,8 +1008,9 @@ 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 = 4,
+ .avail_scan_masks = bme280_avail_scan_masks,
.oversampling_temp_avail = bmp280_oversampling_avail,
.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
@@ -1292,7 +1374,8 @@ const struct bmp280_chip_info bmp380_chip_info = {
.regmap_config = &bmp380_regmap_config,
.start_up_time = 2000,
.channels = bmp380_channels,
- .num_channels = 2,
+ .num_channels = 3,
+ .avail_scan_masks = bmp280_avail_scan_masks,
.oversampling_temp_avail = bmp380_oversampling_avail,
.num_oversampling_temp_avail = ARRAY_SIZE(bmp380_oversampling_avail),
@@ -1787,7 +1870,8 @@ 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 = 3,
+ .avail_scan_masks = bmp280_avail_scan_masks,
.oversampling_temp_avail = bmp580_oversampling_avail,
.num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail),
@@ -2039,7 +2123,8 @@ 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 = 3,
+ .avail_scan_masks = bmp280_avail_scan_masks,
.oversampling_temp_avail = bmp180_oversampling_temp_avail,
.num_oversampling_temp_avail =
@@ -2149,6 +2234,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;
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index dde55b556ea2..c8cb7c417dab 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -427,6 +427,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;
--
2.25.1
On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:
In the Subject: ... read_*() functions
> Add the coefficients for the IIO standard units inside the chip_info
> structure.
>
> Remove the calculations with the coefficients for the IIO compatibility
> from inside the read_(temp/press/humid) functions and move it to the
read_{temp,press,humid}()
> read_raw function.
read_raw()
> Execute the calculations with the coefficients inside the read_raw
read_raw()
> oneshot capture functions.
>
> Also fix raw_* and comp_* values signs.
..
> case IIO_TEMP:
> - ret = data->chip_info->read_temp(data, val, val2);
> + ret = data->chip_info->read_temp(data);
> + *val = data->chip_info->temp_coeffs[0] * ret;
> + *val2 = data->chip_info->temp_coeffs[1];
> + if (!strcmp(indio_dev->name, "bmp580"))
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + else
> + ret = IIO_VAL_FRACTIONAL;
I'm wondering if we may replace these strcmp():s by using enum and respective
values in chip_info.
> break;
--
With Best Regards,
Andy Shevchenko
Sort headers in alphabetical order.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index fe8734468ed3..871b2214121b 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -27,20 +27,20 @@
#include <linux/bitops.h>
#include <linux/bitfield.h>
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/nvmem-provider.h>
-#include <linux/regmap.h>
+#include <linux/completion.h>
#include <linux/delay.h>
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
+#include <linux/device.h>
#include <linux/gpio/consumer.h>
-#include <linux/regulator/consumer.h>
#include <linux/interrupt.h>
#include <linux/irq.h> /* For irq_get_irq_data() */
-#include <linux/completion.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
#include <linux/pm_runtime.h>
#include <linux/random.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
#include <asm/unaligned.h>
--
2.25.1
On Wed, Mar 13, 2024 at 06:40:06PM +0100, Vasileios Amoiridis wrote:
> The scan mask for the BME280 supports humidity measurement needs
> to be distinguished from the rest in order for the timestamp to
> be able to work. Scan masks are added for different combinations
> of measurements. The temperature measurement is always needed for
> pressure and humidity measurements.
(Just to make sure if you used --histogram diff algo when preparing the series)
..
> {
> - .type = IIO_HUMIDITYRELATIVE,
> + .type = IIO_PRESSURE,
> .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) |
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
Stray change
> BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> + .scan_index = 1,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + .endianness = IIO_CPU,
> + },
> },
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 13, 2024 at 08:54:47PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:06PM +0100, Vasileios Amoiridis wrote:
> > The scan mask for the BME280 supports humidity measurement needs
> > to be distinguished from the rest in order for the timestamp to
> > be able to work. Scan masks are added for different combinations
> > of measurements. The temperature measurement is always needed for
> > pressure and humidity measurements.
>
> (Just to make sure if you used --histogram diff algo when preparing the series)
>
> ...
>
> > {
> > - .type = IIO_HUMIDITYRELATIVE,
> > + .type = IIO_PRESSURE,
> > .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) |
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>
> Stray change
>
I didn't notice that, and the checkpatch.pl didn't actually say something,
thanks for pointing out.
> > BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> > + .scan_index = 1,
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 32,
> > + .storagebits = 32,
> > + .endianness = IIO_CPU,
> > + },
> > },
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best regards,
Vasilis Amoiridis
On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:
>
> In the Subject: ... read_*() functions
>
> > Add the coefficients for the IIO standard units inside the chip_info
> > structure.
> >
> > Remove the calculations with the coefficients for the IIO compatibility
> > from inside the read_(temp/press/humid) functions and move it to the
>
> read_{temp,press,humid}()
>
> > read_raw function.
>
> read_raw()
>
> > Execute the calculations with the coefficients inside the read_raw
>
> read_raw()
>
> > oneshot capture functions.
> >
> > Also fix raw_* and comp_* values signs.
>
Thank you very much for pointing these out, I should have thought it.
> ...
>
> > case IIO_TEMP:
> > - ret = data->chip_info->read_temp(data, val, val2);
> > + ret = data->chip_info->read_temp(data);
> > + *val = data->chip_info->temp_coeffs[0] * ret;
> > + *val2 = data->chip_info->temp_coeffs[1];
>
> > + if (!strcmp(indio_dev->name, "bmp580"))
> > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > + else
> > + ret = IIO_VAL_FRACTIONAL;
>
> I'm wondering if we may replace these strcmp():s by using enum and respective
> values in chip_info.
>
The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
not unique for the respective BMPxxx device. You mean to add a new variable
that could store some enum values that would be the actual chip_info IDs? Like:
enum chip_info_ids = {
BMP085,
BMP180,
...
BMP580,
};
and later for every chip_info struct to use it like this:
const struct bmp280_chip_info bmpxxx_chip_info = {
...
.chip_info_id = BIT(BMPxxx),
...
}
And in the read_raw() function to just use the test_bit() function in the same
way that is done with the test_bit() and avail_scan_mask to test for the
enabled channels?
Best regards,
Vasilis Amoiridis
> > break;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, Mar 13, 2024 at 08:22:45PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:
..
> > > case IIO_TEMP:
> > > - ret = data->chip_info->read_temp(data, val, val2);
> > > + ret = data->chip_info->read_temp(data);
> > > + *val = data->chip_info->temp_coeffs[0] * ret;
> > > + *val2 = data->chip_info->temp_coeffs[1];
> >
> > > + if (!strcmp(indio_dev->name, "bmp580"))
> > > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > > + else
> > > + ret = IIO_VAL_FRACTIONAL;
> >
> > I'm wondering if we may replace these strcmp():s by using enum and respective
> > values in chip_info.
>
> The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
> not unique for the respective BMPxxx device. You mean to add a new variable
> that could store some enum values that would be the actual chip_info IDs? Like:
>
> enum chip_info_ids = {
> BMP085,
> BMP180,
> ...
> BMP580,
> };
>
> and later for every chip_info struct to use it like this:
>
> const struct bmp280_chip_info bmpxxx_chip_info = {
> ...
> .chip_info_id = BIT(BMPxxx),
No BIT(), but yes.
> ...
> }
>
> And in the read_raw() function to just use the test_bit() function in the same
> way that is done with the test_bit() and avail_scan_mask to test for the
> enabled channels?
If BIT() is more suitable, than also yes.
--
With Best Regards,
Andy Shevchenko
On Wed, 13 Mar 2024 21:28:47 +0200
Andy Shevchenko <[email protected]> wrote:
> On Wed, Mar 13, 2024 at 08:22:45PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:
>
> ...
>
> > > > case IIO_TEMP:
> > > > - ret = data->chip_info->read_temp(data, val, val2);
> > > > + ret = data->chip_info->read_temp(data);
> > > > + *val = data->chip_info->temp_coeffs[0] * ret;
> > > > + *val2 = data->chip_info->temp_coeffs[1];
> > >
> > > > + if (!strcmp(indio_dev->name, "bmp580"))
> > > > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > > > + else
> > > > + ret = IIO_VAL_FRACTIONAL;
> > >
> > > I'm wondering if we may replace these strcmp():s by using enum and respective
> > > values in chip_info.
> >
> > The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
> > not unique for the respective BMPxxx device. You mean to add a new variable
> > that could store some enum values that would be the actual chip_info IDs? Like:
> >
> > enum chip_info_ids = {
> > BMP085,
> > BMP180,
> > ...
> > BMP580,
> > };
> >
> > and later for every chip_info struct to use it like this:
> >
> > const struct bmp280_chip_info bmpxxx_chip_info = {
> > ...
> > .chip_info_id = BIT(BMPxxx),
>
> No BIT(), but yes.
>
Better to use something more meaningful such as just storing the
type you need to return alongside the values it refers to.
temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2 / IIO_VAL_FRACTIONAL as appropriate.
That way the data and what it is are found in one simple place.
Basic rule of thumb is that if there is a string comparison to identify
what to do in a driver (other than deep in the fw handling code) then
that is a bad design. Likewise any matching on an enum value that correlates
with that chip ID. I want to see all the difference between chips in one place,
not scattered through the code.
Jonathan
> > ...
> > }
> >
> > And in the read_raw() function to just use the test_bit() function in the same
> > way that is done with the test_bit() and avail_scan_mask to test for the
> > enabled channels?
>
> If BIT() is more suitable, than also yes.
>
On Wed, 13 Mar 2024 18:40:03 +0100
Vasileios Amoiridis <[email protected]> wrote:
> Add the coefficients for the IIO standard units inside the chip_info
> structure.
>
> Remove the calculations with the coefficients for the IIO compatibility
> from inside the read_(temp/press/humid) functions and move it to the
> read_raw function.
>
> Execute the calculations with the coefficients inside the read_raw
> oneshot capture functions.
>
> Also fix raw_* and comp_* values signs.
That needs some more explanation. Is this a fix that needs backporting?
>
> Signed-off-by: Vasileios Amoiridis <[email protected]>
> ---
> drivers/iio/pressure/bmp280-core.c | 150 +++++++++++++----------------
> drivers/iio/pressure/bmp280.h | 10 +-
> 2 files changed, 74 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 871b2214121b..dfd845acfa22 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -363,8 +363,7 @@ 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 s32 bmp280_read_temp(struct bmp280_data *data)
It's returning error codes through here. If you need to force a specific type,
don't use the return code. Pass in a variable to put it in.
static int bm280_read_temp(struct bmp280_data *data, u32 *result)
> {
> s32 adc_temp, comp_temp;
> int ret;
> @@ -384,27 +383,17 @@ static int bmp280_read_temp(struct bmp280_data *data,
> }
> comp_temp = bmp280_compensate_temp(data, adc_temp);
>
> - /*
> - * val might be NULL if we're called by the read_press routine,
> - * who only cares about the carry over t_fine value.
> - */
> - if (val) {
> - *val = comp_temp * 10;
> - return IIO_VAL_INT;
> - }
> -
> - return 0;
> + return comp_temp;
> }
>
> -static int bmp280_read_press(struct bmp280_data *data,
> - int *val, int *val2)
> +static u32 bmp280_read_press(struct bmp280_data *data)
> {
> u32 comp_press;
> s32 adc_press;
> int ret;
>
> /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp280_read_temp(data, NULL, NULL);
> + ret = bmp280_read_temp(data);
> if (ret < 0)
> return ret;
>
> @@ -423,20 +412,17 @@ static int bmp280_read_press(struct bmp280_data *data,
> }
> comp_press = bmp280_compensate_press(data, adc_press);
>
> - *val = comp_press;
> - *val2 = 256000;
> -
> - return IIO_VAL_FRACTIONAL;
> + return comp_press;
> }
>
> -static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> +static u32 bmp280_read_humid(struct bmp280_data *data)
> {
> u32 comp_humidity;
> s32 adc_humidity;
> int ret;
>
> /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp280_read_temp(data, NULL, NULL);
> + ret = bmp280_read_temp(data);
> if (ret < 0)
> return ret;
>
> @@ -455,9 +441,7 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> }
> comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
>
> - *val = comp_humidity * 1000 / 1024;
> -
> - return IIO_VAL_INT;
> + return comp_humidity;
> }
>
> static int bmp280_read_raw(struct iio_dev *indio_dev,
> @@ -474,13 +458,27 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_HUMIDITYRELATIVE:
> - ret = data->chip_info->read_humid(data, val, val2);
> + ret = data->chip_info->read_humid(data);
You can still get an error code back from this and you must handle it.
Previously it was fine because both errors and IIO_VAL_ were returned by
the callback so error returns were handled, now they aren't.
> + *val = data->chip_info->humid_coeffs[0] * ret;
> + *val2 = data->chip_info->humid_coeffs[1];
> + ret = IIO_VAL_FRACTIONAL;
> break;
Same applies in all the other cases.
Jonathan
On Thu, Mar 14, 2024 at 02:32:31PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 21:28:47 +0200
> Andy Shevchenko <[email protected]> wrote:
>
> > On Wed, Mar 13, 2024 at 08:22:45PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:
> >
> > ...
> >
> > > > > case IIO_TEMP:
> > > > > - ret = data->chip_info->read_temp(data, val, val2);
> > > > > + ret = data->chip_info->read_temp(data);
> > > > > + *val = data->chip_info->temp_coeffs[0] * ret;
> > > > > + *val2 = data->chip_info->temp_coeffs[1];
> > > >
> > > > > + if (!strcmp(indio_dev->name, "bmp580"))
> > > > > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > > > > + else
> > > > > + ret = IIO_VAL_FRACTIONAL;
> > > >
> > > > I'm wondering if we may replace these strcmp():s by using enum and respective
> > > > values in chip_info.
> > >
> > > The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
> > > not unique for the respective BMPxxx device. You mean to add a new variable
> > > that could store some enum values that would be the actual chip_info IDs? Like:
> > >
> > > enum chip_info_ids = {
> > > BMP085,
> > > BMP180,
> > > ...
> > > BMP580,
> > > };
> > >
> > > and later for every chip_info struct to use it like this:
> > >
> > > const struct bmp280_chip_info bmpxxx_chip_info = {
> > > ...
> > > .chip_info_id = BIT(BMPxxx),
> >
> > No BIT(), but yes.
> >
> Better to use something more meaningful such as just storing the
> type you need to return alongside the values it refers to.
> temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2 / IIO_VAL_FRACTIONAL as appropriate.
> That way the data and what it is are found in one simple place.
>
> Basic rule of thumb is that if there is a string comparison to identify
> what to do in a driver (other than deep in the fw handling code) then
> that is a bad design. Likewise any matching on an enum value that correlates
> with that chip ID. I want to see all the difference between chips in one place,
> not scattered through the code.
>
> Jonathan
>
Yes, sounds totally right. I was just hesitating to add new variables in the
chip_info structure (as you probably noticed in the next commits as well).
Best regards,
Vasilis
>
> > > ...
> > > }
> > >
> > > And in the read_raw() function to just use the test_bit() function in the same
> > > way that is done with the test_bit() and avail_scan_mask to test for the
> > > enabled channels?
> >
> > If BIT() is more suitable, than also yes.
> >
>