[PATCH 0/4] iio: imu: fxos8700: fix few bug in reading raw data and configuring register

[PATCH 0/4] iio: imu: fxos8700: fix few bug in reading raw data and configuring register

[Carlos Song]

Because of illegal use of raw data register and configuration error
of raw data base address, fix data acquisition function error. When the
device is in active mode, any change of the other fields within CTRL_REG1
will lead an invalid configuration. Set the device in standby mode before
configuring CTRL_REG1 register in chip initialization phase and setting
scale phase. Give the correct offset to value when configuring odr bit and
reading ODR data from CTRL_REG1 register. Set magnetometer scale and
available magnetometer a legal scale and rectify iio channel type to get
a correct magnetometer scale.

Carlos Song (4):
  iio: imu: fxos8700: fix get data function error
  iio: imu: fxos8700: fix CTRL_REG1 register configuration error
  iio: imu: fxos8700: fix ODR offset error
  iio: imu: fxos8700: fix magnetometer scale getting error

 drivers/iio/imu/fxos8700_core.c | 88 +++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 25 deletions(-)

-- 
2.34.1

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

[Carlos Song]

Types of raw data include acceleration and magnetism, so base
address should vary with raw data types rightly. 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>
---
 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);
 };
 
 /* 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)
 {
-	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;
 
-	/* Block read 6 bytes of device output registers to avoid data loss */
-	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);
 	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)
+		tmp = tmp >> 2;
 
-	/* Convert to native endianness */
-	*val = sign_extend32(be16_to_cpu(data->buf[reg]), 15);
+	*val = sign_extend32(tmp, 15);
 
 	return 0;
 }
-- 
2.34.1

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

[Jonathan Cameron]

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;
>  }

[PATCH 2/4] iio: imu: fxos8700: fix CTRL_REG1 register configuration error

[Carlos Song]

When the device is in active mode, any change of the other fields within
CTRL_REG1 will lead an invalid configuration. This not align with the
datasheet, but it is a fxos8700 chip behavier.

Set the device in standby mode before configuring CTRL_REG1 register
in chip initialization phase and setting scale phase.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/iio/imu/fxos8700_core.c | 36 ++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index a69122799892..60c08519d8af 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -343,7 +343,8 @@ static int fxos8700_set_active_mode(struct fxos8700_data *data,
 static int fxos8700_set_scale(struct fxos8700_data *data,
 			      enum fxos8700_sensor t, int uscale)
 {
-	int i;
+	int i, ret, val;
+	bool active_mode;
 	static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
 	struct device *dev = regmap_get_device(data->regmap);
 
@@ -352,6 +353,23 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
 		return -EINVAL;
 	}
 
+	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
+	if (ret)
+		return ret;
+
+	active_mode = val & FXOS8700_ACTIVE;
+	/*
+	 * The device must be in standby mode to change any of the
+	 * other fields within CTRL_REG1. This not align with the
+	 * datasheet, but it is a fxos8700 chip behavier.
+	 */
+	if (active_mode) {
+		ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
+				   val & ~FXOS8700_ACTIVE);
+		if (ret)
+			return ret;
+	}
+
 	for (i = 0; i < scale_num; i++)
 		if (fxos8700_accel_scale[i].uscale == uscale)
 			break;
@@ -359,8 +377,12 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
 	if (i == scale_num)
 		return -EINVAL;
 
-	return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
+	ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
 			    fxos8700_accel_scale[i].bits);
+	if (ret)
+		return ret;
+	return regmap_update_bits(data->regmap, FXOS8700_CTRL_REG1,
+				  FXOS8700_ACTIVE, active_mode);
 }
 
 static int fxos8700_get_scale(struct fxos8700_data *data,
@@ -607,14 +629,14 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
 	if (ret)
 		return ret;
 
-	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
-	ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
-			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
+	/* Set for max full-scale range (+/-8G) */
+	ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG, MODE_8G);
 	if (ret)
 		return ret;
 
-	/* Set for max full-scale range (+/-8G) */
-	return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG, MODE_8G);
+	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
+	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
+			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
 }
 
 static void fxos8700_chip_uninit(void *data)
