[PATCH v4 0/4] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability

[PATCH v4 0/4] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability

[carlos.song]

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

Thanks, Jonathan. I have to admit that this has bothered me about how to
modify it reasonably but at the same time make it have the ideal format.
In patch V4, I use ODR_MSK in the first place that I merged the first two
patches in V3 into the first patch in V4. There is no change on other
patches. And sorry about forgetting to add the dividing line above the
changes in V3, I have added it for every patch this time.

Carlos Song (4):
  iio: imu: fxos8700: fix incorrect ODR mode readback
  iio: imu: fxos8700: fix failed initialization ODR mode assignment
  iio: imu: fxos8700: remove definition FXOS8700_CTRL_ODR_MIN
  iio: imu: fxos8700: fix MAGN sensor scale and unit

 drivers/iio/imu/fxos8700_core.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v4 1/4] iio: imu: fxos8700: fix incorrect ODR mode readback

[carlos.song]

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

The absence of a correct offset leads an incorrect ODR mode
readback after use a hexadecimal number to mark the value from
FXOS8700_CTRL_REG1.

Get ODR mode by field mask and FIELD_GET clearly and conveniently.
And attach other additional fix for keeping the original code logic
and a good readability.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V4:
- Use ODR_MSK in the first place that merged the first two patches
  in V3 into this patch.
- Rework commit log
Changes for V3:
- Remove FXOS8700_CTRL_ODR_GENMSK and set FXOS8700_CTRL_ODR_MSK a
  field mask
- Legal use of filed mask and FIELD_PREP() to select ODR mode
- Rework commit log
---
 drivers/iio/imu/fxos8700_core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 773f62203bf0..a1af5d0fde5d 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)
@@ -508,10 +509,8 @@ 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);
+	val = val | FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) | active_mode;
+	return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
 }
 
 static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
@@ -524,7 +523,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)
-- 
2.34.1

[PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization ODR mode assignment

[carlos.song]

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

The absence of correct offset leads a failed initialization ODR mode
assignment.

Select MAX ODR mode as the initialization ODR mode by field mask and
FIELD_PREP.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V4:
- None
Changes for V3:
- Legal use of FIELD_PREP() and field mask to select initialization
  ODR mode
- Rework commit log
---
 drivers/iio/imu/fxos8700_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index a1af5d0fde5d..de4ced979226 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -611,6 +611,7 @@ static const struct iio_info fxos8700_info = {
 static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
 {
 	int ret;
+	int reg;
 	unsigned int val;
 	struct device *dev = regmap_get_device(data->regmap);
 
@@ -663,8 +664,11 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
 		return ret;
 
 	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
-	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
-			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
+	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &reg);
+	if (ret)
+		return ret;
+	reg = reg | FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;
+	return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
 }
 
 static void fxos8700_chip_uninit(void *data)
-- 
2.34.1

[PATCH v4 3/4] iio: imu: fxos8700: remove definition FXOS8700_CTRL_ODR_MIN

[carlos.song]

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

FXOS8700_CTRL_ODR_MIN is not used but value is probably wrong.

Remove it for a good readability.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V4:
- None
Changes for V3:
- Proposed a separate clean fix
---
 drivers/iio/imu/fxos8700_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index de4ced979226..7b370bd643a1 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -146,7 +146,6 @@
 
 /* Bit definitions for FXOS8700_CTRL_REG1 */
 #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 */
-- 
2.34.1

[PATCH v4 4/4] 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.001Gs.

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

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes for V4:
- None
Changes for V3:
- Modify the magnetometer sensitivity unit "g" to standard unit "Gs"
- Check and confirm uscale value is correct. The readback of
  MAGN scale is 0.001 Gs
- Rework commit log
Changes for V2:
- Modify the magnetometer sensitivity unit to be consistent with the
  documentation as 0.001g
- Rework commit log
---
 drivers/iio/imu/fxos8700_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 7b370bd643a1..8320a3b6f942 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -351,7 +351,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.001Gs\n");
 		return -EINVAL;
 	}
 
@@ -396,7 +396,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.001Gs */
 		return 0;
 	}
 
@@ -587,7 +587,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 v4 1/4] iio: imu: fxos8700: fix incorrect ODR mode readback

[Jonathan Cameron]

On Wed, 28 Dec 2022 17:39:38 +0800
carlos.song@nxp.com wrote:

