[PATCH v3 11/15] iio: accel: adxl345: add g-range configuration

Lothar Rubusch posted 15 patches 10 months ago
There is a newer version of this series
[PATCH v3 11/15] iio: accel: adxl345: add g-range configuration
Posted by Lothar Rubusch 10 months ago
Introduce means to configure and work with the available g-ranges
keeping the precision of 13 digits.

This is in preparation for the activity/inactivity feature.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 91 ++++++++++++++++++++++++++++++--
 1 file changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index fa169cac5c05..ab1ab09f348a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -86,6 +86,13 @@ enum adxl345_odr {
 	ADXL345_ODR_3200HZ,
 };
 
+enum adxl345_range {
+	ADXL345_2G_RANGE = 0,
+	ADXL345_4G_RANGE,
+	ADXL345_8G_RANGE,
+	ADXL345_16G_RANGE,
+};
+
 /* Certain features recommend 12.5 Hz - 400 Hz ODR */
 static const int adxl345_odr_tbl[][2] = {
 	[ADXL345_ODR_0P10HZ]	= {    0,  97000 },
@@ -106,6 +113,33 @@ static const int adxl345_odr_tbl[][2] = {
 	[ADXL345_ODR_3200HZ]	= { 3200, 0 },
 };
 
+/*
+ * Full resolution frequency table:
+ * (g * 2 * 9.80665) / (2^(resolution) - 1)
+ *
+ * resolution := 13 (full)
+ * g := 2|4|8|16
+ *
+ *  2g at 13bit: 0.004789
+ *  4g at 13bit: 0.009578
+ *  8g at 13bit: 0.019156
+ * 16g at 16bit: 0.038312
+ */
+static const int adxl345_fullres_range_tbl[][2] = {
+	[ADXL345_2G_RANGE]  = { 0, 4789 },
+	[ADXL345_4G_RANGE]  = { 0, 9578 },
+	[ADXL345_8G_RANGE]  = { 0, 19156 },
+	[ADXL345_16G_RANGE] = { 0, 38312 },
+};
+
+/* scaling */
+static const int adxl345_range_factor_tbl[] = {
+	[ADXL345_2G_RANGE]  = 1,
+	[ADXL345_4G_RANGE]  = 2,
+	[ADXL345_8G_RANGE]  = 4,
+	[ADXL345_16G_RANGE] = 8,
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -157,7 +191,8 @@ static struct iio_event_spec adxl345_events[] = {
 		BIT(IIO_CHAN_INFO_CALIBBIAS),				\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
-	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE) | \
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
 	.scan_index = (index),				\
 	.scan_type = {					\
 		.sign = 's',				\
@@ -483,12 +518,48 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
 	return 0;
 }
 
+static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
+			      enum adxl345_range *range)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(adxl345_fullres_range_tbl); i++)
+		if (val == adxl345_fullres_range_tbl[i][0] &&
+		    val2 == adxl345_fullres_range_tbl[i][1])
+			break;
+
+	if (i == ARRAY_SIZE(adxl345_fullres_range_tbl))
+		return -EINVAL;
+
+	*range = i;
+
+	return 0;
+}
+
+static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
+{
+	int ret;
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
+				 ADXL345_DATA_FORMAT_RANGE,
+				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int adxl345_read_avail(struct iio_dev *indio_dev,
 			      struct iio_chan_spec const *chan,
 			      const int **vals, int *type,
 			      int *length, long mask)
 {
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)adxl345_fullres_range_tbl;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = ARRAY_SIZE(adxl345_fullres_range_tbl) * 2;
+		return IIO_AVAIL_LIST;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*vals = (int *)adxl345_odr_tbl;
 		*type = IIO_VAL_INT_PLUS_MICRO;
@@ -507,6 +578,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 	__le16 accel;
 	unsigned int regval;
 	enum adxl345_odr odr;
+	enum adxl345_range range;
 	int ret;
 
 	switch (mask) {
@@ -525,8 +597,12 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		*val = sign_extend32(le16_to_cpu(accel), 12);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		*val = 0;
-		*val2 = st->info->uscale;
+		ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
+		if (ret)
+			return ret;
+		range = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
+		*val = adxl345_fullres_range_tbl[range][0];
+		*val2 = adxl345_fullres_range_tbl[range][1];
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_CALIBBIAS:
 		ret = regmap_read(st->regmap,
@@ -558,6 +634,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
+	enum adxl345_range range;
 	enum adxl345_odr odr;
 	int ret;
 
@@ -581,6 +658,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 			return ret;
 		ret = adxl345_set_odr(st, odr);
 		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = adxl345_find_range(st, val, val2,	&range);
+		if (ret)
+			return ret;
+		ret = adxl345_set_range(st, range);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -840,6 +923,8 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBBIAS:
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return IIO_VAL_INT_PLUS_MICRO;
 	default:
-- 
2.39.5
Re: [PATCH v3 11/15] iio: accel: adxl345: add g-range configuration
Posted by Jonathan Cameron 9 months, 2 weeks ago
On Thu, 20 Feb 2025 10:42:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Introduce means to configure and work with the available g-ranges
> keeping the precision of 13 digits.
> 
> This is in preparation for the activity/inactivity feature.

I'm not really following why adding range control is anything
much to do with that. Mostly we do this to improve accuracy for
low accelerations.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>


> @@ -483,12 +518,48 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
>  	return 0;
>  }
>  
> +static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
> +			      enum adxl345_range *range)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(adxl345_fullres_range_tbl); i++)
> +		if (val == adxl345_fullres_range_tbl[i][0] &&
> +		    val2 == adxl345_fullres_range_tbl[i][1])
> +			break;
Similar to case in earlier patch, maybe set *range and return in here
so that any finish of the loop is an error.
> +
> +	if (i == ARRAY_SIZE(adxl345_fullres_range_tbl))
> +		return -EINVAL;
> +
> +	*range = i;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> +				 ADXL345_DATA_FORMAT_RANGE,
> +				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> +	if (ret)
> +		return ret;
> +

