[PATCH v2 0/7] iio: imu: fxos8700: fix few bugs in data readback and mode set

[PATCH v2 0/7] iio: imu: fxos8700: fix few bugs in data readback and mode set

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

I'm grateful to receive and accept suggestions for these patches from
Jonathan. I am so sorry for sending patches with much confusion before.
In PATCH v2, I have reworked our patches to give a minimal fix and split
the large patches into more smaller patches, especially rewritten the
commit logs and comments. I hope these can explain more clearly the patch
of the role of FXOS8700 driver.

We have found some bugs exist in fxos8700 IMU sensor as follows,
some patches were proposed correspondingly:
- Confused IIO_ANGL_VEL channel type mapped to MAGN sensor
  PATCH: iio: imu: fxos8700: fix map lable of channel type to MAGN sensor
- Incorrect ACCEL and MAGN channels readback
  PATCH:
  iio: imu: fxos8700: fix swapped ACCEL and MAGN channels readback
  iio: imu: fxos8700: fix incompete ACCEL and MAGN channels readback
  iio: imu: fxos8700: fix IMU data bits returned to user space
- Fail to select an ACCEL measuremen range(2g/4g/8g)
  PATCH: iio: imu: fxos8700: fix ACCEL measurement range selection
- Fail to get and config ODR mode
  PATCH: iio: imu: fxos8700: fix ODR register readback and initialization
- Incorrect MAGN scale readback
  PATCH: iio: imu: fxos8700: fix MAGN sensor scale and unit
---
Changes for V2:
- Divide [PATCHv1 1/4] into three patches to fix ACCEL and MAGN
  channels readback. We rework our patches to fix the bug in a
  more concise way. In this way, we give a minimal and base fix
  compared with PATCHv1 and remain the logic of the original code
  as much as possible.
- For [PATCHv1 2/4], we rework commit log and comments to explain why
  it is necessary to set device in standby mode when select an ACCEL
  measure range(2g/4g/8g).
- For [PATCHv1 3/4], according to the suggestion of Jonathan, with
  unified use of regmap_write(), FIELD_GET()/FIELD_PREP() when read
  and write correct ODR register bits. This will greatly improve the
  readability of the code.
- Divide [PATCHv1 4/4] into two patches: one patch separately fixes
  confused IIO_ANGL_VEL channel type mapped to MAGN sensor, the other
  one fixes MAGN sensor scale and unit.

Carlos Song (7):
  iio: imu: fxos8700: fix map lable of channel type to MAGN sensor
  iio: imu: fxos8700: fix swapped ACCEL and MAGN channels readback
  iio: imu: fxos8700: fix incompete ACCEL and MAGN channels readback
  iio: imu: fxos8700: fix IMU data bits returned to user space
  iio: imu: fxos8700: fix ACCEL measurement range selection
  iio: imu: fxos8700: fix ODR register readback and initialization
  iio: imu: fxos8700: fix MAGN sensor scale and unit

 drivers/iio/imu/fxos8700_core.c | 113 ++++++++++++++++++++++++++------
 1 file changed, 92 insertions(+), 21 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] iio: imu: fxos8700: fix map lable of channel type to MAGN sensor

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

FXOS8700 is an IMU sensor with ACCEL sensor and MAGN sensor.
Sensor type is indexed by corresponding channel type in a switch.
IIO_ANGL_VEL channel type mapped to MAGN sensor has caused confusion.

Fix the mapping lable of "IIO_MAGN" channel type instead of
"IIO_ANGL_VEL" channel type to MAGN sensor.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V2:
- Modify the magnetometer sensitivity unit to be consistent with the
  documentation as 0.001g
- Rework commit log

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 423cfe526f2a..235b02b2f4e5 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -320,7 +320,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;
-- 
2.34.1

Re: [PATCH v2 1/7] iio: imu: fxos8700: fix map lable of channel type to MAGN sensor

[Jonathan Cameron]

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

> From: Carlos Song <carlos.song@nxp.com>
> 
> FXOS8700 is an IMU sensor with ACCEL sensor and MAGN sensor.
> Sensor type is indexed by corresponding channel type in a switch.
> IIO_ANGL_VEL channel type mapped to MAGN sensor has caused confusion.
> 
> Fix the mapping lable of "IIO_MAGN" channel type instead of
> "IIO_ANGL_VEL" channel type to MAGN sensor.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
Applied to the fixes-togreg branch of iio.git and marked for stable
inclusion.

Thanks,

Jonathan

