2022-07-23 17:41:34

by Angel Iglesias

[permalink] [raw]
Subject: [PATCH v4 3/5] iio: pressure: bmp280: Fix alignment for DMA safety

Adds a DMA-safe buffer to driver data struct to store raw data from sensor

Updated calibration read functions to use DMA-safe buffers as well.
In this case, as calibration data is only read at sensor initialization,
the buffers are allocated with kmalloc and freed after being processed.

Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: Angel Iglesias <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 164 ++++++++++++++++++-----------
1 file changed, 103 insertions(+), 61 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 60fba199c7a0..9311eeec8473 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -29,6 +29,7 @@
#include <linux/completion.h>
#include <linux/pm_runtime.h>
#include <linux/random.h>
+#include <asm/unaligned.h>

#include "bmp280.h"

@@ -36,7 +37,19 @@
* These enums are used for indexing into the array of calibration
* coefficients for BMP180.
*/
-enum { AC1, AC2, AC3, AC4, AC5, AC6, B1, B2, MB, MC, MD };
+enum {
+ AC1 = 0,
+ AC2 = 2,
+ AC3 = 4,
+ AC4 = 6,
+ AC5 = 8,
+ AC6 = 10,
+ B1 = 12,
+ B2 = 14,
+ MB = 16,
+ MC = 18,
+ MD = 20,
+};

