| Message ID | 20240303165300.468011-5-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Series to add triggered buffer support to BMP280 driver | expand |
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;
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;
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 Mon, 4 Mar 2024 21:05:47 +0100 Vasileios Amoiridis <vassilisamir@gmail.com> 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:53:00 +0100 Vasileios Amoiridis <vassilisamir@gmail.com> 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 <vassilisamir@gmail.com> 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.
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.
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 <vassilisamir@gmail.com> --- 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(-)