> ---
> Changes for V2:
> - Modify the magnetometer sensitivity unit to be consistent with the
>   documentation as 0.001g
> - Rework commit log
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 423cfe526f2a..235b02b2f4e5 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -320,7 +320,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;

[PATCH v2 2/7] iio: imu: fxos8700: fix swapped ACCEL and MAGN channels readback

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

Because ACCEL and MAGN channels data register base address is
swapped by error judgement, accelerometer and magnetometer
channels readback is swapped.

Fix swapped accelerometer and magnetometer channels readback.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V2:
- Use a switch statement to instead of an enum to index the base
  address of the register by the channel type
- Rework commit log and comments

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 235b02b2f4e5..977eb7dc7dbd 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -395,9 +395,22 @@ static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
 {
 	u8 base, reg;
 	int ret;
-	enum fxos8700_sensor type = fxos8700_to_sensor(chan_type);
 
-	base = type ? FXOS8700_OUT_X_MSB : FXOS8700_M_OUT_X_MSB;
+	/*
+	 * Different register base addresses varies with channel types.
+	 * This bug hasn't been noticed before because using an enum is
+	 * really hard to read. Use an a switch statement to take over that.
+	 */
+	switch (chan_type) {
+	case IIO_ACCEL:
+		base = FXOS8700_OUT_X_MSB;
+		break;
+	case IIO_MAGN:
+		base = FXOS8700_M_OUT_X_MSB;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	/* Block read 6 bytes of device output registers to avoid data loss */
 	ret = regmap_bulk_read(data->regmap, base, data->buf,
-- 
2.34.1

Re: [PATCH v2 2/7] iio: imu: fxos8700: fix swapped ACCEL and MAGN channels readback

[Jonathan Cameron]

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

> From: Carlos Song <carlos.song@nxp.com>
> 
> Because ACCEL and MAGN channels data register base address is
> swapped by error judgement, accelerometer and magnetometer
> channels readback is swapped.
> 
> Fix swapped accelerometer and magnetometer channels readback.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
Applied to the fixes-togreg branch of iio.git and marked
for stable.

I tweaked the description a little for readability.

Thanks,

Jonathan

> ---
> Changes for V2:
> - Use a switch statement to instead of an enum to index the base
>   address of the register by the channel type
> - Rework commit log and comments
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 235b02b2f4e5..977eb7dc7dbd 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -395,9 +395,22 @@ static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
>  {
>  	u8 base, reg;
>  	int ret;
> -	enum fxos8700_sensor type = fxos8700_to_sensor(chan_type);
>  
> -	base = type ? FXOS8700_OUT_X_MSB : FXOS8700_M_OUT_X_MSB;
> +	/*
> +	 * Different register base addresses varies with channel types.
> +	 * This bug hasn't been noticed before because using an enum is
> +	 * really hard to read. Use an a switch statement to take over that.
> +	 */
> +	switch (chan_type) {
> +	case IIO_ACCEL:
> +		base = FXOS8700_OUT_X_MSB;
> +		break;
> +	case IIO_MAGN:
> +		base = FXOS8700_M_OUT_X_MSB;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	/* Block read 6 bytes of device output registers to avoid data loss */
>  	ret = regmap_bulk_read(data->regmap, base, data->buf,

[PATCH v2 3/7] iio: imu: fxos8700: fix incompete ACCEL and MAGN channels readback

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

The length of ACCEL and MAGN 3-aix channels output data is 6 byte
individually. However block only read 3 bytes data into buffer from
ACCEL or MAGN output data registers every time. It causes an incompete
ACCEL and MAGN channels readback.

Set correct value count for regmap_bulk_read to get 6 bytes ACCEL and
MAGN channels readback.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V2:
- Reserve the global DMA safe buffer
- Use sizeof(data->buf) instead of hardcoded FXOS8700_DATA_BUF_SIZE
- Rework commit log

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 977eb7dc7dbd..b62bc92bbacc 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -414,7 +414,7 @@ static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
 
 	/* 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);
+			       sizeof(data->buf));
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v2 3/7] iio: imu: fxos8700: fix incompete ACCEL and MAGN channels readback

[Jonathan Cameron]

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

> From: Carlos Song <carlos.song@nxp.com>
> 
> The length of ACCEL and MAGN 3-aix channels output data is 6 byte
> individually. However block only read 3 bytes data into buffer from
> ACCEL or MAGN output data registers every time. It causes an incompete
> ACCEL and MAGN channels readback.
> 
> Set correct value count for regmap_bulk_read to get 6 bytes ACCEL and
> MAGN channels readback.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
Please spell check patch descriptions.  Anyhow, tidied that up whilst
applying.

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

Thanks,

Jonathan

> ---
> Changes for V2:
> - Reserve the global DMA safe buffer
> - Use sizeof(data->buf) instead of hardcoded FXOS8700_DATA_BUF_SIZE
> - Rework commit log
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 977eb7dc7dbd..b62bc92bbacc 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -414,7 +414,7 @@ static int fxos8700_get_data(struct fxos8700_data *data, int chan_type,
>  
>  	/* 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);
> +			       sizeof(data->buf));
>  	if (ret)
>  		return ret;
>  

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

[carlos.song]

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

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

[Jonathan Cameron]

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

[PATCH v2 5/7] iio: imu: fxos8700: fix ACCEL measurement range selection

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

When device is in active mode, it fails to set an ACCEL full-scale
range(2g/4g/8g) in FXOS8700_XYZ_DATA_CFG. This is not align with the
datasheet, but it is a fxos8700 chip behavier.

Keep the device in standby mode before setting ACCEL full-scale range
into FXOS8700_XYZ_DATA_CFG 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>
---
Changes for V2:
- Rework commit log and comment
- Using regmap_write() instead of regmap_update_bits() for readability

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index d2e784628820..773f62203bf0 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -345,7 +345,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);
 
@@ -354,6 +355,25 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
 		return -EINVAL;
 	}
 
+	/*
+	 * When device is in active mode, it failed to set an ACCEL
+	 * full-scale range(2g/4g/8g) in FXOS8700_XYZ_DATA_CFG.
+	 * This is not align with the datasheet, but it is a fxos8700
+	 * chip behavier. Set the device in standby mode before setting
+	 * an ACCEL full-scale range.
+	 */
+	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
+	if (ret)
+		return ret;
+
+	active_mode = val & FXOS8700_ACTIVE;
+	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;
@@ -361,8 +381,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_write(data->regmap, FXOS8700_CTRL_REG1,
+				  active_mode);
 }
 
 static int fxos8700_get_scale(struct fxos8700_data *data,
@@ -631,14 +655,17 @@ 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 max full-scale range (+/-8G) for ACCEL sensor in chip
+	 * initialization then activate the device.
+	 */
+	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 v2 5/7] iio: imu: fxos8700: fix ACCEL measurement range selection

[Jonathan Cameron]

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

> From: Carlos Song <carlos.song@nxp.com>
> 
> When device is in active mode, it fails to set an ACCEL full-scale
> range(2g/4g/8g) in FXOS8700_XYZ_DATA_CFG. This is not align with the
> datasheet, but it is a fxos8700 chip behavier.
> 
> Keep the device in standby mode before setting ACCEL full-scale range
> into FXOS8700_XYZ_DATA_CFG 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>
Applied to the fixes-togreg branch of iio.git and marked for stable inclusion.

Thanks,

Jonathan

> ---
> Changes for V2:
> - Rework commit log and comment
> - Using regmap_write() instead of regmap_update_bits() for readability
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index d2e784628820..773f62203bf0 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -345,7 +345,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);
>  
> @@ -354,6 +355,25 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * When device is in active mode, it failed to set an ACCEL
> +	 * full-scale range(2g/4g/8g) in FXOS8700_XYZ_DATA_CFG.
> +	 * This is not align with the datasheet, but it is a fxos8700
> +	 * chip behavier. Set the device in standby mode before setting
> +	 * an ACCEL full-scale range.
> +	 */
> +	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
> +	if (ret)
> +		return ret;
> +
> +	active_mode = val & FXOS8700_ACTIVE;
> +	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;
> @@ -361,8 +381,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_write(data->regmap, FXOS8700_CTRL_REG1,
> +				  active_mode);
>  }
>  
>  static int fxos8700_get_scale(struct fxos8700_data *data,
> @@ -631,14 +655,17 @@ 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 max full-scale range (+/-8G) for ACCEL sensor in chip
> +	 * initialization then activate the device.
> +	 */
> +	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)

[PATCH v2 6/7] iio: imu: fxos8700: fix ODR register readback and initialization

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

Use the hexadecimal number to read and write the incorrect bits of
the ODR register. It fails to set MAX ODR mode in chip initialization
and it will return an incorrect ODR mode to userspace.

Use of regmap_write() instead of regmap_update_bits() to update bits
is good for readability. FIELD_GET()/FIELD_PREP() can help clear register
bit definition and avoid separately to define an offset. Unified use of
regmap_write(), FIELD_GET()/FIELD_PREP() to read and write correct ODR
register bits.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V2:
- Modify FXOS8700_CTRL_ODR_MSK MASK instead of hexadecimal
- Use of regmap_write() instead of regmap_update_bits()
- Use FIELD_GET()/FIELD_PREP() to avoid to separately define an offset
- Rework commit log

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 773f62203bf0..b4baef82f6d5 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -10,6 +10,7 @@
 #include <linux/regmap.h>
 #include <linux/acpi.h>
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -144,9 +145,9 @@
 #define FXOS8700_NVM_DATA_BNK0      0xa7
 
 /* Bit definitions for FXOS8700_CTRL_REG1 */
-#define FXOS8700_CTRL_ODR_MSK       0x38
 #define FXOS8700_CTRL_ODR_MAX       0x00
 #define FXOS8700_CTRL_ODR_MIN       GENMASK(4, 3)
+#define FXOS8700_CTRL_ODR_MSK       GENMASK(5, 3)
 
 /* Bit definitions for FXOS8700_M_CTRL_REG1 */
 #define FXOS8700_HMS_MASK           GENMASK(1, 0)
@@ -481,6 +482,7 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
 			    int odr, int uodr)
 {
 	int i, ret, val;
+	int odr_mode;
 	bool active_mode;
 	static const int odr_num = ARRAY_SIZE(fxos8700_odr);
 
@@ -508,10 +510,10 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
 	if (i >= odr_num)
 		return -EINVAL;
 
-	return regmap_update_bits(data->regmap,
-				  FXOS8700_CTRL_REG1,
-				  FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
-				  fxos8700_odr[i].bits << 3 | active_mode);
+	odr_mode &= ~FXOS8700_CTRL_ODR_MSK;
+	odr_mode |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits);
+	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
+			   odr_mode | active_mode);
 }
 
 static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