struct bmp180_calib {
s16 AC1;
@@ -104,6 +117,12 @@ struct bmp280_data {
* calculation.
*/
s32 t_fine;
+
+ /*
+ * DMA (thus cache coherency maintenance) may require the
+ * transfer buffers to live in their own cache lines.
+ */
+ u8 buf[3] __aligned(IIO_DMA_MINALIGN);
};

struct bmp280_chip_info {
@@ -135,8 +154,18 @@ struct bmp280_chip_info {
* These enums are used for indexing into the array of compensation
* parameters for BMP280.
*/
-enum { T1, T2, T3 };
-enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
+enum { T1 = 0, T2 = 2, T3 = 4 };
+enum {
+ P1 = 0,
+ P2 = 2,
+ P3 = 4,
+ P4 = 6,
+ P5 = 8,
+ P6 = 10,
+ P7 = 12,
+ P8 = 14,
+ P9 = 16,
+};

static const struct iio_chan_spec bmp280_channels[] = {
{
@@ -159,51 +188,59 @@ static const struct iio_chan_spec bmp280_channels[] = {
static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
{
int ret;
+ u8 *t_buf;
+ u8 *p_buf;
unsigned int tmp;
- __le16 l16;
- __be16 b16;
struct device *dev = data->dev;
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];

+ t_buf = kmalloc(BMP280_COMP_TEMP_REG_COUNT, GFP_KERNEL);
+ if (!t_buf)
+ return -ENOMEM;
/* Read temperature calibration values. */
ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
t_buf, BMP280_COMP_TEMP_REG_COUNT);
if (ret < 0) {
dev_err(data->dev,
"failed to read temperature calibration parameters\n");
+ kfree(t_buf);
return ret;
}

/* Toss the temperature calibration data into the entropy pool */
add_device_randomness(t_buf, sizeof(t_buf));

- calib->T1 = le16_to_cpu(t_buf[T1]);
- calib->T2 = le16_to_cpu(t_buf[T2]);
- calib->T3 = le16_to_cpu(t_buf[T3]);
+ calib->T1 = get_unaligned_le16(&t_buf[T1]);
+ calib->T2 = get_unaligned_le16(&t_buf[T2]);
+ calib->T3 = get_unaligned_le16(&t_buf[T3]);
+ kfree(t_buf);

+ p_buf = kmalloc(BMP280_COMP_PRESS_REG_COUNT, GFP_KERNEL);
+ if (!p_buf)
+ return -ENOMEM;
/* 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");
+ kfree(p_buf);
return ret;
}

/* Toss the pressure calibration data into the entropy pool */
add_device_randomness(p_buf, sizeof(p_buf));

- calib->P1 = le16_to_cpu(p_buf[P1]);
- calib->P2 = le16_to_cpu(p_buf[P2]);
- calib->P3 = le16_to_cpu(p_buf[P3]);
- calib->P4 = le16_to_cpu(p_buf[P4]);
- calib->P5 = le16_to_cpu(p_buf[P5]);
- calib->P6 = le16_to_cpu(p_buf[P6]);
- calib->P7 = le16_to_cpu(p_buf[P7]);
- calib->P8 = le16_to_cpu(p_buf[P8]);
- calib->P9 = le16_to_cpu(p_buf[P9]);
+ calib->P1 = get_unaligned_le16(&p_buf[P1]);
+ calib->P2 = get_unaligned_le16(&p_buf[P2]);
+ calib->P3 = get_unaligned_le16(&p_buf[P3]);
+ calib->P4 = get_unaligned_le16(&p_buf[P4]);
+ calib->P5 = get_unaligned_le16(&p_buf[P5]);
+ calib->P6 = get_unaligned_le16(&p_buf[P6]);
+ calib->P7 = get_unaligned_le16(&p_buf[P7]);
+ calib->P8 = get_unaligned_le16(&p_buf[P8]);
+ calib->P9 = get_unaligned_le16(&p_buf[P9]);
+ kfree(p_buf);

/*
* Read humidity calibration values.
@@ -222,12 +259,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, 2);
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);

ret = regmap_read(data->regmap, BMP280_REG_COMP_H3, &tmp);
if (ret < 0) {
@@ -236,20 +273,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, 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) >> 4) & 0xff0) |
+ (get_unaligned_be16(data->buf) & 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, 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) >> 4) & 0xfff), 11);

ret = regmap_read(data->regmap, BMP280_REG_COMP_H6, &tmp);
if (ret < 0) {
@@ -344,16 +381,15 @@ static int bmp280_read_temp(struct bmp280_data *data,
int *val)
{
int ret;
- __be32 tmp = 0;
s32 adc_temp, comp_temp;

- ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, &tmp, 3);
+ ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, data->buf, 3);
if (ret < 0) {
dev_err(data->dev, "failed to read temperature\n");
return ret;
}

- adc_temp = be32_to_cpu(tmp) >> 12;
+ adc_temp = get_unaligned_be24(data->buf) >> 4;
if (adc_temp == BMP280_TEMP_SKIPPED) {
/* reading was skipped */
dev_err(data->dev, "reading temperature skipped\n");
@@ -377,7 +413,6 @@ static int bmp280_read_press(struct bmp280_data *data,
int *val, int *val2)
{
int ret;
- __be32 tmp = 0;
s32 adc_press;
u32 comp_press;

@@ -386,13 +421,13 @@ static int bmp280_read_press(struct bmp280_data *data,
if (ret < 0)
return ret;

- ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, &tmp, 3);
+ ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, data->buf, 3);
if (ret < 0) {
dev_err(data->dev, "failed to read pressure\n");
return ret;
}

- adc_press = be32_to_cpu(tmp) >> 12;
+ adc_press = get_unaligned_be24(data->buf) >> 4;
if (adc_press == BMP280_PRESS_SKIPPED) {
/* reading was skipped */
dev_err(data->dev, "reading pressure skipped\n");
@@ -408,7 +443,6 @@ static int bmp280_read_press(struct bmp280_data *data,

static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
{
- __be16 tmp;
int ret;
s32 adc_humidity;
u32 comp_humidity;
@@ -418,13 +452,13 @@ 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, 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);
if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
/* reading was skipped */
dev_err(data->dev, "reading humidity skipped\n");
@@ -755,56 +789,65 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)

static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
{
- __be16 tmp;
int ret;

ret = bmp180_measure(data, BMP180_MEAS_TEMP);
if (ret)
return ret;

- ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 2);
+ ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, data->buf, 2);
if (ret)
return ret;

- *val = be16_to_cpu(tmp);
+ *val = get_unaligned_be16(data->buf);

return 0;
}

static int bmp180_read_calib(struct bmp280_data *data, unsigned int chip)
{
+ struct bmp180_calib *calib = &data->calib.bmp180;
+ u8 *buf;
int ret;
int i;
- struct bmp180_calib *calib = &data->calib.bmp180;
- __be16 buf[BMP180_REG_CALIB_COUNT / 2];
+
+ buf = kmalloc(BMP180_REG_CALIB_COUNT, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;

ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,
- sizeof(buf));
+ BMP180_REG_CALIB_COUNT);

- if (ret < 0)
+ if (ret < 0) {
+ kfree(buf);
return ret;
+ }

/* None of the words has the value 0 or 0xFFFF */
- for (i = 0; i < ARRAY_SIZE(buf); i++) {
- if (buf[i] == cpu_to_be16(0) || buf[i] == cpu_to_be16(0xffff))
+ for (i = 0; i < BMP180_REG_CALIB_COUNT; i += 2) {
+ if (get_unaligned_be16(&buf[i]) == cpu_to_be16(0) ||
+ get_unaligned_be16(&buf[i]) == cpu_to_be16(0xffff)) {
+ kfree(buf);
return -EIO;
+ }
}

/* Toss the calibration data into the entropy pool */
- add_device_randomness(buf, sizeof(buf));
-
- calib->AC1 = be16_to_cpu(buf[AC1]);
- calib->AC2 = be16_to_cpu(buf[AC2]);
- calib->AC3 = be16_to_cpu(buf[AC3]);
- calib->AC4 = be16_to_cpu(buf[AC4]);
- calib->AC5 = be16_to_cpu(buf[AC5]);
- calib->AC6 = be16_to_cpu(buf[AC6]);
- calib->B1 = be16_to_cpu(buf[B1]);
- calib->B2 = be16_to_cpu(buf[B2]);
- calib->MB = be16_to_cpu(buf[MB]);
- calib->MC = be16_to_cpu(buf[MC]);
- calib->MD = be16_to_cpu(buf[MD]);
-
+ add_device_randomness(buf, BMP180_REG_CALIB_COUNT);
+
+ calib->AC1 = get_unaligned_be16(&buf[AC1]);
+ calib->AC2 = get_unaligned_be16(&buf[AC2]);
+ calib->AC3 = get_unaligned_be16(&buf[AC3]);
+ calib->AC4 = get_unaligned_be16(&buf[AC4]);
+ calib->AC5 = get_unaligned_be16(&buf[AC5]);
+ calib->AC6 = get_unaligned_be16(&buf[AC6]);
+ calib->B1 = get_unaligned_be16(&buf[B1]);
+ calib->B2 = get_unaligned_be16(&buf[B2]);
+ calib->MB = get_unaligned_be16(&buf[MB]);
+ calib->MC = get_unaligned_be16(&buf[MC]);
+ calib->MD = get_unaligned_be16(&buf[MD]);
+
+ kfree(buf);
return 0;
}

@@ -852,18 +895,17 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val)
static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
{
int ret;
- __be32 tmp = 0;
u8 oss = data->oversampling_press;

ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
if (ret)
return ret;

- ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 3);
+ ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, data->buf, 3);
if (ret)
return ret;

- *val = (be32_to_cpu(tmp) >> 8) >> (8 - oss);
+ *val = get_unaligned_be24(data->buf) >> (8 - oss);

return 0;
}
--
2.37.1


2022-07-25 21:29:51

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 3/5] iio: pressure: bmp280: Fix alignment for DMA safety

