1. Patchew
  2. linux
  3. [PATCH v5 0/7] iio: accel: adxl345: Add spi-3wire feature
  4. View patch

[PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete

Lothar Rubusch posted 7 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete
Posted by Lothar Rubusch 1 year, 10 months ago 
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)
+
 #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;
-- 
2.25.1
Re: [PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete
Posted by Jonathan Cameron 1 year, 10 months ago 
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;
Re: [PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete
Posted by Lothar Rubusch 1 year, 10 months ago 
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;
>
Re: [PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete
Posted by Jonathan Cameron 1 year, 10 months ago 
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;  
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.