return regmap_update_bits() unless this gets more complex in later patch.

> +	return 0;
> +}
> +

> @@ -558,6 +634,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
>  			     int val, int val2, long mask)
>  {
>  	struct adxl345_state *st = iio_priv(indio_dev);
> +	enum adxl345_range range;
>  	enum adxl345_odr odr;
>  	int ret;
>  
> @@ -581,6 +658,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		ret = adxl345_set_odr(st, odr);
>  		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = adxl345_find_range(st, val, val2,	&range);
> +		if (ret)
> +			return ret;
> +		ret = adxl345_set_range(st, range);
as in previous I'd have the 	
		if (ret)
			return ret;
here for consistency with the other one immediately above this.
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
Re: [PATCH v3 11/15] iio: accel: adxl345: add g-range configuration
Posted by Lothar Rubusch 9 months, 1 week ago
On Tue, Mar 4, 2025 at 2:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:30 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Introduce means to configure and work with the available g-ranges
> > keeping the precision of 13 digits.
> >
> > This is in preparation for the activity/inactivity feature.
>
> I'm not really following why adding range control is anything
> much to do with that. Mostly we do this to improve accuracy for
> low accelerations.
>

As you probably saw the connection comes a bit over the link in
adjusting the activity/inactivity
parameters (times and threshold) by a given range in the follow up patches.

If the question is rather why at all adding this g-range control. My
idea was that adjusting i.e. lowering precision, less odr, etc might
also help adjusting power consumption. In other words
from a user perspective I assume there is more configuration
possibility. I did not pretend to tune
the implementation for lowest possible power consumption, though. It
was just an idea.

[Also, I was curious about implementing it here. My patch here is
rather meant as a proposal,
if you strongly oppose the idea, pls let me know.]

> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
>
> > @@ -483,12 +518,48 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> >       return 0;
> >  }
> >
> > +static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
> > +                           enum adxl345_range *range)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(adxl345_fullres_range_tbl); i++)
> > +             if (val == adxl345_fullres_range_tbl[i][0] &&
> > +                 val2 == adxl345_fullres_range_tbl[i][1])
> > +                     break;
> Similar to case in earlier patch, maybe set *range and return in here
> so that any finish of the loop is an error.
> > +
> > +     if (i == ARRAY_SIZE(adxl345_fullres_range_tbl))
> > +             return -EINVAL;
> > +
> > +     *range = i;
> > +
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> > +                              ADXL345_DATA_FORMAT_RANGE,
> > +                              FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> > +     if (ret)
> > +             return ret;
> > +
>
> return regmap_update_bits() unless this gets more complex in later patch.
>
> > +     return 0;
> > +}
> > +
>
> > @@ -558,6 +634,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> >                            int val, int val2, long mask)
> >  {
> >       struct adxl345_state *st = iio_priv(indio_dev);
> > +     enum adxl345_range range;
> >       enum adxl345_odr odr;
> >       int ret;
> >
> > @@ -581,6 +658,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> >                       return ret;
> >               ret = adxl345_set_odr(st, odr);
> >               break;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             ret = adxl345_find_range(st, val, val2, &range);
> > +             if (ret)
> > +                     return ret;
> > +             ret = adxl345_set_range(st, range);
> as in previous I'd have the
>                 if (ret)
>                         return ret;
> here for consistency with the other one immediately above this.
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
>
Re: [PATCH v3 11/15] iio: accel: adxl345: add g-range configuration
Posted by Jonathan Cameron 9 months, 1 week ago
On Thu, 13 Mar 2025 17:47:08 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Tue, Mar 4, 2025 at 2:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 20 Feb 2025 10:42:30 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Introduce means to configure and work with the available g-ranges
> > > keeping the precision of 13 digits.
> > >
> > > This is in preparation for the activity/inactivity feature.  
> >
> > I'm not really following why adding range control is anything
> > much to do with that. Mostly we do this to improve accuracy for
> > low accelerations.
> >  
> 
> As you probably saw the connection comes a bit over the link in
> adjusting the activity/inactivity
> parameters (times and threshold) by a given range in the follow up patches.
> 
> If the question is rather why at all adding this g-range control. My
> idea was that adjusting i.e. lowering precision, less odr, etc might
> also help adjusting power consumption. In other words
> from a user perspective I assume there is more configuration
> possibility. I did not pretend to tune
> the implementation for lowest possible power consumption, though. It
> was just an idea.
> 
> [Also, I was curious about implementing it here. My patch here is
> rather meant as a proposal,
> if you strongly oppose the idea, pls let me know.]

Control is fine (and lots of drivers do it). It was just that comment
that had me confused. To me this is a mostly unrelated feature.

It used to be the case when I last regularly used multirange accelerometers
that they had approximately matched the quality of the ADC with that of
the sensor.  So normal reason to reduce range was that it actually gave
you better accuracy as long as you didn't saturate. Mind you, for
the applications I had with sensors on sprinters shoes, all accelerometers
used to saturate even on the highest range setting! Not sure if the
reducing range for noise improvements is true on this particular sensor.

Jonathan