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