@@ -524,7 +526,7 @@ static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
 	if (ret)
 		return ret;
 
-	val &= FXOS8700_CTRL_ODR_MSK;
+	val = FIELD_GET(FXOS8700_CTRL_ODR_MSK, val);
 
 	for (i = 0; i < odr_num; i++)
 		if (val == fxos8700_odr[i].bits)
@@ -612,6 +614,7 @@ static const struct iio_info fxos8700_info = {
 static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
 {
 	int ret;
+	int odr_mode;
 	unsigned int val;
 	struct device *dev = regmap_get_device(data->regmap);
 
@@ -664,8 +667,10 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
 		return ret;
 
 	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
+	odr_mode &= ~FXOS8700_CTRL_ODR_MSK;
+	odr_mode |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX);
 	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
-			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
+			   odr_mode | FXOS8700_ACTIVE);
 }
 
 static void fxos8700_chip_uninit(void *data)
-- 
2.34.1

Re: [PATCH v2 6/7] iio: imu: fxos8700: fix ODR register readback and initialization

[Jonathan Cameron]

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

> From: Carlos Song <carlos.song@nxp.com>
> 
> Use the hexadecimal number to read and write the incorrect bits of
> the ODR register. It fails to set MAX ODR mode in chip initialization
> and it will return an incorrect ODR mode to userspace.
> 
> Use of regmap_write() instead of regmap_update_bits() to update bits
> is good for readability. FIELD_GET()/FIELD_PREP() can help clear register
> bit definition and avoid separately to define an offset. Unified use of
> regmap_write(), FIELD_GET()/FIELD_PREP() to read and write correct ODR
> register bits.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
This should probably be split into the minimal fix and then the
cleanup (which can follow later, keeping the code to backport more minimal.)