-- 
2.34.1

Re: [PATCH 2/4] iio: imu: fxos8700: fix CTRL_REG1 register configuration error

[Jonathan Cameron]

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

> When the device is in active mode, any change of the other fields
Other from what?  I.e. say "any change of fields other than xxx within..."

> within
> CTRL_REG1 will lead an invalid configuration. This not align with the
> datasheet, but it is a fxos8700 chip behavier.

Spell check your patch descriptions (I forget to do this sometimes as well.)

Given it's an NXP part, any chance of a datasheet errata to fix this?

> 
> Set the device in standby mode before configuring CTRL_REG1 register
> in chip initialization phase and setting scale phase.

In the initialization you don't do this, you simply reorder the code so
that other settings are written first.  So make sure the patch description
is consistent with the code.

Also the write you've moved isn't in CTRL_REG1 so the above description does
not explain why it is necessary to do this in standby mode.

> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/iio/imu/fxos8700_core.c | 36 ++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index a69122799892..60c08519d8af 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -343,7 +343,8 @@ static int fxos8700_set_active_mode(struct fxos8700_data *data,
>  static int fxos8700_set_scale(struct fxos8700_data *data,
>  			      enum fxos8700_sensor t, int uscale)
>  {
> -	int i;
> +	int i, ret, val;
> +	bool active_mode;
>  	static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
>  	struct device *dev = regmap_get_device(data->regmap);
>  
> @@ -352,6 +353,23 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
>  		return -EINVAL;
>  	}
>  
> +	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
> +	if (ret)
> +		return ret;
> +
> +	active_mode = val & FXOS8700_ACTIVE;
> +	/*
> +	 * The device must be in standby mode to change any of the
> +	 * other fields within CTRL_REG1. This not align with the
> +	 * datasheet, but it is a fxos8700 chip behavier.
> +	 */
> +	if (active_mode) {
> +		ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> +				   val & ~FXOS8700_ACTIVE);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for (i = 0; i < scale_num; i++)
>  		if (fxos8700_accel_scale[i].uscale == uscale)
>  			break;
> @@ -359,8 +377,12 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
>  	if (i == scale_num)
>  		return -EINVAL;
>  
> -	return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> +	ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
>  			    fxos8700_accel_scale[i].bits);

As below, this isn't in CTRL_REG1 so not clear why we need to be
in standby mode to set it.

> +	if (ret)
> +		return ret;
> +	return regmap_update_bits(data->regmap, FXOS8700_CTRL_REG1,
> +				  FXOS8700_ACTIVE, active_mode);
Odd to use update bits here, but not when you clear the bit above.
Given you have the register value read back, either just using regmap_write()
in both cases, or use regmap_update_bits() in both cases would be fine by me.
Mixing the two isn't good for readability.

>  }
>  
>  static int fxos8700_get_scale(struct fxos8700_data *data,
> @@ -607,14 +629,14 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  	if (ret)
>  		return ret;
>  
> -	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> -	ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +	/* Set for max full-scale range (+/-8G) */
> +	ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG, MODE_8G);

Not in CTRL_REG1 so by the patch description, this doesn't need to be
done in standby mode. I'm guessing that description is meant to cover
other registers.

>  	if (ret)
>  		return ret;
>  
> -	/* Set for max full-scale range (+/-8G) */
> -	return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG, MODE_8G);
> +	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> +	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> +			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
>  }
>  
>  static void fxos8700_chip_uninit(void *data)

[PATCH 3/4] iio: imu: fxos8700: fix ODR offset error

[Carlos Song]

Correct offset of ODR configure is needed when configure the register
and read ODR data from the register.

Give the correct offset to value when configuring ODR bit and
reading ODR data from CTRL_REG1 register.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Fixes: 058f2a09e645 ("iio: imu: fxos8700: fix CTRL_REG1 register configuration error")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/iio/imu/fxos8700_core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 60c08519d8af..27e3bd61d054 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -147,6 +147,7 @@
 #define FXOS8700_CTRL_ODR_MSK       0x38
 #define FXOS8700_CTRL_ODR_MAX       0x00
 #define FXOS8700_CTRL_ODR_MIN       GENMASK(4, 3)
