[PATCH v2 0/2]: pressure: bmp280: Improve pushing of data to buffer

[PATCH v2 0/2]: pressure: bmp280: Improve pushing of data to buffer

[Vasileios Amoiridis]

Changes in v2:
	- Add 1st commit with a small fix of some data types. Shouldn't pose a
	  problem.
	- In commit no.2 add logic with "offset" to make it more clear what we
	  are adding in the buffer. Also move the memcpy() operations after the
	  if() statements so the memcpy happens only if the data are correct to
	  avoid overhead for no reason.

P.S. After this patchseries is applied, I will rebase this [1] and resend it.

[1]: https://lore.kernel.org/linux-iio/20240914002900.45158-1-vassilisamir@gmail.com/

---
v1: https://lore.kernel.org/linux-iio/20240823172017.9028-1-vassilisamir@gmail.com/

Vasileios Amoiridis (2):
  iio: pressure: bmp280: Fix type for raw values
  iio: pressure: bmp280: Use char instead of s32 for data buffer

 drivers/iio/pressure/bmp280-core.c | 75 +++++++++++++++++++-----------
 drivers/iio/pressure/bmp280.h      |  5 +-
 2 files changed, 51 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] iio: pressure: bmp280: Fix type for raw values

[Vasileios Amoiridis]

The adc values coming directly from the sensor in the BM{E,P}{2,3}xx
sensors are unsigned values so treat them as such.

Fixes: 80cd23f43ddc ("iio: pressure: bmp280: Add triggered buffer support")
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6c2606f34ec4..472a6696303b 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1023,7 +1023,8 @@ static irqreturn_t bmp280_trigger_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, t_fine;
+	u32 adc_temp, adc_press;
+	s32 t_fine;
 	int ret;
 
 	guard(mutex)(&data->lock);
@@ -1137,7 +1138,8 @@ static irqreturn_t bme280_trigger_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, t_fine;
+	u32 adc_temp, adc_press, adc_humidity;
+	s32 t_fine;
 	int ret;
 
 	guard(mutex)(&data->lock);
@@ -1616,7 +1618,8 @@ static irqreturn_t bmp380_trigger_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, t_fine;
+	u32 adc_temp, adc_press;
+	s32 t_fine;
 	int ret;
 
 	guard(mutex)(&data->lock);
-- 
2.25.1

Re: [PATCH v2 1/2] iio: pressure: bmp280: Fix type for raw values

[Jonathan Cameron]

On Sun, 29 Sep 2024 13:25:10 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> The adc values coming directly from the sensor in the BM{E,P}{2,3}xx
> sensors are unsigned values so treat them as such.
> 
> Fixes: 80cd23f43ddc ("iio: pressure: bmp280: Add triggered buffer support")
Why is this a fix rather than a cleanup?  Looks to me like all the values
are going to be small enough that they fit either way.
So good to tidy up for consistency etc, but why a fixes tag?

I 

> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-core.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 6c2606f34ec4..472a6696303b 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -1023,7 +1023,8 @@ static irqreturn_t bmp280_trigger_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, t_fine;
> +	u32 adc_temp, adc_press;
These are filled as part of a get_unaligned_be24() so the value will never
be big enough that signed / unsigned should make any difference.

> +	s32 t_fine;
>  	int ret;
>  
>  	guard(mutex)(&data->lock);
> @@ -1137,7 +1138,8 @@ static irqreturn_t bme280_trigger_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, t_fine;
> +	u32 adc_temp, adc_press, adc_humidity;
Same with these.
> +	s32 t_fine;
>  	int ret;
>  
>  	guard(mutex)(&data->lock);
> @@ -1616,7 +1618,8 @@ static irqreturn_t bmp380_trigger_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, t_fine;
These are similar but le24.

> +	u32 adc_temp, adc_press;
> +	s32 t_fine;
>  	int ret;
>  
>  	guard(mutex)(&data->lock);

