Re: [PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization ODR mode assignment

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 28 Dec 2022 17:39:39 +0800
carlos.song@nxp.com wrote:

> From: Carlos Song <carlos.song@nxp.com>
> 
> The absence of correct offset leads a failed initialization ODR mode
> assignment.
> 
> Select MAX ODR mode as the initialization ODR mode by field mask and
> FIELD_PREP.
> 
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
> Changes for V4:
> - None
> Changes for V3:
> - Legal use of FIELD_PREP() and field mask to select initialization
>   ODR mode
> - Rework commit log
> ---
>  drivers/iio/imu/fxos8700_core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index a1af5d0fde5d..de4ced979226 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -611,6 +611,7 @@ static const struct iio_info fxos8700_info = {
>  static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  {
>  	int ret;
> +	int reg;
>  	unsigned int val;
>  	struct device *dev = regmap_get_device(data->regmap);
>  
> @@ -663,8 +664,11 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
>  		return ret;
>  
>  	/* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> -	return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> -			   FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> +	ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &reg);
> +	if (ret)
> +		return ret;
> +	reg = reg | FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;
reg |= will work here. However, like in previous patch I'd expect to see the
_CTRL_ODR_MSK used in
	reg &= ~FXOS8700_CTRL_ODR_MASK;
	reg |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) | FXOS8700_ACTIVE;

This is a good place to use regmap_update_bits() as there is no need to see what the previous
values were (unlike in previous patch).

> +	return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
>  }
>  
>  static void fxos8700_chip_uninit(void *data)