All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/5] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability
@ 2022-12-14  3:14 carlos.song
  2022-12-14  3:14 ` [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback carlos.song
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: carlos.song @ 2022-12-14  3:14 UTC (permalink / raw)
  To: jic23, lars
  Cc: rjones, Jonathan.Cameron, haibo.chen, carlos.song, linux-imx, linux-iio

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

In patch v3, the main work is fixing the bug that mask bits out of an
uninitialized variable. It caused by improper use of filed mask and
FIELD_PREP()/FIELD_GET(). Meanwhile give a further fix based patch v2
and make patches to optimize codes and clean up useless codes for a
good readability.
- Patch v2 [6/7] is split into three minimal fix patches:
  iio: imu: fxos8700: fix incorrect ODR mode readback
  iio: imu: fxos8700: improve readability by field mask and regmap_write
  iio: imu: fxos8700: fix failed initialization ODR value assignment
- A new clean fix is proposed:
  iio: imu: fxos8700: remove definition FXOS8700_CTRL_ODR_MIN
- A further fix based on PATCH v2 [7/7]:
  iio: imu: fxos8700: fix MAGN sensor scale and unit

Carlos Song (5):
  iio: imu: fxos8700: fix incorrect ODR mode readback
  iio: imu: fxos8700: improve readability by field mask and regmap_write
  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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback
  2022-12-14  3:14 [PATCH v3 0/5] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability carlos.song
@ 2022-12-14  3:14 ` carlos.song
  2022-12-23 16:55   ` Jonathan Cameron
  2022-12-14  3:15 ` [PATCH v3 2/5] iio: imu: fxos8700: improve readability by field mask and regmap_write carlos.song
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: carlos.song @ 2022-12-14  3:14 UTC (permalink / raw)
  To: jic23, lars
  Cc: rjones, Jonathan.Cameron, haibo.chen, carlos.song, linux-imx, linux-iio

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.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>

Changes for V3:
- Rework commit log

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 773f62203bf0..83ab7d0f79b3 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>
@@ -147,6 +148,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_GENMSK    GENMASK(5, 3)
 
 /* Bit definitions for FXOS8700_M_CTRL_REG1 */
 #define FXOS8700_HMS_MASK           GENMASK(1, 0)
@@ -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_GENMSK, val);
 
 	for (i = 0; i < odr_num; i++)
 		if (val == fxos8700_odr[i].bits)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/5] iio: imu: fxos8700: improve readability by field mask and regmap_write
  2022-12-14  3:14 [PATCH v3 0/5] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability carlos.song
  2022-12-14  3:14 ` [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback carlos.song
@ 2022-12-14  3:15 ` carlos.song
  2022-12-23 16:59   ` Jonathan Cameron
  2022-12-14  3:15 ` [PATCH v3 3/5] iio: imu: fxos8700: fix failed initialization ODR mode assignment carlos.song
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: carlos.song @ 2022-12-14  3:15 UTC (permalink / raw)
  To: jic23, lars
  Cc: rjones, Jonathan.Cameron, haibo.chen, carlos.song, linux-imx, linux-iio

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

FXOS8700_CTRL_ODR_MSK is a hex digit mask and FXOS8700_CTRL_ODR_GENMSK
is a field mask. They have a similar function. And mixing regmap_write
and regmap_update_bits isn't good for readability.

Remove FXOS8700_CTRL_ODR_GENMSK and set FXOS8700_CTRL_ODR_MSK a field
mask definition with a synchronous change. Use regmap_write() instead
of regmap_update_bits() to update bits. They are good for readability.

Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Signed-off-by: Carlos Song <carlos.song@nxp.com>

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

diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
index 83ab7d0f79b3..a1af5d0fde5d 100644
--- a/drivers/iio/imu/fxos8700_core.c
+++ b/drivers/iio/imu/fxos8700_core.c
@@ -145,10 +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_GENMSK    GENMASK(5, 3)
+#define FXOS8700_CTRL_ODR_MSK       GENMASK(5, 3)
 
 /* Bit definitions for FXOS8700_M_CTRL_REG1 */
 #define FXOS8700_HMS_MASK           GENMASK(1, 0)
@@ -510,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,
@@ -526,7 +523,7 @@ static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
 	if (ret)
 		return ret;
 
-	val = FIELD_GET(FXOS8700_CTRL_ODR_GENMSK, val);
+	val = FIELD_GET(FXOS8700_CTRL_ODR_MSK, val);
 
 	for (i = 0; i < odr_num; i++)
 		if (val == fxos8700_odr[i].bits)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 3/5] iio: imu: fxos8700: fix failed initialization ODR mode assignment
  2022-12-14  3:14 [PATCH v3 0/5] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability carlos.song
  2022-12-14  3:14 ` [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback carlos.song
  2022-12-14  3:15 ` [PATCH v3 2/5] iio: imu: fxos8700: improve readability by field mask and regmap_write carlos.song