> From: Carlos Song <carlos.song@nxp.com>
> 
> The absence of a correct offset leads an incorrect ODR mode
> readback after use a hexadecimal number to mark the value from
> FXOS8700_CTRL_REG1.
> 
> Get ODR mode by field mask and FIELD_GET clearly and conveniently.
> And attach other additional fix for keeping the original code logic
> and a good readability.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
Hi Carlos,

I'm fairly sure the new code doesn't quite work correctly. See inline.

Jonathan

> ---
> Changes for V4:
> - Use ODR_MSK in the first place that merged the first two patches
>   in V3 into this patch.
> - Rework commit log
> Changes for V3:
> - Remove FXOS8700_CTRL_ODR_GENMSK and set FXOS8700_CTRL_ODR_MSK a
>   field mask
> - Legal use of filed mask and FIELD_PREP() to select ODR mode
> - Rework commit log
> ---
>  drivers/iio/imu/fxos8700_core.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 773f62203bf0..a1af5d0fde5d 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)
> @@ -508,10 +509,8 @@ 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);
> +	val = val | FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) | active_mode;

val |= would be neater.

Also, if I read the existing code correctly, val hasn't been masked, so if active_mode was
set in val, it still will be, hence no need to or it in again.
You also haven't masked out _CTRL_ODR_MSK so as a result of this call you will get the
bitwise or of whatever ODR value you are trying to set and whatever it was set to before.


> +	return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
>  }
>  
>  static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
> @@ -524,7 +523,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)

Re: [PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization ODR mode assignment

[Jonathan Cameron]

On Wed, 28 Dec 2022 17:39:39 +0800
carlos.song@nxp.com wrote:

> From: Carlos Song <carlos.song@nxp.com>
> 
> The absence of correct offset leads a failed initialization ODR mode
> assignment.
> 
> Select MAX ODR mode as the initialization ODR mode by field mask and
> FIELD_PREP.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
> Changes for V4:
> - None
> Changes for V3:
> - Legal use of FIELD_PREP() and field mask to select initialization
>   ODR mode
> - Rework commit log
> ---
>  drivers/iio/imu/fxos8700_core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index a1af5d0fde5d..de4ced979226 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -611,6 +611,7 @@ static const struct iio_info fxos8700_info = {
>  static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  {
>  	int ret;
> +	int reg;
>  	unsigned int val;
>  	struct device *dev = regmap_get_device(data->regmap);
>  
> @@ -663,8 +664,11 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  		return ret;
>  
>  	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> -	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &reg);
> +	if (ret)
> +		return ret;
> +	reg = reg | FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;
reg |= will work here. However, like in previous patch I'd expect to see the
_CTRL_ODR_MSK used in
	reg &= ~FXOS8700_CTRL_ODR_MASK;
	reg |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;

This is a good place to use regmap_update_bits() as there is no need to see what the previous
values were (unlike in previous patch).

> +	return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
>  }
>  
>  static void fxos8700_chip_uninit(void *data)

RE: [EXT] Re: [PATCH v4 1/4] iio: imu: fxos8700: fix incorrect ODR mode readback

[Carlos Song]

Hi, Jonathan. I have some doubts about how to use regmap_write() and regmap_updata_bits() appropriately
and faced difficult decisions. I propose different modifications as follows and I would like to get some suggestions
from you. Thanks!

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, December 31, 2022 10:51 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: lars@metafoo.de; rjones@gateworks.com;
> Jonathan.Cameron@huawei.com; Bough Chen <haibo.chen@nxp.com>;
> dl-linux-imx <linux-imx@nxp.com>; linux-iio@vger.kernel.org
> Subject: [EXT] Re: [PATCH v4 1/4] iio: imu: fxos8700: fix incorrect ODR mode
> readback
> 
> Caution: EXT Email
> 
> On Wed, 28 Dec 2022 17:39:38 +0800
> carlos.song@nxp.com wrote:
> 
> > From: Carlos Song <carlos.song@nxp.com>
> >
> > The absence of a correct offset leads an incorrect ODR mode readback
> > after use a hexadecimal number to mark the value from
> > FXOS8700_CTRL_REG1.
> >
> > Get ODR mode by field mask and FIELD_GET clearly and conveniently.
> > And attach other additional fix for keeping the original code logic
> > and a good readability.
> >
> > Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Hi Carlos,
> 
> I'm fairly sure the new code doesn't quite work correctly. See inline.
> 
> Jonathan
> 
> > ---
> > Changes for V4:
> > - Use ODR_MSK in the first place that merged the first two patches
> >   in V3 into this patch.
> > - Rework commit log
> > Changes for V3:
> > - Remove FXOS8700_CTRL_ODR_GENMSK and set
> FXOS8700_CTRL_ODR_MSK a
> >   field mask
> > - Legal use of filed mask and FIELD_PREP() to select ODR mode
> > - Rework commit log
> > ---
> >  drivers/iio/imu/fxos8700_core.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/imu/fxos8700_core.c
> > b/drivers/iio/imu/fxos8700_core.c index 773f62203bf0..a1af5d0fde5d
> > 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)
> > @@ -508,10 +509,8 @@ 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);
> > +     val = val | FIELD_PREP(FXOS8700_CTRL_ODR_MSK,
> > + fxos8700_odr[i].bits) | active_mode;
> 
> val |= would be neater.
> 
> Also, if I read the existing code correctly, val hasn't been masked, so if
> active_mode was set in val, it still will be, hence no need to or it in again.
> You also haven't masked out _CTRL_ODR_MSK so as a result of this call you will
> get the bitwise or of whatever ODR value you are trying to set and whatever it
> was set to before.
> 
> 
> > +     return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
> >  }
> >

