On Fri, 29 Mar 2024 01:03:29 +0100 Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote: > On Thu, Mar 28, 2024 at 2:37 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > On Wed, 27 Mar 2024 22:03:14 +0000 > > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote: > > > > > Replace write() data_format by regmap_update_bits(), because bus specific > > > pre-configuration may have happened before on the same register. For > > > further updates to the data_format register then bus pre-configuration > > > needs to be masked out. > > > > > > Remove the data_range field from the struct adxl345_data, because it is > > > not used anymore. > > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx> > > > --- > > > drivers/iio/accel/adxl345_core.c | 18 ++++++++++++------ > > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > > > index 8bd30a23e..35df5e372 100644 > > > --- a/drivers/iio/accel/adxl345_core.c > > > +++ b/drivers/iio/accel/adxl345_core.c > > > @@ -37,7 +37,15 @@ > > > #define ADXL345_POWER_CTL_MEASURE BIT(3) > > > #define ADXL345_POWER_CTL_STANDBY 0x00 > > > > > > +#define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */ > > > +#define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */ > > > #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */ > > > +#define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */ > > > +#define ADXL345_DATA_FORMAT_MSK (ADXL345_DATA_FORMAT_RANGE | \ > > > + ADXL345_DATA_FORMAT_JUSTIFY | \ > > > + ADXL345_DATA_FORMAT_FULL_RES | \ > > > + ADXL345_DATA_FORMAT_SELF_TEST) > > This needs renaming. It's not a mask of everything in the register, or > > even just of everything related to format. > > > > Actually I'd just not have this definition. Use the build up value > > from all the submasks at the call site. Then we are just making it clear > > only a subset of fields are being cleared. > > > I understand this solution was not very useful. I'm not sure, I > understood you correctly. Please have a look into v6 if this matches > your comment. > Now, I remove the mask, instead I use a local variable in core's > probe() for the update mask. I added a comment. Nevertheless, I keep > the used flags for FORMAT_DATA. Does this go into the direction of > using the build up value from the submasks at the call site? > The new code looks good to me. A local variable doesn't carry the same implication of global applicability as the define did. Thanks, J > > Jonathan > > > > > + > > > #define ADXL345_DATA_FORMAT_2G 0 > > > #define ADXL345_DATA_FORMAT_4G 1 > > > #define ADXL345_DATA_FORMAT_8G 2 > > > @@ -48,7 +56,6 @@ > > > struct adxl345_data { > > > const struct adxl345_chip_info *info; > > > struct regmap *regmap; > > > - u8 data_range; > > > }; > > > > > > #define ADXL345_CHANNEL(index, axis) { \ > > > @@ -218,15 +225,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap) > > > > > > data = iio_priv(indio_dev); > > > data->regmap = regmap; > > > - /* Enable full-resolution mode */ > > > - data->data_range = ADXL345_DATA_FORMAT_FULL_RES; > > > data->info = device_get_match_data(dev); > > > if (!data->info) > > > return -ENODEV; > > > > > > - ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT, > > > - data->data_range); > > > - if (ret < 0) > > > + /* Enable full-resolution mode */ > > > + ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT, > > > + ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES); > > > + if (ret) > > > return dev_err_probe(dev, ret, "Failed to set data range\n"); > > > > > > indio_dev->name = data->info->name; > >