+#define FXOS8700_CTRL_ODR_OFFSET    3
 
 /* Bit definitions for FXOS8700_M_CTRL_REG1 */
 #define FXOS8700_HMS_MASK           GENMASK(1, 0)
@@ -498,8 +499,7 @@ static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
 	if (ret)
 		return ret;
 
-	val &= FXOS8700_CTRL_ODR_MSK;
-
+	val = (val & FXOS8700_CTRL_ODR_MSK) >> FXOS8700_CTRL_ODR_OFFSET;
 	for (i = 0; i < odr_num; i++)
 		if (val == fxos8700_odr[i].bits)
 			break;
@@ -636,7 +636,8 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
 
 	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
 	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
-			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
+			   FXOS8700_CTRL_ODR_MAX << FXOS8700_CTRL_ODR_OFFSET |
+			   FXOS8700_ACTIVE);
 }
 
 static void fxos8700_chip_uninit(void *data)
-- 
2.34.1

Re: [PATCH 3/4] iio: imu: fxos8700: fix ODR offset error

[Jonathan Cameron]

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

> Correct offset of ODR configure is needed when configure the register
> and read ODR data from the register.
> 
> Give the correct offset to value when configuring ODR bit and
> reading ODR data from CTRL_REG1 register.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Fixes: 058f2a09e645 ("iio: imu: fxos8700: fix CTRL_REG1 register configuration error")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Minor suggestion inline.

Btw, I'm not particularly keen on internal review tags.
I know the policy varies by company, but it it isn't too much of a problem, I'd
prefer to have seen Haibo Chen's review on list as that then gives me some
way to build trust in Haibo's reviews over the long run!

If not possible, then that's fine.

Jonathan

> ---
>  drivers/iio/imu/fxos8700_core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 60c08519d8af..27e3bd61d054 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -147,6 +147,7 @@
>  #define FXOS8700_CTRL_ODR_MSK       0x38
>  #define FXOS8700_CTRL_ODR_MAX       0x00
>  #define FXOS8700_CTRL_ODR_MIN       GENMASK(4, 3)
> +#define FXOS8700_CTRL_ODR_OFFSET    3
>  
>  /* Bit definitions for FXOS8700_M_CTRL_REG1 */
>  #define FXOS8700_HMS_MASK           GENMASK(1, 0)
> @@ -498,8 +499,7 @@ static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>  	if (ret)
>  		return ret;
>  
> -	val &= FXOS8700_CTRL_ODR_MSK;
> -
> +	val = (val & FXOS8700_CTRL_ODR_MSK) >> FXOS8700_CTRL_ODR_OFFSET;

FIELD_GET() / FIELD_PREP() preferred as it avoids need to separately define
an offset.

>  	for (i = 0; i < odr_num; i++)
>  		if (val == fxos8700_odr[i].bits)
>  			break;
> @@ -636,7 +636,8 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  
>  	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
>  	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +			   FXOS8700_CTRL_ODR_MAX << FXOS8700_CTRL_ODR_OFFSET |
> +			   FXOS8700_ACTIVE);
>  }
>  
>  static void fxos8700_chip_uninit(void *data)

[PATCH 4/4] iio: imu: fxos8700: fix magnetometer scale getting error

[Carlos Song]

Incorrect iio channel type cause a magnetometer scale getting
error. Meanwhile magnetometer scale and available magnetometer
scale should be locked as 0.1uT according to the datasheet.

