iio: proximity: isl29501: use scan struct instead of array

[PATCH] iio: proximity: isl29501: use scan struct instead of array

Posted by David Lechner 2 months, 3 weeks ago

Replace the scan buffer array with a struct that contains a single u32
for the data and an aligned_s64 for the timestamp. This makes it easier
to see the intended layout of the buffer and avoids the need to manually
calculate the number of extra elements needed for an aligned timestamp.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/proximity/isl29501.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/proximity/isl29501.c b/drivers/iio/proximity/isl29501.c
index d1510fe2405088adc0998e28aa9f36e0186fafae..0eed14f66ab700473af10414b25a56458335b381 100644
--- a/drivers/iio/proximity/isl29501.c
+++ b/drivers/iio/proximity/isl29501.c
@@ -938,12 +938,15 @@ static irqreturn_t isl29501_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct isl29501_private *isl29501 = iio_priv(indio_dev);
 	const unsigned long *active_mask = indio_dev->active_scan_mask;
-	u32 buffer[4] __aligned(8) = {}; /* 1x16-bit + naturally aligned ts */
+	struct {
+		u32 data;
+		aligned_s64 ts;
+	} scan;
 
 	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask))
-		isl29501_register_read(isl29501, REG_DISTANCE, buffer);
+		isl29501_register_read(isl29501, REG_DISTANCE, &scan.data);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;

---
base-commit: f8f559752d573a051a984adda8d2d1464f92f954
change-id: 20250711-iio-use-more-iio_declare_buffer_with_ts-7-880ddf1d3070

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH] iio: proximity: isl29501: use scan struct instead of array

Posted by Jonathan Cameron 2 months, 3 weeks ago

On Fri, 11 Jul 2025 11:18:13 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Replace the scan buffer array with a struct that contains a single u32
> for the data and an aligned_s64 for the timestamp. This makes it easier
> to see the intended layout of the buffer and avoids the need to manually
> calculate the number of extra elements needed for an aligned timestamp.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Why are we using a u32 here?  It's a 16 bit
read in that isl29501_register_read() call
and storagebits = 16 in the chan spec.

So to me looks like you found a bug for big endian platforms.
> ---
>  drivers/iio/proximity/isl29501.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/proximity/isl29501.c b/drivers/iio/proximity/isl29501.c
> index d1510fe2405088adc0998e28aa9f36e0186fafae..0eed14f66ab700473af10414b25a56458335b381 100644
> --- a/drivers/iio/proximity/isl29501.c
> +++ b/drivers/iio/proximity/isl29501.c
> @@ -938,12 +938,15 @@ static irqreturn_t isl29501_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct isl29501_private *isl29501 = iio_priv(indio_dev);
>  	const unsigned long *active_mask = indio_dev->active_scan_mask;
> -	u32 buffer[4] __aligned(8) = {}; /* 1x16-bit + naturally aligned ts */
> +	struct {
> +		u32 data;
> +		aligned_s64 ts;
> +	} scan;
>  
>  	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask))
> -		isl29501_register_read(isl29501, REG_DISTANCE, buffer);
> +		isl29501_register_read(isl29501, REG_DISTANCE, &scan.data);
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
>  	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
> 
> ---
> base-commit: f8f559752d573a051a984adda8d2d1464f92f954
> change-id: 20250711-iio-use-more-iio_declare_buffer_with_ts-7-880ddf1d3070
> 
> Best regards,

Re: [PATCH] iio: proximity: isl29501: use scan struct instead of array

Posted by David Laight 2 months, 2 weeks ago

On Sun, 13 Jul 2025 15:04:45 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 11 Jul 2025 11:18:13 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > Replace the scan buffer array with a struct that contains a single u32
> > for the data and an aligned_s64 for the timestamp. This makes it easier
> > to see the intended layout of the buffer and avoids the need to manually
> > calculate the number of extra elements needed for an aligned timestamp.
> > 
> > Signed-off-by: David Lechner <dlechner@baylibre.com>  
> Why are we using a u32 here?  It's a 16 bit
> read in that isl29501_register_read() call
> and storagebits = 16 in the chan spec.
> 
> So to me looks like you found a bug for big endian platforms.

Maybe not - it looks like there is a read on one place and write in another.
Both are passed the same address.
The interface looks very strange though.

But the updated code is writing uninitialised stack.

	David
 
> > ---
> >  drivers/iio/proximity/isl29501.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/proximity/isl29501.c b/drivers/iio/proximity/isl29501.c
> > index d1510fe2405088adc0998e28aa9f36e0186fafae..0eed14f66ab700473af10414b25a56458335b381 100644
> > --- a/drivers/iio/proximity/isl29501.c
> > +++ b/drivers/iio/proximity/isl29501.c
> > @@ -938,12 +938,15 @@ static irqreturn_t isl29501_trigger_handler(int irq, void *p)
> >  	struct iio_dev *indio_dev = pf->indio_dev;
> >  	struct isl29501_private *isl29501 = iio_priv(indio_dev);
> >  	const unsigned long *active_mask = indio_dev->active_scan_mask;
> > -	u32 buffer[4] __aligned(8) = {}; /* 1x16-bit + naturally aligned ts */
> > +	struct {
> > +		u32 data;
> > +		aligned_s64 ts;
> > +	} scan;
> >  
> >  	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask))
> > -		isl29501_register_read(isl29501, REG_DISTANCE, buffer);
> > +		isl29501_register_read(isl29501, REG_DISTANCE, &scan.data);
> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
> >  	iio_trigger_notify_done(indio_dev->trig);
> >  
> >  	return IRQ_HANDLED;
> > 
> > ---
> > base-commit: f8f559752d573a051a984adda8d2d1464f92f954
> > change-id: 20250711-iio-use-more-iio_declare_buffer_with_ts-7-880ddf1d3070
> > 
> > Best regards,  
> 
>

Re: [PATCH] iio: proximity: isl29501: use scan struct instead of array

Posted by Andy Shevchenko 2 months, 3 weeks ago

On Fri, Jul 11, 2025 at 11:18:13AM -0500, David Lechner wrote:
> Replace the scan buffer array with a struct that contains a single u32
> for the data and an aligned_s64 for the timestamp. This makes it easier
> to see the intended layout of the buffer and avoids the need to manually
> calculate the number of extra elements needed for an aligned timestamp.

Same Q, why not macro?

-- 
With Best Regards,
Andy Shevchenko