[PATCH] iio: kx022a: Fix raw read format

[PATCH] iio: kx022a: Fix raw read format

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

The KX022A provides the accelerometer data in two subsequent registers.
The registers are laid out so that the value obtained via bulk-read of
these registers can be interpreted as signed 16-bit little endian value.
The read value is converted to cpu_endianes and stored into 32bit integer.
The le16_to_cpu() casts value to unsigned 16-bit value, and when this is
assigned to 32-bit integer the resulting value will always be positive.

This has not been a problem to users (at least not all users) of the sysfs
interface, who know the data format based on the scan info and who have
converted the read value back to 16-bit signed value.

This, however, will be a problem for those who use the in-kernel
interfaces, especially the iio_read_channel_processed_scale().

The iio_read_channel_processed_scale() performs multiplications to the
returned (always positive) raw value, which will cause strange results
when the data from the sensor has been negative.

Fix the read_raw format by casting the result of the le_to_cpu() to
signed 16-bit value before assigning it to the integer. This will make
the negative readings to be correctly reported as negative.

This fix will be visible to users by changing values returned via sysfs
to appear in correct (negative) format.

Reported-by: Kalle Niemi <kaleposti@gmail.com>
Fixes: 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Tested-by: Kalle Niemi <kaleposti@gmail.com>
---
 drivers/iio/accel/kionix-kx022a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 53d59a04ae15..b6a828a6df93 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -594,7 +594,7 @@ static int kx022a_get_axis(struct kx022a_data *data,
 	if (ret)
 		return ret;
 
-	*val = le16_to_cpu(data->buffer[0]);
+	*val = (s16)le16_to_cpu(data->buffer[0]);
 
 	return IIO_VAL_INT;
 }

base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
-- 
2.47.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: kx022a: Fix raw read format

[Matti Vaittinen]

On 30/10/2024 15:16, Matti Vaittinen wrote:
> The KX022A provides the accelerometer data in two subsequent registers.
> The registers are laid out so that the value obtained via bulk-read of
> these registers can be interpreted as signed 16-bit little endian value.
> The read value is converted to cpu_endianes and stored into 32bit integer.
> The le16_to_cpu() casts value to unsigned 16-bit value, and when this is
> assigned to 32-bit integer the resulting value will always be positive.
> 
> This has not been a problem to users (at least not all users) of the sysfs
> interface, who know the data format based on the scan info and who have
> converted the read value back to 16-bit signed value.
> 
> This, however, will be a problem for those who use the in-kernel
> interfaces, especially the iio_read_channel_processed_scale().
> 
> The iio_read_channel_processed_scale() performs multiplications to the
> returned (always positive) raw value, which will cause strange results
> when the data from the sensor has been negative.
> 
> Fix the read_raw format by casting the result of the le_to_cpu() to
> signed 16-bit value before assigning it to the integer. This will make
> the negative readings to be correctly reported as negative.
> 
> This fix will be visible to users by changing values returned via sysfs
> to appear in correct (negative) format.
> 
> Reported-by: Kalle Niemi <kaleposti@gmail.com>
> Fixes: 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Tested-by: Kalle Niemi <kaleposti@gmail.com>
> ---
>   drivers/iio/accel/kionix-kx022a.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 53d59a04ae15..b6a828a6df93 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -594,7 +594,7 @@ static int kx022a_get_axis(struct kx022a_data *data,
>   	if (ret)
>   		return ret;
>   
> -	*val = le16_to_cpu(data->buffer[0]);
> +	*val = (s16)le16_to_cpu(data->buffer[0]);
>   
>   	return IIO_VAL_INT;
>   }
> 
> base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e


I should have CC's Mehdi who added the kx132-1211 support. Did so now - 
sorry for the noise.

Yours,
	-- Matti

Re: [PATCH] iio: kx022a: Fix raw read format

[Jonathan Cameron]

