linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: proximity: hx9023s: fix scan_type endianness
@ 2025-07-22 23:07 David Lechner
  2025-07-22 23:08 ` David Lechner
  0 siblings, 1 reply; 10+ messages in thread
From: David Lechner @ 2025-07-22 23:07 UTC (permalink / raw)
  To: Jonathan Cameron, Nuno Sá, Andy Shevchenko, David Lechner,
	Yasin Lee
  Cc: linux-iio, linux-kernel, Jonathan Cameron

Change the scan_type endianness from IIO_BE to IIO_LE. This matches
the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
the data before pushing it to the IIO buffer.

Fixes: 60df548277b7 ("iio: proximity: Add driver support for TYHX's HX9023S capacitive proximity sensor")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/proximity/hx9023s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
index 33781c3147286fb3e2f022201ccf7e908d0b6b12..8e5748a5f37a7b6674acc1604710394326818204 100644
--- a/drivers/iio/proximity/hx9023s.c
+++ b/drivers/iio/proximity/hx9023s.c
@@ -251,7 +251,7 @@ static const struct iio_event_spec hx9023s_events[] = {
 		.sign = 's',					\
 		.realbits = 16,					\
 		.storagebits = 16,				\
-		.endianness = IIO_BE,				\
+		.endianness = IIO_LE,				\
 	},							\
 }
 

---
base-commit: cd2731444ee4e35db76f4fb587f12d327eec5446
change-id: 20250722-iio-proximity-hx9023c-fix-scan_type-endianness-8ab26af05f75

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-22 23:07 [PATCH] iio: proximity: hx9023s: fix scan_type endianness David Lechner
@ 2025-07-22 23:08 ` David Lechner
  2025-07-23 14:13   ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: David Lechner @ 2025-07-22 23:08 UTC (permalink / raw)
  To: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Yasin Lee
  Cc: linux-iio, linux-kernel, Jonathan Cameron

On 7/22/25 6:07 PM, David Lechner wrote:
> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
> the data before pushing it to the IIO buffer.
> 
> Fixes: 60df548277b7 ("iio: proximity: Add driver support for TYHX's HX9023S capacitive proximity sensor")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/proximity/hx9023s.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
> index 33781c3147286fb3e2f022201ccf7e908d0b6b12..8e5748a5f37a7b6674acc1604710394326818204 100644
> --- a/drivers/iio/proximity/hx9023s.c
> +++ b/drivers/iio/proximity/hx9023s.c
> @@ -251,7 +251,7 @@ static const struct iio_event_spec hx9023s_events[] = {
>  		.sign = 's',					\
>  		.realbits = 16,					\
>  		.storagebits = 16,				\
> -		.endianness = IIO_BE,				\
> +		.endianness = IIO_LE,				\
>  	},							\
>  }
>  
Just noticed this while reviewing scan structs.

It is odd to have data already in CPU-endian and convert it to LE
before pushing to buffers. So I'm a bit tempted to do this instead
since it probably isn't likely anyone is using this on a big-endian
system:

diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
index 8e5748a5f37a..b8076ca4d0f4 100644
--- a/drivers/iio/proximity/hx9023s.c
+++ b/drivers/iio/proximity/hx9023s.c
@@ -145,7 +145,7 @@ struct hx9023s_data {
 	bool trigger_enabled;
 
 	struct {
-		__le16 channels[HX9023S_CH_NUM];
+		s16 channels[HX9023S_CH_NUM];
 		aligned_s64 ts;
 	} buffer;
 
@@ -251,7 +251,7 @@ static const struct iio_event_spec hx9023s_events[] = {
 		.sign = 's',					\
 		.realbits = 16,					\
 		.storagebits = 16,				\
-		.endianness = IIO_LE,				\
+		.endianness = IIO_CPU,				\
 	},							\
 }
 