Re: [PATCH v2 1/2] iio: pressure: bmp280: Fix type for raw values

[Vasileios Amoiridis]

On Sun, Sep 29, 2024 at 06:04:26PM +0100, Jonathan Cameron wrote:
> On Sun, 29 Sep 2024 13:25:10 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The adc values coming directly from the sensor in the BM{E,P}{2,3}xx
> > sensors are unsigned values so treat them as such.
> > 
> > Fixes: 80cd23f43ddc ("iio: pressure: bmp280: Add triggered buffer support")
> Why is this a fix rather than a cleanup?  Looks to me like all the values
> are going to be small enough that they fit either way.
> So good to tidy up for consistency etc, but why a fixes tag?
> 
> I 
> 

Hi Jonathan,

I used the fixes tag because I though it was appropriate since it was
using a wrong variable type even though it was not posing any
functional thread (I mentioned it in the cover-letter as well).

Since I am doing a new version I can drop the tag, no problem!!!

Cheers,
Vasilis

> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 6c2606f34ec4..472a6696303b 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -1023,7 +1023,8 @@ static irqreturn_t bmp280_trigger_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, t_fine;
> > +	u32 adc_temp, adc_press;
> These are filled as part of a get_unaligned_be24() so the value will never
> be big enough that signed / unsigned should make any difference.
> 
> > +	s32 t_fine;
> >  	int ret;
> >  
> >  	guard(mutex)(&data->lock);
> > @@ -1137,7 +1138,8 @@ static irqreturn_t bme280_trigger_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, t_fine;
> > +	u32 adc_temp, adc_press, adc_humidity;
> Same with these.
> > +	s32 t_fine;
> >  	int ret;
> >  
> >  	guard(mutex)(&data->lock);
> > @@ -1616,7 +1618,8 @@ static irqreturn_t bmp380_trigger_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, t_fine;
> These are similar but le24.
> 
> > +	u32 adc_temp, adc_press;
> > +	s32 t_fine;
> >  	int ret;
> >  
> >  	guard(mutex)(&data->lock);
> 

[PATCH v2 2/2] iio: pressure: bmp280: Use char instead of s32 for data buffer

[Vasileios Amoiridis]

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 | 78 ++++++++++++++++++------------
 drivers/iio/pressure/bmp280.h      |  5 +-
 2 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 472a6696303b..2c62490a40c6 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1023,9 +1023,9 @@ static irqreturn_t bmp280_trigger_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);
-	u32 adc_temp, adc_press;
-	s32 t_fine;
-	int ret;
+	u32 adc_temp, adc_press, comp_press;
+	s32 t_fine, comp_temp;
+	int ret, offset;
 
 	guard(mutex)(&data->lock);
 
@@ -1044,7 +1044,7 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p)
 		goto out;
 	}
 
-	data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/* Pressure calculations */
 	adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0]));
@@ -1054,10 +1054,13 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p)
 	}
 
 	t_fine = bmp280_calc_t_fine(data, adc_temp);
+	comp_press = bmp280_compensate_press(data, adc_press, t_fine);
 
-	data->sensor_data[0] = bmp280_compensate_press(data, adc_press, t_fine);
+	memcpy(&data->buffer.buf[offset], &comp_press, sizeof(s32));
+	offset += sizeof(s32);
+	memcpy(&data->buffer.buf[offset], &comp_temp, sizeof(s32));
 
-	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:
@@ -1138,9 +1141,9 @@ static irqreturn_t bme280_trigger_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);
-	u32 adc_temp, adc_press, adc_humidity;
-	s32 t_fine;
-	int ret;
+	u32 adc_temp, adc_press, adc_humidity, comp_press, comp_humidity;
+	s32 t_fine, comp_temp;
+	int ret, offset;
 
 	guard(mutex)(&data->lock);
 
@@ -1159,7 +1162,7 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p)
 		goto out;
 	}
 
-	data->sensor_data[1] = bmp280_compensate_temp(data, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/* Pressure calculations */
 	adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0]));
