| Message ID | 20240319002925.2121016-7-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Series to add triggered buffer support to BMP280 driver | expand |
On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote: > BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their > temperature, pressure and humidity readings. This facilitates > the use of burst reads in order to acquire data much faster > and in a different way from the one used in oneshot captures. > > BMP085 and BMP180 use a completely different measurement > process that is well defined and is used in their buffer_handler(). ... > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_TEMP_BYTES); > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_PRESS_BYTES); > ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, > - &data->be16, sizeof(data->be16)); > + &data->be16, BME280_NUM_HUMIDITY_BYTES); > - adc_humidity = be16_to_cpu(data->be16); > + adc_humidity = get_unaligned_be16(&data->be16); > ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_TEMP_BYTES); > ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_PRESS_BYTES); > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > - sizeof(data->buf)); > + BMP280_NUM_TEMP_BYTES); > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > - sizeof(data->buf)); > + BMP280_NUM_PRESS_BYTES); These smell to me as candidates to a separate patch with more explanation why. (Yes, with the definitions you introduced.) But I leave it to Jonathan to decide if we need to split. ... The below are applicable to the bmp280_buffer_handler(), bmp380_buffer_handler() implementations as well. ... > + /* Burst read data registers */ > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > + data->buf, 6); Magic size. ... > + /* Temperature calculations */ > + memcpy(&chan_value, &data->buf[3], 3); _le24() + sign_extend32()? ... > + /* Pressure calculations */ > + memcpy(&chan_value, &data->buf[0], 3); _le24() + sign_extend32()? ... > /* > - * Maximum number of consecutive bytes read for a temperature or > - * pressure measurement is 3. > + * Maximum number of a burst read for temperature, pressure, humidity > + * is 8 bytes. > */ > - if (val_size > 3) > + if (val_size > 8) sizeof() / new definition for the buf[] size? > return -EINVAL;
On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote: > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote: > > BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their > > temperature, pressure and humidity readings. This facilitates > > the use of burst reads in order to acquire data much faster > > and in a different way from the one used in oneshot captures. > > > > BMP085 and BMP180 use a completely different measurement > > process that is well defined and is used in their buffer_handler(). > > ... > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_TEMP_BYTES); > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_PRESS_BYTES); > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, > > - &data->be16, sizeof(data->be16)); > > + &data->be16, BME280_NUM_HUMIDITY_BYTES); > > > - adc_humidity = be16_to_cpu(data->be16); > > + adc_humidity = get_unaligned_be16(&data->be16); > > > ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_TEMP_BYTES); > > > ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_PRESS_BYTES); > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > > - sizeof(data->buf)); > > + BMP280_NUM_TEMP_BYTES); > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > > - sizeof(data->buf)); > > + BMP280_NUM_PRESS_BYTES); > > These smell to me as candidates to a separate patch with more explanation why. > (Yes, with the definitions you introduced.) But I leave it to Jonathan to > decide if we need to split. > > ... > > The below are applicable to the bmp280_buffer_handler(), > bmp380_buffer_handler() implementations as well. > > ... > > > + /* Burst read data registers */ > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > + data->buf, 6); > > Magic size. > Hi Andy, Thank you again for your feedback. When I was writing it, it was looking as a magic number to me as well but then I though that since I put the comment above it could be obvious. Now that I see it again, I think it was not a good idea and maybe some type of definition like #define BMP280_BURST_READ_NUM_BYTES 6 #define BME280_BURST_READ_NUM_BYTES 8 could look better and be more intuitive. > ... > > > + /* Temperature calculations */ > > + memcpy(&chan_value, &data->buf[3], 3); > > _le24() + sign_extend32()? > In the next line from your comment the _le24 or _be24 takes place. If the sign_extend32() is needed here, shouldn't it also be used in all the oneshot captures as well? > ... > > > + /* Pressure calculations */ > > + memcpy(&chan_value, &data->buf[0], 3); > > _le24() + sign_extend32()? > > ... > > > /* > > - * Maximum number of consecutive bytes read for a temperature or > > - * pressure measurement is 3. > > + * Maximum number of a burst read for temperature, pressure, humidity > > + * is 8 bytes. > > */ > > - if (val_size > 3) > > + if (val_size > 8) > > sizeof() / new definition for the buf[] size? > In a previous commit that I was fixing this SPI driver, Jonathan had mentioned that there is no need for a specific definition since it will only be used here so that's why I kept it as is. Cheers, Vasilis > > return -EINVAL; > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Mar 20, 2024 at 06:46:02PM +0100, Vasileios Amoiridis wrote: > On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote: ... > > > /* > > > - * Maximum number of consecutive bytes read for a temperature or > > > - * pressure measurement is 3. > > > + * Maximum number of a burst read for temperature, pressure, humidity > > > + * is 8 bytes. > > > */ > > > - if (val_size > 3) > > > + if (val_size > 8) > > > > sizeof() / new definition for the buf[] size? > > In a previous commit that I was fixing this SPI driver, Jonathan had mentioned > that there is no need for a specific definition since it will only be used > here so that's why I kept it as is. It seems not only here, but also in the buf[] definition in the struct.
On Wed, Mar 20, 2024 at 11:25:46PM +0200, Andy Shevchenko wrote: > On Wed, Mar 20, 2024 at 06:46:02PM +0100, Vasileios Amoiridis wrote: > > On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote: > > > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote: > > ... > > > > > /* > > > > - * Maximum number of consecutive bytes read for a temperature or > > > > - * pressure measurement is 3. > > > > + * Maximum number of a burst read for temperature, pressure, humidity > > > > + * is 8 bytes. > > > > */ > > > > - if (val_size > 3) > > > > + if (val_size > 8) > > > > > > sizeof() / new definition for the buf[] size? > > > > In a previous commit that I was fixing this SPI driver, Jonathan had mentioned > > that there is no need for a specific definition since it will only be used > > here so that's why I kept it as is. > > It seems not only here, but also in the buf[] definition in the struct. > You are totally right, I didn't think of that! It makes sense to use a definition. Cheers, Vasilis > -- > With Best Regards, > Andy Shevchenko > >
On Wed, 20 Mar 2024 18:46:02 +0100 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote: > > > BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their > > > temperature, pressure and humidity readings. This facilitates > > > the use of burst reads in order to acquire data much faster > > > and in a different way from the one used in oneshot captures. > > > > > > BMP085 and BMP180 use a completely different measurement > > > process that is well defined and is used in their buffer_handler(). > > > > ... > > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_TEMP_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_PRESS_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, > > > - &data->be16, sizeof(data->be16)); > > > + &data->be16, BME280_NUM_HUMIDITY_BYTES); > > > > > - adc_humidity = be16_to_cpu(data->be16); > > > + adc_humidity = get_unaligned_be16(&data->be16); > > > > > ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_TEMP_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > > > - data->buf, sizeof(data->buf)); > > > + data->buf, BMP280_NUM_PRESS_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, > > > - sizeof(data->buf)); > > > + BMP280_NUM_TEMP_BYTES); > > > > > ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, > > > - sizeof(data->buf)); > > > + BMP280_NUM_PRESS_BYTES); > > > > These smell to me as candidates to a separate patch with more explanation why. > > (Yes, with the definitions you introduced.) But I leave it to Jonathan to > > decide if we need to split. The are somewhat confusing, though only when you start doing bulk reads to do these makes sense - so I'm not sure how to do it as 2 patches. Pity we don't have sizeof(be24) available. > > > > ... > > > > The below are applicable to the bmp280_buffer_handler(), > > bmp380_buffer_handler() implementations as well. > > > > ... > > > > > + /* Burst read data registers */ > > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > > + data->buf, 6); > > > > Magic size. > > > > Hi Andy, > > Thank you again for your feedback. When I was writing it, it was > looking as a magic number to me as well but then I though that > since I put the comment above it could be obvious. Now that I see > it again, I think it was not a good idea and maybe some type of > definition like > > #define BMP280_BURST_READ_NUM_BYTES 6 > #define BME280_BURST_READ_NUM_BYTES 8 I think these are sums of the other quantities. Better to express them as such if possible. > > could look better and be more intuitive. > > > ... > > > > > + /* Temperature calculations */ > > > + memcpy(&chan_value, &data->buf[3], 3); > > > > _le24() + sign_extend32()? > > > > In the next line from your comment the _le24 or _be24 takes place. I think you can get that data directly rather than bouncing it via a memcpy. e.g. adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[3]); > If the sign_extend32() is needed here, shouldn't it also be used > in all the oneshot captures as well? Is this actually signed? Compensated values are, but at this point it's just a raw adc count. So I'm not seeing why we'd sign extend it (unless the device is returning a signed be24 - I haven't checked.) > > > ... > > > > > + /* Pressure calculations */ > > > + memcpy(&chan_value, &data->buf[0], 3); > > > > _le24() + sign_extend32()? > > > > ... > > > > > /* > > > - * Maximum number of consecutive bytes read for a temperature or > > > - * pressure measurement is 3. > > > + * Maximum number of a burst read for temperature, pressure, humidity > > > + * is 8 bytes. > > > */ > > > - if (val_size > 3) > > > + if (val_size > 8) > > > > sizeof() / new definition for the buf[] size? > > > > In a previous commit that I was fixing this SPI driver, Jonathan had mentioned > that there is no need for a specific definition since it will only be used > here so that's why I kept it as is. Never trust me ;) Size of the buf is sensible here. Jonathan > > Cheers, > Vasilis > > > return -EINVAL; > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
On Tue, 19 Mar 2024 01:29:25 +0100 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their > temperature, pressure and humidity readings. This facilitates > the use of burst reads in order to acquire data much faster > and in a different way from the one used in oneshot captures. > > BMP085 and BMP180 use a completely different measurement > process that is well defined and is used in their buffer_handler(). > > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> Various comments inline, Thanks, Jonathan > --- > drivers/iio/pressure/Kconfig | 2 + > drivers/iio/pressure/bmp280-core.c | 231 +++++++++++++++++++++++++++-- > drivers/iio/pressure/bmp280-spi.c | 8 +- > drivers/iio/pressure/bmp280.h | 14 +- > 4 files changed, 241 insertions(+), 14 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 ddfcd23f29a0..ffcd17807cf5 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> > > @@ -454,7 +457,7 @@ static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) > int ret; > > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_TEMP_BYTES); > if (ret < 0) { > dev_err(data->dev, "failed to read temperature\n"); > return ret; > @@ -484,7 +487,7 @@ static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) > return ret; > > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_PRESS_BYTES); > if (ret < 0) { > dev_err(data->dev, "failed to read pressure\n"); > return ret; > @@ -513,13 +516,13 @@ static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity) > return ret; > > ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, > - &data->be16, sizeof(data->be16)); > + &data->be16, BME280_NUM_HUMIDITY_BYTES); > if (ret < 0) { > dev_err(data->dev, "failed to read humidity\n"); > return ret; > } > > - adc_humidity = be16_to_cpu(data->be16); > + adc_humidity = get_unaligned_be16(&data->be16); If it's a be16, how did it become unaligned? > if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { > /* reading was skipped */ > dev_err(data->dev, "reading humidity skipped\n"); > @@ -931,6 +934,73 @@ static int bmp280_chip_config(struct bmp280_data *data) > return ret; > } > > +static irqreturn_t bmp280_buffer_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bmp280_data *data = iio_priv(indio_dev); > + s32 adc_temp, adc_press, adc_humidity; > + u8 size_of_burst_read; > + int ret, chan_value; > + > + guard(mutex)(&data->lock); > + > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) This confuses me a little. Is it allowing reuse of this function for multiple devices or aiming to optimise the read in the case of the humidity channel being disabled (in which case I don't think it works because you aren't providing that combination in avail_scan_masks.) Add a comment to explain. > + size_of_burst_read = 8; > + else > + size_of_burst_read = 6; > + > + /* Burst read data registers */ > + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > + data->buf, 8); > + if (ret) { > + dev_err(data->dev, "failed to read sensor data\n"); > + return ret; > + } > + > + /* Temperature calculations */ > + memcpy(&chan_value, &data->buf[3], 3); > + > + adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value)); > + if (adc_temp == BMP280_TEMP_SKIPPED) { > + /* reading was skipped */ Similar comments on this to below (I read backwards as normally code makes more sense that way :) > + dev_err(data->dev, "reading temperature skipped\n"); > + return -EIO; > + } > + data->iio_buf.sensor_data[0] = bmp280_compensate_temp(data, adc_temp); > + > + /* Pressure calculations */ > + memcpy(&chan_value, &data->buf[0], 3); > + > + adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value)); > + if (adc_press == BMP280_PRESS_SKIPPED) { > + /* reading was skipped */ > + dev_err(data->dev, "reading pressure skipped\n"); > + return -EIO; > + } > + data->iio_buf.sensor_data[1] = bmp280_compensate_press(data, adc_press); > + > + /* Humidity calculations */ > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) { > + memcpy(&chan_value, &data->buf[6], 2); > + > + adc_humidity = get_unaligned_be16(&chan_value); > + if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { > + /* reading was skipped */ > + dev_err(data->dev, "reading humidity skipped\n"); > + return -EIO; > + } > + data->iio_buf.sensor_data[2] = bmp280_compensate_humidity(data, adc_humidity); Alignment of code looks wrong. > + } > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > + iio_get_time_ns(indio_dev)); > + > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > +static irqreturn_t bmp380_buffer_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bmp280_data *data = iio_priv(indio_dev); > + s32 adc_temp, adc_press; > + int ret, chan_value; > + > + guard(mutex)(&data->lock); > + > + /* Burst read data registers */ > + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > + data->buf, 6); > + if (ret) { > + dev_err(data->dev, "failed to read sensor data\n"); > + return ret; > + } > + > + /* Temperature calculations */ > + memcpy(&chan_value, &data->buf[3], 3); > + > + adc_temp = get_unaligned_le24(&chan_value); Same comments as below. > + if (adc_temp == BMP380_TEMP_SKIPPED) { > + /* reading was skipped */ > + dev_err(data->dev, "reading temperature skipped\n"); > + return -EIO; > + } > + data->iio_buf.sensor_data[0] = bmp380_compensate_temp(data, adc_temp); > + > + /* Pressure calculations */ > + memcpy(&chan_value, &data->buf[0], 3); > + > + adc_press = get_unaligned_le24(&chan_value); > + if (adc_press == BMP380_PRESS_SKIPPED) { > + /* reading was skipped */ > + dev_err(data->dev, "reading pressure skipped\n"); > + return -EIO; > + } > + data->iio_buf.sensor_data[1] = bmp380_compensate_press(data, adc_press); > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > + iio_get_time_ns(indio_dev)); > + > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > +static irqreturn_t bmp580_buffer_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bmp280_data *data = iio_priv(indio_dev); > + s32 adc_temp, adc_press; > + int ret, chan_value; > + > + guard(mutex)(&data->lock); > + > + /* Burst read data registers */ > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > + data->buf, 6); > + if (ret) { > + dev_err(data->dev, "failed to read sensor data\n"); > + return ret; > + } > + > + /* Temperature calculations */ > + memcpy(&chan_value, &data->buf[3], 3); > + > + adc_temp = get_unaligned_le24(&chan_value); As in other thread branch, get it directly from &data->buf[3] rather than bouncing it via another buffer. > + if (adc_temp == BMP580_TEMP_SKIPPED) { > + /* reading was skipped */ > + dev_err(data->dev, "reading temperature skipped\n"); > + return -EIO; -EIO isn't irqreturn_t There isn't a good way to return errors from interrupt handlers, so just return IRQ_HANDLED after printing the error message. > + } > + data->iio_buf.sensor_data[0] = adc_temp; > + > + /* Pressure calculations */ > + memcpy(&chan_value, &data->buf[0], 3); > + > + adc_press = get_unaligned_le24(&chan_value); As above, get it directly. > + if (adc_press == BMP580_PRESS_SKIPPED) { > + /* reading was skipped */ > + dev_err(data->dev, "reading pressure skipped\n"); > + return -EIO; return IRQ_HANDLED; > + } > + data->iio_buf.sensor_data[1] = adc_press; Extra space. > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > + iio_get_time_ns(indio_dev)); > + > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > static const int bmp580_temp_coeffs[] = { 1000, 16 }; > @@ -1903,6 +2075,8 @@ const struct bmp280_chip_info bmp580_chip_info = { > .read_temp = bmp580_read_temp, > .read_press = bmp580_read_press, > .preinit = bmp580_preinit, > + > + .buffer_handler = bmp580_buffer_handler > }; > EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); > > @@ -2054,7 +2228,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val) > return ret; > > ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, > - data->buf, sizeof(data->buf)); > + data->buf, BMP280_NUM_PRESS_BYTES); > if (ret) > return ret; > > @@ -2122,6 +2296,35 @@ static int bmp180_chip_config(struct bmp280_data *data) > return 0; > } > > +static irqreturn_t bmp180_buffer_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bmp280_data *data = iio_priv(indio_dev); > + int ret, chan_value; > + > + guard(mutex)(&data->lock); > + > + ret = data->chip_info->read_temp(data, &chan_value); > + if (ret) > + return ret; > + > + data->iio_buf.sensor_data[0] = chan_value; > + > + ret = data->chip_info->read_press(data, &chan_value); Given my earlier suggestion to stop hiding t_fine in the structure, these callbacks will need to bring it all the way out here (from the temperature read) and pass it back into the pressure read. That will have the pleasing side effect of making it obvious why we aren't handling subsets of channels (as they aren't independent) > + if (ret) > + return ret; > + > + data->iio_buf.sensor_data[1] = chan_value; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > + iio_get_time_ns(indio_dev)); > + > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c > index a444d4b2978b..dc297583cac1 100644 > --- a/drivers/iio/pressure/bmp280-spi.c > +++ b/drivers/iio/pressure/bmp280-spi.c > @@ -40,14 +40,14 @@ static int bmp380_regmap_spi_read(void *context, const void *reg, > size_t reg_size, void *val, size_t val_size) > { > struct spi_device *spi = to_spi_device(context); > - u8 rx_buf[4]; > + u8 rx_buf[9]; > ssize_t status; > > /* > - * Maximum number of consecutive bytes read for a temperature or > - * pressure measurement is 3. > + * Maximum number of a burst read for temperature, pressure, humidity > + * is 8 bytes. Once this 8 is expressed as the sum of the 3 types, you can drop the comment as it will add nothing useful. > */ > - if (val_size > 3) > + if (val_size > 8) > return -EINVAL; > > /* > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > index 8cc3eed70c18..32155567faf6 100644 > --- a/drivers/iio/pressure/bmp280.h > +++ b/drivers/iio/pressure/bmp280.h > @@ -301,6 +301,10 @@ > #define BMP280_PRESS_SKIPPED 0x80000 > #define BMP280_HUMIDITY_SKIPPED 0x8000 > > +#define BMP280_NUM_TEMP_BYTES 3 > +#define BMP280_NUM_PRESS_BYTES 3 > +#define BME280_NUM_HUMIDITY_BYTES 2 > + > /* Core exported structs */ > > static const char *const bmp280_supply_names[] = { > @@ -400,13 +404,19 @@ struct bmp280_data { > */ > s32 t_fine; > > + /* Data to push to userspace */ > + struct { > + s32 sensor_data[3]; This doesn't work (as explanation of data) for all cases. If you only have 2 channels, the packing needs to be. s32 sensor_data[2]; //no padding s64 timestamp __aligned(8); > + s64 timestamp __aligned(8); > + } iio_buf; > + So using a structure for this definition isn't a bug as such as the core doesn't care that you provide a bigger buffer than needed, but it is misleading. Use something along lines of /* Up to 3 channels and aligned s64 timestamp */ s32 sensor_data[6] __aligned(8); or use a union of two structures to cover the two layouts making sure to write and read from the correct one in each callback. > /* > * DMA (thus cache coherency maintenance) may require the > * transfer buffers to live in their own cache lines. > */ > union { > /* Sensor data buffer */ > - u8 buf[3]; > + u8 buf[8]; As in previous discussion - build this up as a sum of what can go in it. Maybe via a define for BMP280_BURST_READ_MAX
On Sun, Mar 24, 2024 at 12:14:18PM +0000, Jonathan Cameron wrote: > On Tue, 19 Mar 2024 01:29:25 +0100 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their > > temperature, pressure and humidity readings. This facilitates > > the use of burst reads in order to acquire data much faster > > and in a different way from the one used in oneshot captures. > > > > BMP085 and BMP180 use a completely different measurement > > process that is well defined and is used in their buffer_handler(). > > > > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > Various comments inline, > > Thanks, > Jonathan > > > --- > > drivers/iio/pressure/Kconfig | 2 + > > drivers/iio/pressure/bmp280-core.c | 231 +++++++++++++++++++++++++++-- > > drivers/iio/pressure/bmp280-spi.c | 8 +- > > drivers/iio/pressure/bmp280.h | 14 +- > > 4 files changed, 241 insertions(+), 14 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 ddfcd23f29a0..ffcd17807cf5 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> > > > > @@ -454,7 +457,7 @@ static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) > > int ret; > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_TEMP_BYTES); > > if (ret < 0) { > > dev_err(data->dev, "failed to read temperature\n"); > > return ret; > > @@ -484,7 +487,7 @@ static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) > > return ret; > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_PRESS_BYTES); > > if (ret < 0) { > > dev_err(data->dev, "failed to read pressure\n"); > > return ret; > > @@ -513,13 +516,13 @@ static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity) > > return ret; > > > > ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, > > - &data->be16, sizeof(data->be16)); > > + &data->be16, BME280_NUM_HUMIDITY_BYTES); > > if (ret < 0) { > > dev_err(data->dev, "failed to read humidity\n"); > > return ret; > > } > > > > - adc_humidity = be16_to_cpu(data->be16); > > + adc_humidity = get_unaligned_be16(&data->be16); > > If it's a be16, how did it become unaligned? > > > if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { > > /* reading was skipped */ > > dev_err(data->dev, "reading humidity skipped\n"); > > @@ -931,6 +934,73 @@ static int bmp280_chip_config(struct bmp280_data *data) > > return ret; > > } > > > > +static irqreturn_t bmp280_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + s32 adc_temp, adc_press, adc_humidity; > > + u8 size_of_burst_read; > > + int ret, chan_value; > > + > > + guard(mutex)(&data->lock); > > + > > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) > > This confuses me a little. Is it allowing reuse of this function for > multiple devices or aiming to optimise the read in the case of > the humidity channel being disabled (in which case I don't think > it works because you aren't providing that combination in avail_scan_masks.) > > Add a comment to explain. > Hi Jonathan, It is aimed to reuse the function both for BMP280 and BME280 so that's why is there, it's not in case humidity channel is disabled. I can add a comment it is definitely not obvious. Thanks for pointing this out. By applying the changes that you pointed out + by implementing the changes that you proposed in a previous patch to split the t_fine calculation this patch will become much cleaner, thanks a lot! Cheers, Vasilis > > + size_of_burst_read = 8; > > + else > > + size_of_burst_read = 6; > > + > > + /* Burst read data registers */ > > + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > > + data->buf, 8); > > + if (ret) { > > + dev_err(data->dev, "failed to read sensor data\n"); > > + return ret; > > + } > > + > > + /* Temperature calculations */ > > + memcpy(&chan_value, &data->buf[3], 3); > > + > > + adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value)); > > + if (adc_temp == BMP280_TEMP_SKIPPED) { > > + /* reading was skipped */ > Similar comments on this to below (I read backwards as normally code makes > more sense that way :) > > + dev_err(data->dev, "reading temperature skipped\n"); > > + return -EIO; > > + } > > + data->iio_buf.sensor_data[0] = bmp280_compensate_temp(data, adc_temp); > > + > > + /* Pressure calculations */ > > + memcpy(&chan_value, &data->buf[0], 3); > > + > > + adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value)); > > + if (adc_press == BMP280_PRESS_SKIPPED) { > > + /* reading was skipped */ > > + dev_err(data->dev, "reading pressure skipped\n"); > > + return -EIO; > > + } > > + data->iio_buf.sensor_data[1] = bmp280_compensate_press(data, adc_press); > > + > > + /* Humidity calculations */ > > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) { > > + memcpy(&chan_value, &data->buf[6], 2); > > + > > + adc_humidity = get_unaligned_be16(&chan_value); > > + if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { > > + /* reading was skipped */ > > + dev_err(data->dev, "reading humidity skipped\n"); > > + return -EIO; > > + } > > + data->iio_buf.sensor_data[2] = bmp280_compensate_humidity(data, adc_humidity); > Alignment of code looks wrong. > > > + } > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > > + iio_get_time_ns(indio_dev)); > > + > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > > +static irqreturn_t bmp380_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + s32 adc_temp, adc_press; > > + int ret, chan_value; > > + > > + guard(mutex)(&data->lock); > > + > > + /* Burst read data registers */ > > + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > > + data->buf, 6); > > + if (ret) { > > + dev_err(data->dev, "failed to read sensor data\n"); > > + return ret; > > + } > > + > > + /* Temperature calculations */ > > + memcpy(&chan_value, &data->buf[3], 3); > > + > > + adc_temp = get_unaligned_le24(&chan_value); > > Same comments as below. > > > + if (adc_temp == BMP380_TEMP_SKIPPED) { > > + /* reading was skipped */ > > + dev_err(data->dev, "reading temperature skipped\n"); > > + return -EIO; > > + } > > + data->iio_buf.sensor_data[0] = bmp380_compensate_temp(data, adc_temp); > > + > > + /* Pressure calculations */ > > + memcpy(&chan_value, &data->buf[0], 3); > > + > > + adc_press = get_unaligned_le24(&chan_value); > > + if (adc_press == BMP380_PRESS_SKIPPED) { > > + /* reading was skipped */ > > + dev_err(data->dev, "reading pressure skipped\n"); > > + return -EIO; > > + } > > + data->iio_buf.sensor_data[1] = bmp380_compensate_press(data, adc_press); > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > > + iio_get_time_ns(indio_dev)); > > + > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > > +static irqreturn_t bmp580_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + s32 adc_temp, adc_press; > > + int ret, chan_value; > > + > > + guard(mutex)(&data->lock); > > + > > + /* Burst read data registers */ > > + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, > > + data->buf, 6); > > + if (ret) { > > + dev_err(data->dev, "failed to read sensor data\n"); > > + return ret; > > + } > > + > > + /* Temperature calculations */ > > + memcpy(&chan_value, &data->buf[3], 3); > > + > > + adc_temp = get_unaligned_le24(&chan_value); > > As in other thread branch, get it directly from &data->buf[3] > rather than bouncing it via another buffer. > > > + if (adc_temp == BMP580_TEMP_SKIPPED) { > > + /* reading was skipped */ > > + dev_err(data->dev, "reading temperature skipped\n"); > > + return -EIO; > -EIO isn't irqreturn_t > > There isn't a good way to return errors from interrupt handlers, > so just return IRQ_HANDLED after printing the error message. > > > + } > > + data->iio_buf.sensor_data[0] = adc_temp; > > + > > + /* Pressure calculations */ > > + memcpy(&chan_value, &data->buf[0], 3); > > + > > + adc_press = get_unaligned_le24(&chan_value); > As above, get it directly. > > > + if (adc_press == BMP580_PRESS_SKIPPED) { > > + /* reading was skipped */ > > + dev_err(data->dev, "reading pressure skipped\n"); > > + return -EIO; > return IRQ_HANDLED; > > > + } > > + data->iio_buf.sensor_data[1] = adc_press; > Extra space. > > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > > + iio_get_time_ns(indio_dev)); > > + > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; > > static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; > > static const int bmp580_temp_coeffs[] = { 1000, 16 }; > > @@ -1903,6 +2075,8 @@ const struct bmp280_chip_info bmp580_chip_info = { > > .read_temp = bmp580_read_temp, > > .read_press = bmp580_read_press, > > .preinit = bmp580_preinit, > > + > > + .buffer_handler = bmp580_buffer_handler > > }; > > EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); > > > > @@ -2054,7 +2228,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val) > > return ret; > > > > ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, > > - data->buf, sizeof(data->buf)); > > + data->buf, BMP280_NUM_PRESS_BYTES); > > if (ret) > > return ret; > > > > @@ -2122,6 +2296,35 @@ static int bmp180_chip_config(struct bmp280_data *data) > > return 0; > > } > > > > +static irqreturn_t bmp180_buffer_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bmp280_data *data = iio_priv(indio_dev); > > + int ret, chan_value; > > + > > + guard(mutex)(&data->lock); > > + > > + ret = data->chip_info->read_temp(data, &chan_value); > > + if (ret) > > + return ret; > > + > > + data->iio_buf.sensor_data[0] = chan_value; > > + > > + ret = data->chip_info->read_press(data, &chan_value); > > Given my earlier suggestion to stop hiding t_fine in the > structure, these callbacks will need to bring it all the way out > here (from the temperature read) and pass it back into the > pressure read. > > That will have the pleasing side effect of making it obvious why > we aren't handling subsets of channels (as they aren't > independent) > > > > + if (ret) > > + return ret; > > + > > + data->iio_buf.sensor_data[1] = chan_value; > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, > > + iio_get_time_ns(indio_dev)); > > + > > + iio_trigger_notify_done(indio_dev->trig); > > + > > + return IRQ_HANDLED; > > +} > > + > > > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c > > index a444d4b2978b..dc297583cac1 100644 > > --- a/drivers/iio/pressure/bmp280-spi.c > > +++ b/drivers/iio/pressure/bmp280-spi.c > > @@ -40,14 +40,14 @@ static int bmp380_regmap_spi_read(void *context, const void *reg, > > size_t reg_size, void *val, size_t val_size) > > { > > struct spi_device *spi = to_spi_device(context); > > - u8 rx_buf[4]; > > + u8 rx_buf[9]; > > ssize_t status; > > > > /* > > - * Maximum number of consecutive bytes read for a temperature or > > - * pressure measurement is 3. > > + * Maximum number of a burst read for temperature, pressure, humidity > > + * is 8 bytes. > > Once this 8 is expressed as the sum of the 3 types, you can drop the comment > as it will add nothing useful. > > > */ > > - if (val_size > 3) > > + if (val_size > 8) > > return -EINVAL; > > > > /* > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > index 8cc3eed70c18..32155567faf6 100644 > > --- a/drivers/iio/pressure/bmp280.h > > +++ b/drivers/iio/pressure/bmp280.h > > @@ -301,6 +301,10 @@ > > #define BMP280_PRESS_SKIPPED 0x80000 > > #define BMP280_HUMIDITY_SKIPPED 0x8000 > > > > +#define BMP280_NUM_TEMP_BYTES 3 > > +#define BMP280_NUM_PRESS_BYTES 3 > > +#define BME280_NUM_HUMIDITY_BYTES 2 > > + > > /* Core exported structs */ > > > > static const char *const bmp280_supply_names[] = { > > @@ -400,13 +404,19 @@ struct bmp280_data { > > */ > > s32 t_fine; > > > > + /* Data to push to userspace */ > > + struct { > > + s32 sensor_data[3]; > > This doesn't work (as explanation of data) for all cases. > If you only have 2 channels, the packing needs to be. > > s32 sensor_data[2]; > //no padding > s64 timestamp __aligned(8); > > + s64 timestamp __aligned(8); > > + } iio_buf; > > + > So using a structure for this definition isn't a bug as such as the core > doesn't care that you provide a bigger buffer than needed, but it is misleading. > Use something along lines of > > /* Up to 3 channels and aligned s64 timestamp */ > s32 sensor_data[6] __aligned(8); > > or use a union of two structures to cover the two layouts making > sure to write and read from the correct one in each callback. > > > /* > > * DMA (thus cache coherency maintenance) may require the > > * transfer buffers to live in their own cache lines. > > */ > > union { > > /* Sensor data buffer */ > > - u8 buf[3]; > > + u8 buf[8]; > As in previous discussion - build this up as a sum of what can go in it. > Maybe via a define for BMP280_BURST_READ_MAX >
> > > > > > +static irqreturn_t bmp280_buffer_handler(int irq, void *p) > > > +{ > > > + struct iio_poll_func *pf = p; > > > + struct iio_dev *indio_dev = pf->indio_dev; > > > + struct bmp280_data *data = iio_priv(indio_dev); > > > + s32 adc_temp, adc_press, adc_humidity; > > > + u8 size_of_burst_read; > > > + int ret, chan_value; > > > + > > > + guard(mutex)(&data->lock); > > > + > > > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) > > > > This confuses me a little. Is it allowing reuse of this function for > > multiple devices or aiming to optimise the read in the case of > > the humidity channel being disabled (in which case I don't think > > it works because you aren't providing that combination in avail_scan_masks.) > > > > Add a comment to explain. > > > > Hi Jonathan, > > It is aimed to reuse the function both for BMP280 and BME280 so that's why is > there, it's not in case humidity channel is disabled. I can add a comment it > is definitely not obvious. Thanks for pointing this out. > > By applying the changes that you pointed out + by implementing the changes > that you proposed in a previous patch to split the t_fine calculation this > patch will become much cleaner, thanks a lot! A comment would do the job nicely. Thanks, J
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 ddfcd23f29a0..ffcd17807cf5 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> @@ -454,7 +457,7 @@ static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp) int ret; ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_TEMP_BYTES); if (ret < 0) { dev_err(data->dev, "failed to read temperature\n"); return ret; @@ -484,7 +487,7 @@ static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press) return ret; ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_PRESS_BYTES); if (ret < 0) { dev_err(data->dev, "failed to read pressure\n"); return ret; @@ -513,13 +516,13 @@ static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity) return ret; ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, - &data->be16, sizeof(data->be16)); + &data->be16, BME280_NUM_HUMIDITY_BYTES); if (ret < 0) { dev_err(data->dev, "failed to read humidity\n"); return ret; } - adc_humidity = be16_to_cpu(data->be16); + adc_humidity = get_unaligned_be16(&data->be16); if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { /* reading was skipped */ dev_err(data->dev, "reading humidity skipped\n"); @@ -931,6 +934,73 @@ static int bmp280_chip_config(struct bmp280_data *data) return ret; } +static irqreturn_t bmp280_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + s32 adc_temp, adc_press, adc_humidity; + u8 size_of_burst_read; + int ret, chan_value; + + guard(mutex)(&data->lock); + + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) + size_of_burst_read = 8; + else + size_of_burst_read = 6; + + /* Burst read data registers */ + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, + data->buf, 8); + if (ret) { + dev_err(data->dev, "failed to read sensor data\n"); + return ret; + } + + /* Temperature calculations */ + memcpy(&chan_value, &data->buf[3], 3); + + adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value)); + if (adc_temp == BMP280_TEMP_SKIPPED) { + /* reading was skipped */ + dev_err(data->dev, "reading temperature skipped\n"); + return -EIO; + } + data->iio_buf.sensor_data[0] = bmp280_compensate_temp(data, adc_temp); + + /* Pressure calculations */ + memcpy(&chan_value, &data->buf[0], 3); + + adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value)); + if (adc_press == BMP280_PRESS_SKIPPED) { + /* reading was skipped */ + dev_err(data->dev, "reading pressure skipped\n"); + return -EIO; + } + data->iio_buf.sensor_data[1] = bmp280_compensate_press(data, adc_press); + + /* Humidity calculations */ + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) { + memcpy(&chan_value, &data->buf[6], 2); + + adc_humidity = get_unaligned_be16(&chan_value); + if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { + /* reading was skipped */ + dev_err(data->dev, "reading humidity skipped\n"); + return -EIO; + } + data->iio_buf.sensor_data[2] = bmp280_compensate_humidity(data, adc_humidity); + } + + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, + iio_get_time_ns(indio_dev)); + + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID }; static const int bmp280_temp_coeffs[] = { 10, 1 }; @@ -973,6 +1043,8 @@ const struct bmp280_chip_info bmp280_chip_info = { .read_temp = bmp280_read_temp, .read_press = bmp280_read_press, .read_calib = bmp280_read_calib, + + .buffer_handler = bmp280_buffer_handler }; EXPORT_SYMBOL_NS(bmp280_chip_info, IIO_BMP280); @@ -1032,6 +1104,8 @@ const struct bmp280_chip_info bme280_chip_info = { .read_press = bmp280_read_press, .read_humid = bmp280_read_humid, .read_calib = bme280_read_calib, + + .buffer_handler = bmp280_buffer_handler }; EXPORT_SYMBOL_NS(bme280_chip_info, IIO_BMP280); @@ -1158,7 +1232,7 @@ static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp) int ret; ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_TEMP_BYTES); if (ret) { dev_err(data->dev, "failed to read temperature\n"); return ret; @@ -1187,7 +1261,7 @@ static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press) return ret; ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_PRESS_BYTES); if (ret) { dev_err(data->dev, "failed to read pressure\n"); return ret; @@ -1366,6 +1440,54 @@ static int bmp380_chip_config(struct bmp280_data *data) return 0; } +static irqreturn_t bmp380_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + s32 adc_temp, adc_press; + int ret, chan_value; + + guard(mutex)(&data->lock); + + /* Burst read data registers */ + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, + data->buf, 6); + if (ret) { + dev_err(data->dev, "failed to read sensor data\n"); + return ret; + } + + /* Temperature calculations */ + memcpy(&chan_value, &data->buf[3], 3); + + adc_temp = get_unaligned_le24(&chan_value); + if (adc_temp == BMP380_TEMP_SKIPPED) { + /* reading was skipped */ + dev_err(data->dev, "reading temperature skipped\n"); + return -EIO; + } + data->iio_buf.sensor_data[0] = bmp380_compensate_temp(data, adc_temp); + + /* Pressure calculations */ + memcpy(&chan_value, &data->buf[0], 3); + + adc_press = get_unaligned_le24(&chan_value); + if (adc_press == BMP380_PRESS_SKIPPED) { + /* reading was skipped */ + dev_err(data->dev, "reading pressure skipped\n"); + return -EIO; + } + data->iio_buf.sensor_data[1] = bmp380_compensate_press(data, adc_press); + + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, + iio_get_time_ns(indio_dev)); + + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128}; static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID }; @@ -1408,6 +1530,8 @@ const struct bmp280_chip_info bmp380_chip_info = { .read_press = bmp380_read_press, .read_calib = bmp380_read_calib, .preinit = bmp380_preinit, + + .buffer_handler = bmp380_buffer_handler }; EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280); @@ -1528,7 +1652,7 @@ static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp) int ret; ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, - sizeof(data->buf)); + BMP280_NUM_TEMP_BYTES); if (ret) { dev_err(data->dev, "failed to read temperature\n"); return ret; @@ -1548,7 +1672,7 @@ static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press) int ret; ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, - sizeof(data->buf)); + BMP280_NUM_PRESS_BYTES); if (ret) { dev_err(data->dev, "failed to read pressure\n"); return ret; @@ -1863,6 +1987,54 @@ static int bmp580_chip_config(struct bmp280_data *data) return 0; } +static irqreturn_t bmp580_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + s32 adc_temp, adc_press; + int ret, chan_value; + + guard(mutex)(&data->lock); + + /* Burst read data registers */ + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, + data->buf, 6); + if (ret) { + dev_err(data->dev, "failed to read sensor data\n"); + return ret; + } + + /* Temperature calculations */ + memcpy(&chan_value, &data->buf[3], 3); + + adc_temp = get_unaligned_le24(&chan_value); + if (adc_temp == BMP580_TEMP_SKIPPED) { + /* reading was skipped */ + dev_err(data->dev, "reading temperature skipped\n"); + return -EIO; + } + data->iio_buf.sensor_data[0] = adc_temp; + + /* Pressure calculations */ + memcpy(&chan_value, &data->buf[0], 3); + + adc_press = get_unaligned_le24(&chan_value); + if (adc_press == BMP580_PRESS_SKIPPED) { + /* reading was skipped */ + dev_err(data->dev, "reading pressure skipped\n"); + return -EIO; + } + data->iio_buf.sensor_data[1] = adc_press; + + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, + iio_get_time_ns(indio_dev)); + + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 }; static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT }; static const int bmp580_temp_coeffs[] = { 1000, 16 }; @@ -1903,6 +2075,8 @@ const struct bmp280_chip_info bmp580_chip_info = { .read_temp = bmp580_read_temp, .read_press = bmp580_read_press, .preinit = bmp580_preinit, + + .buffer_handler = bmp580_buffer_handler }; EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); @@ -2054,7 +2228,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val) return ret; ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, - data->buf, sizeof(data->buf)); + data->buf, BMP280_NUM_PRESS_BYTES); if (ret) return ret; @@ -2122,6 +2296,35 @@ static int bmp180_chip_config(struct bmp280_data *data) return 0; } +static irqreturn_t bmp180_buffer_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bmp280_data *data = iio_priv(indio_dev); + int ret, chan_value; + + guard(mutex)(&data->lock); + + ret = data->chip_info->read_temp(data, &chan_value); + if (ret) + return ret; + + data->iio_buf.sensor_data[0] = chan_value; + + ret = data->chip_info->read_press(data, &chan_value); + if (ret) + return ret; + + data->iio_buf.sensor_data[1] = chan_value; + + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf, + iio_get_time_ns(indio_dev)); + + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const int bmp180_oversampling_temp_avail[] = { 1 }; static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID }; @@ -2157,6 +2360,8 @@ const struct bmp280_chip_info bmp180_chip_info = { .read_temp = bmp180_read_temp, .read_press = bmp180_read_press, .read_calib = bmp180_read_calib, + + .buffer_handler = bmp180_buffer_handler }; EXPORT_SYMBOL_NS(bmp180_chip_info, IIO_BMP280); @@ -2332,6 +2537,14 @@ int bmp280_common_probe(struct device *dev, "failed to read calibration coefficients\n"); } + ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev, + iio_pollfunc_store_time, + data->chip_info->buffer_handler, + NULL); + if (ret) + return dev_err_probe(data->dev, ret, + "iio triggered buffer setup failed\n"); + /* * Attempt to grab an optional EOC IRQ - only the BMP085 has this * however as it happens, the BMP085 shares the chip ID of BMP180 diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c index a444d4b2978b..dc297583cac1 100644 --- a/drivers/iio/pressure/bmp280-spi.c +++ b/drivers/iio/pressure/bmp280-spi.c @@ -40,14 +40,14 @@ static int bmp380_regmap_spi_read(void *context, const void *reg, size_t reg_size, void *val, size_t val_size) { struct spi_device *spi = to_spi_device(context); - u8 rx_buf[4]; + u8 rx_buf[9]; ssize_t status; /* - * Maximum number of consecutive bytes read for a temperature or - * pressure measurement is 3. + * Maximum number of a burst read for temperature, pressure, humidity + * is 8 bytes. */ - if (val_size > 3) + if (val_size > 8) return -EINVAL; /* diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index 8cc3eed70c18..32155567faf6 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -301,6 +301,10 @@ #define BMP280_PRESS_SKIPPED 0x80000 #define BMP280_HUMIDITY_SKIPPED 0x8000 +#define BMP280_NUM_TEMP_BYTES 3 +#define BMP280_NUM_PRESS_BYTES 3 +#define BME280_NUM_HUMIDITY_BYTES 2 + /* Core exported structs */ static const char *const bmp280_supply_names[] = { @@ -400,13 +404,19 @@ struct bmp280_data { */ s32 t_fine; + /* Data to push to userspace */ + struct { + s32 sensor_data[3]; + s64 timestamp __aligned(8); + } iio_buf; + /* * DMA (thus cache coherency maintenance) may require the * transfer buffers to live in their own cache lines. */ union { /* Sensor data buffer */ - u8 buf[3]; + u8 buf[8]; /* Calibration data buffers */ __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; @@ -462,6 +472,8 @@ struct bmp280_chip_info { int (*read_humid)(struct bmp280_data *, u32 *); int (*read_calib)(struct bmp280_data *); int (*preinit)(struct bmp280_data *); + + irqreturn_t (*buffer_handler)(int, void *); }; /* Chip infos for each variant */
BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their temperature, pressure and humidity readings. This facilitates the use of burst reads in order to acquire data much faster and in a different way from the one used in oneshot captures. BMP085 and BMP180 use a completely different measurement process that is well defined and is used in their buffer_handler(). Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/pressure/Kconfig | 2 + drivers/iio/pressure/bmp280-core.c | 231 +++++++++++++++++++++++++++-- drivers/iio/pressure/bmp280-spi.c | 8 +- drivers/iio/pressure/bmp280.h | 14 +- 4 files changed, 241 insertions(+), 14 deletions(-)