@@ -950,7 +950,7 @@ static irqreturn_t hx9023s_trigger_handler(int irq, void *private)
 
 	iio_for_each_active_channel(indio_dev, bit) {
 		index = indio_dev->channels[bit].channel;
-		data->buffer.channels[i++] = cpu_to_le16(data->ch_data[index].diff);
+		data->buffer.channels[i++] = data->ch_data[index].diff;
 	}
 
 	iio_push_to_buffers_with_ts(indio_dev, &data->buffer,


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-22 23:08 ` David Lechner
@ 2025-07-23 14:13   ` Andy Shevchenko
  2025-07-23 14:29     ` David Lechner
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2025-07-23 14:13 UTC (permalink / raw)
  To: David Lechner
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:
> On 7/22/25 6:07 PM, David Lechner wrote:
> > Change the scan_type endianness from IIO_BE to IIO_LE. This matches
> > the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
> > the data before pushing it to the IIO buffer.

> It is odd to have data already in CPU-endian and convert it to LE
> before pushing to buffers. So I'm a bit tempted to do this instead
> since it probably isn't likely anyone is using this on a big-endian
> system:

I can say that first of all, we need to consult with the datasheet for the
actual HW endianess. And second, I do not believe that CPU endianess may be
used, I can't imagine when this (discrete?) component can be integrated in such
a way. That said, I think your second approach even worse.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-23 14:13   ` Andy Shevchenko
@ 2025-07-23 14:29     ` David Lechner
  2025-07-23 14:37       ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: David Lechner @ 2025-07-23 14:29 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On 7/23/25 9:13 AM, Andy Shevchenko wrote:
> On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:
>> On 7/22/25 6:07 PM, David Lechner wrote:
>>> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
>>> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
>>> the data before pushing it to the IIO buffer.
> 
>> It is odd to have data already in CPU-endian and convert it to LE
>> before pushing to buffers. So I'm a bit tempted to do this instead
>> since it probably isn't likely anyone is using this on a big-endian
>> system:
> 
> I can say that first of all, we need to consult with the datasheet for the
> actual HW endianess. And second, I do not believe that CPU endianess may be
> used, 

Why not? Lot's of IIO drivers use IIO_CPU in their scan buffers.

> I can't imagine when this (discrete?) component can be integrated in such
> a way. That said, I think your second approach even worse.
> 

hx9023s_sample() is calling get_unaligned_le16() on all of the data
read over the bus, so in the driver, all data is stored CPU-endian
already rather than passing actual raw bus data to the buffer.

So it seems a waste of CPU cycles to convert it back to little-endian
to push to the buffer only for consumers to have to convert it back
to CPU-endian again. But since most systems are little-endian already
this doesn't really matter since no actual conversion is done in this
case.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-23 14:29     ` David Lechner
@ 2025-07-23 14:37       ` Andy Shevchenko
  2025-07-23 14:57         ` David Lechner
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2025-07-23 14:37 UTC (permalink / raw)
  To: David Lechner
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On Wed, Jul 23, 2025 at 09:29:37AM -0500, David Lechner wrote:
> On 7/23/25 9:13 AM, Andy Shevchenko wrote:
> > On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:
> >> On 7/22/25 6:07 PM, David Lechner wrote:
> >>> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
> >>> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
> >>> the data before pushing it to the IIO buffer.
> > 
> >> It is odd to have data already in CPU-endian and convert it to LE
> >> before pushing to buffers. So I'm a bit tempted to do this instead
> >> since it probably isn't likely anyone is using this on a big-endian
> >> system:
> > 
> > I can say that first of all, we need to consult with the datasheet for the
> > actual HW endianess. And second, I do not believe that CPU endianess may be
> > used, 
> 
> Why not? Lot's of IIO drivers use IIO_CPU in their scan buffers.
> 
> > I can't imagine when this (discrete?) component can be integrated in such
> > a way. That said, I think your second approach even worse.
> 
> hx9023s_sample() is calling get_unaligned_le16() on all of the data
> read over the bus, so in the driver, all data is stored CPU-endian
> already rather than passing actual raw bus data to the buffer.

I see, now it makes a lot of sense. Thanks for clarifying this to me.

> So it seems a waste of CPU cycles to convert it back to little-endian
> to push to the buffer only for consumers to have to convert it back
> to CPU-endian again. But since most systems are little-endian already
> this doesn't really matter since no actual conversion is done in this
> case.

Right, but it's buggy on BE, isn't it?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-23 14:37       ` Andy Shevchenko
@ 2025-07-23 14:57         ` David Lechner
  2025-07-23 15:06           ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: David Lechner @ 2025-07-23 14:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On 7/23/25 9:37 AM, Andy Shevchenko wrote:
> On Wed, Jul 23, 2025 at 09:29:37AM -0500, David Lechner wrote:
>> On 7/23/25 9:13 AM, Andy Shevchenko wrote:
>>> On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:
>>>> On 7/22/25 6:07 PM, David Lechner wrote:
>>>>> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
>>>>> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
>>>>> the data before pushing it to the IIO buffer.
>>>
>>>> It is odd to have data already in CPU-endian and convert it to LE
>>>> before pushing to buffers. So I'm a bit tempted to do this instead
>>>> since it probably isn't likely anyone is using this on a big-endian
>>>> system:
>>>
>>> I can say that first of all, we need to consult with the datasheet for the
>>> actual HW endianess. And second, I do not believe that CPU endianess may be
>>> used, 
>>
>> Why not? Lot's of IIO drivers use IIO_CPU in their scan buffers.
>>
>>> I can't imagine when this (discrete?) component can be integrated in such
>>> a way. That said, I think your second approach even worse.
>>
>> hx9023s_sample() is calling get_unaligned_le16() on all of the data
>> read over the bus, so in the driver, all data is stored CPU-endian
>> already rather than passing actual raw bus data to the buffer.
> 
> I see, now it makes a lot of sense. Thanks for clarifying this to me.
> 
>> So it seems a waste of CPU cycles to convert it back to little-endian
>> to push to the buffer only for consumers to have to convert it back
>> to CPU-endian again. But since most systems are little-endian already
>> this doesn't really matter since no actual conversion is done in this
>> case.
> 
> Right, but it's buggy on BE, isn't it?
> 

Right now, the driver is buggy everywhere. The scan info says that the
scan data is BE, but in reality, it is LE (no matter the CPU-endianness).

With the simple patch, it fixes the scan info to reflect reality that
the data is LE in the buffer. This works on BE systems. They just have
an extra conversion from BE to LE in the kernel when pushing to the
buffer and userspace would have to convert back to BE to do math on it.

With the alternate patch you didn't like, the forced conversion to LE
when pushing to buffers is dropped, so nothing would change on LE
systems but BE systems wouldn't have the extra order swapping.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-23 14:57         ` David Lechner
@ 2025-07-23 15:06           ` Andy Shevchenko
  2025-07-23 15:11             ` David Lechner
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2025-07-23 15:06 UTC (permalink / raw)
  To: David Lechner
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On Wed, Jul 23, 2025 at 09:57:58AM -0500, David Lechner wrote:
> On 7/23/25 9:37 AM, Andy Shevchenko wrote:
> > On Wed, Jul 23, 2025 at 09:29:37AM -0500, David Lechner wrote:
> >> On 7/23/25 9:13 AM, Andy Shevchenko wrote:
> >>> On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:
> >>>> On 7/22/25 6:07 PM, David Lechner wrote:
> >>>>> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
> >>>>> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
> >>>>> the data before pushing it to the IIO buffer.
> >>>
> >>>> It is odd to have data already in CPU-endian and convert it to LE
> >>>> before pushing to buffers. So I'm a bit tempted to do this instead
> >>>> since it probably isn't likely anyone is using this on a big-endian
> >>>> system:
> >>>
> >>> I can say that first of all, we need to consult with the datasheet for the
> >>> actual HW endianess. And second, I do not believe that CPU endianess may be
> >>> used, 
> >>
> >> Why not? Lot's of IIO drivers use IIO_CPU in their scan buffers.
> >>
> >>> I can't imagine when this (discrete?) component can be integrated in such
> >>> a way. That said, I think your second approach even worse.
> >>
> >> hx9023s_sample() is calling get_unaligned_le16() on all of the data
> >> read over the bus, so in the driver, all data is stored CPU-endian
> >> already rather than passing actual raw bus data to the buffer.
> > 
> > I see, now it makes a lot of sense. Thanks for clarifying this to me.
> > 
> >> So it seems a waste of CPU cycles to convert it back to little-endian
> >> to push to the buffer only for consumers to have to convert it back
> >> to CPU-endian again. But since most systems are little-endian already
> >> this doesn't really matter since no actual conversion is done in this
> >> case.
> > 
> > Right, but it's buggy on BE, isn't it?
> > 
> 
> Right now, the driver is buggy everywhere. The scan info says that the
> scan data is BE, but in reality, it is LE (no matter the CPU-endianness).
> 
> With the simple patch, it fixes the scan info to reflect reality that
> the data is LE in the buffer. This works on BE systems. They just have
> an extra conversion from BE to LE in the kernel when pushing to the
> buffer and userspace would have to convert back to BE to do math on it.
> 
> With the alternate patch you didn't like, the forced conversion to LE
> when pushing to buffers is dropped, so nothing would change on LE
> systems but BE systems wouldn't have the extra order swapping.

But do they need that? If you supply CPU order (and it is already in a such
after get_unaligned_*() calls) then everything would be good, no?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-23 15:06           ` Andy Shevchenko
@ 2025-07-23 15:11             ` David Lechner
  2025-07-23 15:56               ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: David Lechner @ 2025-07-23 15:11 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On 7/23/25 10:06 AM, Andy Shevchenko wrote:
> On Wed, Jul 23, 2025 at 09:57:58AM -0500, David Lechner wrote:
>> On 7/23/25 9:37 AM, Andy Shevchenko wrote:
>>> On Wed, Jul 23, 2025 at 09:29:37AM -0500, David Lechner wrote:
>>>> On 7/23/25 9:13 AM, Andy Shevchenko wrote:
>>>>> On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:
>>>>>> On 7/22/25 6:07 PM, David Lechner wrote:
>>>>>>> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
>>>>>>> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
>>>>>>> the data before pushing it to the IIO buffer.
>>>>>
>>>>>> It is odd to have data already in CPU-endian and convert it to LE
>>>>>> before pushing to buffers. So I'm a bit tempted to do this instead
>>>>>> since it probably isn't likely anyone is using this on a big-endian
>>>>>> system:
>>>>>
>>>>> I can say that first of all, we need to consult with the datasheet for the
>>>>> actual HW endianess. And second, I do not believe that CPU endianess may be
>>>>> used, 
>>>>
>>>> Why not? Lot's of IIO drivers use IIO_CPU in their scan buffers.
>>>>
>>>>> I can't imagine when this (discrete?) component can be integrated in such
>>>>> a way. That said, I think your second approach even worse.
>>>>
>>>> hx9023s_sample() is calling get_unaligned_le16() on all of the data
>>>> read over the bus, so in the driver, all data is stored CPU-endian
>>>> already rather than passing actual raw bus data to the buffer.
>>>
>>> I see, now it makes a lot of sense. Thanks for clarifying this to me.
>>>
>>>> So it seems a waste of CPU cycles to convert it back to little-endian
>>>> to push to the buffer only for consumers to have to convert it back
>>>> to CPU-endian again. But since most systems are little-endian already
>>>> this doesn't really matter since no actual conversion is done in this
>>>> case.
>>>
>>> Right, but it's buggy on BE, isn't it?
>>>
>>
>> Right now, the driver is buggy everywhere. The scan info says that the
>> scan data is BE, but in reality, it is LE (no matter the CPU-endianness).
>>
>> With the simple patch, it fixes the scan info to reflect reality that
>> the data is LE in the buffer. This works on BE systems. They just have
>> an extra conversion from BE to LE in the kernel when pushing to the
>> buffer and userspace would have to convert back to BE to do math on it.
>>
>> With the alternate patch you didn't like, the forced conversion to LE
>> when pushing to buffers is dropped, so nothing would change on LE
>> systems but BE systems wouldn't have the extra order swapping.
> 
> But do they need that? If you supply CPU order (and it is already in a such
> after get_unaligned_*() calls) then everything would be good, no?
> 

It doesn't make sense to my why, but the existing code is changing
back to LE before pushing to buffers for some reason.


	iio_for_each_active_channel(indio_dev, bit) {
		index = indio_dev->channels[bit].channel;
		data->buffer.channels[i++] = cpu_to_le16(data->ch_data[index].diff);
	}

	iio_push_to_buffers_with_ts(indio_dev, &data->buffer,
				    sizeof(data->buffer), pf->timestamp);

I agree that it seems unnecessary which is why I suggested the
alternate patch to drop the cpu_to_le16() and just leave it
CPU-endian when pushing to the buffers.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-23 15:11             ` David Lechner
@ 2025-07-23 15:56               ` Jonathan Cameron
  2025-08-25 17:07                 ` David Lechner
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2025-07-23 15:56 UTC (permalink / raw)
  To: David Lechner
  Cc: Andy Shevchenko, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On Wed, 23 Jul 2025 10:11:50 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/23/25 10:06 AM, Andy Shevchenko wrote:
> > On Wed, Jul 23, 2025 at 09:57:58AM -0500, David Lechner wrote:  
> >> On 7/23/25 9:37 AM, Andy Shevchenko wrote:  
> >>> On Wed, Jul 23, 2025 at 09:29:37AM -0500, David Lechner wrote:  
> >>>> On 7/23/25 9:13 AM, Andy Shevchenko wrote:  
> >>>>> On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:  
> >>>>>> On 7/22/25 6:07 PM, David Lechner wrote:  
> >>>>>>> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
> >>>>>>> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
> >>>>>>> the data before pushing it to the IIO buffer.  
> >>>>>  
> >>>>>> It is odd to have data already in CPU-endian and convert it to LE
> >>>>>> before pushing to buffers. So I'm a bit tempted to do this instead
> >>>>>> since it probably isn't likely anyone is using this on a big-endian
> >>>>>> system:  
> >>>>>
> >>>>> I can say that first of all, we need to consult with the datasheet for the
> >>>>> actual HW endianess. And second, I do not believe that CPU endianess may be
> >>>>> used,   
> >>>>
> >>>> Why not? Lot's of IIO drivers use IIO_CPU in their scan buffers.
> >>>>  
> >>>>> I can't imagine when this (discrete?) component can be integrated in such
> >>>>> a way. That said, I think your second approach even worse.  
> >>>>
> >>>> hx9023s_sample() is calling get_unaligned_le16() on all of the data
> >>>> read over the bus, so in the driver, all data is stored CPU-endian
> >>>> already rather than passing actual raw bus data to the buffer.  
> >>>
> >>> I see, now it makes a lot of sense. Thanks for clarifying this to me.
> >>>  
> >>>> So it seems a waste of CPU cycles to convert it back to little-endian
> >>>> to push to the buffer only for consumers to have to convert it back
> >>>> to CPU-endian again. But since most systems are little-endian already
> >>>> this doesn't really matter since no actual conversion is done in this
> >>>> case.  
> >>>
> >>> Right, but it's buggy on BE, isn't it?
> >>>  
> >>
> >> Right now, the driver is buggy everywhere. The scan info says that the
> >> scan data is BE, but in reality, it is LE (no matter the CPU-endianness).
> >>
> >> With the simple patch, it fixes the scan info to reflect reality that
> >> the data is LE in the buffer. This works on BE systems. They just have
> >> an extra conversion from BE to LE in the kernel when pushing to the
> >> buffer and userspace would have to convert back to BE to do math on it.
> >>
> >> With the alternate patch you didn't like, the forced conversion to LE
> >> when pushing to buffers is dropped, so nothing would change on LE
> >> systems but BE systems wouldn't have the extra order swapping.  
> > 
> > But do they need that? If you supply CPU order (and it is already in a such
> > after get_unaligned_*() calls) then everything would be good, no?
> >   
> 
> It doesn't make sense to my why, but the existing code is changing
> back to LE before pushing to buffers for some reason.
> 
> 
> 	iio_for_each_active_channel(indio_dev, bit) {
> 		index = indio_dev->channels[bit].channel;
> 		data->buffer.channels[i++] = cpu_to_le16(data->ch_data[index].diff);
> 	}
> 
> 	iio_push_to_buffers_with_ts(indio_dev, &data->buffer,
> 				    sizeof(data->buffer), pf->timestamp);
> 
> I agree that it seems unnecessary which is why I suggested the
> alternate patch to drop the cpu_to_le16() and just leave it
> CPU-endian when pushing to the buffers.
> 

This one was seriously odd and went though a lot of discussions around
endian conversions during review.  Definitely wait for Yasin to reply.

That's not to say I can recall how we ended up with the dance that
you are seeing now (and I'm too far behind on reviews to get to digging
through threads to refresh my memory).

Jonathan



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: proximity: hx9023s: fix scan_type endianness
  2025-07-23 15:56               ` Jonathan Cameron
@ 2025-08-25 17:07                 ` David Lechner
  0 siblings, 0 replies; 10+ messages in thread
From: David Lechner @ 2025-08-25 17:07 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Andy Shevchenko, Nuno Sá, Andy Shevchenko, Yasin Lee,
	linux-iio, linux-kernel, Jonathan Cameron

On 7/23/25 10:56 AM, Jonathan Cameron wrote:
> On Wed, 23 Jul 2025 10:11:50 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 7/23/25 10:06 AM, Andy Shevchenko wrote:
>>> On Wed, Jul 23, 2025 at 09:57:58AM -0500, David Lechner wrote:  
>>>> On 7/23/25 9:37 AM, Andy Shevchenko wrote:  
>>>>> On Wed, Jul 23, 2025 at 09:29:37AM -0500, David Lechner wrote:  
>>>>>> On 7/23/25 9:13 AM, Andy Shevchenko wrote:  
>>>>>>> On Tue, Jul 22, 2025 at 06:08:37PM -0500, David Lechner wrote:  
>>>>>>>> On 7/22/25 6:07 PM, David Lechner wrote:  
>>>>>>>>> Change the scan_type endianness from IIO_BE to IIO_LE. This matches
>>>>>>>>> the call to cpu_to_le16() in hx9023s_trigger_handler() that formats
>>>>>>>>> the data before pushing it to the IIO buffer.  
>>>>>>>  
>>>>>>>> It is odd to have data already in CPU-endian and convert it to LE
>>>>>>>> before pushing to buffers. So I'm a bit tempted to do this instead
>>>>>>>> since it probably isn't likely anyone is using this on a big-endian
>>>>>>>> system:  
>>>>>>>
>>>>>>> I can say that first of all, we need to consult with the datasheet for the
>>>>>>> actual HW endianess. And second, I do not believe that CPU endianess may be
>>>>>>> used,   
>>>>>>
>>>>>> Why not? Lot's of IIO drivers use IIO_CPU in their scan buffers.
>>>>>>  
>>>>>>> I can't imagine when this (discrete?) component can be integrated in such
>>>>>>> a way. That said, I think your second approach even worse.  
>>>>>>
>>>>>> hx9023s_sample() is calling get_unaligned_le16() on all of the data
>>>>>> read over the bus, so in the driver, all data is stored CPU-endian
>>>>>> already rather than passing actual raw bus data to the buffer.  
>>>>>
>>>>> I see, now it makes a lot of sense. Thanks for clarifying this to me.
>>>>>  
>>>>>> So it seems a waste of CPU cycles to convert it back to little-endian
>>>>>> to push to the buffer only for consumers to have to convert it back
>>>>>> to CPU-endian again. But since most systems are little-endian already
>>>>>> this doesn't really matter since no actual conversion is done in this
>>>>>> case.  
>>>>>
>>>>> Right, but it's buggy on BE, isn't it?
>>>>>  
>>>>
>>>> Right now, the driver is buggy everywhere. The scan info says that the
>>>> scan data is BE, but in reality, it is LE (no matter the CPU-endianness).
>>>>
>>>> With the simple patch, it fixes the scan info to reflect reality that
>>>> the data is LE in the buffer. This works on BE systems. They just have
>>>> an extra conversion from BE to LE in the kernel when pushing to the
>>>> buffer and userspace would have to convert back to BE to do math on it.
>>>>
>>>> With the alternate patch you didn't like, the forced conversion to LE
>>>> when pushing to buffers is dropped, so nothing would change on LE
>>>> systems but BE systems wouldn't have the extra order swapping.  
>>>
>>> But do they need that? If you supply CPU order (and it is already in a such
>>> after get_unaligned_*() calls) then everything would be good, no?
>>>   
>>
>> It doesn't make sense to my why, but the existing code is changing
>> back to LE before pushing to buffers for some reason.
>>
>>
>> 	iio_for_each_active_channel(indio_dev, bit) {
>> 		index = indio_dev->channels[bit].channel;
>> 		data->buffer.channels[i++] = cpu_to_le16(data->ch_data[index].diff);
>> 	}
>>
>> 	iio_push_to_buffers_with_ts(indio_dev, &data->buffer,
>> 				    sizeof(data->buffer), pf->timestamp);
>>
>> I agree that it seems unnecessary which is why I suggested the
>> alternate patch to drop the cpu_to_le16() and just leave it
>> CPU-endian when pushing to the buffers.
>>
> 
> This one was seriously odd and went though a lot of discussions around
> endian conversions during review.  Definitely wait for Yasin to reply.
> 
> That's not to say I can recall how we ended up with the dance that
> you are seeing now (and I'm too far behind on reviews to get to digging
> through threads to refresh my memory).
> 
> Jonathan
> 
> 

I found the message you are likely referring to [1]. It explains why we
can't use the raw data directly. There is some processing of the values
that is required. However, it doesn't explain the choice to convert things
to little-endian after the processing. Given the patch snippet in the
linked message, I think the intention (albeit misguided - we should have
suggested to just leave things CPU-endian) was to change from big-endian
to little-endian everywhere, but changing the scan_type to match was
overlooked in later revisions.

Applying this patch seems like the simplest thing to do. Or, if we don't want
others copying this questionable code, we could consider the heavier-handed
alternative to keep things CPU-endian.

[1]: https://lore.kernel.org/linux-iio/SN7PR12MB810159A22C9814AA7FC1AB96A4CE2@SN7PR12MB8101.namprd12.prod.outlook.com/

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-08-25 17:07 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-22 23:07 [PATCH] iio: proximity: hx9023s: fix scan_type endianness David Lechner
2025-07-22 23:08 ` David Lechner
2025-07-23 14:13   ` Andy Shevchenko
2025-07-23 14:29     ` David Lechner
2025-07-23 14:37       ` Andy Shevchenko
2025-07-23 14:57         ` David Lechner
2025-07-23 15:06           ` Andy Shevchenko
2025-07-23 15:11             ` David Lechner
2025-07-23 15:56               ` Jonathan Cameron
2025-08-25 17:07                 ` David Lechner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).