I am so sorry that I don't use the FIELD_PREP correctly due to my rustiness.
Firstly I fix the issue I haven't masked out _CTRL_ODR_MSK. But activating the device
is required after that so I or FXOS8700_ACTIVE instead or active_mode. Then I want to
discuss about the appropriate usage scenarios about regmap_write and regmap_update_bits.

In source code, regmap_write use _regmap_write only while regmap_update_bits encapsulates 
_regmap_read, modify mask bits and _regmap write. So when need to see what the previous values
or the value has been already got before and is used at other place, it is better to use regmap_write.
We just renew the value and use regmap_write to write it to the register. If we just need modify
the register bits but there is no need to see what the previous values were, it is better to use
regmap_update_bits. It is a simple and direct means and can avoid using regmap_read to get a value
and perform bit operations.
To sum up, if the value of the register has been read by regmap_read or other methods, then use
regmap_write correspondingly to renew the value. If no value has been obtained from the register,
modifying the register using regmap_update_bits is the preferred method. I'm not sure if that's the
right understanding.

So based on it, there are two reasons that I choose regmap_write to replace regmap_update_bits:
1. There is a val which has been get by regmap_read and is used, so just use regmap_write and FIELD_PREP
to renew the val. 
2. The code block used regmap_read and regmap_write to renew the value, uniform use of regmap_write
can have a good readability.

So I think the using regmap_write than regmap_update_bits is more reasonable.
@@ -508,10 +509,9 @@ 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);
+       val &= ~FXOS8700_CTRL_ODR_MSK;
+       val |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) | FXOS8700_ACTIVE;
+       return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
 }

However there is a minimal fix, the patch looks more graceful:
@@ -511,7 +512,8 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
        return regmap_update_bits(data->regmap,
                                  FXOS8700_CTRL_REG1,
                                  FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
-                                 fxos8700_odr[i].bits << 3 | active_mode);
+                                 FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) |
+                                 FXOS8700_ACTIVE);
 }

Which is better? In next patch I also faced a difficult decision about it.
> >  static int fxos8700_get_odr(struct fxos8700_data *data, enum
> > fxos8700_sensor t, @@ -524,7 +523,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)