On Sat, Jul 23, 2022 at 7:40 PM Angel Iglesias <[email protected]> wrote:
>
> Adds a DMA-safe buffer to driver data struct to store raw data from sensor
>
> Updated calibration read functions to use DMA-safe buffers as well.
> In this case, as calibration data is only read at sensor initialization,
> the buffers are allocated with kmalloc and freed after being processed.

> Suggested-by: Jonathan Cameron <[email protected]>

Jonathan, maybe it's easier to fix in regmap once for all (something
like a new flag there dma_safe)? Also a side note: i2c doesn't need
that, since it has its own implementation.

P.S. Use sizeof() in regmap calls where it's appropriate.

--
With Best Regards,
Andy Shevchenko

2022-07-31 16:04:04

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v4 3/5] iio: pressure: bmp280: Fix alignment for DMA safety

On Mon, 25 Jul 2022 23:08:47 +0200
Andy Shevchenko <[email protected]> wrote:

> On Sat, Jul 23, 2022 at 7:40 PM Angel Iglesias <[email protected]> wrote:
> >
> > Adds a DMA-safe buffer to driver data struct to store raw data from sensor
> >
> > Updated calibration read functions to use DMA-safe buffers as well.
> > In this case, as calibration data is only read at sensor initialization,
> > the buffers are allocated with kmalloc and freed after being processed.
>
> > Suggested-by: Jonathan Cameron <[email protected]>
>
> Jonathan, maybe it's easier to fix in regmap once for all (something
> like a new flag there dma_safe)? Also a side note: i2c doesn't need
> that, since it has its own implementation.

Possibly... Not something I want to tie up with an individual driver though
as it might take a while and be (somewhat) controversial. Nice idea though as, if
a bounce buffer is needed for a particular regmap operation it can implement one
and when not, it can elide that step and just use the provided buffer directly.

Jonathan

>
> P.S. Use sizeof() in regmap calls where it's appropriate.
>


2022-07-31 16:18:36

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v4 3/5] iio: pressure: bmp280: Fix alignment for DMA safety

On Sat, 23 Jul 2022 19:39:10 +0200
Angel Iglesias <[email protected]> wrote:

> Adds a DMA-safe buffer to driver data struct to store raw data from sensor
>
> Updated calibration read functions to use DMA-safe buffers as well.
> In this case, as calibration data is only read at sensor initialization,
> the buffers are allocated with kmalloc and freed after being processed.
>
> Suggested-by: Jonathan Cameron <[email protected]>
> Signed-off-by: Angel Iglesias <[email protected]>
A few comments inline. This is more complex than it needs to be because
you can still use __be16 *buf; and index in the right steps. Side effect
being it's aligned, so conversions will be cheaper as well.

