| Message ID | 20240823172017.9028-1-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: pressure: bmp280: Use char instead of s32 for data buffer | expand |
On Fri, Aug 23, 2024 at 07:20:17PM +0200, Vasileios Amoiridis wrote: > As it was reported and discussed here [1], storing the sensor data in an > endian aware s32 buffer is not optimal. Advertising the timestamp as an > addition of 2 s32 variables which is also implied is again not the best > practice. For that reason, change the s32 sensor_data buffer to a char > buffer with an extra value for the timestamp (as it is common practice). > > [1]: https://lore.kernel.org/linux-iio/73d13cc0-afb9-4306-b498-5d821728c3ba@stanley.mountain/ > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > --- > drivers/iio/pressure/bmp280-core.c | 43 +++++++++++++++++------------- > drivers/iio/pressure/bmp280.h | 5 +++- > 2 files changed, 28 insertions(+), 20 deletions(-) > Hi Jonathan, I noticed that this patch didn't receive any comments nor applied. Was this intentional because we wait for something from mainline to be merged back? Cheers, Vasilis
On Fri, 23 Aug 2024 19:20:17 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > As it was reported and discussed here [1], storing the sensor data in an > endian aware s32 buffer is not optimal. Advertising the timestamp as an > addition of 2 s32 variables which is also implied is again not the best > practice. For that reason, change the s32 sensor_data buffer to a char > buffer with an extra value for the timestamp (as it is common practice). > > [1]: https://lore.kernel.org/linux-iio/73d13cc0-afb9-4306-b498-5d821728c3ba@stanley.mountain/ > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> Hi. Thanks for poking. Looks like I'd accidentally marked this one handled without actually doing so :( > --- > drivers/iio/pressure/bmp280-core.c | 43 +++++++++++++++++------------- > drivers/iio/pressure/bmp280.h | 5 +++- > 2 files changed, 28 insertions(+), 20 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index 3deaa57bb3f5..71e481c2f30d 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -1035,7 +1035,8 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p) > goto out; > } > > - data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); > + ret = bmp280_compensate_temp(data, adc_temp); > + memcpy(&data->buffer.buf[4], &ret, 4); The 4's in here are magic indexes. Probably use sizeof(s32) in both cases. Maybe for the first one make the fact it's after an s32 explicit with &data->buffer.buf[1 * sizeof(s32)] Of carry an offset through the code that you update when you fill in part of the buffer. That is probably cleaner. int offset = 0; stuff offset += sizeof(s32); mempcy(&data->bufer.buf[offset], &ret, 4); if offset is used for a later entry then add the new element. > > /* Pressure calculations */ > adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); > @@ -1045,10 +1046,10 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p) > } > > t_fine = bmp280_calc_t_fine(data, adc_temp); > + ret = bmp280_compensate_press(data, adc_press, t_fine); > + memcpy(&data->buffer.buf[0], &ret, 4); > > - data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); > - > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > iio_get_time_ns(indio_dev)); > > out: > @@ -1148,7 +1149,8 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) > goto out; > } > > - data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); > + ret = bmp280_compensate_temp(data, adc_temp); > + memcpy(&data->buffer.buf[4], &ret, 4); > > /* Pressure calculations */ > adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); > @@ -1158,8 +1160,8 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) > } > > t_fine = bmp280_calc_t_fine(data, adc_temp); > - > - data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); > + ret = bmp280_compensate_press(data, adc_press, t_fine); > + memcpy(&data->buffer.buf[0], &ret, 4); > > /* Humidity calculations */ > adc_humidity = get_unaligned_be16(&data->buf[6]); > @@ -1168,9 +1170,11 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) > dev_err(data->dev, "reading humidity skipped\n"); > goto out; > } > - data->sensor_data[2] = bme280_compensate_humidity(data, adc_humidity, t_fine); > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > + ret = bme280_compensate_humidity(data, adc_humidity, t_fine); > + memcpy(&data->buffer.buf[8], &ret, 4); as above, but here it is I guess after 2 other elements, so 2 * sizeof(s32) > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > iio_get_time_ns(indio_dev)); > > out: > @@ -1628,7 +1632,8 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p) > goto out; > } > > - data->sensor_data[1] = bmp380_compensate_temp(data, adc_temp); > + ret = bmp380_compensate_temp(data, adc_temp); > + memcpy(&data->buffer.buf[4], &ret, 4); > > /* Pressure calculations */ > adc_press = get_unaligned_le24(&data->buf[0]); > @@ -1638,10 +1643,10 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p) > } > > t_fine = bmp380_calc_t_fine(data, adc_temp); > + ret = bmp380_compensate_press(data, adc_press, t_fine); > + memcpy(&data->buffer.buf[0], &ret, 4); > > - data->sensor_data[0] = bmp380_compensate_press(data, adc_press, t_fine); > - > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > iio_get_time_ns(indio_dev)); > > out: > @@ -2203,12 +2208,12 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p) > } > > /* Temperature calculations */ > - memcpy(&data->sensor_data[1], &data->buf[0], 3); > + memcpy(&data->buffer.buf[4], &data->buf[0], 3); > > /* Pressure calculations */ > - memcpy(&data->sensor_data[0], &data->buf[3], 3); > + memcpy(&data->buffer.buf[0], &data->buf[3], 3); > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > iio_get_time_ns(indio_dev)); > > out: > @@ -2522,15 +2527,15 @@ static irqreturn_t bmp180_trigger_handler(int irq, void *p) > if (ret) > goto out; > > - data->sensor_data[1] = chan_value; > + memcpy(&data->buffer.buf[4], &chan_value, 4); > > ret = bmp180_read_press(data, &chan_value); > if (ret) > goto out; > > - data->sensor_data[0] = chan_value; > + memcpy(&data->buffer.buf[0], &chan_value, 4); > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > iio_get_time_ns(indio_dev)); > > out: > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > index ccacc67c1473..a853b6d5bdfa 100644 > --- a/drivers/iio/pressure/bmp280.h > +++ b/drivers/iio/pressure/bmp280.h > @@ -411,7 +411,10 @@ struct bmp280_data { > * Data to push to userspace triggered buffer. Up to 3 channels and > * s64 timestamp, aligned. > */ > - s32 sensor_data[6] __aligned(8); > + struct { > + u8 buf[12]; > + s64 ts __aligned(8); We have a new aligned_s64 that is cleaner for these cases. It's only in my togreg branch currently though so you'll need to base on that to use it. Jonathan > + } buffer; > > /* > * DMA (thus cache coherency maintenance) may require the
On Sat, Sep 14, 2024 at 12:36:11PM +0100, Jonathan Cameron wrote: > On Fri, 23 Aug 2024 19:20:17 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > As it was reported and discussed here [1], storing the sensor data in an > > endian aware s32 buffer is not optimal. Advertising the timestamp as an > > addition of 2 s32 variables which is also implied is again not the best > > practice. For that reason, change the s32 sensor_data buffer to a char > > buffer with an extra value for the timestamp (as it is common practice). > > > > [1]: https://lore.kernel.org/linux-iio/73d13cc0-afb9-4306-b498-5d821728c3ba@stanley.mountain/ > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > Hi. > Thanks for poking. Looks like I'd accidentally marked this one handled > without actually doing so :( > Hi Jonathan, No problem at all!!! > > --- > > drivers/iio/pressure/bmp280-core.c | 43 +++++++++++++++++------------- > > drivers/iio/pressure/bmp280.h | 5 +++- > > 2 files changed, 28 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > index 3deaa57bb3f5..71e481c2f30d 100644 > > --- a/drivers/iio/pressure/bmp280-core.c > > +++ b/drivers/iio/pressure/bmp280-core.c > > @@ -1035,7 +1035,8 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p) > > goto out; > > } > > > > - data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); > > + ret = bmp280_compensate_temp(data, adc_temp); > > + memcpy(&data->buffer.buf[4], &ret, 4); > > The 4's in here are magic indexes. Probably use sizeof(s32) in both cases. > Maybe for the first one make the fact it's after an s32 explicit with > &data->buffer.buf[1 * sizeof(s32)] > > Of carry an offset through the code that you update when you fill in part of > the buffer. That is probably cleaner. > > int offset = 0; > stuff > offset += sizeof(s32); > mempcy(&data->bufer.buf[offset], &ret, 4); > > if offset is used for a later entry then add the new element. > > Looks nice. I could also move both memcpy() after the if's so in case there is an error I save myself from memcpy() once for no reason. > > > > /* Pressure calculations */ > > adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); > > @@ -1045,10 +1046,10 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p) > > } > > > > t_fine = bmp280_calc_t_fine(data, adc_temp); > > + ret = bmp280_compensate_press(data, adc_press, t_fine); > > + memcpy(&data->buffer.buf[0], &ret, 4); > > > > - data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); > > - > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > iio_get_time_ns(indio_dev)); > > > > out: > > @@ -1148,7 +1149,8 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) > > goto out; > > } > > > > - data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); > > + ret = bmp280_compensate_temp(data, adc_temp); > > + memcpy(&data->buffer.buf[4], &ret, 4); > > > > /* Pressure calculations */ > > adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); > > @@ -1158,8 +1160,8 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) > > } > > > > t_fine = bmp280_calc_t_fine(data, adc_temp); > > - > > - data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); > > + ret = bmp280_compensate_press(data, adc_press, t_fine); > > + memcpy(&data->buffer.buf[0], &ret, 4); > > > > /* Humidity calculations */ > > adc_humidity = get_unaligned_be16(&data->buf[6]); > > @@ -1168,9 +1170,11 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) > > dev_err(data->dev, "reading humidity skipped\n"); > > goto out; > > } > > - data->sensor_data[2] = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + ret = bme280_compensate_humidity(data, adc_humidity, t_fine); > > + memcpy(&data->buffer.buf[8], &ret, 4); > as above, but here it is I guess after 2 other elements, so > > 2 * sizeof(s32) > > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > iio_get_time_ns(indio_dev)); > > > > out: > > @@ -1628,7 +1632,8 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p) > > goto out; > > } > > > > - data->sensor_data[1] = bmp380_compensate_temp(data, adc_temp); > > + ret = bmp380_compensate_temp(data, adc_temp); > > + memcpy(&data->buffer.buf[4], &ret, 4); > > > > /* Pressure calculations */ > > adc_press = get_unaligned_le24(&data->buf[0]); > > @@ -1638,10 +1643,10 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p) > > } > > > > t_fine = bmp380_calc_t_fine(data, adc_temp); > > + ret = bmp380_compensate_press(data, adc_press, t_fine); > > + memcpy(&data->buffer.buf[0], &ret, 4); > > > > - data->sensor_data[0] = bmp380_compensate_press(data, adc_press, t_fine); > > - > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > iio_get_time_ns(indio_dev)); > > > > out: > > @@ -2203,12 +2208,12 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p) > > } > > > > /* Temperature calculations */ > > - memcpy(&data->sensor_data[1], &data->buf[0], 3); > > + memcpy(&data->buffer.buf[4], &data->buf[0], 3); > > > > /* Pressure calculations */ > > - memcpy(&data->sensor_data[0], &data->buf[3], 3); > > + memcpy(&data->buffer.buf[0], &data->buf[3], 3); > > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > iio_get_time_ns(indio_dev)); > > > > out: > > @@ -2522,15 +2527,15 @@ static irqreturn_t bmp180_trigger_handler(int irq, void *p) > > if (ret) > > goto out; > > > > - data->sensor_data[1] = chan_value; > > + memcpy(&data->buffer.buf[4], &chan_value, 4); > > > > ret = bmp180_read_press(data, &chan_value); > > if (ret) > > goto out; > > > > - data->sensor_data[0] = chan_value; > > + memcpy(&data->buffer.buf[0], &chan_value, 4); > > > > - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > iio_get_time_ns(indio_dev)); > > > > out: > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > index ccacc67c1473..a853b6d5bdfa 100644 > > --- a/drivers/iio/pressure/bmp280.h > > +++ b/drivers/iio/pressure/bmp280.h > > @@ -411,7 +411,10 @@ struct bmp280_data { > > * Data to push to userspace triggered buffer. Up to 3 channels and > > * s64 timestamp, aligned. > > */ > > - s32 sensor_data[6] __aligned(8); > > + struct { > > + u8 buf[12]; > > + s64 ts __aligned(8); > > We have a new aligned_s64 that is cleaner for these cases. It's only > in my togreg branch currently though so you'll need to base on that > to use it. > > Jonathan > > Looks nice, I will try it. Cheers, Vasilis > > + } buffer; > > > > /* > > * DMA (thus cache coherency maintenance) may require the >
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 3deaa57bb3f5..71e481c2f30d 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -1035,7 +1035,8 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p) goto out; } - data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); + ret = bmp280_compensate_temp(data, adc_temp); + memcpy(&data->buffer.buf[4], &ret, 4); /* Pressure calculations */ adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); @@ -1045,10 +1046,10 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p) } t_fine = bmp280_calc_t_fine(data, adc_temp); + ret = bmp280_compensate_press(data, adc_press, t_fine); + memcpy(&data->buffer.buf[0], &ret, 4); - data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); - - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, iio_get_time_ns(indio_dev)); out: @@ -1148,7 +1149,8 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) goto out; } - data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp); + ret = bmp280_compensate_temp(data, adc_temp); + memcpy(&data->buffer.buf[4], &ret, 4); /* Pressure calculations */ adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0])); @@ -1158,8 +1160,8 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) } t_fine = bmp280_calc_t_fine(data, adc_temp); - - data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine); + ret = bmp280_compensate_press(data, adc_press, t_fine); + memcpy(&data->buffer.buf[0], &ret, 4); /* Humidity calculations */ adc_humidity = get_unaligned_be16(&data->buf[6]); @@ -1168,9 +1170,11 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) dev_err(data->dev, "reading humidity skipped\n"); goto out; } - data->sensor_data[2] = bme280_compensate_humidity(data, adc_humidity, t_fine); - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + ret = bme280_compensate_humidity(data, adc_humidity, t_fine); + memcpy(&data->buffer.buf[8], &ret, 4); + + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, iio_get_time_ns(indio_dev)); out: @@ -1628,7 +1632,8 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p) goto out; } - data->sensor_data[1] = bmp380_compensate_temp(data, adc_temp); + ret = bmp380_compensate_temp(data, adc_temp); + memcpy(&data->buffer.buf[4], &ret, 4); /* Pressure calculations */ adc_press = get_unaligned_le24(&data->buf[0]); @@ -1638,10 +1643,10 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p) } t_fine = bmp380_calc_t_fine(data, adc_temp); + ret = bmp380_compensate_press(data, adc_press, t_fine); + memcpy(&data->buffer.buf[0], &ret, 4); - data->sensor_data[0] = bmp380_compensate_press(data, adc_press, t_fine); - - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, iio_get_time_ns(indio_dev)); out: @@ -2203,12 +2208,12 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p) } /* Temperature calculations */ - memcpy(&data->sensor_data[1], &data->buf[0], 3); + memcpy(&data->buffer.buf[4], &data->buf[0], 3); /* Pressure calculations */ - memcpy(&data->sensor_data[0], &data->buf[3], 3); + memcpy(&data->buffer.buf[0], &data->buf[3], 3); - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, iio_get_time_ns(indio_dev)); out: @@ -2522,15 +2527,15 @@ static irqreturn_t bmp180_trigger_handler(int irq, void *p) if (ret) goto out; - data->sensor_data[1] = chan_value; + memcpy(&data->buffer.buf[4], &chan_value, 4); ret = bmp180_read_press(data, &chan_value); if (ret) goto out; - data->sensor_data[0] = chan_value; + memcpy(&data->buffer.buf[0], &chan_value, 4); - iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data, + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, iio_get_time_ns(indio_dev)); out: diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index ccacc67c1473..a853b6d5bdfa 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -411,7 +411,10 @@ struct bmp280_data { * Data to push to userspace triggered buffer. Up to 3 channels and * s64 timestamp, aligned. */ - s32 sensor_data[6] __aligned(8); + struct { + u8 buf[12]; + s64 ts __aligned(8); + } buffer; /* * DMA (thus cache coherency maintenance) may require the
As it was reported and discussed here [1], storing the sensor data in an endian aware s32 buffer is not optimal. Advertising the timestamp as an addition of 2 s32 variables which is also implied is again not the best practice. For that reason, change the s32 sensor_data buffer to a char buffer with an extra value for the timestamp (as it is common practice). [1]: https://lore.kernel.org/linux-iio/73d13cc0-afb9-4306-b498-5d821728c3ba@stanley.mountain/ Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 43 +++++++++++++++++------------- drivers/iio/pressure/bmp280.h | 5 +++- 2 files changed, 28 insertions(+), 20 deletions(-)