| Message ID | 20240327220320.15509-2-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: Add spi-3wire feature | expand |
On Wed, 27 Mar 2024 22:03:14 +0000 Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com> > --- > 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. 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;
On Thu, Mar 28, 2024 at 2:37 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 27 Mar 2024 22:03:14 +0000 > Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com> > > --- > > 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? > 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; >
On Fri, 29 Mar 2024 01:03:29 +0100 Lothar Rubusch <l.rubusch@gmail.com> wrote: > On Thu, Mar 28, 2024 at 2:37 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 27 Mar 2024 22:03:14 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com> > > > --- > > > 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; > >
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) + #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;
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@gmail.com> --- drivers/iio/accel/adxl345_core.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)