Comments inline.

> ---
> Changes for V2:
> - Modify FXOS8700_CTRL_ODR_MSK MASK instead of hexadecimal
> - Use of regmap_write() instead of regmap_update_bits()
> - Use FIELD_GET()/FIELD_PREP() to avoid to separately define an offset
> - Rework commit log
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 773f62203bf0..b4baef82f6d5 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -10,6 +10,7 @@
>  #include <linux/regmap.h>
>  #include <linux/acpi.h>
>  #include <linux/bitops.h>
> +#include <linux/bitfield.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -144,9 +145,9 @@
>  #define FXOS8700_NVM_DATA_BNK0      0xa7
>  
>  /* Bit definitions for FXOS8700_CTRL_REG1 */
> -#define FXOS8700_CTRL_ODR_MSK       0x38
>  #define FXOS8700_CTRL_ODR_MAX       0x00
>  #define FXOS8700_CTRL_ODR_MIN       GENMASK(4, 3)

Unrelated but this is a very odd thing to see.  A mask for something called _MIN.
It's not used but value is probably wrong.  Best thing is probably just to remove it.

> +#define FXOS8700_CTRL_ODR_MSK       GENMASK(5, 3)
>  
>  /* Bit definitions for FXOS8700_M_CTRL_REG1 */
>  #define FXOS8700_HMS_MASK           GENMASK(1, 0)
> @@ -481,6 +482,7 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>  			    int odr, int uodr)
>  {
>  	int i, ret, val;
> +	int odr_mode;
>  	bool active_mode;
>  	static const int odr_num = ARRAY_SIZE(fxos8700_odr);
>  
> @@ -508,10 +510,10 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>  	if (i >= odr_num)
>  		return -EINVAL;
>  
> -	return regmap_update_bits(data->regmap,
> -				  FXOS8700_CTRL_REG1,
> -				  FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
> -				  fxos8700_odr[i].bits << 3 | active_mode);
> +	odr_mode &= ~FXOS8700_CTRL_ODR_MSK;

This doesn't look right.   You are masking bits out of an uninitialized variable.
I'm guessing intent was to use the value read from the register which is in
val.


> +	odr_mode |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits);