@@ -1169,8 +1172,7 @@ 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);
+	comp_press = bmp280_compensate_press(data, adc_press, t_fine);
 
 	/* Humidity calculations */
 	adc_humidity = get_unaligned_be16(&data->buf[6]);
@@ -1179,9 +1181,16 @@ 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,
+	comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
+
+	memcpy(&data->buffer.buf[offset], &comp_press, sizeof(s32));
+	offset += sizeof(s32);
+	memcpy(&data->buffer.buf[offset], &comp_temp, sizeof(s32));
+	offset += sizeof(s32);
+	memcpy(&data->buffer.buf[offset], &comp_humidity, sizeof(s32));
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
 					   iio_get_time_ns(indio_dev));
 
 out:
@@ -1618,9 +1627,9 @@ static irqreturn_t bmp380_trigger_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);
-	u32 adc_temp, adc_press;
-	s32 t_fine;
-	int ret;
+	u32 adc_temp, adc_press, comp_press;
+	s32 t_fine, comp_temp;
+	int ret, offset;
 
 	guard(mutex)(&data->lock);
 
@@ -1639,7 +1648,7 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p)
 		goto out;
 	}
 
-	data->sensor_data[1] = bmp380_compensate_temp(data, adc_temp);
+	comp_temp = bmp380_compensate_temp(data, adc_temp);
 
 	/* Pressure calculations */
 	adc_press = get_unaligned_le24(&data->buf[0]);
@@ -1649,10 +1658,13 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p)
 	}
 
 	t_fine = bmp380_calc_t_fine(data, adc_temp);
+	comp_press = bmp380_compensate_press(data, adc_press, t_fine);
 
-	data->sensor_data[0] = bmp380_compensate_press(data, adc_press, t_fine);
+	memcpy(&data->buffer.buf[offset], &comp_press, sizeof(s32));
+	offset += sizeof(s32);
+	memcpy(&data->buffer.buf[offset], &comp_temp, sizeof(s32));
 
-	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:
@@ -2199,7 +2211,7 @@ static irqreturn_t bmp580_trigger_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;
+	int ret, offset;
 
 	guard(mutex)(&data->lock);
 
@@ -2211,13 +2223,16 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
 		goto out;
 	}
 
+	/* Pressure calculations */
+	memcpy(&data->buffer.buf[offset], &data->buf[3], 3);
+
+	offset += sizeof(s32);
+
 	/* Temperature calculations */
-	memcpy(&data->sensor_data[1], &data->buf[0], 3);
+	memcpy(&data->buffer.buf[offset], &data->buf[0], 3);
 
-	/* Pressure calculations */
-	memcpy(&data->sensor_data[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:
@@ -2523,23 +2538,24 @@ static irqreturn_t bmp180_trigger_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;
+	int ret, comp_temp, comp_press, offset;
 
 	guard(mutex)(&data->lock);
 
-	ret = bmp180_read_temp(data, &chan_value);
+	ret = bmp180_read_temp(data, &comp_temp);
 	if (ret)
 		goto out;
 
-	data->sensor_data[1] = chan_value;
 
-	ret = bmp180_read_press(data, &chan_value);
+	ret = bmp180_read_press(data, &comp_press);
 	if (ret)
 		goto out;
 
-	data->sensor_data[0] = chan_value;
+	memcpy(&data->buffer.buf[offset], &comp_press, sizeof(s32));
+	offset += sizeof(s32);
+	memcpy(&data->buffer.buf[offset], &comp_temp, sizeof(s32));
 
-	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 a9f220c1f77a..b0c26f55c6af 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -419,7 +419,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];
+		aligned_s64 ts;
+	} buffer;
 
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
-- 
2.25.1

Re: [PATCH v2 2/2] iio: pressure: bmp280: Use char instead of s32 for data buffer

[Jonathan Cameron]

On Sun, 29 Sep 2024 13:25:11 +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 Vasileois.

