Re: [PATCH v3 2/5] iio: imu: fxos8700: improve readability by field mask and regmap_write

[Jonathan Cameron <jic23@kernel.org>]

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)