On Tue, 10 Jan 2023 07:44:20 +0000
Carlos Song <carlos.song@xxxxxxx> 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@xxxxxxxxxx>
> > Sent: Saturday, December 31, 2022 10:51 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 1/4] iio: imu: fxos8700: fix incorrect ODR mode
> > readback
> >
> > Caution: EXT Email
> >
> > On Wed, 28 Dec 2022 17:39:38 +0800
> > carlos.song@xxxxxxx wrote:
> >
> > > From: Carlos Song <carlos.song@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > 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)
>