I missed a purely semantic issue in previous versions :( 

A few other places where you can achieve the same effect with less code
and clear casting to correct types.

Jonathan


> ---
>  drivers/iio/pressure/bmp280-core.c | 78 ++++++++++++++++++------------
>  drivers/iio/pressure/bmp280.h      |  5 +-
>  2 files changed, 51 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 472a6696303b..2c62490a40c6 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c


> @@ -2523,23 +2538,24 @@ static irqreturn_t bmp180_trigger_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;
> +	int ret, comp_temp, comp_press, offset;
>  
>  	guard(mutex)(&data->lock);
>  
> -	ret = bmp180_read_temp(data, &chan_value);
> +	ret = bmp180_read_temp(data, &comp_temp);
>  	if (ret)
>  		goto out;
>  
> -	data->sensor_data[1] = chan_value;
>  
> -	ret = bmp180_read_press(data, &chan_value);
> +	ret = bmp180_read_press(data, &comp_press);
>  	if (ret)
>  		goto out;
>  
> -	data->sensor_data[0] = chan_value;
> +	memcpy(&data->buffer.buf[offset], &comp_press, sizeof(s32));
> +	offset += sizeof(s32);
> +	memcpy(&data->buffer.buf[offset], &comp_temp, sizeof(s32));
I suppose there is a consistency argument for using memcpy even for the s32
cases but you 'could' if you like do
	s32 *chans = (s32 *)data->buffer.buf;
at top
and 
	chans[0] = comp_press;
	chans[1] = comp_temp;
here, which is functionally equivalent, particularly as we are forcing the
buffer alignment to be larger than this s32.

Similar for the other simple native endian s32 cases.

The memcpy is needed for the le24 one.


>  
> -	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 a9f220c1f77a..b0c26f55c6af 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -419,7 +419,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];
> +		aligned_s64 ts;

I'd missed that this depends on the number of channels.  It makes no functional
difference because the core code will happily write over the end of buf, but
from a representation point of view this might be

		u8 buf[8];
		aligned_s64 ts;
or
		u8 buf[12];
		aligned_s64 ts;

So given we can't actually fix on one or the other normal convention is
to just use something that forces a large enough aligned u8 buffer like
		u8 buf[ALIGN(sizeof(s32) * BMP280_MAX_CHANNELS, 8) + sizeof(s64)]
			__aligned(sizeof(s64));

Sorry for leading you astray on this!

Jonathan


> +	} buffer;
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) may require the

Re: [PATCH v2 2/2] iio: pressure: bmp280: Use char instead of s32 for data buffer

[Vasileios Amoiridis]

