Add a buffer struct that will hold the values of the measurements
and will be pushed to userspace and a buffer_handler function to
read the data and push them.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/Kconfig | 2 +
drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
drivers/iio/pressure/bmp280.h | 7 ++++
3 files changed, 70 insertions(+)
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 f2cf9bef522c..7c889cda396a 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -40,7 +40,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>
@@ -2188,6 +2191,57 @@ 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, temp;
+
+ /*
+ * data->buf[3] is used to transfer data from the device. Whenever a
+ * pressure or a humidity reading takes place, the data written in the
+ * data->buf[3] overwrites the iio_buf.temperature value. Keep the
+ * temperature value and apply it after the readings.
+ */
+ mutex_lock(&data->lock);
+
+ if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
+ ret = data->chip_info->read_temp(data);
+ if (ret < 0)
+ goto done;
+
+ temp = ret;
+ }
+
+ if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
+ ret = data->chip_info->read_press(data);
+ if (ret < 0)
+ goto done;
+
+ data->iio_buf.pressure = ret;
+ data->iio_buf.temperature = temp;
+ }
+
+ if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
+ ret = data->chip_info->read_humid(data);
+ if (ret < 0)
+ goto done;
+
+ data->iio_buf.humidity = ret;
+ data->iio_buf.temperature = temp;
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
+ iio_get_time_ns(indio_dev));
+
+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;
@@ -2329,6 +2383,13 @@ 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 c8cb7c417dab..b5369dd496ba 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -407,6 +407,13 @@ struct bmp280_data {
union {
/* Sensor data buffer */
u8 buf[3];
+ /* Data buffer to push to userspace */
+ struct {
+ s32 temperature;
+ u32 pressure;
+ u32 humidity;
+ s64 timestamp __aligned(8);
+ } iio_buf;
/* Calibration data buffers */
__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
--
2.25.1
On Wed, Mar 13, 2024 at 06:40:07PM +0100, Vasileios Amoiridis wrote:
> Add a buffer struct that will hold the values of the measurements
> and will be pushed to userspace and a buffer_handler function to
> read the data and push them.
..
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
dev here
> + iio_pollfunc_store_time,
> + &bmp280_buffer_handler, NULL);
> + if (ret)
> + return dev_err_probe(data->dev, ret,
data->dev here
Are they the same? If not, why this difference?
> + "iio triggered buffer setup failed\n");
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 13, 2024 at 08:58:07PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:07PM +0100, Vasileios Amoiridis wrote:
> > Add a buffer struct that will hold the values of the measurements
> > and will be pushed to userspace and a buffer_handler function to
> > read the data and push them.
>
> ...
>
> > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>
> dev here
>
> > + iio_pollfunc_store_time,
> > + &bmp280_buffer_handler, NULL);
> > + if (ret)
> > + return dev_err_probe(data->dev, ret,
>
> data->dev here
>
> Are they the same? If not, why this difference?
>
They are the same. I didn't notice it, but I will fix it for consistency.
> > + "iio triggered buffer setup failed\n");
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best regards,
Vasileios Amoiridis
On Wed, 13 Mar 2024 18:40:07 +0100
Vasileios Amoiridis <[email protected]> wrote:
> Add a buffer struct that will hold the values of the measurements
> and will be pushed to userspace and a buffer_handler function to
> read the data and push them.
>
> Signed-off-by: Vasileios Amoiridis <[email protected]>
> ---
> drivers/iio/pressure/Kconfig | 2 +
> drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
> drivers/iio/pressure/bmp280.h | 7 ++++
> 3 files changed, 70 insertions(+)
>
> 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 f2cf9bef522c..7c889cda396a 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -40,7 +40,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>
>
> @@ -2188,6 +2191,57 @@ 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, temp;
> +
> + /*
> + * data->buf[3] is used to transfer data from the device. Whenever a
> + * pressure or a humidity reading takes place, the data written in the
> + * data->buf[3] overwrites the iio_buf.temperature value. Keep the
> + * temperature value and apply it after the readings.
See comment below. Given you saw this problem did you not think maybe you
were doing something a little unusual / wrong? Should have rung alarm
bells beyond just putting a comment here to explain you needed to work around
the issue.
> + */
> + mutex_lock(&data->lock);
> +
> + if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> + ret = data->chip_info->read_temp(data);
> + if (ret < 0)
> + goto done;
> +
> + temp = ret;
> + }
> +
> + if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> + ret = data->chip_info->read_press(data);
> + if (ret < 0)
> + goto done;
> +
> + data->iio_buf.pressure = ret;
> + data->iio_buf.temperature = temp;
Try running this with the tooling in tools/iio and you'll see that
you are getting the wrong output if you have just humidity and
temperature enabled - IIO packs channels, so disable an early
one and everything moves down in address.
If you an this device without timestamps and only a single channel
the buffer used will have one s32 per scan for example.
> + }
> +
> + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> + ret = data->chip_info->read_humid(data);
> + if (ret < 0)
> + goto done;
> +
> + data->iio_buf.humidity = ret;
> + data->iio_buf.temperature = temp;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> + iio_get_time_ns(indio_dev));
> +
> +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;
> @@ -2329,6 +2383,13 @@ 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 c8cb7c417dab..b5369dd496ba 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -407,6 +407,13 @@ struct bmp280_data {
> union {
> /* Sensor data buffer */
> u8 buf[3];
> + /* Data buffer to push to userspace */
Why is this in the union? This union is to ensure cache safe buffers for
DMAing directly into. That's not applicable here. Even though it may
be safe to do this (I was reading backwards so wrote a comment here
on you corrupting temperature before seeing the comment above)
it is giving a misleading impression of how this struct is used.
Pull the struct to after t_fine - It only needs to
be 8 byte aligned and the aligned marking you have will ensure that
> + struct {
> + s32 temperature;
> + u32 pressure;
> + u32 humidity;
As above, this is 3 4 byte buffers but they don't have these meanings
unless all channels are enabled.
You could set available mask to enforce that, but don't as it makes
limited sense for this hardware. Just make these
u32 chans[3];
and fill them in with an index incremented for each channel that is
enabled.
Jonathan
> + s64 timestamp __aligned(8);
> + } iio_buf;
> /* Calibration data buffers */
> __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
On Thu, Mar 14, 2024 at 03:25:25PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 18:40:07 +0100
> Vasileios Amoiridis <[email protected]> wrote:
>
> > Add a buffer struct that will hold the values of the measurements
> > and will be pushed to userspace and a buffer_handler function to
> > read the data and push them.
> >
> > Signed-off-by: Vasileios Amoiridis <[email protected]>
> > ---
> > drivers/iio/pressure/Kconfig | 2 +
> > drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
> > drivers/iio/pressure/bmp280.h | 7 ++++
> > 3 files changed, 70 insertions(+)
> >
> > 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 f2cf9bef522c..7c889cda396a 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -40,7 +40,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>
> >
> > @@ -2188,6 +2191,57 @@ 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, temp;
> > +
> > + /*
> > + * data->buf[3] is used to transfer data from the device. Whenever a
> > + * pressure or a humidity reading takes place, the data written in the
> > + * data->buf[3] overwrites the iio_buf.temperature value. Keep the
> > + * temperature value and apply it after the readings.
>
> See comment below. Given you saw this problem did you not think maybe you
> were doing something a little unusual / wrong? Should have rung alarm
> bells beyond just putting a comment here to explain you needed to work around
> the issue.
>
> > + */
> > + mutex_lock(&data->lock);
> > +
> > + if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> > + ret = data->chip_info->read_temp(data);
> > + if (ret < 0)
> > + goto done;
> > +
> > + temp = ret;
> > + }
> > +
> > + if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> > + ret = data->chip_info->read_press(data);
> > + if (ret < 0)
> > + goto done;
> > +
> > + data->iio_buf.pressure = ret;
> > + data->iio_buf.temperature = temp;
> Try running this with the tooling in tools/iio and you'll see that
> you are getting the wrong output if you have just humidity and
> temperature enabled - IIO packs channels, so disable an early
> one and everything moves down in address.
>
> If you an this device without timestamps and only a single channel
> the buffer used will have one s32 per scan for example.
>
> > + }
> > +
> > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> > + ret = data->chip_info->read_humid(data);
> > + if (ret < 0)
> > + goto done;
> > +
> > + data->iio_buf.humidity = ret;
> > + data->iio_buf.temperature = temp;
> > + }
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > + iio_get_time_ns(indio_dev));
> > +
> > +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;
> > @@ -2329,6 +2383,13 @@ 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 c8cb7c417dab..b5369dd496ba 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -407,6 +407,13 @@ struct bmp280_data {
> > union {
> > /* Sensor data buffer */
> > u8 buf[3];
> > + /* Data buffer to push to userspace */
>
> Why is this in the union? This union is to ensure cache safe buffers for
> DMAing directly into. That's not applicable here. Even though it may
> be safe to do this (I was reading backwards so wrote a comment here
> on you corrupting temperature before seeing the comment above)
> it is giving a misleading impression of how this struct is used.
> Pull the struct to after t_fine - It only needs to
> be 8 byte aligned and the aligned marking you have will ensure that
>
> > + struct {
> > + s32 temperature;
> > + u32 pressure;
> > + u32 humidity;
>
> As above, this is 3 4 byte buffers but they don't have these meanings
> unless all channels are enabled.
>
> You could set available mask to enforce that, but don't as it makes
> limited sense for this hardware. Just make these
> u32 chans[3];
> and fill them in with an index incremented for each channel that is
> enabled.
>
> Jonathan
>
I wanted to put it inside the union to save some space but you are right
that it is quite misleading. I was just trying in general, along with the
previous patches to avoid reading the temperature twice. Along with your
comments in the previous patch, if a user has enabled both temperature
and pressure and humidity we could save ourselves from reading the
temperature 3 times instead of 1. But in any case, your previous proposal
with a separate get_t_fine structure looks good.
Best regards,
Vasilis
> > + s64 timestamp __aligned(8);
> > + } iio_buf;
> > /* Calibration data buffers */
> > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
>
On Thu, 14 Mar 2024 21:14:31 +0100
Vasileios Amoiridis <[email protected]> wrote:
> On Thu, Mar 14, 2024 at 03:25:25PM +0000, Jonathan Cameron wrote:
> > On Wed, 13 Mar 2024 18:40:07 +0100
> > Vasileios Amoiridis <[email protected]> wrote:
> >
> > > Add a buffer struct that will hold the values of the measurements
> > > and will be pushed to userspace and a buffer_handler function to
> > > read the data and push them.
> > >
> > > Signed-off-by: Vasileios Amoiridis <[email protected]>
> > > ---
> > > drivers/iio/pressure/Kconfig | 2 +
> > > drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
> > > drivers/iio/pressure/bmp280.h | 7 ++++
> > > 3 files changed, 70 insertions(+)
> > >
> > > 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 f2cf9bef522c..7c889cda396a 100644
> > > --- a/drivers/iio/pressure/bmp280-core.c
> > > +++ b/drivers/iio/pressure/bmp280-core.c
> > > @@ -40,7 +40,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>
> > >
> > > @@ -2188,6 +2191,57 @@ 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, temp;
> > > +
> > > + /*
> > > + * data->buf[3] is used to transfer data from the device. Whenever a
> > > + * pressure or a humidity reading takes place, the data written in the
> > > + * data->buf[3] overwrites the iio_buf.temperature value. Keep the
> > > + * temperature value and apply it after the readings.
> >
> > See comment below. Given you saw this problem did you not think maybe you
> > were doing something a little unusual / wrong? Should have rung alarm
> > bells beyond just putting a comment here to explain you needed to work around
> > the issue.
> >
> > > + */
> > > + mutex_lock(&data->lock);
> > > +
> > > + if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> > > + ret = data->chip_info->read_temp(data);
> > > + if (ret < 0)
> > > + goto done;
> > > +
> > > + temp = ret;
> > > + }
> > > +
> > > + if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> > > + ret = data->chip_info->read_press(data);
> > > + if (ret < 0)
> > > + goto done;
> > > +
> > > + data->iio_buf.pressure = ret;
> > > + data->iio_buf.temperature = temp;
> > Try running this with the tooling in tools/iio and you'll see that
> > you are getting the wrong output if you have just humidity and
> > temperature enabled - IIO packs channels, so disable an early
> > one and everything moves down in address.
> >
> > If you an this device without timestamps and only a single channel
> > the buffer used will have one s32 per scan for example.
> >
> > > + }
> > > +
> > > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> > > + ret = data->chip_info->read_humid(data);
> > > + if (ret < 0)
> > > + goto done;
> > > +
> > > + data->iio_buf.humidity = ret;
> > > + data->iio_buf.temperature = temp;
> > > + }
> > > +
> > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > > + iio_get_time_ns(indio_dev));
> > > +
> > > +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;
> > > @@ -2329,6 +2383,13 @@ 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 c8cb7c417dab..b5369dd496ba 100644
> > > --- a/drivers/iio/pressure/bmp280.h
> > > +++ b/drivers/iio/pressure/bmp280.h
> > > @@ -407,6 +407,13 @@ struct bmp280_data {
> > > union {
> > > /* Sensor data buffer */
> > > u8 buf[3];
> > > + /* Data buffer to push to userspace */
> >
> > Why is this in the union? This union is to ensure cache safe buffers for
> > DMAing directly into. That's not applicable here. Even though it may
> > be safe to do this (I was reading backwards so wrote a comment here
> > on you corrupting temperature before seeing the comment above)
> > it is giving a misleading impression of how this struct is used.
> > Pull the struct to after t_fine - It only needs to
> > be 8 byte aligned and the aligned marking you have will ensure that
> >
> > > + struct {
> > > + s32 temperature;
> > > + u32 pressure;
> > > + u32 humidity;
> >
> > As above, this is 3 4 byte buffers but they don't have these meanings
> > unless all channels are enabled.
> >
> > You could set available mask to enforce that, but don't as it makes
> > limited sense for this hardware. Just make these
> > u32 chans[3];
> > and fill them in with an index incremented for each channel that is
> > enabled.
> >
> > Jonathan
> >
>
> I wanted to put it inside the union to save some space but you are right
> that it is quite misleading.
It may not save any space. The force alignment here tends to lead to some
room between the first chunk of this structure and the __aligned(IIO_DMA_MINALIGN) part.
This may well fit in the hole - I've not checked though.
> I was just trying in general, along with the
> previous patches to avoid reading the temperature twice. Along with your
> comments in the previous patch, if a user has enabled both temperature
> and pressure and humidity we could save ourselves from reading the
> temperature 3 times instead of 1. But in any case, your previous proposal
> with a separate get_t_fine structure looks good.
If it gets complex, just implement a buffered read that always gets all the
channels and set available_scan_masks to express that. The core code
has channel data shuffling that will result in your userspace seeing
whatever subset of channels it requests.
Jonathan
>
> Best regards,
> Vasilis
> > > + s64 timestamp __aligned(8);
> > > + } iio_buf;
> > > /* Calibration data buffers */
> > > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> > > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
> >