> ---
> drivers/iio/pressure/bmp280-core.c | 164 ++++++++++++++++++-----------
> 1 file changed, 103 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 60fba199c7a0..9311eeec8473 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -29,6 +29,7 @@
> #include <linux/completion.h>
> #include <linux/pm_runtime.h>
> #include <linux/random.h>
> +#include <asm/unaligned.h>
>
> #include "bmp280.h"
>
> @@ -36,7 +37,19 @@
> * These enums are used for indexing into the array of calibration
> * coefficients for BMP180.
> */
> -enum { AC1, AC2, AC3, AC4, AC5, AC6, B1, B2, MB, MC, MD };
> +enum {
> + AC1 = 0,
> + AC2 = 2,
> + AC3 = 4,
> + AC4 = 6,
> + AC5 = 8,
> + AC6 = 10,
> + B1 = 12,
> + B2 = 14,
> + MB = 16,
> + MC = 18,
> + MD = 20,
Looks like we are specifying a size wrong somewhere as these are all
multiples of 2.

> +};
>
> struct bmp180_calib {
> s16 AC1;
> @@ -104,6 +117,12 @@ struct bmp280_data {
> * calculation.
> */
> s32 t_fine;
> +
> + /*
> + * DMA (thus cache coherency maintenance) may require the
> + * transfer buffers to live in their own cache lines.
> + */
> + u8 buf[3] __aligned(IIO_DMA_MINALIGN);
So, on many platforms of interest (anything arm64) we are going to be
padding this struture out after this to 128 bytes anyway. So no advantage
to keeping this short. I'd just use this buffer for the calibration cases
as well and drop the extra allocations. You'll waste space on x86/x86_64 but
meh, who cares about those legacy architectures :)


> };
>
> struct bmp280_chip_info {
> @@ -135,8 +154,18 @@ struct bmp280_chip_info {
> * These enums are used for indexing into the array of compensation
> * parameters for BMP280.
> */
> -enum { T1, T2, T3 };
> -enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
> +enum { T1 = 0, T2 = 2, T3 = 4 };
> +enum {
> + P1 = 0,
> + P2 = 2,
> + P3 = 4,
> + P4 = 6,
> + P5 = 8,
> + P6 = 10,
> + P7 = 12,
> + P8 = 14,
> + P9 = 16,
> +};
>
> static const struct iio_chan_spec bmp280_channels[] = {
> {
> @@ -159,51 +188,59 @@ static const struct iio_chan_spec bmp280_channels[] = {
> static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
> {
> int ret;
> + u8 *t_buf;
> + u8 *p_buf;
> unsigned int tmp;
> - __le16 l16;
> - __be16 b16;
> struct device *dev = data->dev;
> 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];
>
> + t_buf = kmalloc(BMP280_COMP_TEMP_REG_COUNT, GFP_KERNEL);
> + if (!t_buf)
> + return -ENOMEM;
> /* Read temperature calibration values. */
> ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
> t_buf, BMP280_COMP_TEMP_REG_COUNT);
> if (ret < 0) {
> dev_err(data->dev,
> "failed to read temperature calibration parameters\n");
> + kfree(t_buf);
> return ret;
> }
>
> /* Toss the temperature calibration data into the entropy pool */
> add_device_randomness(t_buf, sizeof(t_buf));
>
> - calib->T1 = le16_to_cpu(t_buf[T1]);
> - calib->T2 = le16_to_cpu(t_buf[T2]);
> - calib->T3 = le16_to_cpu(t_buf[T3]);
> + calib->T1 = get_unaligned_le16(&t_buf[T1]);
> + calib->T2 = get_unaligned_le16(&t_buf[T2]);
> + calib->T3 = get_unaligned_le16(&t_buf[T3]);
> + kfree(t_buf);
>
> + p_buf = kmalloc(BMP280_COMP_PRESS_REG_COUNT, GFP_KERNEL);
> + if (!p_buf)
> + return -ENOMEM;
> /* 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");
> + kfree(p_buf);
> return ret;
> }
>
> /* Toss the pressure calibration data into the entropy pool */
> add_device_randomness(p_buf, sizeof(p_buf));
>
> - calib->P1 = le16_to_cpu(p_buf[P1]);
> - calib->P2 = le16_to_cpu(p_buf[P2]);
> - calib->P3 = le16_to_cpu(p_buf[P3]);
> - calib->P4 = le16_to_cpu(p_buf[P4]);
> - calib->P5 = le16_to_cpu(p_buf[P5]);
> - calib->P6 = le16_to_cpu(p_buf[P6]);
> - calib->P7 = le16_to_cpu(p_buf[P7]);
> - calib->P8 = le16_to_cpu(p_buf[P8]);
> - calib->P9 = le16_to_cpu(p_buf[P9]);
> + calib->P1 = get_unaligned_le16(&p_buf[P1]);
> + calib->P2 = get_unaligned_le16(&p_buf[P2]);
> + calib->P3 = get_unaligned_le16(&p_buf[P3]);
> + calib->P4 = get_unaligned_le16(&p_buf[P4]);
> + calib->P5 = get_unaligned_le16(&p_buf[P5]);
> + calib->P6 = get_unaligned_le16(&p_buf[P6]);
> + calib->P7 = get_unaligned_le16(&p_buf[P7]);
> + calib->P8 = get_unaligned_le16(&p_buf[P8]);
> + calib->P9 = get_unaligned_le16(&p_buf[P9]);
> + kfree(p_buf);
>
> /*
> * Read humidity calibration values.
> @@ -222,12 +259,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, 2);
> 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);
>
> ret = regmap_read(data->regmap, BMP280_REG_COMP_H3, &tmp);
> if (ret < 0) {
> @@ -236,20 +273,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, 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) >> 4) & 0xff0) |
> + (get_unaligned_be16(data->buf) & 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, 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) >> 4) & 0xfff), 11);
>
> ret = regmap_read(data->regmap, BMP280_REG_COMP_H6, &tmp);
> if (ret < 0) {
> @@ -344,16 +381,15 @@ static int bmp280_read_temp(struct bmp280_data *data,
> int *val)
> {
> int ret;
> - __be32 tmp = 0;
> s32 adc_temp, comp_temp;
>
> - ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, &tmp, 3);
> + ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, data->buf, 3);
> if (ret < 0) {
> dev_err(data->dev, "failed to read temperature\n");
> return ret;
> }
>
> - adc_temp = be32_to_cpu(tmp) >> 12;
> + adc_temp = get_unaligned_be24(data->buf) >> 4;
> if (adc_temp == BMP280_TEMP_SKIPPED) {
> /* reading was skipped */
> dev_err(data->dev, "reading temperature skipped\n");
> @@ -377,7 +413,6 @@ static int bmp280_read_press(struct bmp280_data *data,
> int *val, int *val2)
> {
> int ret;
> - __be32 tmp = 0;
> s32 adc_press;
> u32 comp_press;
>
> @@ -386,13 +421,13 @@ static int bmp280_read_press(struct bmp280_data *data,
> if (ret < 0)
> return ret;
>
> - ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, &tmp, 3);
> + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, data->buf, 3);
> if (ret < 0) {
> dev_err(data->dev, "failed to read pressure\n");
> return ret;
> }
>
> - adc_press = be32_to_cpu(tmp) >> 12;
> + adc_press = get_unaligned_be24(data->buf) >> 4;
> if (adc_press == BMP280_PRESS_SKIPPED) {
> /* reading was skipped */
> dev_err(data->dev, "reading pressure skipped\n");
> @@ -408,7 +443,6 @@ static int bmp280_read_press(struct bmp280_data *data,
>
> static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> {
> - __be16 tmp;
> int ret;
> s32 adc_humidity;
> u32 comp_humidity;
> @@ -418,13 +452,13 @@ 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, 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);
> if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
> /* reading was skipped */
> dev_err(data->dev, "reading humidity skipped\n");
> @@ -755,56 +789,65 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
>
> static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
> {
> - __be16 tmp;
> int ret;
>
> ret = bmp180_measure(data, BMP180_MEAS_TEMP);
> if (ret)
> return ret;
>
> - ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 2);
> + ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, data->buf, 2);
> if (ret)
> return ret;
>
> - *val = be16_to_cpu(tmp);
> + *val = get_unaligned_be16(data->buf);
>
> return 0;
> }
>
> static int bmp180_read_calib(struct bmp280_data *data, unsigned int chip)
> {
> + struct bmp180_calib *calib = &data->calib.bmp180;
> + u8 *buf;
> int ret;
> int i;
> - struct bmp180_calib *calib = &data->calib.bmp180;
> - __be16 buf[BMP180_REG_CALIB_COUNT / 2];
> +
> + buf = kmalloc(BMP180_REG_CALIB_COUNT, GFP_KERNEL);
No need to change the type of buf. Keep it as __be16 *buf = kmalloc()...
That way your indexing still works nicely without needing to scatter *2 and += 2
throughout. Note that if you embed this as I suggest above, just use a union so
you can overlap it with the u8 buffer.

> + if (!buf)
> + return -ENOMEM;
>
> ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,
> - sizeof(buf));
> + BMP180_REG_CALIB_COUNT);
>
> - if (ret < 0)
> + if (ret < 0) {
> + kfree(buf);
> return ret;
> + }
>
> /* None of the words has the value 0 or 0xFFFF */
> - for (i = 0; i < ARRAY_SIZE(buf); i++) {
> - if (buf[i] == cpu_to_be16(0) || buf[i] == cpu_to_be16(0xffff))
> + for (i = 0; i < BMP180_REG_CALIB_COUNT; i += 2) {
> + if (get_unaligned_be16(&buf[i]) == cpu_to_be16(0) ||
> + get_unaligned_be16(&buf[i]) == cpu_to_be16(0xffff)) {
> + kfree(buf);
> return -EIO;
> + }
> }
>
> /* Toss the calibration data into the entropy pool */
> - add_device_randomness(buf, sizeof(buf));
> -
> - calib->AC1 = be16_to_cpu(buf[AC1]);
> - calib->AC2 = be16_to_cpu(buf[AC2]);
> - calib->AC3 = be16_to_cpu(buf[AC3]);
> - calib->AC4 = be16_to_cpu(buf[AC4]);
> - calib->AC5 = be16_to_cpu(buf[AC5]);
> - calib->AC6 = be16_to_cpu(buf[AC6]);
> - calib->B1 = be16_to_cpu(buf[B1]);
> - calib->B2 = be16_to_cpu(buf[B2]);
> - calib->MB = be16_to_cpu(buf[MB]);
> - calib->MC = be16_to_cpu(buf[MC]);
> - calib->MD = be16_to_cpu(buf[MD]);
> -
> + add_device_randomness(buf, BMP180_REG_CALIB_COUNT);
> +
> + calib->AC1 = get_unaligned_be16(&buf[AC1]);
> + calib->AC2 = get_unaligned_be16(&buf[AC2]);
> + calib->AC3 = get_unaligned_be16(&buf[AC3]);
> + calib->AC4 = get_unaligned_be16(&buf[AC4]);
> + calib->AC5 = get_unaligned_be16(&buf[AC5]);
> + calib->AC6 = get_unaligned_be16(&buf[AC6]);
> + calib->B1 = get_unaligned_be16(&buf[B1]);
> + calib->B2 = get_unaligned_be16(&buf[B2]);
> + calib->MB = get_unaligned_be16(&buf[MB]);
> + calib->MC = get_unaligned_be16(&buf[MC]);
> + calib->MD = get_unaligned_be16(&buf[MD]);
> +
> + kfree(buf);
> return 0;
> }
>
> @@ -852,18 +895,17 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val)
> static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
> {
> int ret;
> - __be32 tmp = 0;
> u8 oss = data->oversampling_press;
>
> ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
> if (ret)
> return ret;
>
> - ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 3);
> + ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, data->buf, 3);
> if (ret)
> return ret;
>
> - *val = (be32_to_cpu(tmp) >> 8) >> (8 - oss);
> + *val = get_unaligned_be24(data->buf) >> (8 - oss);
>
> return 0;
> }