@ 2022-12-14  3:15 ` carlos.song
  2022-12-14  3:15 ` [PATCH v3 4/5] iio: imu: fxos8700: remove definition FXOS8700_CTRL_ODR_MIN carlos.song
  2022-12-14  3:15 ` [PATCH v3 5/5] iio: imu: fxos8700: fix MAGN sensor scale and unit carlos.song
  4 siblings, 0 replies; 10+ messages in thread
From: carlos.song @ 2022-12-14  3:15 UTC (permalink / raw)
  To: jic23, lars
  Cc: rjones, Jonathan.Cameron, haibo.chen, carlos.song, linux-imx, linux-iio

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 V3:
- Legal use of FIELD_PREP() and field mask to select initialization
  ODR mode
- Rework commit log

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 4/5] iio: imu: fxos8700: remove definition FXOS8700_CTRL_ODR_MIN
  2022-12-14  3:14 [PATCH v3 0/5] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability carlos.song
                   ` (2 preceding siblings ...)
  2022-12-14  3:15 ` [PATCH v3 3/5] iio: imu: fxos8700: fix failed initialization ODR mode assignment carlos.song
@ 2022-12-14  3:15 ` carlos.song
  2022-12-14  3:15 ` [PATCH v3 5/5] iio: imu: fxos8700: fix MAGN sensor scale and unit carlos.song
  4 siblings, 0 replies; 10+ messages in thread
From: carlos.song @ 2022-12-14  3:15 UTC (permalink / raw)
  To: jic23, lars
  Cc: rjones, Jonathan.Cameron, haibo.chen, carlos.song, linux-imx, linux-iio

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 V3:
- Proposed a separate clean fix

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 5/5] iio: imu: fxos8700: fix MAGN sensor scale and unit
  2022-12-14  3:14 [PATCH v3 0/5] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability carlos.song
                   ` (3 preceding siblings ...)
  2022-12-14  3:15 ` [PATCH v3 4/5] iio: imu: fxos8700: remove definition FXOS8700_CTRL_ODR_MIN carlos.song
@ 2022-12-14  3:15 ` carlos.song
  2022-12-23 17:01   ` Jonathan Cameron
  4 siblings, 1 reply; 10+ messages in thread
From: carlos.song @ 2022-12-14  3:15 UTC (permalink / raw)
  To: jic23, lars
  Cc: rjones, Jonathan.Cameron, haibo.chen, carlos.song, linux-imx, linux-iio

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 V2:
- Modify the magnetometer sensitivity unit to be consistent with the
  documentation as 0.001g
- Rework commit log
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

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback
  2022-12-14  3:14 ` [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback carlos.song
@ 2022-12-23 16:55   ` Jonathan Cameron
  2022-12-23 16:58     ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2022-12-23 16:55 UTC (permalink / raw)
  To: carlos.song
  Cc: lars, rjones, Jonathan.Cameron, haibo.chen, linux-imx, linux-iio

On Wed, 14 Dec 2022 11:14:59 +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.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> 
You need a 
---

above the change log to avoid git picking it up when I apply the patch.
I've fixed that up whilst applying.

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

> Changes for V3:
> - Rework commit log
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 773f62203bf0..83ab7d0f79b3 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>
> @@ -147,6 +148,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_GENMSK    GENMASK(5, 3)
>  
>  /* Bit definitions for FXOS8700_M_CTRL_REG1 */
>  #define FXOS8700_HMS_MASK           GENMASK(1, 0)
> @@ -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_GENMSK, val);
>  
>  	for (i = 0; i < odr_num; i++)
>  		if (val == fxos8700_odr[i].bits)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback
  2022-12-23 16:55   ` Jonathan Cameron
@ 2022-12-23 16:58     ` Jonathan Cameron
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2022-12-23 16:58 UTC (permalink / raw)
  To: carlos.song
  Cc: lars, rjones, Jonathan.Cameron, haibo.chen, linux-imx, linux-iio

On Fri, 23 Dec 2022 16:55:29 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 14 Dec 2022 11:14:59 +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.
> > 
> > Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> >   
> You need a 
> ---
> 
> above the change log to avoid git picking it up when I apply the patch.
> I've fixed that up whilst applying.
> 
> Applied to the fixes-togreg branch of iio.git and marked for stable.

Dropped again because this makes no sense wrt to split of what is
going on in patch 2.
> 
> > Changes for V3:
> > - Rework commit log
> > 
> > diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> > index 773f62203bf0..83ab7d0f79b3 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>
> > @@ -147,6 +148,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_GENMSK    GENMASK(5, 3)
> >  
> >  /* Bit definitions for FXOS8700_M_CTRL_REG1 */
> >  #define FXOS8700_HMS_MASK           GENMASK(1, 0)
> > @@ -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_GENMSK, val);
> >  
> >  	for (i = 0; i < odr_num; i++)
> >  		if (val == fxos8700_odr[i].bits)  
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/5] iio: imu: fxos8700: improve readability by field mask and regmap_write
  2022-12-14  3:15 ` [PATCH v3 2/5] iio: imu: fxos8700: improve readability by field mask and regmap_write carlos.song
@ 2022-12-23 16:59   ` Jonathan Cameron
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2022-12-23 16:59 UTC (permalink / raw)
  To: carlos.song
  Cc: lars, rjones, Jonathan.Cameron, haibo.chen, linux-imx, linux-iio

On Wed, 14 Dec 2022 11:15:00 +0800
carlos.song@nxp.com wrote:

> From: Carlos Song <carlos.song@nxp.com>
> 
> FXOS8700_CTRL_ODR_MSK is a hex digit mask and FXOS8700_CTRL_ODR_GENMSK
> is a field mask. They have a similar function. And mixing regmap_write
> and regmap_update_bits isn't good for readability.
> 
> Remove FXOS8700_CTRL_ODR_GENMSK and set FXOS8700_CTRL_ODR_MSK a field
> mask definition with a synchronous change. Use regmap_write() instead
> of regmap_update_bits() to update bits. They are good for readability.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
Needs a comment on why this deserves a fixes tag.

> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> 
---

needed here.

> 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
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 83ab7d0f79b3..a1af5d0fde5d 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -145,10 +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_GENMSK    GENMASK(5, 3)
> +#define FXOS8700_CTRL_ODR_MSK       GENMASK(5, 3)

Why jump through this loop in patch 1 and then this?  Just use
ODR_MSK in that fix in the first place. Doesn't matter for
purposes of a fix that it is in a less than ideal form.

>  
>  /* Bit definitions for FXOS8700_M_CTRL_REG1 */
>  #define FXOS8700_HMS_MASK           GENMASK(1, 0)
> @@ -510,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,
> @@ -526,7 +523,7 @@ static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>  	if (ret)
>  		return ret;
>  
> -	val = FIELD_GET(FXOS8700_CTRL_ODR_GENMSK, val);
> +	val = FIELD_GET(FXOS8700_CTRL_ODR_MSK, val);
>  
>  	for (i = 0; i < odr_num; i++)
>  		if (val == fxos8700_odr[i].bits)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 5/5] iio: imu: fxos8700: fix MAGN sensor scale and unit
  2022-12-14  3:15 ` [PATCH v3 5/5] iio: imu: fxos8700: fix MAGN sensor scale and unit carlos.song
@ 2022-12-23 17:01   ` Jonathan Cameron
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2022-12-23 17:01 UTC (permalink / raw)
  To: carlos.song
  Cc: lars, rjones, Jonathan.Cameron, haibo.chen, linux-imx, linux-iio

On Wed, 14 Dec 2022 11:15:03 +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.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>
> 
---

as for all the other patches.

3,4,5 look fine to me other than that.  So I'll pick them up once
the patch 1/2 split is sorted out.

Jonathan


> Changes for V2:
> - Modify the magnetometer sensitivity unit to be consistent with the
>   documentation as 0.001g
> - Rework commit log
> 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
> 
> 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,


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-12-23 16:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-14  3:14 [PATCH v3 0/5] iio: imu: fxos8700: fix bugs about ODR and changes for a good readability carlos.song
2022-12-14  3:14 ` [PATCH v3 1/5] iio: imu: fxos8700: fix incorrect ODR mode readback carlos.song
2022-12-23 16:55   ` Jonathan Cameron
2022-12-23 16:58     ` Jonathan Cameron
2022-12-14  3:15 ` [PATCH v3 2/5] iio: imu: fxos8700: improve readability by field mask and regmap_write carlos.song
2022-12-23 16:59   ` Jonathan Cameron
2022-12-14  3:15 ` [PATCH v3 3/5] iio: imu: fxos8700: fix failed initialization ODR mode assignment carlos.song
2022-12-14  3:15 ` [PATCH v3 4/5] iio: imu: fxos8700: remove definition FXOS8700_CTRL_ODR_MIN carlos.song
2022-12-14  3:15 ` [PATCH v3 5/5] iio: imu: fxos8700: fix MAGN sensor scale and unit carlos.song
2022-12-23 17:01   ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.