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@xxxxxxxxxx>
> Sent: Saturday, December 31, 2022 10:51 PM
> To: Carlos Song <carlos.song@xxxxxxx>
> Cc: lars@xxxxxxxxxx; rjones@xxxxxxxxxxxxx;
> Jonathan.Cameron@xxxxxxxxxx; Bough Chen <haibo.chen@xxxxxxx>;
> dl-linux-imx <linux-imx@xxxxxxx>; linux-iio@xxxxxxxxxxxxxxx
> 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@xxxxxxx wrote:
>
> > From: Carlos Song <carlos.song@xxxxxxx>
> >
> > 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@xxxxxxx>
> 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)