2022-08-01 08:14:44

by Angel Iglesias

[permalink] [raw]
Subject: Re: [PATCH v4 3/5] iio: pressure: bmp280: Fix alignment for DMA safety

On dom, 2022-07-31 at 17:13 +0100, Jonathan Cameron wrote:
> On Sat, 23 Jul 2022 19:39:10 +0200
> Angel Iglesias <[email protected]> wrote:
>
> > Adds a DMA-safe buffer to driver data struct to store raw data from sensor
> >
> > Updated calibration read functions to use DMA-safe buffers as well.
> > In this case, as calibration data is only read at sensor initialization,
> > the buffers are allocated with kmalloc and freed after being processed.
> >
> > Suggested-by: Jonathan Cameron <[email protected]>
> > Signed-off-by: Angel Iglesias <[email protected]>
> A few comments inline.  This is more complex than it needs to be because
> you can still use __be16 *buf; and index in the right steps.  Side effect
> being it's aligned, so conversions will be cheaper as well.
>
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 164 ++++++++++++++++++-----------
> >  1 file changed, 103 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c
> > b/drivers/iio/pressure/bmp280-core.c
> > index 60fba199c7a0..9311eeec8473 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/completion.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/random.h>
> > +#include <asm/unaligned.h>
> >  
> >  #include "bmp280.h"
> >  
> > @@ -36,7 +37,19 @@
> >   * These enums are used for indexing into the array of calibration
> >   * coefficients for BMP180.
> >   */
> > -enum { AC1, AC2, AC3, AC4, AC5, AC6, B1, B2, MB, MC, MD };
> > +enum {
> > +       AC1 = 0,
> > +       AC2 = 2,
> > +       AC3 = 4,
> > +       AC4 = 6,
> > +       AC5 = 8,
> > +       AC6 = 10,
> > +       B1 = 12,
> > +       B2 = 14,
> > +       MB = 16,
> > +       MC = 18,
> > +       MD = 20,
> Looks like we are specifying a size wrong somewhere as these are all
> multiples of 2.
>
> > +};
> >  
> >  struct bmp180_calib {
> >         s16 AC1;
> > @@ -104,6 +117,12 @@ struct bmp280_data {
> >          * calculation.
> >          */
> >         s32 t_fine;
> > +
> > +       /*
> > +        * DMA (thus cache coherency maintenance) may require the
> > +        * transfer buffers to live in their own cache lines.
> > +        */
> > +       u8 buf[3] __aligned(IIO_DMA_MINALIGN);
> So, on many platforms of interest (anything arm64) we are going to be
> padding this struture out after this to 128 bytes anyway.  So no advantage
> to keeping this short.  I'd just use this buffer for the calibration cases
> as well and drop the extra allocations.  You'll waste space on x86/x86_64 but
> meh, who cares about those legacy architectures :)

Oh I see, I tried to save memory only using this 3 bytes to read the adcs raw
values from the sensors and allocating dynamically the buffers to read
calibration values and ended wasting more memory :(
Thanks for the insight, I'll do my homework and study in more detail DMA and
memory allocation in this platforms.

Yes, this will probably end running on fancier archs like arm64 or risc-v :)
(I'm using a couple of arm64 boards to test the sensors)

> >  };
> >  
> >  struct bmp280_chip_info {
> > @@ -135,8 +154,18 @@ struct bmp280_chip_info {
> >   * These enums are used for indexing into the array of compensation
> >   * parameters for BMP280.
> >   */
> > -enum { T1, T2, T3 };
> > -enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
> > +enum { T1 = 0, T2 = 2, T3 = 4 };
> > +enum {
> > +       P1 = 0,
> > +       P2 = 2,
> > +       P3 = 4,
> > +       P4 = 6,
> > +       P5 = 8,
> > +       P6 = 10,
> > +       P7 = 12,
> > +       P8 = 14,
> > +       P9 = 16,
> > +};
> >  
> >  static const struct iio_chan_spec bmp280_channels[] = {
> >         {
> > @@ -159,51 +188,59 @@ static const struct iio_chan_spec bmp280_channels[] =
> > {
> >  static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
> >  {
> >         int ret;
> > +       u8 *t_buf;
> > +       u8 *p_buf;
> >         unsigned int tmp;
> > -       __le16 l16;
> > -       __be16 b16;
> >         struct device *dev = data->dev;
> >         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];
> >  
> > +       t_buf = kmalloc(BMP280_COMP_TEMP_REG_COUNT, GFP_KERNEL);
> > +       if (!t_buf)
> > +               return -ENOMEM;
> >         /* Read temperature calibration values. */
> >         ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
> >                                t_buf, BMP280_COMP_TEMP_REG_COUNT);
> >         if (ret < 0) {
> >                 dev_err(data->dev,
> >                         "failed to read temperature calibration
> > parameters\n");
> > +               kfree(t_buf);
> >                 return ret;
> >         }
> >  
> >         /* Toss the temperature calibration data into the entropy pool */
> >         add_device_randomness(t_buf, sizeof(t_buf));
> >  
> > -       calib->T1 = le16_to_cpu(t_buf[T1]);
> > -       calib->T2 = le16_to_cpu(t_buf[T2]);
> > -       calib->T3 = le16_to_cpu(t_buf[T3]);
> > +       calib->T1 = get_unaligned_le16(&t_buf[T1]);
> > +       calib->T2 = get_unaligned_le16(&t_buf[T2]);
> > +       calib->T3 = get_unaligned_le16(&t_buf[T3]);
> > +       kfree(t_buf);
> >  
> > +       p_buf = kmalloc(BMP280_COMP_PRESS_REG_COUNT, GFP_KERNEL);
> > +       if (!p_buf)
> > +               return -ENOMEM;
> >         /* 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");
> > +               kfree(p_buf);
> >                 return ret;
> >         }
> >  
> >         /* Toss the pressure calibration data into the entropy pool */
> >         add_device_randomness(p_buf, sizeof(p_buf));
> >  
> > -       calib->P1 = le16_to_cpu(p_buf[P1]);
> > -       calib->P2 = le16_to_cpu(p_buf[P2]);
> > -       calib->P3 = le16_to_cpu(p_buf[P3]);
> > -       calib->P4 = le16_to_cpu(p_buf[P4]);
> > -       calib->P5 = le16_to_cpu(p_buf[P5]);
> > -       calib->P6 = le16_to_cpu(p_buf[P6]);
> > -       calib->P7 = le16_to_cpu(p_buf[P7]);
> > -       calib->P8 = le16_to_cpu(p_buf[P8]);
> > -       calib->P9 = le16_to_cpu(p_buf[P9]);
> > +       calib->P1 = get_unaligned_le16(&p_buf[P1]);
> > +       calib->P2 = get_unaligned_le16(&p_buf[P2]);
> > +       calib->P3 = get_unaligned_le16(&p_buf[P3]);
> > +       calib->P4 = get_unaligned_le16(&p_buf[P4]);
> > +       calib->P5 = get_unaligned_le16(&p_buf[P5]);
> > +       calib->P6 = get_unaligned_le16(&p_buf[P6]);
> > +       calib->P7 = get_unaligned_le16(&p_buf[P7]);
> > +       calib->P8 = get_unaligned_le16(&p_buf[P8]);
> > +       calib->P9 = get_unaligned_le16(&p_buf[P9]);
> > +       kfree(p_buf);
> >  
> >         /*
> >          * Read humidity calibration values.
> > @@ -222,12 +259,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,
> > 2);
> >         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);
> >  
> >         ret = regmap_read(data->regmap, BMP280_REG_COMP_H3, &tmp);
> >         if (ret < 0) {
> > @@ -236,20 +273,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,
> > 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) >> 4) &
> > 0xff0) |
> > +                                 (get_unaligned_be16(data->buf) & 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,
> > 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) >> 4) &
> > 0xfff), 11);
> >  
> >         ret = regmap_read(data->regmap, BMP280_REG_COMP_H6, &tmp);
> >         if (ret < 0) {
> > @@ -344,16 +381,15 @@ static int bmp280_read_temp(struct bmp280_data *data,
> >                             int *val)
> >  {
> >         int ret;
> > -       __be32 tmp = 0;
> >         s32 adc_temp, comp_temp;
> >  
> > -       ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, &tmp, 3);
> > +       ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, data->buf,
> > 3);
> >         if (ret < 0) {
> >                 dev_err(data->dev, "failed to read temperature\n");
> >                 return ret;
> >         }
> >  
> > -       adc_temp = be32_to_cpu(tmp) >> 12;
> > +       adc_temp = get_unaligned_be24(data->buf) >> 4;
> >         if (adc_temp == BMP280_TEMP_SKIPPED) {
> >                 /* reading was skipped */
> >                 dev_err(data->dev, "reading temperature skipped\n");
> > @@ -377,7 +413,6 @@ static int bmp280_read_press(struct bmp280_data *data,
> >                              int *val, int *val2)
> >  {
> >         int ret;
> > -       __be32 tmp = 0;
> >         s32 adc_press;
> >         u32 comp_press;
> >  
> > @@ -386,13 +421,13 @@ static int bmp280_read_press(struct bmp280_data *data,
> >         if (ret < 0)
> >                 return ret;
> >  
> > -       ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, &tmp, 3);
> > +       ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, data-
> > >buf, 3);
> >         if (ret < 0) {
> >                 dev_err(data->dev, "failed to read pressure\n");
> >                 return ret;
> >         }
> >  
> > -       adc_press = be32_to_cpu(tmp) >> 12;
> > +       adc_press = get_unaligned_be24(data->buf) >> 4;
> >         if (adc_press == BMP280_PRESS_SKIPPED) {
> >                 /* reading was skipped */
> >                 dev_err(data->dev, "reading pressure skipped\n");
> > @@ -408,7 +443,6 @@ static int bmp280_read_press(struct bmp280_data *data,
> >  
> >  static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> >  {
> > -       __be16 tmp;
> >         int ret;
> >         s32 adc_humidity;
> >         u32 comp_humidity;
> > @@ -418,13 +452,13 @@ 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, 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);
> >         if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
> >                 /* reading was skipped */
> >                 dev_err(data->dev, "reading humidity skipped\n");
> > @@ -755,56 +789,65 @@ static int bmp180_measure(struct bmp280_data *data, u8
> > ctrl_meas)
> >  
> >  static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
> >  {
> > -       __be16 tmp;
> >         int ret;
> >  
> >         ret = bmp180_measure(data, BMP180_MEAS_TEMP);
> >         if (ret)
> >                 return ret;
> >  
> > -       ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 2);
> > +       ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, data->buf,
> > 2);
> >         if (ret)
> >                 return ret;
> >  
> > -       *val = be16_to_cpu(tmp);
> > +       *val = get_unaligned_be16(data->buf);
> >  
> >         return 0;
> >  }
> >  
> >  static int bmp180_read_calib(struct bmp280_data *data, unsigned int chip)
> >  {
> > +       struct bmp180_calib *calib = &data->calib.bmp180;
> > +       u8 *buf;
> >         int ret;
> >         int i;
> > -       struct bmp180_calib *calib = &data->calib.bmp180;
> > -       __be16 buf[BMP180_REG_CALIB_COUNT / 2];
> > +
> > +       buf = kmalloc(BMP180_REG_CALIB_COUNT, GFP_KERNEL);
> No need to change the type of buf.  Keep it as __be16 *buf = kmalloc()...
> That way your indexing still works nicely without needing to scatter *2 and +=
> 2
> throughout.  Note that if you embed this as I suggest above, just use a union
> so
> you can overlap it with the u8 buffer.
>
> > +       if (!buf)
> > +               return -ENOMEM;
> >  
> >         ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,
> > -                              sizeof(buf));
> > +                              BMP180_REG_CALIB_COUNT);
> >  
> > -       if (ret < 0)
> > +       if (ret < 0) {
> > +               kfree(buf);
> >                 return ret;
> > +       }
> >  
> >         /* None of the words has the value 0 or 0xFFFF */
> > -       for (i = 0; i < ARRAY_SIZE(buf); i++) {
> > -               if (buf[i] == cpu_to_be16(0) || buf[i] ==
> > cpu_to_be16(0xffff))
> > +       for (i = 0; i < BMP180_REG_CALIB_COUNT; i += 2) {
> > +               if (get_unaligned_be16(&buf[i]) == cpu_to_be16(0) ||
> > +                   get_unaligned_be16(&buf[i]) == cpu_to_be16(0xffff)) {
> > +                       kfree(buf);
> >                         return -EIO;
> > +               }
> >         }
> >  
> >         /* Toss the calibration data into the entropy pool */
> > -       add_device_randomness(buf, sizeof(buf));
> > -
> > -       calib->AC1 = be16_to_cpu(buf[AC1]);
> > -       calib->AC2 = be16_to_cpu(buf[AC2]);
> > -       calib->AC3 = be16_to_cpu(buf[AC3]);
> > -       calib->AC4 = be16_to_cpu(buf[AC4]);
> > -       calib->AC5 = be16_to_cpu(buf[AC5]);
> > -       calib->AC6 = be16_to_cpu(buf[AC6]);
> > -       calib->B1 = be16_to_cpu(buf[B1]);
> > -       calib->B2 = be16_to_cpu(buf[B2]);
> > -       calib->MB = be16_to_cpu(buf[MB]);
> > -       calib->MC = be16_to_cpu(buf[MC]);
> > -       calib->MD = be16_to_cpu(buf[MD]);
> > -
> > +       add_device_randomness(buf, BMP180_REG_CALIB_COUNT);
> > +
> > +       calib->AC1 = get_unaligned_be16(&buf[AC1]);
> > +       calib->AC2 = get_unaligned_be16(&buf[AC2]);
> > +       calib->AC3 = get_unaligned_be16(&buf[AC3]);
> > +       calib->AC4 = get_unaligned_be16(&buf[AC4]);
> > +       calib->AC5 = get_unaligned_be16(&buf[AC5]);
> > +       calib->AC6 = get_unaligned_be16(&buf[AC6]);
> > +       calib->B1 = get_unaligned_be16(&buf[B1]);
> > +       calib->B2 = get_unaligned_be16(&buf[B2]);
> > +       calib->MB = get_unaligned_be16(&buf[MB]);
> > +       calib->MC = get_unaligned_be16(&buf[MC]);
> > +       calib->MD = get_unaligned_be16(&buf[MD]);
> > +
> > +       kfree(buf);
> >         return 0;
> >  }
> >  
> > @@ -852,18 +895,17 @@ static int bmp180_read_temp(struct bmp280_data *data,
> > int *val)
> >  static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
> >  {
> >         int ret;
> > -       __be32 tmp = 0;
> >         u8 oss = data->oversampling_press;
> >  
> >         ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
> >         if (ret)
> >                 return ret;
> >  
> > -       ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 3);
> > +       ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, data->buf,
> > 3);
> >         if (ret)
> >                 return ret;
> >  
> > -       *val = (be32_to_cpu(tmp) >> 8) >> (8 - oss);
> > +       *val = get_unaligned_be24(data->buf) >> (8 - oss);
> >  
> >         return 0;
> >  }
>

As always, thanks for your comments,

Kind regards,
Angel