Re: [PATCH v2 4/7] iio: imu: fxos8700: fix IMU data bits returned to user space

[Jonathan Cameron <jic23@kernel.org>]

On Thu,  8 Dec 2022 15:19:08 +0800
carlos.song@nxp.com wrote:

> From: Carlos Song <carlos.song@nxp.com>
> 
> ACCEL output data registers contain the X-axis, Y-axis, and Z-axis
> 14-bit left-justified sample data and MAGN output data registers
> contain the X-axis, Y-axis, and Z-axis 16-bit sample data. The ACCEL
> raw register output data should be divided by 4 before sent to
> userspace.
> 
> Apply a 2 bits signed right shift to the raw data from ACCEL output
> data register but keep that from MAGN sensor as the origin.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
I made one tweak (see below). 

Applied to the fixes-togreg branch of iio.git and marked for stable inclusion.

> ---
> Changes for V2:
> - Store the shift in the switch and apply a shift by 2 for ACCEL
>   and shift by 0 for MAGN
> - Confirm the scaling is still correct for the acceleration channels
>   given we are effectively dividing by 4 compared to the previous code
> - Rework the comment
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index b62bc92bbacc..d2e784628820 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -394,6 +394,7 @@ static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
>  			     int axis, int *val)
>  {
>  	u8 base, reg;
> +	s16 tmp;
>  	int ret;
>  
>  	/*
> @@ -421,8 +422,33 @@ static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
>  	/* Convert axis to buffer index */
>  	reg = axis - IIO_MOD_X;
>  
> +	/*
> +	 * Convert to native endianness. The accel data and magn data
> +	 * are signed, so a forced type conversion is needed.
> +	 */
> +	tmp = be16_to_cpu(data->buf[reg]);
> +
> +	/*
> +	 * ACCEL output data registers contain the X-axis, Y-axis, and Z-axis
> +	 * 14-bit left-justified sample data and MAGN output data registers
> +	 * contain the X-axis, Y-axis, and Z-axis 16-bit sample data. Apply
> +	 * a signed 2 bits right shift to the readback raw data from ACCEL
> +	 * output data register and keep that from MAGN sensor as the origin.
> +	 * Value should be extended to 32 bit.
> +	 */
> +	switch (chan_type) {
> +	case IIO_ACCEL:
> +		tmp = tmp >> 2;
> +		break;
> +	case IIO_MAGN:
> +		tmp = tmp >> 0;
I replaced this no operation line with a comment
/* Nothing to do */ 

otherwise it's the sort of thing some static checker might moan about :)

Jonathan

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>  	/* Convert to native endianness */
> -	*val = sign_extend32(be16_to_cpu(data->buf[reg]), 15);
> +	*val = sign_extend32(tmp, 15);
>  
>  	return 0;
>  }