On Sun, 7 Aug 2022 13:55:15 +0200
Angel Iglesias <ang.iglesiasg@xxxxxxxxx> wrote:
> Adds DMA-safe buffers to driver data struct to store raw data from sensors
>
> The multiple buffers used thorough the driver share the same memory
> allocated as part of the device data instance. The union containing
> the buffers is aligned to allow safe usage with DMA operations, such
> as regmap bulk read calls.
>
> Updated measurement and calibration reading functions to use the new,
> DMA-safe, buffers.
>
> Suggested-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> Signed-off-by: Angel Iglesias <ang.iglesiasg@xxxxxxxxx>
Hi Angel,
As you are doing a v6 anyway to address Andy's review, I
took another look and have a few additional comments below.
> ---
> drivers/iio/pressure/bmp280-core.c | 122 ++++++++++++++---------------
> drivers/iio/pressure/bmp280.h | 3 +
> 2 files changed, 64 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index a109c2609896..4cd657dcbfed 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -31,6 +31,7 @@
> #include <linux/completion.h>
> #include <linux/pm_runtime.h>
> #include <linux/random.h>
> +#include <asm/unaligned.h>
>
> #include "bmp280.h"
>
> @@ -106,6 +107,18 @@ struct bmp280_data {
> * calculation.
> */
> s32 t_fine;
> +
> + /*
> + * DMA (thus cache coherency maintenance) may require the
> + * transfer buffers to live in their own cache lines.
> + */
> + union {
> + /* sensor data buffer */
> + u8 data[3];
__le16 le16;
__be16 be16;
added here would simplify some of the other changes below.
> + /* calibration data buffers */
> + __le16 bmp280_cal[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> + __be16 bmp180_cal[BMP180_REG_CALIB_COUNT / 2];
> + } buf __aligned(IIO_DMA_MINALIGN);
> };
>
> struct bmp280_chip_info {
> @@ -162,41 +175,29 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
> {
> int ret;
> unsigned int tmp;
> - __le16 l16;
> - __be16 b16;
> struct device *dev = data->dev;
> + __le16 *t_buf = data->buf.bmp280_cal;
> + __le16 *p_buf = &data->buf.bmp280_cal[T3+1];
spaces around +
> struct bmp280_calib *calib = &data->calib.bmp280;
> - __le16 t_buf[BMP280_COMP_TEMP_REG_COUNT / 2];
> - __le16 p_buf[BMP280_COMP_PRESS_REG_COUNT / 2];
>
> /* Read temperature calibration values. */
> ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
> - t_buf, BMP280_COMP_TEMP_REG_COUNT);
> + data->buf.bmp280_cal, sizeof(data->buf.bmp280_cal));
Given we don't use the local variable until later (as can't get the size from it,
I would suggest only setting it further down.
> if (ret < 0) {
> dev_err(data->dev,
> "failed to read temperature calibration parameters\n");
> return ret;
> }
>
> - /* Toss the temperature calibration data into the entropy pool */
> - add_device_randomness(t_buf, sizeof(t_buf));
> + /* Toss the temperature and pressure calibration data into the entropy pool */
> + add_device_randomness(data->buf.bmp280_cal, sizeof(data->buf.bmp280_cal));
>
like here.
t_buf = data->bmp280_cal;
or just don't bother with those at all as they don't greatly help readability over
calib->T1 = le16_to_cpu(data->bmp280_cal[T1]);
similar for p_buf.
Note I haven't looked at the existing code to see if there are other reasons to
use t_buf etc, so may be wrong on this!
It will as make this patch more complex even if the code ends up simpler as a result.
> + /* parse temperature calibration data */
> calib->T1 = le16_to_cpu(t_buf[T1]);
> calib->T2 = le16_to_cpu(t_buf[T2]);
> calib->T3 = le16_to_cpu(t_buf[T3]);
>
> - /* Read pressure calibration values. */
> - ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
> - p_buf, BMP280_COMP_PRESS_REG_COUNT);
> - if (ret < 0) {
> - dev_err(data->dev,
> - "failed to read pressure calibration parameters\n");
> - return ret;
> - }
> -
> - /* Toss the pressure calibration data into the entropy pool */
> - add_device_randomness(p_buf, sizeof(p_buf));
> -
> + /* parse pressure calibration data */
> calib->P1 = le16_to_cpu(p_buf[P1]);
> calib->P2 = le16_to_cpu(p_buf[P2]);
> calib->P3 = le16_to_cpu(p_buf[P3]);
> @@ -224,12 +225,12 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
> }
> calib->H1 = tmp;
>
> - ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H2, &l16, 2);
> + ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H2, data->buf.data, 2);
Bit nasty to read little endian into buf.data which I think is a u8 array.
Maybe we need another entry in that union for each of le16 and be16 similar to the
local variables previously in this function?
> if (ret < 0) {
> dev_err(dev, "failed to read H2 comp value\n");
> return ret;
> }
> - calib->H2 = sign_extend32(le16_to_cpu(l16), 15);
> + calib->H2 = get_unaligned_le16(data->buf.data);
>
> ret = regmap_read(data->regmap, BMP280_REG_COMP_H3, &tmp);
> if (ret < 0) {
> @@ -238,20 +239,20 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
> }
> calib->H3 = tmp;
>
> - ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H4, &b16, 2);
> + ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H4, data->buf.data, 2);
> if (ret < 0) {
> dev_err(dev, "failed to read H4 comp value\n");
> return ret;
> }
> - calib->H4 = sign_extend32(((be16_to_cpu(b16) >> 4) & 0xff0) |
> - (be16_to_cpu(b16) & 0xf), 11);
> + calib->H4 = sign_extend32(((get_unaligned_be16(data->buf.data) >> 4) & 0xff0) |
> + (get_unaligned_be16(data->buf.data) & 0xf), 11);
>
> - ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H5, &l16, 2);
> + ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H5, data->buf.data, 2);
> if (ret < 0) {
> dev_err(dev, "failed to read H5 comp value\n");
> return ret;
> }
> - calib->H5 = sign_extend32(((le16_to_cpu(l16) >> 4) & 0xfff), 11);
> + calib->H5 = sign_extend32(((get_unaligned_le16(data->buf.data) >> 4) & 0xfff), 11);
>
> ret = regmap_read(data->regmap, BMP280_REG_COMP_H6, &tmp);
> if (ret < 0) {
...
> static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> {
> - __be16 tmp;
> int ret;
> s32 adc_humidity;
> u32 comp_humidity;
> @@ -420,13 +420,14 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> if (ret < 0)
> return ret;
>
> - ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, &tmp, 2);
> + ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> + data->buf.data, 2);
> if (ret < 0) {
> dev_err(data->dev, "failed to read humidity\n");
> return ret;
> }
>
> - adc_humidity = be16_to_cpu(tmp);
> + adc_humidity = get_unaligned_be16(data->buf.data);
We are only unaligned because data is u8 and a dumb compiler might not realize the
alignment is forced. Use the suggestion above of a couple more entries in the
union and we can switch back to be16_to_cpu()
> if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
> /* reading was skipped */
> dev_err(data->dev, "reading humidity skipped\n");
> @@ -767,7 +768,6 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
>
