| Message ID | 20221214031503.3104251-3-carlos.song@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: imu: fxos8700: fix bugs about ODR and changes for a good readability | expand |
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)
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)