Re: [EXT] Re: [PATCH v4 1/4] iio: imu: fxos8700: fix incorrect ODR mode readback

[Jonathan Cameron <jic23@kernel.org>]

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

> Hi, Jonathan. I have some doubts about how to use regmap_write() and regmap_updata_bits() appropriately
> and faced difficult decisions. I propose different modifications as follows and I would like to get some suggestions
> from you. Thanks!
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, December 31, 2022 10:51 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 1/4] iio: imu: fxos8700: fix incorrect ODR mode
> > readback
> > 
> > Caution: EXT Email
> > 
> > On Wed, 28 Dec 2022 17:39:38 +0800
> > carlos.song@nxp.com wrote:
> >   
> > > From: Carlos Song <carlos.song@nxp.com>
> > >
> > > The absence of a correct offset leads an incorrect ODR mode readback
> > > after use a hexadecimal number to mark the value from
> > > FXOS8700_CTRL_REG1.
> > >
> > > Get ODR mode by field mask and FIELD_GET clearly and conveniently.
> > > And attach other additional fix for keeping the original code logic
> > > and a good readability.
> > >
> > > Fixes: 84e5ddd5c46e ("iio: imu: Add support for the FXOS8700 IMU")
> > > Signed-off-by: Carlos Song <carlos.song@nxp.com>  
> > Hi Carlos,
> > 
> > I'm fairly sure the new code doesn't quite work correctly. See inline.
> > 
> > Jonathan
> >   
> > > ---
> > > Changes for V4:
> > > - Use ODR_MSK in the first place that merged the first two patches
> > >   in V3 into this patch.
> > > - Rework commit log
> > > Changes for V3:
> > > - Remove FXOS8700_CTRL_ODR_GENMSK and set  
> > FXOS8700_CTRL_ODR_MSK a  
> > >   field mask
> > > - Legal use of filed mask and FIELD_PREP() to select ODR mode
> > > - Rework commit log
> > > ---
> > >  drivers/iio/imu/fxos8700_core.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/fxos8700_core.c
> > > b/drivers/iio/imu/fxos8700_core.c index 773f62203bf0..a1af5d0fde5d
> > > 100644
> > > --- a/drivers/iio/imu/fxos8700_core.c
> > > +++ b/drivers/iio/imu/fxos8700_core.c
> > > @@ -10,6 +10,7 @@
> > >  #include <linux/regmap.h>
> > >  #include <linux/acpi.h>
> > >  #include <linux/bitops.h>
> > > +#include <linux/bitfield.h>
> > >
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > > @@ -144,9 +145,9 @@
> > >  #define FXOS8700_NVM_DATA_BNK0      0xa7
> > >
> > >  /* Bit definitions for FXOS8700_CTRL_REG1 */
> > > -#define FXOS8700_CTRL_ODR_MSK       0x38
> > >  #define FXOS8700_CTRL_ODR_MAX       0x00
> > >  #define FXOS8700_CTRL_ODR_MIN       GENMASK(4, 3)
> > > +#define FXOS8700_CTRL_ODR_MSK       GENMASK(5, 3)
> > >
> > >  /* Bit definitions for FXOS8700_M_CTRL_REG1 */
> > >  #define FXOS8700_HMS_MASK           GENMASK(1, 0)
> > > @@ -508,10 +509,8 @@ static int fxos8700_set_odr(struct fxos8700_data  
> > *data, enum fxos8700_sensor t,  
> > >       if (i >= odr_num)
> > >               return -EINVAL;
> > >
> > > -     return regmap_update_bits(data->regmap,
> > > -                               FXOS8700_CTRL_REG1,
> > > -                               FXOS8700_CTRL_ODR_MSK +  
> > FXOS8700_ACTIVE,  
> > > -                               fxos8700_odr[i].bits << 3 |  
> > active_mode);  
> > > +     val = val | FIELD_PREP(FXOS8700_CTRL_ODR_MSK,
> > > + fxos8700_odr[i].bits) | active_mode;  
> > 
> > val |= would be neater.
> > 
> > Also, if I read the existing code correctly, val hasn't been masked, so if
> > active_mode was set in val, it still will be, hence no need to or it in again.
> > You also haven't masked out _CTRL_ODR_MSK so as a result of this call you will
> > get the bitwise or of whatever ODR value you are trying to set and whatever it
> > was set to before.
> > 
> >   
> > > +     return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
> > >  }
> > >  
> 
> I am so sorry that I don't use the FIELD_PREP correctly due to my rustiness.
> Firstly I fix the issue I haven't masked out _CTRL_ODR_MSK. But activating the device
> is required after that so I or FXOS8700_ACTIVE instead or active_mode. Then I want to
> discuss about the appropriate usage scenarios about regmap_write and regmap_update_bits.
> 
> In source code, regmap_write use _regmap_write only while regmap_update_bits encapsulates 
> _regmap_read, modify mask bits and _regmap write. So when need to see what the previous values
> or the value has been already got before and is used at other place, it is better to use regmap_write.
> We just renew the value and use regmap_write to write it to the register. If we just need modify
> the register bits but there is no need to see what the previous values were, it is better to use
> regmap_update_bits. It is a simple and direct means and can avoid using regmap_read to get a value
> and perform bit operations.
> To sum up, if the value of the register has been read by regmap_read or other methods, then use
> regmap_write correspondingly to renew the value. If no value has been obtained from the register,
> modifying the register using regmap_update_bits is the preferred method. I'm not sure if that's the
> right understanding.
> 
> So based on it, there are two reasons that I choose regmap_write to replace regmap_update_bits:
> 1. There is a val which has been get by regmap_read and is used, so just use regmap_write and FIELD_PREP
> to renew the val. 
> 2. The code block used regmap_read and regmap_write to renew the value, uniform use of regmap_write
> can have a good readability.
> 
> So I think the using regmap_write than regmap_update_bits is more reasonable.
> @@ -508,10 +509,9 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>         if (i >= odr_num)
>                 return -EINVAL;
> 
> -       return regmap_update_bits(data->regmap,
> -                                 FXOS8700_CTRL_REG1,
> -                                 FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
> -                                 fxos8700_odr[i].bits << 3 | active_mode);
> +       val &= ~FXOS8700_CTRL_ODR_MSK;
> +       val |= FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) | FXOS8700_ACTIVE;
> +       return regmap_write(data->regmap, FXOS8700_CTRL_REG1, val);
>  }
> 
> However there is a minimal fix, the patch looks more graceful:
> @@ -511,7 +512,8 @@ static int fxos8700_set_odr(struct fxos8700_data *data, enum fxos8700_sensor t,
>         return regmap_update_bits(data->regmap,
>                                   FXOS8700_CTRL_REG1,
>                                   FXOS8700_CTRL_ODR_MSK + FXOS8700_ACTIVE,
| not + for combining masks. 
> -                                 fxos8700_odr[i].bits << 3 | active_mode);
> +                                 FIELD_PREP(FXOS8700_CTRL_ODR_MSK, fxos8700_odr[i].bits) |
> +                                 FXOS8700_ACTIVE);
>  }
> 
> Which is better? In next patch I also faced a difficult decision about it.

I would go with the regmap_write() choice - though in cases like this I think
most important concern is readability.  Sometimes that means regmap_update_bits()
is a better choice even if we already have the read value available.
I think that's not true here so regmap_write() is better option.

> > >  static int fxos8700_get_odr(struct fxos8700_data *data, enum
> > > fxos8700_sensor t, @@ -524,7 +523,7 @@ static int  
> > fxos8700_get_odr(struct fxos8700_data *data, enum fxos8700_sensor t,  
> > >       if (ret)
> > >               return ret;
> > >
> > > -     val &= FXOS8700_CTRL_ODR_MSK;
> > > +     val = FIELD_GET(FXOS8700_CTRL_ODR_MSK, val);
> > >
> > >       for (i = 0; i < odr_num; i++)
> > >               if (val == fxos8700_odr[i].bits)  
>