> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Saturday, December 31, 2022 10:55 PM
> To: Carlos Song <carlos.song@xxxxxxx>
> Cc: lars@xxxxxxxxxx; rjones@xxxxxxxxxxxxx;
> Jonathan.Cameron@xxxxxxxxxx; Bough Chen <haibo.chen@xxxxxxx>;
> dl-linux-imx <linux-imx@xxxxxxx>; linux-iio@xxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH v4 2/4] iio: imu: fxos8700: fix failed initialization
> ODR mode assignment
>
> Caution: EXT Email
>
> On Wed, 28 Dec 2022 17:39:39 +0800
> carlos.song@xxxxxxx wrote:
>
> > From: Carlos Song <carlos.song@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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, ®);
> > + 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)
This is a good place to use regmap_update_bits(), because I don't need using the regmap_read to
get the value and perform bit operations:
@@ -666,8 +666,10 @@ 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);
+ return regmap_update_bits(data->regmap, FXOS8700_CTRL_REG1,
+ FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
+ FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) |
+ FXOS8700_ACTIVE);
}
static void fxos8700_chip_uninit(void *data)
Here I also faced a difficult decision:
most code block of the entire driver code uses regmap_read and regmap_write to modify registers,
only my two patches use regmap_update_bits. I admit that this is indeed a good place to
use regmap_update_bits, but do I need to consider the uniformity of the entire driver code
style when proposing a patch? When using regmap_read and regmap_write, although the
patch is a bit lengthy and jumbled, it is very uniform in terms of the overall code style.
Like this:
@@ -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, ®);
+ if (ret)
+ return ret;
+ reg &= ~FXOS8700_CTRL_ODR_MASK;
+ reg |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, FXOS8700_CTRL_ODR_MAX) |
+ FXOS8700_ACTIVE;
+ return regmap_write(data->regmap, FXOS8700_CTRL_REG1, reg);
}
static void fxos8700_chip_uninit(void *data)
How should I weigh them?
