This patch series aims to add triggered buffer support in the BMP280 driver.
The patches have been separated in a way that a single functionality is added
with each commit. The 1st commit is independent but the others not. Commit
no.3 needs commit no.2 and commit no.4 needs both commit no.3 and commit no.2.
More details below:
PATCH 1/4: Sort the headers of the bmp280-core.c file before adding new ones.
PATCH 2/4: The scale value for every channel is needed in order to be able to
calculate the final value in userspace. Every measurement from every device
requires a different scaling in order to apply to the IIO measurement unit
standards.
PATCH 3/4: This commit adds a channel for a soft timestamp. The indexing of
temperature and pressure channels is changed and temperature comes first. This
is because the temperature measurement is always needed for a measurement so
it is better to have it as first for the available_scan_masks. The values have
already the CPU endiannes and are already compensated. As mentioned before,
only the scale value to convert them to IIO measurement unit standards is
missing.
PATCH 4/4: This commit is adding the actual triggered buffer. An extra buffer
is added to hold the values of the measurements. This buffer is not inside the
union but rather an external buffer. The reasons for that are presented below:
i) The sensor is built in a way that first you read the raw values out of the
sensor, then you have to compensate those values in software and then you have
to convert them in IIO measurement units. This means that the values of the
data->buf (which is in the DMA safe union) cannot be used directly to push data
to userspace because even though we can have the SCALE value to convert to IIO
measurement units, we still need to compensate first. Only the latest version
of the BMP58x sensor returns directly compensated values.
ii) In order to have a pressure or a humidity measurement, a temperature
measurement needs to happen first so the t_fine variable is filled/updated.
The read_press() and read_humid() functions contain the read_temp() measurement
inside their bodies. This means that if we use an extra buffer inside the DMA
safe union in order to push data to userspace, the first 3 bytes of that buffer
will always be overwritten by a read_press() or read_humid() operation.
In order to overcome the above 2 problems the following things need to be done:
a) Remove the read_temp() function from the inside bodies of read_press/humid()
and call it before you call them.
b) Modify all the read_temp/press/humid() functions and instead of returning
the IIO measurement unit (with *val, *val2, and return IIO value) just return
the compensated value so it can be used with the SCALE value in userspace for
buffer reads and for oneshot captures just do the extra calculations for *val,
*val2 and return IIO value in the bmp280_read_raw() body.
If the solution that I have already sent in the commits is acceptable it's OK.
If it is necessary to have this iio_buffer structure that I used, inside the
union which is also DMA safe then I can immediately implement the a) and b)
solutions and resend the patches for discussion.
Vasileios Amoiridis (4):
iio: pressure: BMP280 core driver headers sorting
iio: pressure: Add scale value for channels
iio: pressure: Add timestamp and scan_masks for BMP280 driver
iio: pressure: Add triggered buffer support for BMP280 driver
drivers/iio/pressure/Kconfig | 2 +
drivers/iio/pressure/bmp280-core.c | 352 ++++++++++++++++++++++++-----
drivers/iio/pressure/bmp280.h | 8 +
3 files changed, 311 insertions(+), 51 deletions(-)
--
2.25.1
Sort headers in alphabetical order.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index fe8734468ed3..29a8b7195076 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/device.h>
+#include <linux/gpio/consumer.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.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 <asm/unaligned.h>
--
2.25.1
Add extra IIO_CHAN_INFO_SCALE in order to be able to have the scales
for the values in userspace. Can be used for triggered buffers.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 70 ++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 29a8b7195076..acdf6138d317 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -138,16 +138,19 @@ static const struct iio_chan_spec bmp280_channels[] = {
{
.type = IIO_PRESSURE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
},
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
},
{
.type = IIO_HUMIDITYRELATIVE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
},
};
@@ -156,6 +159,7 @@ static const struct iio_chan_spec bmp380_channels[] = {
{
.type = IIO_PRESSURE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ 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),
@@ -163,6 +167,7 @@ static const struct iio_chan_spec bmp380_channels[] = {
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ 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),
@@ -170,6 +175,7 @@ static const struct iio_chan_spec bmp380_channels[] = {
{
.type = IIO_HUMIDITYRELATIVE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ 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),
@@ -487,6 +493,70 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
break;
}
break;
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_HUMIDITYRELATIVE:
+ if (!strcmp(indio_dev->name, "bme280")) {
+ *val = 1000;
+ *val2 = 1024;
+ ret = IIO_VAL_FRACTIONAL;
+ } else {
+ ret = -EINVAL;
+ }
+ break;
+ case IIO_PRESSURE:
+ if ((!strcmp(indio_dev->name, "bmp085")) ||
+ (!strcmp(indio_dev->name, "bmp180")) ||
+ (!strcmp(indio_dev->name, "bmp181"))) {
+ *val = 1;
+ *val2 = 1000;
+ ret = IIO_VAL_FRACTIONAL;
+ } else if ((!strcmp(indio_dev->name, "bmp280")) ||
+ (!strcmp(indio_dev->name, "bme280"))) {
+ *val = 1;
+ *val2 = 256000;
+ ret = IIO_VAL_FRACTIONAL;
+ } else if (!strcmp(indio_dev->name, "bmp380")) {
+ *val = 1;
+ *val2 = 100000;
+ ret = IIO_VAL_FRACTIONAL;
+ } else if (!strcmp(indio_dev->name, "bmp580")) {
+ *val = 1;
+ *val2 = 64000;
+ ret = IIO_VAL_FRACTIONAL;
+ } else {
+ ret = -EINVAL;
+ }
+ break;
+ case IIO_TEMP:
+ if ((!strcmp(indio_dev->name, "bmp085")) ||
+ (!strcmp(indio_dev->name, "bmp180")) ||
+ (!strcmp(indio_dev->name, "bmp181"))) {
+ *val = 100;
+ *val2 = 1;
+ ret = IIO_VAL_FRACTIONAL;
+ } else if ((!strcmp(indio_dev->name, "bmp280")) ||
+ (!strcmp(indio_dev->name, "bme280"))) {
+ *val = 10;
+ *val2 = 1;
+ ret = IIO_VAL_FRACTIONAL;
+ } else if (!strcmp(indio_dev->name, "bmp380")) {
+ *val = 10;
+ *val2 = 1;
+ ret = IIO_VAL_FRACTIONAL;
+ } else if (!strcmp(indio_dev->name, "bmp580")) {
+ *val = 1000;
+ *val2 = 16;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ } else {
+ ret = -EINVAL;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
switch (chan->type) {
case IIO_HUMIDITYRELATIVE:
--
2.25.1
The scan mask for the BME280 device which contains humidity
measurement needs to become different in order for the timestamp
to be able to work. Scan masks are added for different combinations
of measurements. The temperature measurement is needed for either
pressure or humidity measurements.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 113 +++++++++++++++++++++++++----
drivers/iio/pressure/bmp280.h | 1 +
2 files changed, 100 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index acdf6138d317..3bdf6002983f 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -134,36 +134,86 @@ 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_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_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 'u',
+ .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_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_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_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) |
@@ -171,15 +221,30 @@ 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_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 = 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)
@@ -835,6 +900,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) |
@@ -874,7 +953,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 +1007,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),
@@ -1305,7 +1386,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),
@@ -1807,7 +1889,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),
@@ -2072,7 +2155,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 =
@@ -2179,6 +2263,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 4012387d7956..d77402cb3121 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -426,6 +426,7 @@ struct bmp280_chip_info {
const struct iio_chan_spec *channels;
int num_channels;
+ const unsigned long *avail_scan_masks;
unsigned int start_up_time;
const int *oversampling_temp_avail;
--
2.25.1
Add a buffer struct that will hold the values of the measurements
and will be pushed to userspace. Modify all read_* functions in order
to just read and compensate the data without though converting to the
required IIO measurement units which are used for the oneshot captures.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/Kconfig | 2 +
drivers/iio/pressure/bmp280-core.c | 155 +++++++++++++++++++++++------
drivers/iio/pressure/bmp280.h | 7 ++
3 files changed, 134 insertions(+), 30 deletions(-)
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 79adfd059c3a..5145b94b4679 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 3bdf6002983f..3b1a159e57ea 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -31,8 +31,12 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/gpio/consumer.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
#include <linux/interrupt.h>
#include <linux/irq.h> /* For irq_get_irq_data() */
#include <linux/module.h>
@@ -457,13 +461,16 @@ static int bmp280_read_temp(struct bmp280_data *data,
/*
* val might be NULL if we're called by the read_press routine,
- * who only cares about the carry over t_fine value.
+ * who only cares about the carry over t_fine value or if we're called
+ * by the buffer handler function.
*/
if (val) {
*val = comp_temp * 10;
return IIO_VAL_INT;
}
+ data->iio_buffer.temperature = comp_temp;
+
return 0;
}
@@ -494,10 +501,16 @@ static int bmp280_read_press(struct bmp280_data *data,
}
comp_press = bmp280_compensate_press(data, adc_press);
- *val = comp_press;
- *val2 = 256000;
+ /* val might be NULL if we're called by the buffer handler */
+ if (val) {
+ *val = comp_press;
+ *val2 = 256000;
+ return IIO_VAL_FRACTIONAL;
+ }
+
+ data->iio_buffer.pressure = comp_press;
- return IIO_VAL_FRACTIONAL;
+ return 0;
}
static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
@@ -526,9 +539,15 @@ 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;
+ /* val might be NULL if we're called by the buffer handler */
+ if (val) {
+ *val = comp_humidity * 1000 / 1024;
+ return IIO_VAL_INT;
+ }
- return IIO_VAL_INT;
+ data->iio_buffer.humidity = comp_humidity;
+
+ return 0;
}
static int bmp280_read_raw(struct iio_dev *indio_dev,
@@ -1170,7 +1189,8 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
/*
* Val might be NULL if we're called by the read_press routine,
- * who only cares about the carry over t_fine value.
+ * who only cares about the carry over t_fine value or if we're called
+ * by the buffer handler.
*/
if (val) {
/* IIO reports temperatures in milli Celsius */
@@ -1178,6 +1198,8 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
return IIO_VAL_INT;
}
+ data->iio_buffer.temperature = comp_temp;
+
return 0;
}
@@ -1206,11 +1228,17 @@ 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;
+ /* val might be NULL if we're called by the buffer handler */
+ if (val) {
+ *val = comp_press;
+ /* Compensated pressure is in cPa (centipascals) */
+ *val2 = 100000;
+ return IIO_VAL_FRACTIONAL;
+ }
+
+ data->iio_buffer.pressure = comp_press;
- return IIO_VAL_FRACTIONAL;
+ return 0;
}
static int bmp380_read_calib(struct bmp280_data *data)
@@ -1543,14 +1571,21 @@ 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;
+ /* val might be NULL if we're called by the buffer handler */
+ if (val) {
+ /*
+ * 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;
+ }
+
+ data->iio_buffer.temperature = raw_temp;
+
+ return 0;
}
static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
@@ -1570,13 +1605,21 @@ 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;
+
+ /* val might be NULL if we're called by the buffer handler */
+ if (val) {
+ /*
+ * 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;
+ }
+
+ data->iio_buffer.pressure = raw_press;
+
+ return 0;
}
static const int bmp580_odr_table[][2] = {
@@ -2048,13 +2091,16 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
/*
* val might be NULL if we're called by the read_press routine,
- * who only cares about the carry over t_fine value.
+ * who only cares about the carry over t_fine value or if we're called
+ * by the buffer handler.
*/
if (val) {
*val = comp_temp * 100;
return IIO_VAL_INT;
}
+ data->iio_buffer.temperature = comp_temp;
+
return 0;
}
@@ -2133,10 +2179,16 @@ static int bmp180_read_press(struct bmp280_data *data,
comp_press = bmp180_compensate_press(data, adc_press);
- *val = comp_press;
- *val2 = 1000;
+ /* val might be NULL if we're called by the buffer handler */
+ if (val) {
+ *val = comp_press;
+ *val2 = 1000;
+ return IIO_VAL_FRACTIONAL;
+ }
+
+ data->iio_buffer.pressure = comp_press;
- return IIO_VAL_FRACTIONAL;
+ return 0;
}
static int bmp180_chip_config(struct bmp280_data *data)
@@ -2217,6 +2269,43 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
return 0;
}
+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);
+ int ret;
+
+ mutex_lock(&data->lock);
+
+ if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
+ ret = data->chip_info->read_temp(data, NULL, NULL);
+ if (ret < 0)
+ goto done;
+ }
+
+ if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
+ ret = data->chip_info->read_press(data, NULL, NULL);
+ if (ret < 0)
+ goto done;
+ }
+
+ if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
+ ret = data->chip_info->read_humid(data, NULL, NULL);
+ if (ret < 0)
+ goto done;
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buffer,
+ pf->timestamp);
+
+done:
+ mutex_unlock(&data->lock);
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+
+}
static void bmp280_pm_disable(void *data)
{
struct device *dev = data;
@@ -2358,6 +2447,12 @@ int bmp280_common_probe(struct device *dev,
return ret;
}
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ iio_pollfunc_store_time,
+ &bmp280_buffer_handler, NULL);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "iio triggered buffer setup failed\n");
/* Enable runtime PM */
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index d77402cb3121..d5c0451ebf68 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -400,6 +400,13 @@ struct bmp280_data {
*/
s32 t_fine;
+ /* IIO sysfs buffer */
+ struct {
+ s32 temperature;
+ u32 pressure;
+ u32 humidity;
+ s64 timestamp;
+ } iio_buffer;
/*
* DMA (thus cache coherency maintenance) may require the
* transfer buffers to live in their own cache lines.
--
2.25.1
On Sun, Mar 03, 2024 at 05:52:57PM +0100, Vasileios Amoiridis wrote:
> Sort headers in alphabetical order.
..
> #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/device.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
(see below)
> -#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>
While at it, please, move iio/* group to be here (and surrounded by
blank lines).
> #include <asm/unaligned.h>
--
With Best Regards,
Andy Shevchenko
On Sun, Mar 03, 2024 at 05:52:58PM +0100, Vasileios Amoiridis wrote:
> Add extra IIO_CHAN_INFO_SCALE in order to be able to have the scales
> for the values in userspace. Can be used for triggered buffers.
..
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_HUMIDITYRELATIVE:
> + if (!strcmp(indio_dev->name, "bme280")) {
> + *val = 1000;
> + *val2 = 1024;
> + ret = IIO_VAL_FRACTIONAL;
> + } else {
> + ret = -EINVAL;
> + }
No, just make these int arrays part of chip_info, then
case IIO_HUMIDITYRELATIVE:
if (chip_info->hrel) {
*val = chip_info->hrel[0];
*val2 = chip_info->hrel[1];
ret = IIO_VAL_FRACTIONAL;
} else {
ret = -EINVAL;
}
> + break;
> + case IIO_PRESSURE:
> + if ((!strcmp(indio_dev->name, "bmp085")) ||
> + (!strcmp(indio_dev->name, "bmp180")) ||
> + (!strcmp(indio_dev->name, "bmp181"))) {
> + *val = 1;
> + *val2 = 1000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if ((!strcmp(indio_dev->name, "bmp280")) ||
> + (!strcmp(indio_dev->name, "bme280"))) {
> + *val = 1;
> + *val2 = 256000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp380")) {
> + *val = 1;
> + *val2 = 100000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp580")) {
> + *val = 1;
> + *val2 = 64000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else {
> + ret = -EINVAL;
> + }
> + break;
Ditto.
> + case IIO_TEMP:
> + if ((!strcmp(indio_dev->name, "bmp085")) ||
> + (!strcmp(indio_dev->name, "bmp180")) ||
> + (!strcmp(indio_dev->name, "bmp181"))) {
> + *val = 100;
> + *val2 = 1;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if ((!strcmp(indio_dev->name, "bmp280")) ||
> + (!strcmp(indio_dev->name, "bme280"))) {
> + *val = 10;
> + *val2 = 1;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp380")) {
> + *val = 10;
> + *val2 = 1;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp580")) {
> + *val = 1000;
> + *val2 = 16;
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + } else {
> + ret = -EINVAL;
> + }
> + break;
Ditto.
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
--
With Best Regards,
Andy Shevchenko
On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:
> The scan mask for the BME280 device which contains humidity
> measurement needs to become different in order for the timestamp
> to be able to work. Scan masks are added for different combinations
> of measurements. The temperature measurement is needed for either
> pressure or humidity measurements.
..
> +enum bmp280_scan {
> + BMP280_TEMP,
> + BMP280_PRESS,
> + BME280_HUMID,
> +};
Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
directly (or in some way)?
..
> +static const unsigned long bmp280_avail_scan_masks[] = {
> + BIT(BMP280_TEMP),
> + BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> + 0,
No comma for the terminator line.
> +};
> +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,
Ditto.
> +};
..
> const struct iio_chan_spec *channels;
> int num_channels;
> + const unsigned long *avail_scan_masks;
> unsigned int start_up_time;
Please, run `pahole` every time you are changing data structure layout.
Here you efficiently wasted 8 bytes of memory AFAICS.
--
With Best Regards,
Andy Shevchenko
On Sun, Mar 03, 2024 at 05:53:00PM +0100, Vasileios Amoiridis wrote:
> Add a buffer struct that will hold the values of the measurements
> and will be pushed to userspace. Modify all read_* functions in order
> to just read and compensate the data without though converting to the
> required IIO measurement units which are used for the oneshot captures.
> +#include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
Yes, taking into account the comment against patch 1, this will become the part
of iio/* group.
..
> + /* val might be NULL if we're called by the buffer handler */
> + if (val) {
> + *val = comp_press;
> + /* Compensated pressure is in cPa (centipascals) */
> + *val2 = 100000;
Here and everywhere else where it makes sense
*val2 = CENTI * 1000; // (What is 1000 here?)
from units.h?
> + return IIO_VAL_FRACTIONAL;
> + }
..
> + struct {
> + s32 temperature;
> + u32 pressure;
> + u32 humidity;
> + s64 timestamp;
Shouldn't this be aligned properly?
> + } iio_buffer;
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 04, 2024 at 01:47:47PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:
> > The scan mask for the BME280 device which contains humidity
> > measurement needs to become different in order for the timestamp
> > to be able to work. Scan masks are added for different combinations
> > of measurements. The temperature measurement is needed for either
> > pressure or humidity measurements.
>
> ...
>
> > +enum bmp280_scan {
> > + BMP280_TEMP,
> > + BMP280_PRESS,
> > + BME280_HUMID,
> > +};
>
> Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
> directly (or in some way)?
>
What do you mean exactly by copying the IIO ones? These values are used as
indexes to enable channels in the avail_scan_masks below.
> ...
>
> > +static const unsigned long bmp280_avail_scan_masks[] = {
> > + BIT(BMP280_TEMP),
> > + BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> > + 0,
>
> No comma for the terminator line.
>
> > +};
>
> > +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,
>
> Ditto.
>
> > +};
>
> ...
>
> > const struct iio_chan_spec *channels;
> > int num_channels;
> > + const unsigned long *avail_scan_masks;
> > unsigned int start_up_time;
>
> Please, run `pahole` every time you are changing data structure layout.
> Here you efficiently wasted 8 bytes of memory AFAICS.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Mar 04, 2024 at 07:50:17PM +0100, Vasileios Amoiridis wrote:
> On Mon, Mar 04, 2024 at 01:47:47PM +0200, Andy Shevchenko wrote:
> > On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:
..
> > > +enum bmp280_scan {
> > > + BMP280_TEMP,
> > > + BMP280_PRESS,
> > > + BME280_HUMID,
> > > +};
> >
> > Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
> > directly (or in some way)?
>
> What do you mean exactly by copying the IIO ones? These values are used as
> indexes to enable channels in the avail_scan_masks below.
Yeah, I have now an answer to my question. The IIO drivers provide these lists
as mapping between available channels (as starting from 0) and real channels
as per IIO specifications (which can be anything, although limited currently
by 40 or so).
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 04, 2024 at 01:52:05PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 03, 2024 at 05:53:00PM +0100, Vasileios Amoiridis wrote:
> > Add a buffer struct that will hold the values of the measurements
> > and will be pushed to userspace. Modify all read_* functions in order
> > to just read and compensate the data without though converting to the
> > required IIO measurement units which are used for the oneshot captures.
>
> > +#include <linux/iio/buffer.h>
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
>
> Yes, taking into account the comment against patch 1, this will become the part
> of iio/* group.
>
> ...
>
> > + /* val might be NULL if we're called by the buffer handler */
> > + if (val) {
> > + *val = comp_press;
> > + /* Compensated pressure is in cPa (centipascals) */
> > + *val2 = 100000;
>
> Here and everywhere else where it makes sense
>
> *val2 = CENTI * 1000; // (What is 1000 here?)
>
> from units.h?
>
I didn't change these values, the addition here is that I put them under an
if statement that checks if we were called by the buffer handler or by the
oneshot capture read_raw function. The point is that every sensor provides
values that need different scaling in order to have the IIO standard
measurement units. In the above code I guess since 1kPa=100000cPa that's
why *val2=100000.
The *val and *val2 values could be moved to the read_raw function as it will
already happen for the IIO_CHAN_INFO_SCALE values from chip_info arrays as
you proposed in Patch No.2. This would require though that all the functions
like this one you commented would need to change. Is that something that you
think as better?
> > + return IIO_VAL_FRACTIONAL;
> > + }
>
> ...
>
> > + struct {
> > + s32 temperature;
> > + u32 pressure;
> > + u32 humidity;
>
> > + s64 timestamp;
>
> Shouldn't this be aligned properly?
>
I saw that in some drivers it was added and in some it was not. What is the
difference of aligning just the timestamp of the kernel?
> > + } iio_buffer;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Mar 04, 2024 at 08:08:38PM +0100, Vasileios Amoiridis wrote:
> On Mon, Mar 04, 2024 at 01:52:05PM +0200, Andy Shevchenko wrote:
> > On Sun, Mar 03, 2024 at 05:53:00PM +0100, Vasileios Amoiridis wrote:
..
> > > + struct {
> > > + s32 temperature;
> > > + u32 pressure;
> > > + u32 humidity;
> >
> > > + s64 timestamp;
> >
> > Shouldn't this be aligned properly?
>
> I saw that in some drivers it was added and in some it was not. What is the
> difference of aligning just the timestamp of the kernel?
You can count yourself. With provided structure as above there is a high
probability of misaligned timeout field. The latter has to be aligned on
8 bytes.
> > > + } iio_buffer;
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 04, 2024 at 09:18:27PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 04, 2024 at 08:08:38PM +0100, Vasileios Amoiridis wrote:
> > On Mon, Mar 04, 2024 at 01:52:05PM +0200, Andy Shevchenko wrote:
> > > On Sun, Mar 03, 2024 at 05:53:00PM +0100, Vasileios Amoiridis wrote:
>
> ...
>
> > > > + struct {
> > > > + s32 temperature;
> > > > + u32 pressure;
> > > > + u32 humidity;
> > >
> > > > + s64 timestamp;
> > >
> > > Shouldn't this be aligned properly?
> >
> > I saw that in some drivers it was added and in some it was not. What is the
> > difference of aligning just the timestamp of the kernel?
>
> You can count yourself. With provided structure as above there is a high
> probability of misaligned timeout field. The latter has to be aligned on
> 8 bytes.
>
I was unaware, but now I am not. Thank you very much for the feedback.
> > > > + } iio_buffer;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Sun, 3 Mar 2024 17:52:57 +0100
Vasileios Amoiridis <[email protected]> wrote:
> Sort headers in alphabetical order.
>
> Signed-off-by: Vasileios Amoiridis <[email protected]>
> ---
> drivers/iio/pressure/bmp280-core.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index fe8734468ed3..29a8b7195076 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/device.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
I'm fairly sure nothing from sysfs.h is used in this file so good to drop it.
> -#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 <asm/unaligned.h>
>
On Mon, 4 Mar 2024 21:07:09 +0200
Andy Shevchenko <[email protected]> wrote:
> On Mon, Mar 04, 2024 at 07:50:17PM +0100, Vasileios Amoiridis wrote:
> > On Mon, Mar 04, 2024 at 01:47:47PM +0200, Andy Shevchenko wrote:
> > > On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:
>
> ...
>
> > > > +enum bmp280_scan {
> > > > + BMP280_TEMP,
> > > > + BMP280_PRESS,
> > > > + BME280_HUMID,
> > > > +};
> > >
> > > Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
> > > directly (or in some way)?
> >
> > What do you mean exactly by copying the IIO ones? These values are used as
> > indexes to enable channels in the avail_scan_masks below.
>
> Yeah, I have now an answer to my question. The IIO drivers provide these lists
> as mapping between available channels (as starting from 0) and real channels
> as per IIO specifications (which can be anything, although limited currently
> by 40 or so).
>
These are the scan indexes. It would be better to specify them as such
in the channel definitions so that the two sets of values are forced to match.
Jonathan
On Mon, 4 Mar 2024 21:05:47 +0100
Vasileios Amoiridis <[email protected]> wrote:
> On Mon, Mar 04, 2024 at 09:18:27PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 04, 2024 at 08:08:38PM +0100, Vasileios Amoiridis wrote:
> > > On Mon, Mar 04, 2024 at 01:52:05PM +0200, Andy Shevchenko wrote:
> > > > On Sun, Mar 03, 2024 at 05:53:00PM +0100, Vasileios Amoiridis wrote:
> >
> > ...
> >
> > > > > + struct {
> > > > > + s32 temperature;
> > > > > + u32 pressure;
> > > > > + u32 humidity;
> > > >
> > > > > + s64 timestamp;
> > > >
> > > > Shouldn't this be aligned properly?
> > >
> > > I saw that in some drivers it was added and in some it was not. What is the
> > > difference of aligning just the timestamp of the kernel?
> >
> > You can count yourself. With provided structure as above there is a high
> > probability of misaligned timeout field. The latter has to be aligned on
> > 8 bytes.
> >
>
> I was unaware, but now I am not. Thank you very much for the feedback.
Fun bit of C is that you aren't actually aligning just the timestamp.
A C structure is aligned to the alignment of the maximum element within it.
So by specifying that timestamp is aligned to 8 bytes, you also force the
alignment of the whole structure to 8 bytes.
When you see the outer buffer aligned as well (typically the potentially larger
__aligned (IIO_DMA_MINALIGN)) that's for a different reason. Used on a trailing
element of a structure via iio_priv() that ensures there is nothing else in the
cacheline (maximum one in the system) on systems where this matters due to non
coherent DMA. Still need the __aligned(8) on the timestamp though as otherwise
the internal padding may be wrong (like here).
On some architectures small buffers are always bounced - if that were true on
all of them we could get rid of the complexity of IIO_DMA_MINALIGN.
Alignment is so much fun - particularly with x86_32 which does 8 byte values aligned
to 4 bytes. We had a massive set of patches fixing subtle issues around that a
few years ago.
Jonathan
> > > > > + } iio_buffer;
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
On Sun, 3 Mar 2024 17:52:58 +0100
Vasileios Amoiridis <[email protected]> wrote:
> Add extra IIO_CHAN_INFO_SCALE in order to be able to have the scales
> for the values in userspace. Can be used for triggered buffers.
>
> Signed-off-by: Vasileios Amoiridis <[email protected]>
So providing scale and processed is a mess. With hindsight this should have
always provided raw + scale, but too late :(
This retrofitting buffered code onto a drive that does processed channels
is one of the few reasons I'll let through channels that do both processed and raw.
So add raw as well. Hopefully raw * scale always equals processed.
The reason is that for a channel doing processed only - assumption is
normally that the buffer is processed as well. We can't remove processed
as that would be ABI breakage, but we can add raw.
Jonathan
> ---
> drivers/iio/pressure/bmp280-core.c | 70 ++++++++++++++++++++++++++++++
> 1 file changed, 70 insertions(+)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 29a8b7195076..acdf6138d317 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -138,16 +138,19 @@ static const struct iio_chan_spec bmp280_channels[] = {
> {
> .type = IIO_PRESSURE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> },
> {
> .type = IIO_TEMP,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> },
> {
> .type = IIO_HUMIDITYRELATIVE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> },
> };
> @@ -156,6 +159,7 @@ static const struct iio_chan_spec bmp380_channels[] = {
> {
> .type = IIO_PRESSURE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + 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),
> @@ -163,6 +167,7 @@ static const struct iio_chan_spec bmp380_channels[] = {
> {
> .type = IIO_TEMP,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + 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),
> @@ -170,6 +175,7 @@ static const struct iio_chan_spec bmp380_channels[] = {
> {
> .type = IIO_HUMIDITYRELATIVE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + 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),
> @@ -487,6 +493,70 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> break;
> }
> break;
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_HUMIDITYRELATIVE:
> + if (!strcmp(indio_dev->name, "bme280")) {
> + *val = 1000;
> + *val2 = 1024;
> + ret = IIO_VAL_FRACTIONAL;
> + } else {
> + ret = -EINVAL;
> + }
> + break;
> + case IIO_PRESSURE:
> + if ((!strcmp(indio_dev->name, "bmp085")) ||
> + (!strcmp(indio_dev->name, "bmp180")) ||
> + (!strcmp(indio_dev->name, "bmp181"))) {
> + *val = 1;
> + *val2 = 1000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if ((!strcmp(indio_dev->name, "bmp280")) ||
> + (!strcmp(indio_dev->name, "bme280"))) {
> + *val = 1;
> + *val2 = 256000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp380")) {
> + *val = 1;
> + *val2 = 100000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp580")) {
> + *val = 1;
> + *val2 = 64000;
> + ret = IIO_VAL_FRACTIONAL;
> + } else {
> + ret = -EINVAL;
> + }
> + break;
> + case IIO_TEMP:
> + if ((!strcmp(indio_dev->name, "bmp085")) ||
> + (!strcmp(indio_dev->name, "bmp180")) ||
> + (!strcmp(indio_dev->name, "bmp181"))) {
> + *val = 100;
> + *val2 = 1;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if ((!strcmp(indio_dev->name, "bmp280")) ||
> + (!strcmp(indio_dev->name, "bme280"))) {
> + *val = 10;
> + *val2 = 1;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp380")) {
> + *val = 10;
> + *val2 = 1;
> + ret = IIO_VAL_FRACTIONAL;
> + } else if (!strcmp(indio_dev->name, "bmp580")) {
> + *val = 1000;
> + *val2 = 16;
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + } else {
> + ret = -EINVAL;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> switch (chan->type) {
> case IIO_HUMIDITYRELATIVE:
On Sun, 3 Mar 2024 17:53:00 +0100
Vasileios Amoiridis <[email protected]> wrote:
> Add a buffer struct that will hold the values of the measurements
> and will be pushed to userspace. Modify all read_* functions in order
> to just read and compensate the data without though converting to the
> required IIO measurement units which are used for the oneshot captures.
>
> Signed-off-by: Vasileios Amoiridis <[email protected]>
Hi Vasileios,
This falls into the usual problem hole of drivers that provide
only processed channels. The assumption is that the data in the buffer
obeys the same description as the sysfs files. So if we only have
processsed assumption is that scale should not be applied (it's rare
enough I suspect our test code doesn't know this subtlety.
We can resolve it as per comment on earlier patch to add _raw as well.
> ---
> drivers/iio/pressure/Kconfig | 2 +
> drivers/iio/pressure/bmp280-core.c | 155 +++++++++++++++++++++++------
> drivers/iio/pressure/bmp280.h | 7 ++
> 3 files changed, 134 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index 79adfd059c3a..5145b94b4679 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 3bdf6002983f..3b1a159e57ea 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -31,8 +31,12 @@
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
This is normally only needed for devices registering a trigger. This one doesn't.
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> #include <linux/interrupt.h>
> #include <linux/irq.h> /* For irq_get_irq_data() */
> #include <linux/module.h>
>
> @@ -2133,10 +2179,16 @@ static int bmp180_read_press(struct bmp280_data *data,
>
> comp_press = bmp180_compensate_press(data, adc_press);
>
> - *val = comp_press;
> - *val2 = 1000;
> + /* val might be NULL if we're called by the buffer handler */
> + if (val) {
> + *val = comp_press;
> + *val2 = 1000;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + data->iio_buffer.pressure = comp_press;
Putting the filling of the internal cache in here makes this hard to read.
I think I'd prefer seeing the code shared by the sysfs and this path
factored out to a separate function then a simple
bmp180_read_press_raw() as an additional callback so we can see this value
being set at the caller.
>
> - return IIO_VAL_FRACTIONAL;
> + return 0;
> }
>
> static int bmp180_chip_config(struct bmp280_data *data)
> @@ -2217,6 +2269,43 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
> return 0;
> }
>
> +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);
> + int ret;
> +
> + mutex_lock(&data->lock);
> +
> + if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> + ret = data->chip_info->read_temp(data, NULL, NULL);
> + if (ret < 0)
> + goto done;
> + }
> +
> + if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> + ret = data->chip_info->read_press(data, NULL, NULL);
> + if (ret < 0)
> + goto done;
> + }
> +
> + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> + ret = data->chip_info->read_humid(data, NULL, NULL);
> + if (ret < 0)
> + goto done;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buffer,
> + pf->timestamp);
> +
> +done:
> + mutex_unlock(&data->lock);
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +
> +}
blank line here.
> static void bmp280_pm_disable(void *data)
> {
> struct device *dev = data;
> @@ -2358,6 +2447,12 @@ int bmp280_common_probe(struct device *dev,
> return ret;
> }
>
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + iio_pollfunc_store_time,
> + &bmp280_buffer_handler, NULL);
> + if (ret)
> + return dev_err_probe(data->dev, ret,
> + "iio triggered buffer setup failed\n");
> /* Enable runtime PM */
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index d77402cb3121..d5c0451ebf68 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -400,6 +400,13 @@ struct bmp280_data {
> */
> s32 t_fine;
>
> + /* IIO sysfs buffer */
Sysfs is very much one thing it isn't if you have a timestamp. This is for
the chardev flow. So drop the missleading comment.
> + struct {
> + s32 temperature;
> + u32 pressure;
> + u32 humidity;
> + s64 timestamp;
> + } iio_buffer;
> /*
> * DMA (thus cache coherency maintenance) may require the
> * transfer buffers to live in their own cache lines.