RE: [EXT] Re: [PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization ODR mode assignment

[Carlos Song]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, December 31, 2022 10:55 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: lars@metafoo.de; rjones@gateworks.com;
> Jonathan.Cameron@huawei.com; Bough Chen <haibo.chen@nxp.com>;
> dl-linux-imx <linux-imx@nxp.com>; linux-iio@vger.kernel.org
> Subject: [EXT] Re: [PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization
> ODR mode assignment
> 
> Caution: EXT Email
> 
> On Wed, 28 Dec 2022 17:39:39 +0800
> carlos.song@nxp.com wrote:
> 
> > From: Carlos Song <carlos.song@nxp.com>
> >
> > The absence of correct offset leads a failed initialization ODR mode
> > assignment.
> >
> > Select MAX ODR mode as the initialization ODR mode by field mask and
> > FIELD_PREP.
> >
> > Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > ---
> > Changes for V4:
> > - None
> > Changes for V3:
> > - Legal use of FIELD_PREP() and field mask to select initialization
> >   ODR mode
> > - Rework commit log
> > ---
> >  drivers/iio/imu/fxos8700_core.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/imu/fxos8700_core.c
> > b/drivers/iio/imu/fxos8700_core.c index a1af5d0fde5d..de4ced979226
> > 100644
> > --- a/drivers/iio/imu/fxos8700_core.c
> > +++ b/drivers/iio/imu/fxos8700_core.c
> > @@ -611,6 +611,7 @@ static const struct iio_info fxos8700_info = {
> > static int fxos8700_chip_init(struct fxos8700_data *data, bool
> > use_spi)  {
> >       int ret;
> > +     int reg;
> >       unsigned int val;
> >       struct device *dev = regmap_get_device(data->regmap);
> >
> > @@ -663,8 +664,11 @@ static int fxos8700_chip_init(struct fxos8700_data
> *data, bool use_spi)
> >               return ret;
> >
> >       /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> > -     return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> > -                        FXOS8700_CTRL_ODR_MAX |
> FXOS8700_ACTIVE);
> > +     ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &reg);
> > +     if (ret)
> > +             return ret;
> > +     reg = reg | FIELD_PREP(FXOS8700_CTRL_ODR_MSK,
> > + FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;
> reg |= will work here. However, like in previous patch I'd expect to see the
> _CTRL_ODR_MSK used in
>         reg &= ~FXOS8700_CTRL_ODR_MASK;
>         reg |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK,
> FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;
> 
> This is a good place to use regmap_update_bits() as there is no need to see
> what the previous values were (unlike in previous patch).
> 
> > +     return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
> >  }
> >
> >  static void fxos8700_chip_uninit(void *data)

This is a good place to use regmap_update_bits(), because I don't need using the regmap_read to
get the value and perform bit operations:
@@ -666,8 +666,10 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
                return ret;

        /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
-       return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
-                          FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
+       return regmap_update_bits(data->regmap, FXOS8700_CTRL_REG1,
+                               FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
+                               FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) |
+                               FXOS8700_ACTIVE);
 }


 static void fxos8700_chip_uninit(void *data)

Here I also faced a difficult decision:
most code block of the entire driver code uses regmap_read and regmap_write to modify registers,
only my two patches use regmap_update_bits. I admit that this is indeed a good place to
use regmap_update_bits, but do I need to consider the uniformity of the entire driver code
style when proposing a patch? When using regmap_read and regmap_write, although the
patch is a bit lengthy and jumbled, it is very uniform in terms of the overall code style.
Like this:

@@ -663,8 +664,11 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
                return ret;

        /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
-       return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
-                          FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
+       ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &reg);
+       if (ret)
+               return ret;
+       reg &= ~FXOS8700_CTRL_ODR_MASK;
+       reg |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) |
+ FXOS8700_ACTIVE;
+       return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
 }

 static void fxos8700_chip_uninit(void *data)

How should I weigh them?

Re: [EXT] Re: [PATCH v4 1/4] iio: imu: fxos8700: fix incorrect ODR mode readback

[Jonathan Cameron]

On Tue, 10 Jan 2023 07:44:20 +0000
Carlos Song <carlos.song@nxp.com> wrote:

> Hi, Jonathan. I have some doubts about how to use regmap_write() and regmap_updata_bits() appropriately
> and faced difficult decisions. I propose different modifications as follows and I would like to get some suggestions
> from you. Thanks!
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, December 31, 2022 10:51 PM
> > To: Carlos Song <carlos.song@nxp.com>
> > Cc: lars@metafoo.de; rjones@gateworks.com;
> > Jonathan.Cameron@huawei.com; Bough Chen <haibo.chen@nxp.com>;
> > dl-linux-imx <linux-imx@nxp.com>; linux-iio@vger.kernel.org
> > Subject: [EXT] Re: [PATCH v4 1/4] iio: imu: fxos8700: fix incorrect ODR mode
> > readback
> > 
> > Caution: EXT Email
> > 
> > On Wed, 28 Dec 2022 17:39:38 +0800
> > carlos.song@nxp.com wrote:
> >   
> > > From: Carlos Song <carlos.song@nxp.com>
> > >
> > > The absence of a correct offset leads an incorrect ODR mode readback
> > > after use a hexadecimal number to mark the value from
> > > FXOS8700_CTRL_REG1.
> > >
> > > Get ODR mode by field mask and FIELD_GET clearly and conveniently.
> > > And attach other additional fix for keeping the original code logic
> > > and a good readability.
> > >
> > > Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> > > Signed-off-by: Carlos Song <carlos.song@nxp.com>  
> > Hi Carlos,
> > 
> > I'm fairly sure the new code doesn't quite work correctly. See inline.
> > 
> > Jonathan
> >   
> > > ---
> > > Changes for V4:
> > > - Use ODR_MSK in the first place that merged the first two patches
> > >   in V3 into this patch.
> > > - Rework commit log
> > > Changes for V3:
> > > - Remove FXOS8700_CTRL_ODR_GENMSK and set  
> > FXOS8700_CTRL_ODR_MSK a  
> > >   field mask
> > > - Legal use of filed mask and FIELD_PREP() to select ODR mode
> > > - Rework commit log
> > > ---
> > >  drivers/iio/imu/fxos8700_core.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/fxos8700_core.c
> > > b/drivers/iio/imu/fxos8700_core.c index 773f62203bf0..a1af5d0fde5d
> > > 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)
> > > @@ -508,10 +509,8 @@ 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);  
> > > +     val = val | FIELD_PREP(FXOS8700_CTRL_ODR_MSK,
> > > + fxos8700_odr[i].bits) | active_mode;  
> > 
> > val |= would be neater.
> > 
> > Also, if I read the existing code correctly, val hasn't been masked, so if
> > active_mode was set in val, it still will be, hence no need to or it in again.
> > You also haven't masked out _CTRL_ODR_MSK so as a result of this call you will
> > get the bitwise or of whatever ODR value you are trying to set and whatever it
> > was set to before.
> > 
> >   
> > > +     return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
> > >  }
> > >  
> 
> I am so sorry that I don't use the FIELD_PREP correctly due to my rustiness.
> Firstly I fix the issue I haven't masked out _CTRL_ODR_MSK. But activating the device
> is required after that so I or FXOS8700_ACTIVE instead or active_mode. Then I want to
> discuss about the appropriate usage scenarios about regmap_write and regmap_update_bits.
> 
> In source code, regmap_write use _regmap_write only while regmap_update_bits encapsulates 
> _regmap_read, modify mask bits and _regmap write. So when need to see what the previous values
> or the value has been already got before and is used at other place, it is better to use regmap_write.
> We just renew the value and use regmap_write to write it to the register. If we just need modify
> the register bits but there is no need to see what the previous values were, it is better to use
> regmap_update_bits. It is a simple and direct means and can avoid using regmap_read to get a value
> and perform bit operations.
> To sum up, if the value of the register has been read by regmap_read or other methods, then use
> regmap_write correspondingly to renew the value. If no value has been obtained from the register,
> modifying the register using regmap_update_bits is the preferred method. I'm not sure if that's the
> right understanding.
> 
> So based on it, there are two reasons that I choose regmap_write to replace regmap_update_bits:
> 1. There is a val which has been get by regmap_read and is used, so just use regmap_write and FIELD_PREP
> to renew the val. 
> 2. The code block used regmap_read and regmap_write to renew the value, uniform use of regmap_write
> can have a good readability.
> 
> So I think the using regmap_write than regmap_update_bits is more reasonable.
> @@ -508,10 +509,9 @@ 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);
> +       val &= ~FXOS8700_CTRL_ODR_MSK;
> +       val |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) | FXOS8700_ACTIVE;
> +       return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
>  }
> 
> However there is a minimal fix, the patch looks more graceful:
> @@ -511,7 +512,8 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>         return regmap_update_bits(data->regmap,
>                                   FXOS8700_CTRL_REG1,
>                                   FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
| not + for combining masks. 
> -                                 fxos8700_odr[i].bits << 3 | active_mode);
> +                                 FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) |
> +                                 FXOS8700_ACTIVE);
>  }
> 
> Which is better? In next patch I also faced a difficult decision about it.

I would go with the regmap_write() choice - though in cases like this I think
most important concern is readability.  Sometimes that means regmap_update_bits()
is a better choice even if we already have the read value available.
I think that's not true here so regmap_write() is better option.

> > >  static int fxos8700_get_odr(struct fxos8700_data *data, enum
> > > fxos8700_sensor t, @@ -524,7 +523,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)  
> 

