On Fri, 2 Dec 2022 18:35:36 +0800
Carlos Song <carlos.song@xxxxxxx> wrote:
> When the device is in active mode, any change of the other fields
Other from what? I.e. say "any change of fields other than xxx within..."
> within
> CTRL_REG1 will lead an invalid configuration. This not align with the
> datasheet, but it is a fxos8700 chip behavier.
Spell check your patch descriptions (I forget to do this sometimes as well.)
Given it's an NXP part, any chance of a datasheet errata to fix this?
>
> Set the device in standby mode before configuring CTRL_REG1 register
> in chip initialization phase and setting scale phase.
In the initialization you don't do this, you simply reorder the code so
that other settings are written first. So make sure the patch description
is consistent with the code.
Also the write you've moved isn't in CTRL_REG1 so the above description does
not explain why it is necessary to do this in standby mode.
>
> Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> Signed-off-by: Carlos Song <carlos.song@xxxxxxx>
> Reviewed-by: Haibo Chen <haibo.chen@xxxxxxx>
> ---
> drivers/iio/imu/fxos8700_core.c | 36 ++++++++++++++++++++++++++-------
> 1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/imu/fxos8700_core.c b/drivers/iio/imu/fxos8700_core.c
> index a69122799892..60c08519d8af 100644
> --- a/drivers/iio/imu/fxos8700_core.c
> +++ b/drivers/iio/imu/fxos8700_core.c
> @@ -343,7 +343,8 @@ static int fxos8700_set_active_mode(struct fxos8700_data *data,
> static int fxos8700_set_scale(struct fxos8700_data *data,
> enum fxos8700_sensor t, int uscale)
> {
> - int i;
> + int i, ret, val;
> + bool active_mode;
> static const int scale_num = ARRAY_SIZE(fxos8700_accel_scale);
> struct device *dev = regmap_get_device(data->regmap);
>
> @@ -352,6 +353,23 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
> return -EINVAL;
> }
>
> + ret = regmap_read(data->regmap, FXOS8700_CTRL_REG1, &val);
> + if (ret)
> + return ret;
> +
> + active_mode = val & FXOS8700_ACTIVE;
> + /*
> + * The device must be in standby mode to change any of the
> + * other fields within CTRL_REG1. This not align with the
> + * datasheet, but it is a fxos8700 chip behavier.
> + */
> + if (active_mode) {
> + ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> + val & ~FXOS8700_ACTIVE);
> + if (ret)
> + return ret;
> + }
> +
> for (i = 0; i < scale_num; i++)
> if (fxos8700_accel_scale[i].uscale == uscale)
> break;
> @@ -359,8 +377,12 @@ static int fxos8700_set_scale(struct fxos8700_data *data,
> if (i == scale_num)
> return -EINVAL;
>
> - return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> + ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG,
> fxos8700_accel_scale[i].bits);
As below, this isn't in CTRL_REG1 so not clear why we need to be
in standby mode to set it.
> + if (ret)
> + return ret;
> + return regmap_update_bits(data->regmap, FXOS8700_CTRL_REG1,
> + FXOS8700_ACTIVE, active_mode);
Odd to use update bits here, but not when you clear the bit above.
Given you have the register value read back, either just using regmap_write()
in both cases, or use regmap_update_bits() in both cases would be fine by me.
Mixing the two isn't good for readability.
> }
>
> static int fxos8700_get_scale(struct fxos8700_data *data,
> @@ -607,14 +629,14 @@ static int fxos8700_chip_init(struct fxos8700_data *data, bool use_spi)
> if (ret)
> return ret;
>
> - /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> - ret = regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> - FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> + /* Set for max full-scale range (+/-8G) */
> + ret = regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG, MODE_8G);
Not in CTRL_REG1 so by the patch description, this doesn't need to be
done in standby mode. I'm guessing that description is meant to cover
other registers.
> if (ret)
> return ret;
>
> - /* Set for max full-scale range (+/-8G) */
> - return regmap_write(data->regmap, FXOS8700_XYZ_DATA_CFG, MODE_8G);
> + /* Max ODR (800Hz individual or 400Hz hybrid), active mode */
> + return regmap_write(data->regmap, FXOS8700_CTRL_REG1,
> + FXOS8700_CTRL_ODR_MAX | FXOS8700_ACTIVE);
> }
>
> static void fxos8700_chip_uninit(void *data)
