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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Jan 2023 07:44:23 +0000
Carlos Song <carlos.song@nxp.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, December 31, 2022 10:55 PM
> > To: Carlos Song <carlos.song@nxp.com>
> > Cc: lars@metafoo.de; rjones@gateworks.com;
> > Jonathan.Cameron@huawei.com; Bough Chen <haibo.chen@nxp.com>;
> > dl-linux-imx <linux-imx@nxp.com>; linux-iio@vger.kernel.org
> > 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@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)  
> 
> 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, &reg);
> +       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?

If code is simpler / more readable with regmap_update_bits() then that is the better
option.  If there are other places in the driver where it is appropriate to change
to this function then it would be great to make that improvement as well (I haven't
looked!)

Jonathan