On Sun, Sep 29, 2024 at 06:10:03PM +0100, Jonathan Cameron wrote:
> On Sun, 29 Sep 2024 13:25:11 +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 Vasileois.
> 
> I missed a purely semantic issue in previous versions :( 
> 
> A few other places where you can achieve the same effect with less code
> and clear casting to correct types.
> 
> Jonathan
> 
> 

Hi Jonathan,

> > ---
> >  drivers/iio/pressure/bmp280-core.c | 78 ++++++++++++++++++------------
> >  drivers/iio/pressure/bmp280.h      |  5 +-
> >  2 files changed, 51 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 472a6696303b..2c62490a40c6 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> 
> 
> > @@ -2523,23 +2538,24 @@ static irqreturn_t bmp180_trigger_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;
> > +	int ret, comp_temp, comp_press, offset;
> >  
> >  	guard(mutex)(&data->lock);
> >  
> > -	ret = bmp180_read_temp(data, &chan_value);
> > +	ret = bmp180_read_temp(data, &comp_temp);
> >  	if (ret)
> >  		goto out;
> >  
> > -	data->sensor_data[1] = chan_value;
> >  
> > -	ret = bmp180_read_press(data, &chan_value);
> > +	ret = bmp180_read_press(data, &comp_press);
> >  	if (ret)
> >  		goto out;
> >  
> > -	data->sensor_data[0] = chan_value;
> > +	memcpy(&data->buffer.buf[offset], &comp_press, sizeof(s32));
> > +	offset += sizeof(s32);
> > +	memcpy(&data->buffer.buf[offset], &comp_temp, sizeof(s32));
> I suppose there is a consistency argument for using memcpy even for the s32
> cases but you 'could' if you like do
> 	s32 *chans = (s32 *)data->buffer.buf;
> at top
> and 
> 	chans[0] = comp_press;
> 	chans[1] = comp_temp;
> here, which is functionally equivalent, particularly as we are forcing the
> buffer alignment to be larger than this s32.
> 
> Similar for the other simple native endian s32 cases.
> 
> The memcpy is needed for the le24 one.
> 
> 

I see what you mean, indeed I think your proposal will beautify it a lot!

> >  
> > -	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 a9f220c1f77a..b0c26f55c6af 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -419,7 +419,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];
> > +		aligned_s64 ts;
> 
> I'd missed that this depends on the number of channels.  It makes no functional
> difference because the core code will happily write over the end of buf, but
> from a representation point of view this might be
> 
> 		u8 buf[8];
> 		aligned_s64 ts;
> or
> 		u8 buf[12];
> 		aligned_s64 ts;
> 
> So given we can't actually fix on one or the other normal convention is
> to just use something that forces a large enough aligned u8 buffer like
> 		u8 buf[ALIGN(sizeof(s32) * BMP280_MAX_CHANNELS, 8) + sizeof(s64)]
> 			__aligned(sizeof(s64));
> 
> Sorry for leading you astray on this!
> 
> Jonathan
> 
> 

I see your point. I can fix it in next version!

This is a neat issue here that requires indeed extra attention since
this type of buffers is used basically by the majority of the drivers.
Some type of runtime check in those registers would have been very
very helpful, but I can't see of an easy way of doing it in the core
code..

Thanks for the review :)

Cheers,
Vasilis

> > +	} buffer;
> >  
> >  	/*
> >  	 * DMA (thus cache coherency maintenance) may require the
> 

Re: [PATCH v2 2/2] iio: pressure: bmp280: Use char instead of s32 for data buffer

[Jonathan Cameron]

> > > -	s32 sensor_data[6] __aligned(8);
> > > +	struct {
> > > +		u8 buf[12];
> > > +		aligned_s64 ts;  
> > 
> > I'd missed that this depends on the number of channels.  It makes no functional
> > difference because the core code will happily write over the end of buf, but
> > from a representation point of view this might be
> > 
> > 		u8 buf[8];
> > 		aligned_s64 ts;
> > or
> > 		u8 buf[12];
> > 		aligned_s64 ts;
> > 
> > So given we can't actually fix on one or the other normal convention is
> > to just use something that forces a large enough aligned u8 buffer like
> > 		u8 buf[ALIGN(sizeof(s32) * BMP280_MAX_CHANNELS, 8) + sizeof(s64)]
> > 			__aligned(sizeof(s64));
> > 
> > Sorry for leading you astray on this!
> > 
> > Jonathan
> > 
> >   
> 
> I see your point. I can fix it in next version!
> 
> This is a neat issue here that requires indeed extra attention since
> this type of buffers is used basically by the majority of the drivers.
> Some type of runtime check in those registers would have been very
> very helpful, but I can't see of an easy way of doing it in the core
> code..
Adding size description has been on my todo list for a while so as to allow
the kernel to check the buffer is big enough and get rid of subtle overrun
bugs due to that oddity of the buffer needing to include the timestamp
space even though it's not obvious it will be written to.
That would also allow us to check alignment.  What we can't do is finer
grained checking of these structures but arguably we don't want to as this
is an elegance not correctness issue!

> 
> Thanks for the review :)
> 
> Cheers,
> Vasilis
> 
> > > +	} buffer;
> > >  
> > >  	/*
> > >  	 * DMA (thus cache coherency maintenance) may require the  
> >