Re: [EXT] Re: [PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization ODR mode assignment

[Jonathan Cameron]

On Tue, 10 Jan 2023 07:44:23 +0000
Carlos Song <carlos.song@nxp.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, December 31, 2022 10:55 PM
> > To: Carlos Song <carlos.song@nxp.com>
> > Cc: lars@metafoo.de; rjones@gateworks.com;
> > Jonathan.Cameron@huawei.com; Bough Chen <haibo.chen@nxp.com>;
> > dl-linux-imx <linux-imx@nxp.com>; linux-iio@vger.kernel.org
> > Subject: [EXT] Re: [PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization
> > ODR mode assignment
> > 
> > Caution: EXT Email
> > 
> > On Wed, 28 Dec 2022 17:39:39 +0800
> > carlos.song@nxp.com wrote:
> >   
> > > From: Carlos Song <carlos.song@nxp.com>
> > >
> > > The absence of correct offset leads a failed initialization ODR mode
> > > assignment.
> > >
> > > Select MAX ODR mode as the initialization ODR mode by field mask and
> > > FIELD_PREP.
> > >
> > > Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> > > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > > ---
> > > Changes for V4:
> > > - None
> > > Changes for V3:
> > > - Legal use of FIELD_PREP() and field mask to select initialization
> > >   ODR mode
> > > - Rework commit log
> > > ---
> > >  drivers/iio/imu/fxos8700_core.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/fxos8700_core.c
> > > b/drivers/iio/imu/fxos8700_core.c index a1af5d0fde5d..de4ced979226
> > > 100644
> > > --- a/drivers/iio/imu/fxos8700_core.c
> > > +++ b/drivers/iio/imu/fxos8700_core.c
> > > @@ -611,6 +611,7 @@ static const struct iio_info fxos8700_info = {
> > > static int fxos8700_chip_init(struct fxos8700_data *data, bool
> > > use_spi)  {
> > >       int ret;
> > > +     int reg;
> > >       unsigned int val;
> > >       struct device *dev = regmap_get_device(data->regmap);
> > >
> > > @@ -663,8 +664,11 @@ static int fxos8700_chip_init(struct fxos8700_data  
> > *data, bool use_spi)  
> > >               return ret;
> > >
> > >       /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> > > -     return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> > > -                        FXOS8700_CTRL_ODR_MAX |  
> > FXOS8700_ACTIVE);  
> > > +     ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &reg);
> > > +     if (ret)
> > > +             return ret;
> > > +     reg = reg | FIELD_PREP(FXOS8700_CTRL_ODR_MSK,
> > > + FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;  
> > reg |= will work here. However, like in previous patch I'd expect to see the
> > _CTRL_ODR_MSK used in
> >         reg &= ~FXOS8700_CTRL_ODR_MASK;
> >         reg |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK,
> > FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;
> > 
> > This is a good place to use regmap_update_bits() as there is no need to see
> > what the previous values were (unlike in previous patch).
> >   
> > > +     return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
> > >  }
> > >
> > >  static void fxos8700_chip_uninit(void *data)  
> 
> This is a good place to use regmap_update_bits(), because I don't need using the regmap_read to
> get the value and perform bit operations:
> @@ -666,8 +666,10 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>                 return ret;
> 
>         /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> -       return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -                          FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +       return regmap_update_bits(data->regmap, FXOS8700_CTRL_REG1,
> +                               FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
> +                               FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) |
> +                               FXOS8700_ACTIVE);
>  }
> 
> 
>  static void fxos8700_chip_uninit(void *data)
> 
> Here I also faced a difficult decision:
> most code block of the entire driver code uses regmap_read and regmap_write to modify registers,
> only my two patches use regmap_update_bits. I admit that this is indeed a good place to
> use regmap_update_bits, but do I need to consider the uniformity of the entire driver code
> style when proposing a patch? When using regmap_read and regmap_write, although the
> patch is a bit lengthy and jumbled, it is very uniform in terms of the overall code style.
> Like this:
> 
> @@ -663,8 +664,11 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>                 return ret;
> 
>         /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> -       return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -                          FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +       ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &reg);
> +       if (ret)
> +               return ret;
> +       reg &= ~FXOS8700_CTRL_ODR_MASK;
> +       reg |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) |
> + FXOS8700_ACTIVE;
> +       return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
>  }
> 
>  static void fxos8700_chip_uninit(void *data)
> 
> How should I weigh them?

If code is simpler / more readable with regmap_update_bits() then that is the better
option.  If there are other places in the driver where it is appropriate to change
to this function then it would be great to make that improvement as well (I haven't
looked!)

Jonathan