On Wed, 30 Oct 2024 15:16:11 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The KX022A provides the accelerometer data in two subsequent registers.
> The registers are laid out so that the value obtained via bulk-read of
> these registers can be interpreted as signed 16-bit little endian value.
> The read value is converted to cpu_endianes and stored into 32bit integer.
> The le16_to_cpu() casts value to unsigned 16-bit value, and when this is
> assigned to 32-bit integer the resulting value will always be positive.
> 
> This has not been a problem to users (at least not all users) of the sysfs
> interface, who know the data format based on the scan info and who have
> converted the read value back to 16-bit signed value.
They shouldn't be doing that. Scaninfo is for buffered values only
This should indeed be signed.

> 
> This, however, will be a problem for those who use the in-kernel
> interfaces, especially the iio_read_channel_processed_scale().
> 
> The iio_read_channel_processed_scale() performs multiplications to the
> returned (always positive) raw value, which will cause strange results
> when the data from the sensor has been negative.
> 
> Fix the read_raw format by casting the result of the le_to_cpu() to
> signed 16-bit value before assigning it to the integer. This will make
> the negative readings to be correctly reported as negative.
> 
> This fix will be visible to users by changing values returned via sysfs
> to appear in correct (negative) format.
> 
> Reported-by: Kalle Niemi <kaleposti@gmail.com>
> Fixes: 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Tested-by: Kalle Niemi <kaleposti@gmail.com>
> ---
>  drivers/iio/accel/kionix-kx022a.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 53d59a04ae15..b6a828a6df93 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -594,7 +594,7 @@ static int kx022a_get_axis(struct kx022a_data *data,
>  	if (ret)
>  		return ret;
>  
> -	*val = le16_to_cpu(data->buffer[0]);
> +	*val = (s16)le16_to_cpu(data->buffer[0]);
LGTM.
>  
>  	return IIO_VAL_INT;
>  }
> 
> base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e

Re: [PATCH] iio: kx022a: Fix raw read format

[Jonathan Cameron]

On Wed, 30 Oct 2024 18:06:58 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 30 Oct 2024 15:16:11 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> > The KX022A provides the accelerometer data in two subsequent registers.
> > The registers are laid out so that the value obtained via bulk-read of
> > these registers can be interpreted as signed 16-bit little endian value.
> > The read value is converted to cpu_endianes and stored into 32bit integer.
> > The le16_to_cpu() casts value to unsigned 16-bit value, and when this is
> > assigned to 32-bit integer the resulting value will always be positive.
> > 
> > This has not been a problem to users (at least not all users) of the sysfs
> > interface, who know the data format based on the scan info and who have
> > converted the read value back to 16-bit signed value.  
> They shouldn't be doing that. Scaninfo is for buffered values only
> This should indeed be signed.
I added a note saying this isn't compliant with the ABI.
Applied to the fixes-togreg branch fo iio.git and marked for stable.

> 
> > 
> > This, however, will be a problem for those who use the in-kernel
> > interfaces, especially the iio_read_channel_processed_scale().
> > 
> > The iio_read_channel_processed_scale() performs multiplications to the
> > returned (always positive) raw value, which will cause strange results
> > when the data from the sensor has been negative.
> > 
> > Fix the read_raw format by casting the result of the le_to_cpu() to
> > signed 16-bit value before assigning it to the integer. This will make
> > the negative readings to be correctly reported as negative.
> > 
> > This fix will be visible to users by changing values returned via sysfs
> > to appear in correct (negative) format.
> > 
> > Reported-by: Kalle Niemi <kaleposti@gmail.com>
> > Fixes: 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > Tested-by: Kalle Niemi <kaleposti@gmail.com>
> > ---
> >  drivers/iio/accel/kionix-kx022a.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> > index 53d59a04ae15..b6a828a6df93 100644
> > --- a/drivers/iio/accel/kionix-kx022a.c
> > +++ b/drivers/iio/accel/kionix-kx022a.c
> > @@ -594,7 +594,7 @@ static int kx022a_get_axis(struct kx022a_data *data,
> >  	if (ret)
> >  		return ret;
> >  
> > -	*val = le16_to_cpu(data->buffer[0]);
> > +	*val = (s16)le16_to_cpu(data->buffer[0]);  
> LGTM.
> >  
> >  	return IIO_VAL_INT;
> >  }
> > 
> > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e  
>