> +	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> +			   odr_mode | active_mode);
>  }
>  
>  static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
> @@ -524,7 +526,7 @@ static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>  	if (ret)
>  		return ret;
>  
> -	val &= FXOS8700_CTRL_ODR_MSK;
> +	val = FIELD_GET(FXOS8700_CTRL_ODR_MSK, val);
>  
>  	for (i = 0; i < odr_num; i++)
>  		if (val == fxos8700_odr[i].bits)
> @@ -612,6 +614,7 @@ static const struct iio_info fxos8700_info = {
>  static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  {
>  	int ret;
> +	int odr_mode;
>  	unsigned int val;
>  	struct device *dev = regmap_get_device(data->regmap);
>  
> @@ -664,8 +667,10 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  		return ret;
>  
>  	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> +	odr_mode &= ~FXOS8700_CTRL_ODR_MSK;
odr_mode not set to anything so you shouldn't be masking bits out of it.

> +	odr_mode |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX);
>  	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +			   odr_mode | FXOS8700_ACTIVE);
>  }
>  
>  static void fxos8700_chip_uninit(void *data)

[PATCH v2 7/7] iio: imu: fxos8700: fix MAGN sensor scale and unit

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

+/-1200uT is a MAGN sensor full measurement range. Magnetometer scale
is the magnetic sensitivity parameter. It is referenced as 0.1uT
according to datasheet and magnetometer channel unit is Gauss in
sysfs-bus-iio documentation. Gauss and uTesla unit conversion
relationship as follows: 0.1uT = 0.001g.

Set magnetometer scale and available magnetometer scale as fixed 0.001g.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V2:
- Modify the magnetometer sensitivity unit to be consistent with the
  documentation as 0.001g
- Rework commit log

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index b4baef82f6d5..1780ed99d2f9 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -352,7 +352,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.001g\n");
 		return -EINVAL;
 	}
 
@@ -397,7 +397,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 = 1000; /* Magnetometer is locked at 0.001g */
 		return 0;
 	}
 
@@ -591,7 +591,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.001000");
 
 static struct attribute *fxos8700_attrs[] = {
 	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,
-- 
2.34.1

Re: [PATCH v2 7/7] iio: imu: fxos8700: fix MAGN sensor scale and unit

[Jonathan Cameron]

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

> From: Carlos Song <carlos.song@nxp.com>
> 
> +/-1200uT is a MAGN sensor full measurement range. Magnetometer scale
> is the magnetic sensitivity parameter. It is referenced as 0.1uT
> according to datasheet and magnetometer channel unit is Gauss in
> sysfs-bus-iio documentation. Gauss and uTesla unit conversion
> relationship as follows: 0.1uT = 0.001g.
> 
> Set magnetometer scale and available magnetometer scale as fixed 0.001g.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
> Changes for V2:
> - Modify the magnetometer sensitivity unit to be consistent with the
>   documentation as 0.001g
> - Rework commit log
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index b4baef82f6d5..1780ed99d2f9 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -352,7 +352,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.001g\n");

Gauss unit (according to wikipedia) is G or Gs

>  		return -EINVAL;
>  	}
>  
> @@ -397,7 +397,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 = 1000; /* Magnetometer is locked at 0.001g */
>  		return 0;
>  	}
>  
> @@ -591,7 +591,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.001000");

Check that against what you get reading the value.  If it is 0.001Gs then
this is right, but the uscale above isn't. (that is 0.0001Gs)

>  
>  static struct attribute *fxos8700_attrs[] = {
>  	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,