Re: [PATCH 3/4] iio: imu: fxos8700: fix ODR offset error

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  2 Dec 2022 18:35:37 +0800
Carlos Song <carlos.song@nxp.com> wrote:

> Correct offset of ODR configure is needed when configure the register
> and read ODR data from the register.
> 
> Give the correct offset to value when configuring ODR bit and
> reading ODR data from CTRL_REG1 register.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Fixes: 058f2a09e645 ("iio: imu: fxos8700: fix CTRL_REG1 register configuration error")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Minor suggestion inline.

Btw, I'm not particularly keen on internal review tags.
I know the policy varies by company, but it it isn't too much of a problem, I'd
prefer to have seen Haibo Chen's review on list as that then gives me some
way to build trust in Haibo's reviews over the long run!

If not possible, then that's fine.

Jonathan

> ---
>  drivers/iio/imu/fxos8700_core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index 60c08519d8af..27e3bd61d054 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -147,6 +147,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_OFFSET    3
>  
>  /* Bit definitions for FXOS8700_M_CTRL_REG1 */
>  #define FXOS8700_HMS_MASK           GENMASK(1, 0)
> @@ -498,8 +499,7 @@ static int fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>  	if (ret)
>  		return ret;
>  
> -	val &= FXOS8700_CTRL_ODR_MSK;
> -
> +	val = (val & FXOS8700_CTRL_ODR_MSK) >> FXOS8700_CTRL_ODR_OFFSET;

FIELD_GET() / FIELD_PREP() preferred as it avoids need to separately define
an offset.

>  	for (i = 0; i < odr_num; i++)
>  		if (val == fxos8700_odr[i].bits)
>  			break;
> @@ -636,7 +636,8 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  
>  	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
>  	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +			   FXOS8700_CTRL_ODR_MAX << FXOS8700_CTRL_ODR_OFFSET |
> +			   FXOS8700_ACTIVE);
>  }
>  
>  static void fxos8700_chip_uninit(void *data)