Re: [PATCH 1/4] iio: imu: fxos8700: fix get data function error

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  2 Dec 2022 18:35:35 +0800
Carlos Song <carlos.song@nxp.com> wrote:

> Types of raw data include acceleration and magnetism, so base
> address should vary with raw data types rightly.

You need to be much clearer on what you saying here.  I 'think'
the point could be put as:

"Fix swapped accelerometer and magnetometer channels readback."
> The function
> for data acquisition is incorrect because of not considering
> msb data and lsb data. Acceleration raw data is 14 bits and
> magnetism raw data is 16 bits, data reprocessing is necessary
> accordingly.
> 
> Rewrite the function for data acquisition. Base data register
> address is configured correctly varied with acceleration and
> magnetism raw data, and data reprocessing method is added.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Fixes: c9a8417a13ed ("iio: imu: fxos8700: Fix alignment for DMA safety")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

Hi Carlos

This needs to be a minimal fix for backporting.  So keep reading all the
channels, just fix the base. We do not want to backport more than
the absolute minimum needed to fix the issue.  So I think that
means one patch to fix the switch registers only and a second
one to apply the shift.

Once that is done you can then send optimizations.

Please also confirm that the scaling is still correct for the
acceleration channels given we are effectively dividing by 4 compared
to the previous code.  If it always assumed 14 bits then just state
that in the cover letter, if it didn't then you need to fix that as well.



> ---
>  drivers/iio/imu/fxos8700_core.c | 39 +++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 423cfe526f2a..a69122799892 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -162,12 +162,10 @@
>  
>  #define FXOS8700_DEVICE_ID          0xC7
>  #define FXOS8700_PRE_DEVICE_ID      0xC4
> -#define FXOS8700_DATA_BUF_SIZE      3
>  
>  struct fxos8700_data {
>  	struct regmap *regmap;
>  	struct iio_trigger *trig;
> -	__be16 buf[FXOS8700_DATA_BUF_SIZE] __aligned(IIO_DMA_MINALIGN);

I'd definitely expect to see a statement in the patch text on why
a DMA safe buffer is no longer required.

Unless things have changed, regmap may require DMA safe buffers for SPI in the
future even if it does not happen to do so today.  That's why we go through
this dance. Now as there are only bulk reads you may be fine, but you
definitely don't do just make this sort of change without a clearly
reasoned explanation.  Also if it is justified, do it in a separate patch 
so that we can clearly analyse that change on it's own.

>  };
>  
>  /* Regmap info */
> @@ -391,25 +389,42 @@ static int fxos8700_get_scale(struct fxos8700_data *data,
>  }
>  
>  static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
> -			     int axis, int *val)
> +				 int axis, int *val)

Avoid white space changes in a patch that does anything else.
I'm also fairly sure this doesn't comply with the kernel preferred convention
of aligning parameters on later lines with the start of the ones after the (


>  {
> -	u8 base, reg;
> +	u8 base, offset;
> +	__be16 buf;
> +	s16 tmp;
>  	int ret;
>  	enum fxos8700_sensor type = fxos8700_to_sensor(chan_type);
>  
> -	base = type ? FXOS8700_OUT_X_MSB : FXOS8700_M_OUT_X_MSB;
> +	/*
> +	 * FXOS8700_M_OUT_X_MSB is magnetic X-axis output data register address.
> +	 * FXOS8700_OUT_X_MSB is acceler X-axis output data register address.
> +	 * Type is 1 for FXOS8700_MAGN, 0 for FXOS8700_ACCEL.
> +	 */
> +	base = type ? FXOS8700_M_OUT_X_MSB : FXOS8700_OUT_X_MSB;

This bug hasn't been noticed before because using an enum is a ternary is
really hard to read. Either use an explicit match or a switch statement.

There is also no point in converting the IIO type to the local one for this.
	switch (chan_type) {
	case IIO_ACCEL:
		base = FXOS8700_OUT_X_MSB;
		break;
	case IIO_ANGL_VEL: /* WHERE DID THIS COME FROM!!! SHOULD BE IIO_MAGN I THINK */
		base = FXOS8700_M_OUT_X_MSB;
		break;
	default:
		return -EINVAL;
	}
	
> -	/* Block read 6 bytes of device output registers to avoid data loss */

If this comment was untrue, please state that very clearly in the patch introduction
seeing as you are changing the logic so that whatever that was warning against
isn't going to happen any more.

> -	ret = regmap_bulk_read(data->regmap, base, data->buf,
> -			       FXOS8700_DATA_BUF_SIZE);
> +	/* Convert axis to offset index */
> +	offset = axis - IIO_MOD_X;
> +
> +	ret = regmap_bulk_read(data->regmap, base + offset, &buf, 2);
sizeof(buf) instead of hardcoded 2.

>  	if (ret)
>  		return ret;
>  
> -	/* 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(buf);
> +
> +	/*
> +	 * Accel raw data is 14 bit, magn raw data is 16 bit, value should be
> +	 * extended to 32 bit.
> +	 */
> +	if (!type)

Store the needed shift in the switch and apply it here unconditionally.
A shift by 0 is fine for the case where it's 16 bits.

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