| Message ID | 20250318230843.76068-7-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: add interrupt based sensor events | expand |
On Tue, 18 Mar 2025 23:08:38 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Introduce enums and functions to work with the sample frequency > adjustments. Let the sample frequency adjust via IIO and configure > a reasonable default. > > Replace the old static sample frequency handling. During adjustment of > bw registers, measuring is disabled and afterwards enabled again. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> One minor thing inline. > return -EINVAL; > @@ -504,7 +581,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, > int val, int val2, long mask) > { > struct adxl345_state *st = iio_priv(indio_dev); > - s64 n; > + enum adxl345_odr odr; > + int ret; > + > + ret = adxl345_set_measure_en(st, false); Why is this necessary but wasn't before? If it should always have been done for existing calibbias etc, perhaps a separate precursor patch is appropriate? > + if (ret) > + return ret; > > switch (mask) { > case IIO_CHAN_INFO_CALIBBIAS: > @@ -512,20 +594,26 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, > * 8-bit resolution at +/- 2g, that is 4x accel data scale > * factor > */ > - return regmap_write(st->regmap, > - ADXL345_REG_OFS_AXIS(chan->address), > - val / 4); > + ret = regmap_write(st->regmap, > + ADXL345_REG_OFS_AXIS(chan->address), > + val / 4); > + if (ret) > + return ret; > + break; > case IIO_CHAN_INFO_SAMP_FREQ: > - n = div_s64(val * NANOHZ_PER_HZ + val2, > - ADXL345_BASE_RATE_NANO_HZ); > + ret = adxl345_find_odr(st, val, val2, &odr); > + if (ret) > + return ret; > > - return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE, > - ADXL345_BW_RATE, > - clamp_val(ilog2(n), 0, > - ADXL345_BW_RATE)); > + ret = adxl345_set_odr(st, odr); > + if (ret) > + return ret; > + break; > + default: > + return -EINVAL; > } > > - return -EINVAL; > + return adxl345_set_measure_en(st, true); > }
On Mon, Mar 31, 2025 at 12:33 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 18 Mar 2025 23:08:38 +0000 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > Introduce enums and functions to work with the sample frequency > > adjustments. Let the sample frequency adjust via IIO and configure > > a reasonable default. > > > > Replace the old static sample frequency handling. During adjustment of > > bw registers, measuring is disabled and afterwards enabled again. > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > One minor thing inline. > > > > return -EINVAL; > > @@ -504,7 +581,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, > > int val, int val2, long mask) > > { > > struct adxl345_state *st = iio_priv(indio_dev); > > - s64 n; > > + enum adxl345_odr odr; > > + int ret; > > + > > + ret = adxl345_set_measure_en(st, false); > > Why is this necessary but wasn't before? > If it should always have been done for existing calibbias etc, > perhaps a separate precursor patch is appropriate? > On the one side the datasheet recommends to have measurement disabled, when adjusting certain sensor registers, mostly related to interrupt events. Before the sensor was operated in FIFO_BYPASS mode only without using the sensor events. With interrupt based events, it will operate in FIFO_STREAM or similar. Then it seems to me to be a better approach to put it generally in standby mode when configuring registers to avoid ending up e.g. in FIFO overrun or the like. On the other side, I saw similar approaches in the sources of Analog sensors. Enable/disable measurement was done there in the particular functions. In the particular case of adxl345_write_raw, odr and range values are going to be set and I implement enable/disable measurement directly in the write_raw. In comparison to the ADXL380 (different sensor!) where this is done, too, but down in the specific setter functions. I can see a bit of an advantage, if something fails, the sensor generally stays turned off. I'll keep this in v6 of the patches. Pls, note, I did not observe faulty behavior due to that or analyzed it thoroughly if and where it is probably better to have measurement turned off. At best, it won't make any difference and is probably rather kind of "best practice". If not, I would expect rather sporadic minor issues. As always, pls consider my patch(es) as a proposal, sometimes with an invisible question mark ;) If you have a contrary opinion and/or experience, please let me know. > > > + if (ret) > > + return ret; > > > > switch (mask) { > > case IIO_CHAN_INFO_CALIBBIAS: > > @@ -512,20 +594,26 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, > > * 8-bit resolution at +/- 2g, that is 4x accel data scale > > * factor > > */ > > - return regmap_write(st->regmap, > > - ADXL345_REG_OFS_AXIS(chan->address), > > - val / 4); > > + ret = regmap_write(st->regmap, > > + ADXL345_REG_OFS_AXIS(chan->address), > > + val / 4); > > + if (ret) > > + return ret; > > + break; > > case IIO_CHAN_INFO_SAMP_FREQ: > > - n = div_s64(val * NANOHZ_PER_HZ + val2, > > - ADXL345_BASE_RATE_NANO_HZ); > > + ret = adxl345_find_odr(st, val, val2, &odr); > > + if (ret) > > + return ret; > > > > - return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE, > > - ADXL345_BW_RATE, > > - clamp_val(ilog2(n), 0, > > - ADXL345_BW_RATE)); > > + ret = adxl345_set_odr(st, odr); > > + if (ret) > > + return ret; > > + break; > > + default: > > + return -EINVAL; > > } > > > > - return -EINVAL; > > + return adxl345_set_measure_en(st, true); > > }
On Mon, 14 Apr 2025 13:41:20 +0200 Lothar Rubusch <l.rubusch@gmail.com> wrote: > On Mon, Mar 31, 2025 at 12:33 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Tue, 18 Mar 2025 23:08:38 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > > > Introduce enums and functions to work with the sample frequency > > > adjustments. Let the sample frequency adjust via IIO and configure > > > a reasonable default. > > > > > > Replace the old static sample frequency handling. During adjustment of > > > bw registers, measuring is disabled and afterwards enabled again. > > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > One minor thing inline. > > > > > > > return -EINVAL; > > > @@ -504,7 +581,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, > > > int val, int val2, long mask) > > > { > > > struct adxl345_state *st = iio_priv(indio_dev); > > > - s64 n; > > > + enum adxl345_odr odr; > > > + int ret; > > > + > > > + ret = adxl345_set_measure_en(st, false); > > > > Why is this necessary but wasn't before? > > If it should always have been done for existing calibbias etc, > > perhaps a separate precursor patch is appropriate? > > > > On the one side the datasheet recommends to have measurement disabled, > when adjusting certain sensor registers, mostly related to interrupt > events. Before the sensor was operated in FIFO_BYPASS mode only > without using the sensor events. With interrupt based events, it will > operate in FIFO_STREAM or similar. Then it seems to me to be a better > approach to put it generally in standby mode when configuring > registers to avoid ending up e.g. in FIFO overrun or the like. On the > other side, I saw similar approaches in the sources of Analog sensors. > Enable/disable measurement was done there in the particular functions. > In the particular case of adxl345_write_raw, odr and range values are > going to be set and I implement enable/disable measurement directly in > the write_raw. In comparison to the ADXL380 (different sensor!) where > this is done, too, but down in the specific setter functions. I can > see a bit of an advantage, if something fails, the sensor generally > stays turned off. I'll keep this in v6 of the patches. Thanks for the explanation. That answered my question nicely and this is fine. > > Pls, note, I did not observe faulty behavior due to that or analyzed > it thoroughly if and where it is probably better to have measurement > turned off. At best, it won't make any difference and is probably > rather kind of "best practice". If not, I would expect rather sporadic > minor issues. > > As always, pls consider my patch(es) as a proposal, sometimes with an > invisible question mark ;) If you have a contrary opinion and/or > experience, please let me know. > > > > > > + if (ret) > > > + return ret; > > > > > > switch (mask) { > > > case IIO_CHAN_INFO_CALIBBIAS: > > > @@ -512,20 +594,26 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, > > > * 8-bit resolution at +/- 2g, that is 4x accel data scale > > > * factor > > > */ > > > - return regmap_write(st->regmap, > > > - ADXL345_REG_OFS_AXIS(chan->address), > > > - val / 4); > > > + ret = regmap_write(st->regmap, > > > + ADXL345_REG_OFS_AXIS(chan->address), > > > + val / 4); > > > + if (ret) > > > + return ret; > > > + break; > > > case IIO_CHAN_INFO_SAMP_FREQ: > > > - n = div_s64(val * NANOHZ_PER_HZ + val2, > > > - ADXL345_BASE_RATE_NANO_HZ); > > > + ret = adxl345_find_odr(st, val, val2, &odr); > > > + if (ret) > > > + return ret; > > > > > > - return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE, > > > - ADXL345_BW_RATE, > > > - clamp_val(ilog2(n), 0, > > > - ADXL345_BW_RATE)); > > > + ret = adxl345_set_odr(st, odr); > > > + if (ret) > > > + return ret; > > > + break; > > > + default: > > > + return -EINVAL; > > > } > > > > > > - return -EINVAL; > > > + return adxl345_set_measure_en(st, true); > > > }
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h index 7d482dd595fa..6c1f96406136 100644 --- a/drivers/iio/accel/adxl345.h +++ b/drivers/iio/accel/adxl345.h @@ -69,7 +69,7 @@ * BW_RATE bits - Bandwidth and output data rate. The default value is * 0x0A, which translates to a 100 Hz output data rate */ -#define ADXL345_BW_RATE GENMASK(3, 0) +#define ADXL345_BW_RATE_MSK GENMASK(3, 0) #define ADXL345_BW_LOW_POWER BIT(4) #define ADXL345_BASE_RATE_NANO_HZ 97656250LL diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c index 76ee657e958a..d9d786e15156 100644 --- a/drivers/iio/accel/adxl345_core.c +++ b/drivers/iio/accel/adxl345_core.c @@ -64,6 +64,45 @@ static const unsigned int adxl345_tap_time_reg[] = { [ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR, }; +enum adxl345_odr { + ADXL345_ODR_0P10HZ = 0, + ADXL345_ODR_0P20HZ, + ADXL345_ODR_0P39HZ, + ADXL345_ODR_0P78HZ, + ADXL345_ODR_1P56HZ, + ADXL345_ODR_3P13HZ, + ADXL345_ODR_6P25HZ, + ADXL345_ODR_12P50HZ, + ADXL345_ODR_25HZ, + ADXL345_ODR_50HZ, + ADXL345_ODR_100HZ, + ADXL345_ODR_200HZ, + ADXL345_ODR_400HZ, + ADXL345_ODR_800HZ, + ADXL345_ODR_1600HZ, + ADXL345_ODR_3200HZ, +}; + +/* Certain features recommend 12.5 Hz - 400 Hz ODR */ +static const int adxl345_odr_tbl[][2] = { + [ADXL345_ODR_0P10HZ] = { 0, 97000 }, + [ADXL345_ODR_0P20HZ] = { 0, 195000 }, + [ADXL345_ODR_0P39HZ] = { 0, 390000 }, + [ADXL345_ODR_0P78HZ] = { 0, 781000 }, + [ADXL345_ODR_1P56HZ] = { 1, 562000 }, + [ADXL345_ODR_3P13HZ] = { 3, 125000 }, + [ADXL345_ODR_6P25HZ] = { 6, 250000 }, + [ADXL345_ODR_12P50HZ] = { 12, 500000 }, + [ADXL345_ODR_25HZ] = { 25, 0 }, + [ADXL345_ODR_50HZ] = { 50, 0 }, + [ADXL345_ODR_100HZ] = { 100, 0 }, + [ADXL345_ODR_200HZ] = { 200, 0 }, + [ADXL345_ODR_400HZ] = { 400, 0 }, + [ADXL345_ODR_800HZ] = { 800, 0 }, + [ADXL345_ODR_1600HZ] = { 1600, 0 }, + [ADXL345_ODR_3200HZ] = { 3200, 0 }, +}; + struct adxl345_state { const struct adxl345_chip_info *info; struct regmap *regmap; @@ -116,6 +155,7 @@ 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), \ .scan_index = (index), \ .scan_type = { \ .sign = 's', \ @@ -443,14 +483,53 @@ static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int, return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff)); } +static int adxl345_find_odr(struct adxl345_state *st, int val, + int val2, enum adxl345_odr *odr) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(adxl345_odr_tbl); i++) { + if (val == adxl345_odr_tbl[i][0] && + val2 == adxl345_odr_tbl[i][1]) { + *odr = i; + return 0; + } + } + + return -EINVAL; +} + +static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr) +{ + return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE, + ADXL345_BW_RATE_MSK, + FIELD_PREP(ADXL345_BW_RATE_MSK, odr)); +} + +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_SAMP_FREQ: + *vals = (int *)adxl345_odr_tbl; + *type = IIO_VAL_INT_PLUS_MICRO; + *length = ARRAY_SIZE(adxl345_odr_tbl) * 2; + return IIO_AVAIL_LIST; + } + + return -EINVAL; +} + static int adxl345_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) { struct adxl345_state *st = iio_priv(indio_dev); __le16 accel; - long long samp_freq_nhz; unsigned int regval; + enum adxl345_odr odr; int ret; switch (mask) { @@ -488,12 +567,10 @@ static int adxl345_read_raw(struct iio_dev *indio_dev, ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val); if (ret) return ret; - - samp_freq_nhz = ADXL345_BASE_RATE_NANO_HZ << - (regval & ADXL345_BW_RATE); - *val = div_s64_rem(samp_freq_nhz, NANOHZ_PER_HZ, val2); - - return IIO_VAL_INT_PLUS_NANO; + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval); + *val = adxl345_odr_tbl[odr][0]; + *val2 = adxl345_odr_tbl[odr][1]; + return IIO_VAL_INT_PLUS_MICRO; } return -EINVAL; @@ -504,7 +581,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, int val, int val2, long mask) { struct adxl345_state *st = iio_priv(indio_dev); - s64 n; + enum adxl345_odr odr; + int ret; + + ret = adxl345_set_measure_en(st, false); + if (ret) + return ret; switch (mask) { case IIO_CHAN_INFO_CALIBBIAS: @@ -512,20 +594,26 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, * 8-bit resolution at +/- 2g, that is 4x accel data scale * factor */ - return regmap_write(st->regmap, - ADXL345_REG_OFS_AXIS(chan->address), - val / 4); + ret = regmap_write(st->regmap, + ADXL345_REG_OFS_AXIS(chan->address), + val / 4); + if (ret) + return ret; + break; case IIO_CHAN_INFO_SAMP_FREQ: - n = div_s64(val * NANOHZ_PER_HZ + val2, - ADXL345_BASE_RATE_NANO_HZ); + ret = adxl345_find_odr(st, val, val2, &odr); + if (ret) + return ret; - return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE, - ADXL345_BW_RATE, - clamp_val(ilog2(n), 0, - ADXL345_BW_RATE)); + ret = adxl345_set_odr(st, odr); + if (ret) + return ret; + break; + default: + return -EINVAL; } - return -EINVAL; + return adxl345_set_measure_en(st, true); } static int adxl345_read_event_config(struct iio_dev *indio_dev, @@ -754,7 +842,7 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev, case IIO_CHAN_INFO_CALIBBIAS: return IIO_VAL_INT; case IIO_CHAN_INFO_SAMP_FREQ: - return IIO_VAL_INT_PLUS_NANO; + return IIO_VAL_INT_PLUS_MICRO; default: return -EINVAL; } @@ -767,19 +855,6 @@ static void adxl345_powerdown(void *ptr) adxl345_set_measure_en(st, false); } -static IIO_CONST_ATTR_SAMP_FREQ_AVAIL( -"0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200" -); - -static struct attribute *adxl345_attrs[] = { - &iio_const_attr_sampling_frequency_available.dev_attr.attr, - NULL -}; - -static const struct attribute_group adxl345_attrs_group = { - .attrs = adxl345_attrs, -}; - static int adxl345_set_fifo(struct adxl345_state *st) { unsigned int intio; @@ -1042,9 +1117,9 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p) } static const struct iio_info adxl345_info = { - .attrs = &adxl345_attrs_group, .read_raw = adxl345_read_raw, .write_raw = adxl345_write_raw, + .read_avail = adxl345_read_avail, .write_raw_get_fmt = adxl345_write_raw_get_fmt, .read_event_config = adxl345_read_event_config, .write_event_config = adxl345_write_event_config, @@ -1114,6 +1189,15 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, indio_dev->num_channels = ARRAY_SIZE(adxl345_channels); indio_dev->available_scan_masks = adxl345_scan_masks; + /* + * Using I2C at 100kHz would limit the maximum ODR to 200Hz, operation + * at an output rate above the recommended maximum may result in + * undesired behavior. + */ + ret = adxl345_set_odr(st, ADXL345_ODR_200HZ); + if (ret) + return ret; + /* Reset interrupts at start up */ ret = regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, 0x00); if (ret)
Introduce enums and functions to work with the sample frequency adjustments. Let the sample frequency adjust via IIO and configure a reasonable default. Replace the old static sample frequency handling. During adjustment of bw registers, measuring is disabled and afterwards enabled again. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- drivers/iio/accel/adxl345.h | 2 +- drivers/iio/accel/adxl345_core.c | 150 ++++++++++++++++++++++++------- 2 files changed, 118 insertions(+), 34 deletions(-)