Set magn sensor type "IIO_MAGN" and modify magn_scale fixed "0.1uT".

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/iio/imu/fxos8700_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 27e3bd61d054..8d46462dca76 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -319,7 +319,7 @@ static enum fxos8700_sensor fxos8700_to_sensor(enum iio_chan_type iio_type)
 	switch (iio_type) {
 	case IIO_ACCEL:
 		return FXOS8700_ACCEL;
-	case IIO_ANGL_VEL:
+	case IIO_MAGN:
 		return FXOS8700_MAGN;
 	default:
 		return -EINVAL;
@@ -350,7 +350,7 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
 	struct device *dev = regmap_get_device(data->regmap);
 
 	if (t == FXOS8700_MAGN) {
-		dev_err(dev, "Magnetometer scale is locked at 1200uT\n");
+		dev_err(dev, "Magnetometer scale is locked at 0.1uT\n");
 		return -EINVAL;
 	}
 
@@ -393,7 +393,7 @@ static int fxos8700_get_scale(struct fxos8700_data *data,
 	static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
 
 	if (t == FXOS8700_MAGN) {
-		*uscale = 1200; /* Magnetometer is locked at 1200uT */
+		*uscale = 100000; /* Magnetometer scale is locked at 0.1 uT */
 		return 0;
 	}
 
@@ -563,7 +563,7 @@ static IIO_CONST_ATTR(in_accel_sampling_frequency_available,
 static IIO_CONST_ATTR(in_magn_sampling_frequency_available,
 		      "1.5625 6.25 12.5 50 100 200 400 800");
 static IIO_CONST_ATTR(in_accel_scale_available, "0.000244 0.000488 0.000976");
-static IIO_CONST_ATTR(in_magn_scale_available, "0.000001200");
+static IIO_CONST_ATTR(in_magn_scale_available, "0.100000");
 
 static struct attribute *fxos8700_attrs[] = {
 	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,
-- 
2.34.1

Re: [PATCH 4/4] iio: imu: fxos8700: fix magnetometer scale getting error

[Jonathan Cameron]

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

> Incorrect iio channel type cause a magnetometer scale getting
> error. Meanwhile magnetometer scale and available magnetometer
> scale should be locked as 0.1uT according to the datasheet.
> 
> Set magn sensor type "IIO_MAGN" and modify magn_scale fixed "0.1uT".
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/iio/imu/fxos8700_core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 27e3bd61d054..8d46462dca76 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -319,7 +319,7 @@ static enum fxos8700_sensor fxos8700_to_sensor(enum iio_chan_type iio_type)
>  	switch (iio_type) {
>  	case IIO_ACCEL:
>  		return FXOS8700_ACCEL;
> -	case IIO_ANGL_VEL:
> +	case IIO_MAGN:

Ahah. This had me confused on looking at earlier patch ;)

Amusingly much of the code 'worked' because it was use as a boolean return
so -EINVAL had same affect as FXOS8700_MAGN.

This fix stands on it's own as a separate fix, so I'd rather see it in a
trivial patch of it's own before this one.


>  		return FXOS8700_MAGN;
>  	default:
>  		return -EINVAL;
> @@ -350,7 +350,7 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
>  	struct device *dev = regmap_get_device(data->regmap);
>  
>  	if (t == FXOS8700_MAGN) {
> -		dev_err(dev, "Magnetometer scale is locked at 1200uT\n");
> +		dev_err(dev, "Magnetometer scale is locked at 0.1uT\n");
>  		return -EINVAL;
>  	}
>  
> @@ -393,7 +393,7 @@ static int fxos8700_get_scale(struct fxos8700_data *data,
>  	static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
>  
>  	if (t == FXOS8700_MAGN) {
> -		*uscale = 1200; /* Magnetometer is locked at 1200uT */
> +		*uscale = 100000; /* Magnetometer scale is locked at 0.1 uT */
See Documentation/ABI/testing/sysfs-bus-iio
magnetometer channel units are Gauss. 
0.1ut = 0.001g so uscale should be 1000 I think with available changed to match.

>  		return 0;
>  	}
>  
> @@ -563,7 +563,7 @@ static IIO_CONST_ATTR(in_accel_sampling_frequency_available,
>  static IIO_CONST_ATTR(in_magn_sampling_frequency_available,
>  		      "1.5625 6.25 12.5 50 100 200 400 800");
>  static IIO_CONST_ATTR(in_accel_scale_available, "0.000244 0.000488 0.000976");
> -static IIO_CONST_ATTR(in_magn_scale_available, "0.000001200");
> +static IIO_CONST_ATTR(in_magn_scale_available, "0.100000")
>  
>  static struct attribute *fxos8700_attrs[] = {
>  	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,