| Message ID | 20241205171343.308963-11-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events | expand |
On Thu, 5 Dec 2024 17:13:43 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Add a basic setup for FIFO with configurable watermark. Add a handler > for watermark interrupt events and extend the channel for the > scan_index needed for the iio channel. The sensor is configurable to use > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now > a watermark can be configured, or disabled by setting 0. Further features > require a working FIFO setup. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> Various comments inline. Thanks, Jonathan > --- > drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++ > 1 file changed, 300 insertions(+) > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > index 3067a70c54e..58ed82d66dc 100644 > --- a/drivers/iio/accel/adxl345_core.c > +++ b/drivers/iio/accel/adxl345_core.c > @@ -15,15 +15,28 @@ > > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/events.h> I'm not seeing any use of this header yet. Bring it in when you need it. > +#include <linux/iio/kfifo_buf.h> > > #include "adxl345.h" > > +#define ADXL345_FIFO_BYPASS 0 > +#define ADXL345_FIFO_FIFO 1 > +#define ADXL345_FIFO_STREAM 2 > + > +#define ADXL345_DIRS 3 > + > struct adxl345_state { > int irq; > const struct adxl345_chip_info *info; > struct regmap *regmap; > + __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE]; > bool fifo_delay; /* delay: delay is needed for SPI */ > u8 intio; > + u8 int_map; > + u8 watermark; > + u8 fifo_mode; > }; > > #define ADXL345_CHANNEL(index, reg, axis) { \ > @@ -36,6 +49,13 @@ struct adxl345_state { > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > .scan_index = (index), \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 13, \ > + .storagebits = 16, \ > + .shift = 0, \ No need to specify shift of 0. It's the 'obvious' default and C will set it to 0 for you anyway. > + .endianness = IIO_LE, \ > + }, \ > } > > enum adxl345_chans { > @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = { > ADXL345_CHANNEL(2, chan_z, Z), > }; > > +static int adxl345_set_interrupts(struct adxl345_state *st) > +{ > + int ret; > + unsigned int int_enable = st->int_map; > + unsigned int int_map; > + > + /* Any bits set to 0 in the INT map register send their respective /* * Any bits.... > + * interrupts to the INT1 pin, whereas bits set to 1 send their respective > + * interrupts to the INT2 pin. The intio shall convert this accordingly. > + */ > + int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map); > + > + ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map); > + if (ret) > + return ret; > + > + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable); > +} > +/** > + * adxl345_get_samples() - Read number of FIFO entries. > + * @st: The initialized state instance of this driver. > + * > + * The sensor does not support treating any axis individually, or exclude them > + * from measuring. > + * > + * Return: negative error, or value. > + */ > +static int adxl345_get_samples(struct adxl345_state *st) > +{ > + unsigned int regval = 0; > + int ret; > + > + ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, ®val); > + if (ret < 0) > + return ret; > + > + return 0x3f & regval; FIELD_GET() for all stuff like this with appropriate #define for the mask. > +} > + > +/** > + * adxl345_fifo_transfer() - Read samples number of elements. > + * @st: The instance of the state object of this sensor. > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > + * > + * It is recommended that a multiple-byte read of all registers be performed to > + * prevent a change in data between reads of sequential registers. That is to > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. Doesn't match the code which is reading just one register lots of times. > + * > + * Return: 0 or error value. > + */ > +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples) > +{ > + size_t count; > + int i, ret; > + > + count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS; > + for (i = 0; i < samples; i++) { > + ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE, > + st->fifo_buf + (i * count / 2), count); > + if (ret < 0) > + return ret; This is where I'd expect to see the delay mentioned below. > + } > + return ret; > +} > + > +static int adxl345_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct adxl345_state *st = iio_priv(indio_dev); > + int ret; > + > + st->int_map = 0x00; > + > + ret = adxl345_set_interrupts(st); > + if (ret < 0) > + return ret; > + > + st->fifo_mode = ADXL345_FIFO_BYPASS; > + return adxl345_set_fifo(st); I'd normally expect order in predisable to be reverse of that in postenable. Why isn't it? That is why is the set_fifo here after set_interrupts and not before it. Add a comment. > +} > + > +static const struct iio_buffer_setup_ops adxl345_buffer_ops = { > + .postenable = adxl345_buffer_postenable, > + .predisable = adxl345_buffer_predisable, > +}; > + > +static int adxl345_get_status(struct adxl345_state *st) > +{ > + int ret; > + unsigned int regval; > + > + ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); > + if (ret < 0) > + return ret; > + > + return (0xff & regval); No brackets needed. > +} > + > +static int adxl345_fifo_push(struct iio_dev *indio_dev, > + int samples) > +{ > + struct adxl345_state *st = iio_priv(indio_dev); > + int i, ret; > + > + if (samples <= 0) > + return -EINVAL; > + > + ret = adxl345_fifo_transfer(st, samples); > + if (ret) > + return ret; > + > + for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS) { > + /* > + * To ensure that the FIFO has completely popped, there must be at least 5 > + * us between the end of reading the data registers, signified by the > + * transition to register 0x38 from 0x37 or the CS pin going high, and the > + * start of new reads of the FIFO or reading the FIFO_STATUS register. For > + * SPI operation at 1.5 MHz or lower, the register addressing portion of the > + * transmission is sufficient delay to ensure the FIFO has completely > + * popped. It is necessary for SPI operation greater than 1.5 MHz to > + * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us > + * at 5 MHz operation. > + */ > + if (st->fifo_delay && (samples > 1)) > + udelay(3); I'm not following why a delay here helps. At this point you're read masses of data from the fifo without the delays you mention. > + > + iio_push_to_buffers(indio_dev, &st->fifo_buf[i]); > + } > + > + return 0; > +} > + > +/** > + * adxl345_event_handler() - Handle events of the ADXL345. Up to you but... Given it's an IIO driver and that we have a very specific meaning for events, maybe just call this adxl345_irq_handler()? > + * @irq: The irq being handled. > + * @p: The struct iio_device pointer for the device. > + * > + * Return: The interrupt was handled. > + */ > +static irqreturn_t adxl345_event_handler(int irq, void *p) > +{ > + struct iio_dev *indio_dev = p; > + struct adxl345_state *st = iio_priv(indio_dev); > + u8 int_stat; > + int samples; > + > + int_stat = adxl345_get_status(st); > + if (int_stat < 0) > + return IRQ_NONE; > + > + if (int_stat == 0x0) Doesn't this correspond to 'not our interrupt'? If that's the case return IRQ_NONE is the right way to go and not reset the interrupt. You have registered it as maybe shared, and if it is, then this is a common thing to happen as interrupt from another device. > + goto err; > + > + if (int_stat & ADXL345_INT_OVERRUN) > + goto err; > + > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { I think you only ever enable the INT_WATERMARK? If so does it make sense to check for DATA_READY as well? > + samples = adxl345_get_samples(st); > + if (samples < 0) > + goto err; > + > + if (adxl345_fifo_push(indio_dev, samples) < 0) > + goto err; > + > + } > + return IRQ_HANDLED; > + > +err: > + adxl345_fifo_reset(st); > + > + return IRQ_HANDLED; > +} > + > static const struct iio_info adxl345_info = { > .attrs = &adxl345_attrs_group, > .read_raw = adxl345_read_raw, > .write_raw = adxl345_write_raw, > .write_raw_get_fmt = adxl345_write_raw_get_fmt, > + .hwfifo_set_watermark = adxl345_set_watermark, > }; > > /** > @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | > ADXL345_DATA_FORMAT_FULL_RES | > ADXL345_DATA_FORMAT_SELF_TEST); > + u8 fifo_ctl; > int ret; > > indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > if (ret < 0) > return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > + if (st->irq > 0) { > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > + > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > + if (ret) > + return ret; > + > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > + IRQF_SHARED | IRQF_ONESHOT, > + indio_dev->name, indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > + > + } else { > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > + Given you haven't removed this code from elsewhere, was the driver relying on the defaults after reset before this patch? I don't mind having this branch as a form of documentation even if that's true but maybe add a note to the patch description. > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > + > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > + if (ret < 0) > + return ret; > + } > return devm_iio_device_register(dev, indio_dev); > } > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
Hi Lothar,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lothar-Rubusch/iio-accel-adxl345-refrase-comment-on-probe/20241206-011802
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20241205171343.308963-11-l.rubusch%40gmail.com
patch subject: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events
config: nios2-randconfig-r072-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101132.Kj6R6i3h-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 14.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202412101132.Kj6R6i3h-lkp@intel.com/
smatch warnings:
drivers/iio/accel/adxl345_core.c:441 adxl345_event_handler() warn: unsigned 'int_stat' is never less than zero.
vim +/ret +321 drivers/iio/accel/adxl345_core.c
55d2386488598bb Lothar Rubusch 2024-12-05 433 static irqreturn_t adxl345_event_handler(int irq, void *p)
55d2386488598bb Lothar Rubusch 2024-12-05 434 {
55d2386488598bb Lothar Rubusch 2024-12-05 435 struct iio_dev *indio_dev = p;
55d2386488598bb Lothar Rubusch 2024-12-05 436 struct adxl345_state *st = iio_priv(indio_dev);
55d2386488598bb Lothar Rubusch 2024-12-05 437 u8 int_stat;
^^^^^^^^^^^
55d2386488598bb Lothar Rubusch 2024-12-05 438 int samples;
55d2386488598bb Lothar Rubusch 2024-12-05 439
55d2386488598bb Lothar Rubusch 2024-12-05 440 int_stat = adxl345_get_status(st);
55d2386488598bb Lothar Rubusch 2024-12-05 @441 if (int_stat < 0)
^^^^^^^^^^^^
signedness bug
55d2386488598bb Lothar Rubusch 2024-12-05 442 return IRQ_NONE;
55d2386488598bb Lothar Rubusch 2024-12-05 443
55d2386488598bb Lothar Rubusch 2024-12-05 444 if (int_stat == 0x0)
55d2386488598bb Lothar Rubusch 2024-12-05 445 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 446
55d2386488598bb Lothar Rubusch 2024-12-05 447 if (int_stat & ADXL345_INT_OVERRUN)
55d2386488598bb Lothar Rubusch 2024-12-05 448 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 449
55d2386488598bb Lothar Rubusch 2024-12-05 450 if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
55d2386488598bb Lothar Rubusch 2024-12-05 451 samples = adxl345_get_samples(st);
55d2386488598bb Lothar Rubusch 2024-12-05 452 if (samples < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 453 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 454
55d2386488598bb Lothar Rubusch 2024-12-05 455 if (adxl345_fifo_push(indio_dev, samples) < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 456 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 457
55d2386488598bb Lothar Rubusch 2024-12-05 458 }
55d2386488598bb Lothar Rubusch 2024-12-05 459 return IRQ_HANDLED;
55d2386488598bb Lothar Rubusch 2024-12-05 460
55d2386488598bb Lothar Rubusch 2024-12-05 461 err:
55d2386488598bb Lothar Rubusch 2024-12-05 462 adxl345_fifo_reset(st);
55d2386488598bb Lothar Rubusch 2024-12-05 463
55d2386488598bb Lothar Rubusch 2024-12-05 464 return IRQ_HANDLED;
55d2386488598bb Lothar Rubusch 2024-12-05 465 }
Hi Jonathan, Find my answers down below. Best, L On Sun, Dec 8, 2024 at 5:34 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 5 Dec 2024 17:13:43 +0000 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > Add a basic setup for FIFO with configurable watermark. Add a handler > > for watermark interrupt events and extend the channel for the > > scan_index needed for the iio channel. The sensor is configurable to use > > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now > > a watermark can be configured, or disabled by setting 0. Further features > > require a working FIFO setup. > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > Various comments inline. > > Thanks, > > Jonathan > > > --- > > drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++ > > 1 file changed, 300 insertions(+) > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > > index 3067a70c54e..58ed82d66dc 100644 > > --- a/drivers/iio/accel/adxl345_core.c > > +++ b/drivers/iio/accel/adxl345_core.c > > @@ -15,15 +15,28 @@ > > > > #include <linux/iio/iio.h> > > #include <linux/iio/sysfs.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/events.h> > > I'm not seeing any use of this header yet. Bring it in when you need it. > > > +#include <linux/iio/kfifo_buf.h> > > > > #include "adxl345.h" > > > > +#define ADXL345_FIFO_BYPASS 0 > > +#define ADXL345_FIFO_FIFO 1 > > +#define ADXL345_FIFO_STREAM 2 > > + > > +#define ADXL345_DIRS 3 > > + > > struct adxl345_state { > > int irq; > > const struct adxl345_chip_info *info; > > struct regmap *regmap; > > + __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE]; > > bool fifo_delay; /* delay: delay is needed for SPI */ > > u8 intio; > > + u8 int_map; > > + u8 watermark; > > + u8 fifo_mode; > > }; > > > > #define ADXL345_CHANNEL(index, reg, axis) { \ > > @@ -36,6 +49,13 @@ struct adxl345_state { > > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > .scan_index = (index), \ > > + .scan_type = { \ > > + .sign = 's', \ > > + .realbits = 13, \ > > + .storagebits = 16, \ > > + .shift = 0, \ > > No need to specify shift of 0. It's the 'obvious' default and C will set it to 0 > for you anyway. > > > + .endianness = IIO_LE, \ > > + }, \ > > } > > > > enum adxl345_chans { > > @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = { > > ADXL345_CHANNEL(2, chan_z, Z), > > }; > > > > +static int adxl345_set_interrupts(struct adxl345_state *st) > > +{ > > + int ret; > > + unsigned int int_enable = st->int_map; > > + unsigned int int_map; > > + > > + /* Any bits set to 0 in the INT map register send their respective > > /* > * Any bits.... > > > > + * interrupts to the INT1 pin, whereas bits set to 1 send their respective > > + * interrupts to the INT2 pin. The intio shall convert this accordingly. > > + */ > > + int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map); > > + > > + ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map); > > + if (ret) > > + return ret; > > + > > + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable); > > +} > > > > +/** > > + * adxl345_get_samples() - Read number of FIFO entries. > > + * @st: The initialized state instance of this driver. > > + * > > + * The sensor does not support treating any axis individually, or exclude them > > + * from measuring. > > + * > > + * Return: negative error, or value. > > + */ > > +static int adxl345_get_samples(struct adxl345_state *st) > > +{ > > + unsigned int regval = 0; > > + int ret; > > + > > + ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, ®val); > > + if (ret < 0) > > + return ret; > > + > > + return 0x3f & regval; > FIELD_GET() for all stuff like this with appropriate #define for the mask. > > > +} > > + > > +/** > > + * adxl345_fifo_transfer() - Read samples number of elements. > > + * @st: The instance of the state object of this sensor. > > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > > + * > > + * It is recommended that a multiple-byte read of all registers be performed to > > + * prevent a change in data between reads of sequential registers. That is to > > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. > > Doesn't match the code which is reading just one register lots of times. This is one of my painpoints, regmap_noinc_read() worked here, for a linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1 in one command. Also, I tried here regmap_bulk_read(). At all, I find this solution is working, but I'm not sure if there is not a total differnt way to do the read out. > > + * > > + * Return: 0 or error value. > > + */ > > +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples) > > +{ > > + size_t count; > > + int i, ret; > > + > > + count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS; > > + for (i = 0; i < samples; i++) { > > + ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE, > > + st->fifo_buf + (i * count / 2), count); > > + if (ret < 0) > > + return ret; > > This is where I'd expect to see the delay mentioned below. I agree with the delay, I'll move it. > > + } > > + return ret; > > +} > > + > > > +static int adxl345_buffer_predisable(struct iio_dev *indio_dev) > > +{ > > + struct adxl345_state *st = iio_priv(indio_dev); > > + int ret; > > + > > + st->int_map = 0x00; > > + > > + ret = adxl345_set_interrupts(st); > > + if (ret < 0) > > + return ret; > > + > > + st->fifo_mode = ADXL345_FIFO_BYPASS; > > + return adxl345_set_fifo(st); > I'd normally expect order in predisable to be reverse of that in postenable. > Why isn't it? That is why is the set_fifo here after set_interrupts and > not before it. Add a comment. > > > +} > > + > > +static const struct iio_buffer_setup_ops adxl345_buffer_ops = { > > + .postenable = adxl345_buffer_postenable, > > + .predisable = adxl345_buffer_predisable, > > +}; > > + > > +static int adxl345_get_status(struct adxl345_state *st) > > +{ > > + int ret; > > + unsigned int regval; > > + > > + ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); > > + if (ret < 0) > > + return ret; > > + > > + return (0xff & regval); > > No brackets needed. > > > +} > > + > > +static int adxl345_fifo_push(struct iio_dev *indio_dev, > > + int samples) > > +{ > > + struct adxl345_state *st = iio_priv(indio_dev); > > + int i, ret; > > + > > + if (samples <= 0) > > + return -EINVAL; > > + > > + ret = adxl345_fifo_transfer(st, samples); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS) { > > + /* > > + * To ensure that the FIFO has completely popped, there must be at least 5 > > + * us between the end of reading the data registers, signified by the > > + * transition to register 0x38 from 0x37 or the CS pin going high, and the > > + * start of new reads of the FIFO or reading the FIFO_STATUS register. For > > + * SPI operation at 1.5 MHz or lower, the register addressing portion of the > > + * transmission is sufficient delay to ensure the FIFO has completely > > + * popped. It is necessary for SPI operation greater than 1.5 MHz to > > + * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us > > + * at 5 MHz operation. > > + */ > > + if (st->fifo_delay && (samples > 1)) > > + udelay(3); > > I'm not following why a delay here helps. At this point you're read masses of > data from the fifo without the delays you mention. > > > + > > + iio_push_to_buffers(indio_dev, &st->fifo_buf[i]); > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * adxl345_event_handler() - Handle events of the ADXL345. > > Up to you but... > Given it's an IIO driver and that we have a very specific meaning > for events, maybe just call this adxl345_irq_handler()? > > > + * @irq: The irq being handled. > > + * @p: The struct iio_device pointer for the device. > > + * > > + * Return: The interrupt was handled. > > + */ > > +static irqreturn_t adxl345_event_handler(int irq, void *p) > > +{ > > + struct iio_dev *indio_dev = p; > > + struct adxl345_state *st = iio_priv(indio_dev); > > + u8 int_stat; > > + int samples; > > + > > + int_stat = adxl345_get_status(st); > > + if (int_stat < 0) > > + return IRQ_NONE; > > + > > + if (int_stat == 0x0) > Doesn't this correspond to 'not our interrupt'? > If that's the case return IRQ_NONE is the right way to go and not reset the > interrupt. You have registered it as maybe shared, and if it is, then this > is a common thing to happen as interrupt from another device. > Here I see actually + int_stat = adxl345_get_status(st); + if (int_stat < 0) + return IRQ_NONE; // a bus error, reading register not possible ...and then... + if (int_stat == 0x0) + // interrupt sources were 0, so IRQ not from our sensor I'm unsure if the first IRQ_NONE here is actually correct. I mean, if the bus is not working, actually any IRQ usage should be considered broken. Is there a way to break out of measuring? > > + goto err; > > + > > + if (int_stat & ADXL345_INT_OVERRUN) > > + goto err; > > + > > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > > I think you only ever enable the INT_WATERMARK? If so does it > make sense to check for DATA_READY as well? > Watermark comes usually with data ready or overrun. I guess for the FIFO watermark, just evaluating watermark is probably sufficient. For other events, it then might be notification w/ a data ready set. Probably better to introduce data ready when it's actually really needed? > > + samples = adxl345_get_samples(st); > > + if (samples < 0) > > + goto err; > > + > > + if (adxl345_fifo_push(indio_dev, samples) < 0) > > + goto err; > > + > > + } > > + return IRQ_HANDLED; > > + > > +err: > > + adxl345_fifo_reset(st); > > + > > + return IRQ_HANDLED; > > +} > > + > > static const struct iio_info adxl345_info = { > > .attrs = &adxl345_attrs_group, > > .read_raw = adxl345_read_raw, > > .write_raw = adxl345_write_raw, > > .write_raw_get_fmt = adxl345_write_raw_get_fmt, > > + .hwfifo_set_watermark = adxl345_set_watermark, > > }; > > > > /** > > @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > > unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | > > ADXL345_DATA_FORMAT_FULL_RES | > > ADXL345_DATA_FORMAT_SELF_TEST); > > + u8 fifo_ctl; > > int ret; > > > > indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > > @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > > if (ret < 0) > > return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > > > + if (st->irq > 0) { > > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > > + > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > > + if (ret) > > + return ret; > > + > > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > > + IRQF_SHARED | IRQF_ONESHOT, > > + indio_dev->name, indio_dev); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > > + > > + } else { > > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > > + > Given you haven't removed this code from elsewhere, was the driver relying > on the defaults after reset before this patch? > > I don't mind having this branch as a form of documentation even if that's > true but maybe add a note to the patch description. > I'm not sure if I get you correctly. The driver before only implemented "BYPASS" mode. This was the case w/o a defined interrupt-name. My intention now is to keep this behavior as fallback. If no IRQ is around, i.e. interrupt + interrupt-name, the sensor driver will operate the sensor in BYPASS mode. I was interpreting this also as the "default behavior", you mentioned in the dt-binding patch? Is this correct? What do you mean when the driver is relying on the defaults after reset? > > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > > + > > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > > + if (ret < 0) > > + return ret; > > + } > > return devm_iio_device_register(dev, indio_dev); > > } > > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); >
> > > +} > > > + > > > +/** > > > + * adxl345_fifo_transfer() - Read samples number of elements. > > > + * @st: The instance of the state object of this sensor. > > > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > > > + * > > > + * It is recommended that a multiple-byte read of all registers be performed to > > > + * prevent a change in data between reads of sequential registers. That is to > > > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. > > > > Doesn't match the code which is reading just one register lots of times. > > This is one of my painpoints, regmap_noinc_read() worked here, for a > linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1 > in one command. Also, I tried here regmap_bulk_read(). At all, I find > this solution is working, but I'm not sure if there is not a total > differnt way to do the read out. A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2 etc. regmap_noinc_read() just keeps reading the same register, so is typically used for fifos. I opened the datasheet. It seems to say you need to read 3 registers repeatedly rather than one 3 times as often. There isn't a good way to do that sort of sequenced read in one go. So you will need a loop like you have, but it should need a bulk read. Curious it doesn't seem to... Ah. Device auto increments for both SPI and I2C. So in that case the noinc_read and normal bulk read will actually issue the same thing and as these are volatile registers it doesn't matter (it would if you were caching the result as the data would end up cached in different places). It will do the wrong thing though if you have an i2c controller that is less capable and can't do large reads. So you should definitely use bulk_read not noinc. Have you set available_scan_masks? If not you want to do so as per comment I made in the cover letter. > > > + * @irq: The irq being handled. > > > + * @p: The struct iio_device pointer for the device. > > > + * > > > + * Return: The interrupt was handled. > > > + */ > > > +static irqreturn_t adxl345_event_handler(int irq, void *p) > > > +{ > > > + struct iio_dev *indio_dev = p; > > > + struct adxl345_state *st = iio_priv(indio_dev); > > > + u8 int_stat; > > > + int samples; > > > + > > > + int_stat = adxl345_get_status(st); > > > + if (int_stat < 0) > > > + return IRQ_NONE; > > > + > > > + if (int_stat == 0x0) > > Doesn't this correspond to 'not our interrupt'? > > If that's the case return IRQ_NONE is the right way to go and not reset the > > interrupt. You have registered it as maybe shared, and if it is, then this > > is a common thing to happen as interrupt from another device. > > > > Here I see actually > + int_stat = adxl345_get_status(st); > + if (int_stat < 0) > + return IRQ_NONE; // a bus error, reading register not possible > ...and then... > + if (int_stat == 0x0) > + // interrupt sources were 0, so IRQ not from our sensor > > I'm unsure if the first IRQ_NONE here is actually correct. I mean, if > the bus is not working, > actually any IRQ usage should be considered broken. Is there a way to > break out of measuring? > It is a much debated thing on what you should return if you have no idea if it is our interrupt or not. There isn't really a right answer. If you get a lot of IRQ_NONE and no one else claims it eventually the interrupt will be disabled (to break the interrupt storm freezing the machine). > > > + goto err; > > > + > > > + if (int_stat & ADXL345_INT_OVERRUN) > > > + goto err; > > > + > > > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > > > > I think you only ever enable the INT_WATERMARK? If so does it > > make sense to check for DATA_READY as well? > > > > Watermark comes usually with data ready or overrun. I guess for the > FIFO watermark, just evaluating watermark is probably sufficient. For > other events, it then might be notification w/ a data ready set. > Probably better to introduce data ready when it's actually really > needed? Yes. That dataready is normally used when you are doing capture without the fifo and want to read each sample - kind of same as a watermark depth of 1, but less hardware turned on. As such, may be no need to ever support it. return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > > > > > + if (st->irq > 0) { > > > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > > > + > > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > > > + if (ret) > > > + return ret; > > > + > > > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > > > + IRQF_SHARED | IRQF_ONESHOT, > > > + indio_dev->name, indio_dev); > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > > > + > > > + } else { > > > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > > > + > > Given you haven't removed this code from elsewhere, was the driver relying > > on the defaults after reset before this patch? > > > > I don't mind having this branch as a form of documentation even if that's > > true but maybe add a note to the patch description. > > > > I'm not sure if I get you correctly. The driver before only > implemented "BYPASS" mode. This was the case w/o a defined > interrupt-name. My intention now is to keep this behavior as fallback. > If no IRQ is around, i.e. interrupt + interrupt-name, the sensor > driver will operate the sensor in BYPASS mode. > > I was interpreting this also as the "default behavior", you mentioned > in the dt-binding patch? Is this correct? > > What do you mean when the driver is relying on the defaults after reset? The driver worked without irq before now. Which is same as this path. So how was the register written here being configured correctly before this patch? I'm guessing it wasn't and that the value written here is the default on power up. Either that or I'm miss understanding what this branch of the if / else is about. Jonathan > > > > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > > > + > > > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > > > + if (ret < 0) > > > + return ret; > > > + } > > > return devm_iio_device_register(dev, indio_dev); > > > } > > > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); > > >
On Tue, 10 Dec 2024 11:47:37 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > Hi Lothar, > > kernel test robot noticed the following build warnings: Ah. This was the report you were referring to I guess. Just fix these in v6 version of this patch. Thanks, Jonathan > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Lothar-Rubusch/iio-accel-adxl345-refrase-comment-on-probe/20241206-011802 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > patch link: https://lore.kernel.org/r/20241205171343.308963-11-l.rubusch%40gmail.com > patch subject: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events > config: nios2-randconfig-r072-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101132.Kj6R6i3h-lkp@intel.com/config) > compiler: nios2-linux-gcc (GCC) 14.2.0 > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://lore.kernel.org/r/202412101132.Kj6R6i3h-lkp@intel.com/ > > smatch warnings: > drivers/iio/accel/adxl345_core.c:441 adxl345_event_handler() warn: unsigned 'int_stat' is never less than zero. > > vim +/ret +321 drivers/iio/accel/adxl345_core.c > > 55d2386488598bb Lothar Rubusch 2024-12-05 433 static irqreturn_t adxl345_event_handler(int irq, void *p) > 55d2386488598bb Lothar Rubusch 2024-12-05 434 { > 55d2386488598bb Lothar Rubusch 2024-12-05 435 struct iio_dev *indio_dev = p; > 55d2386488598bb Lothar Rubusch 2024-12-05 436 struct adxl345_state *st = iio_priv(indio_dev); > 55d2386488598bb Lothar Rubusch 2024-12-05 437 u8 int_stat; > ^^^^^^^^^^^ > > 55d2386488598bb Lothar Rubusch 2024-12-05 438 int samples; > 55d2386488598bb Lothar Rubusch 2024-12-05 439 > 55d2386488598bb Lothar Rubusch 2024-12-05 440 int_stat = adxl345_get_status(st); > 55d2386488598bb Lothar Rubusch 2024-12-05 @441 if (int_stat < 0) > ^^^^^^^^^^^^ > signedness bug > > 55d2386488598bb Lothar Rubusch 2024-12-05 442 return IRQ_NONE; > 55d2386488598bb Lothar Rubusch 2024-12-05 443 > 55d2386488598bb Lothar Rubusch 2024-12-05 444 if (int_stat == 0x0) > 55d2386488598bb Lothar Rubusch 2024-12-05 445 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 446 > 55d2386488598bb Lothar Rubusch 2024-12-05 447 if (int_stat & ADXL345_INT_OVERRUN) > 55d2386488598bb Lothar Rubusch 2024-12-05 448 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 449 > 55d2386488598bb Lothar Rubusch 2024-12-05 450 if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > 55d2386488598bb Lothar Rubusch 2024-12-05 451 samples = adxl345_get_samples(st); > 55d2386488598bb Lothar Rubusch 2024-12-05 452 if (samples < 0) > 55d2386488598bb Lothar Rubusch 2024-12-05 453 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 454 > 55d2386488598bb Lothar Rubusch 2024-12-05 455 if (adxl345_fifo_push(indio_dev, samples) < 0) > 55d2386488598bb Lothar Rubusch 2024-12-05 456 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 457 > 55d2386488598bb Lothar Rubusch 2024-12-05 458 } > 55d2386488598bb Lothar Rubusch 2024-12-05 459 return IRQ_HANDLED; > 55d2386488598bb Lothar Rubusch 2024-12-05 460 > 55d2386488598bb Lothar Rubusch 2024-12-05 461 err: > 55d2386488598bb Lothar Rubusch 2024-12-05 462 adxl345_fifo_reset(st); > 55d2386488598bb Lothar Rubusch 2024-12-05 463 > 55d2386488598bb Lothar Rubusch 2024-12-05 464 return IRQ_HANDLED; > 55d2386488598bb Lothar Rubusch 2024-12-05 465 } >
Hello Jonathan, Thank you again. This brought several pieces together for me. See answeres down below. Best, L On Wed, Dec 11, 2024 at 8:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > +} > > > > + > > > > +/** > > > > + * adxl345_fifo_transfer() - Read samples number of elements. > > > > + * @st: The instance of the state object of this sensor. > > > > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > > > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > > > > + * > > > > + * It is recommended that a multiple-byte read of all registers be performed to > > > > + * prevent a change in data between reads of sequential registers. That is to > > > > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. > > > > > > Doesn't match the code which is reading just one register lots of times. > > > > This is one of my painpoints, regmap_noinc_read() worked here, for a > > linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1 > > in one command. Also, I tried here regmap_bulk_read(). At all, I find > > this solution is working, but I'm not sure if there is not a total > > differnt way to do the read out. > > A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2 > etc. regmap_noinc_read() just keeps reading the same register, so is typically > used for fifos. > > I opened the datasheet. It seems to say you need to read 3 registers repeatedly > rather than one 3 times as often. There isn't a good way to do that sort of > sequenced read in one go. So you will need a loop like you have, but it > should need a bulk read. Curious it doesn't seem to... > > Ah. Device auto increments for both SPI and I2C. So in that case > the noinc_read and normal bulk read will actually issue the same thing and > as these are volatile registers it doesn't matter (it would if you were > caching the result as the data would end up cached in different places). > > It will do the wrong thing though if you have an i2c controller that > is less capable and can't do large reads. So you should definitely use > bulk_read not noinc. > > Have you set available_scan_masks? If not you want to do so as > per comment I made in the cover letter. > For v6 I'll setup available_scan_masks, as I understand its usage. I'm not sure if I understood what it actually does. I can see it needs the channel indexes available, but I'll need to read a bit more code. Hopefully for now it'll be sufficient. > > > > > + * @irq: The irq being handled. > > > > + * @p: The struct iio_device pointer for the device. > > > > + * > > > > + * Return: The interrupt was handled. > > > > + */ > > > > +static irqreturn_t adxl345_event_handler(int irq, void *p) > > > > +{ > > > > + struct iio_dev *indio_dev = p; > > > > + struct adxl345_state *st = iio_priv(indio_dev); > > > > + u8 int_stat; > > > > + int samples; > > > > + > > > > + int_stat = adxl345_get_status(st); > > > > + if (int_stat < 0) > > > > + return IRQ_NONE; > > > > + > > > > + if (int_stat == 0x0) > > > Doesn't this correspond to 'not our interrupt'? > > > If that's the case return IRQ_NONE is the right way to go and not reset the > > > interrupt. You have registered it as maybe shared, and if it is, then this > > > is a common thing to happen as interrupt from another device. > > > > > > > Here I see actually > > + int_stat = adxl345_get_status(st); > > + if (int_stat < 0) > > + return IRQ_NONE; // a bus error, reading register not possible > > ...and then... > > + if (int_stat == 0x0) > > + // interrupt sources were 0, so IRQ not from our sensor > > > > I'm unsure if the first IRQ_NONE here is actually correct. I mean, if > > the bus is not working, > > actually any IRQ usage should be considered broken. Is there a way to > > break out of measuring? > > Here actually, I merged them, when returning IRQ_NONE in both statements. I mean, if the bus is broken, the sensor driver then is anyway not operational anymore. It cannot process interrupts anymore be it from someone else or own ones. When the interrupt source register was 0, then the interrupt was not ours for sure. As I understand, if such happens, there will be no more IRQs anyway as long as interrupt source reg and fifo are not cleaned up. > It is a much debated thing on what you should return if you have no > idea if it is our interrupt or not. There isn't really a right > answer. If you get a lot of IRQ_NONE and no one else claims it eventually > the interrupt will be disabled (to break the interrupt storm freezing the > machine). > > > > > > + goto err; > > > > + > > > > + if (int_stat & ADXL345_INT_OVERRUN) > > > > + goto err; > > > > + > > > > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > > > > > > I think you only ever enable the INT_WATERMARK? If so does it > > > make sense to check for DATA_READY as well? > > > > > > > Watermark comes usually with data ready or overrun. I guess for the > > FIFO watermark, just evaluating watermark is probably sufficient. For > > other events, it then might be notification w/ a data ready set. > > Probably better to introduce data ready when it's actually really > > needed? > > Yes. That dataready is normally used when you are doing capture without > the fifo and want to read each sample - kind of same as a watermark depth > of 1, but less hardware turned on. As such, may be no need to ever support it. > > return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > > > > > > > + if (st->irq > 0) { > > > > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > > > > + > > > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > > > > + IRQF_SHARED | IRQF_ONESHOT, > > > > + indio_dev->name, indio_dev); > > > > + if (ret) > > > > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > > > > + > > > > + } else { > > > > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > > > > + > > > Given you haven't removed this code from elsewhere, was the driver relying > > > on the defaults after reset before this patch? > > > > > > I don't mind having this branch as a form of documentation even if that's > > > true but maybe add a note to the patch description. > > > > > > > I'm not sure if I get you correctly. The driver before only > > implemented "BYPASS" mode. This was the case w/o a defined > > interrupt-name. My intention now is to keep this behavior as fallback. > > If no IRQ is around, i.e. interrupt + interrupt-name, the sensor > > driver will operate the sensor in BYPASS mode. > > > > I was interpreting this also as the "default behavior", you mentioned > > in the dt-binding patch? Is this correct? > > > > What do you mean when the driver is relying on the defaults after reset? > > The driver worked without irq before now. Which is same as this path. > So how was the register written here being configured correctly before > this patch? I'm guessing it wasn't and that the value written here > is the default on power up. > > Either that or I'm miss understanding what this branch of the if / else is > about. Well, I'm pretty convinced the sensor before was operated in FIFO_BYPASS mode. TBH I'm not sure now if this is default setting, or explicitely configured. But I remember that also from somewhere in the datasheet. That's why I used this as fallback. > Jonathan > > > > > > > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > > > > + > > > > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > > > return devm_iio_device_register(dev, indio_dev); > > > > } > > > > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); > > > > > >
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c index 3067a70c54e..58ed82d66dc 100644 --- a/drivers/iio/accel/adxl345_core.c +++ b/drivers/iio/accel/adxl345_core.c @@ -15,15 +15,28 @@ #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> +#include <linux/iio/buffer.h> +#include <linux/iio/events.h> +#include <linux/iio/kfifo_buf.h> #include "adxl345.h" +#define ADXL345_FIFO_BYPASS 0 +#define ADXL345_FIFO_FIFO 1 +#define ADXL345_FIFO_STREAM 2 + +#define ADXL345_DIRS 3 + struct adxl345_state { int irq; const struct adxl345_chip_info *info; struct regmap *regmap; + __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE]; bool fifo_delay; /* delay: delay is needed for SPI */ u8 intio; + u8 int_map; + u8 watermark; + u8 fifo_mode; }; #define ADXL345_CHANNEL(index, reg, axis) { \ @@ -36,6 +49,13 @@ struct adxl345_state { .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ BIT(IIO_CHAN_INFO_SAMP_FREQ), \ .scan_index = (index), \ + .scan_type = { \ + .sign = 's', \ + .realbits = 13, \ + .storagebits = 16, \ + .shift = 0, \ + .endianness = IIO_LE, \ + }, \ } enum adxl345_chans { @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = { ADXL345_CHANNEL(2, chan_z, Z), }; +static int adxl345_set_interrupts(struct adxl345_state *st) +{ + int ret; + unsigned int int_enable = st->int_map; + unsigned int int_map; + + /* Any bits set to 0 in the INT map register send their respective + * interrupts to the INT1 pin, whereas bits set to 1 send their respective + * interrupts to the INT2 pin. The intio shall convert this accordingly. + */ + int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map); + + ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map); + if (ret) + return ret; + + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable); +} + static int adxl345_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -133,6 +172,31 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, return -EINVAL; } +static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value) +{ + struct adxl345_state *st = iio_priv(indio_dev); + unsigned int fifo_mask = 0x1F; + int ret; + + if (value == 0) { + st->int_map &= ~ADXL345_INT_WATERMARK; + return 0; + } + + if (value > ADXL345_FIFO_SIZE) + value = ADXL345_FIFO_SIZE; + + ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL, + fifo_mask, value); + if (ret) + return ret; + + st->watermark = value; + st->int_map |= ADXL345_INT_WATERMARK; + + return 0; +} + static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, long mask) @@ -188,11 +252,224 @@ static const struct attribute_group adxl345_attrs_group = { .attrs = adxl345_attrs, }; +static int adxl345_set_fifo(struct adxl345_state *st) +{ + u8 fifo_ctl; + int ret; + + /* FIFO should only be configured while in standby mode */ + ret = adxl345_set_measure_en(st, false); + if (ret < 0) + return ret; + + fifo_ctl = ADXL345_FIFO_CTL_SAMLPES(st->watermark) | + ADXL345_FIFO_CTL_TRIGGER(st->intio) | + ADXL345_FIFO_CTL_MODE(st->fifo_mode); + + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); + if (ret < 0) + return ret; + + return adxl345_set_measure_en(st, true); +} + +/** + * adxl345_get_samples() - Read number of FIFO entries. + * @st: The initialized state instance of this driver. + * + * The sensor does not support treating any axis individually, or exclude them + * from measuring. + * + * Return: negative error, or value. + */ +static int adxl345_get_samples(struct adxl345_state *st) +{ + unsigned int regval = 0; + int ret; + + ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, ®val); + if (ret < 0) + return ret; + + return 0x3f & regval; +} + +/** + * adxl345_fifo_transfer() - Read samples number of elements. + * @st: The instance of the state object of this sensor. + * @samples: The number of lines in the FIFO referred to as fifo_entry, + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. + * + * It is recommended that a multiple-byte read of all registers be performed to + * prevent a change in data between reads of sequential registers. That is to + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. + * + * Return: 0 or error value. + */ +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples) +{ + size_t count; + int i, ret; + + count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS; + for (i = 0; i < samples; i++) { + ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE, + st->fifo_buf + (i * count / 2), count); + if (ret < 0) + return ret; + } + return ret; +} + +/** + * adxl345_fifo_reset() - Empty the FIFO in error condition. + * @st: The instance to the state object of the sensor. + * + * Read all elements of the FIFO. Reading the interrupt source register + * resets the sensor. + */ +static void adxl345_fifo_reset(struct adxl345_state *st) +{ + int regval; + int samples; + + adxl345_set_measure_en(st, false); + + samples = adxl345_get_samples(st); + if (samples > 0) + adxl345_fifo_transfer(st, samples); + + regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); + + adxl345_set_measure_en(st, true); +} + +static int adxl345_buffer_postenable(struct iio_dev *indio_dev) +{ + struct adxl345_state *st = iio_priv(indio_dev); + int ret; + + ret = adxl345_set_interrupts(st); + if (ret < 0) + return ret; + + st->fifo_mode = ADXL345_FIFO_STREAM; + return adxl345_set_fifo(st); +} + +static int adxl345_buffer_predisable(struct iio_dev *indio_dev) +{ + struct adxl345_state *st = iio_priv(indio_dev); + int ret; + + st->int_map = 0x00; + + ret = adxl345_set_interrupts(st); + if (ret < 0) + return ret; + + st->fifo_mode = ADXL345_FIFO_BYPASS; + return adxl345_set_fifo(st); +} + +static const struct iio_buffer_setup_ops adxl345_buffer_ops = { + .postenable = adxl345_buffer_postenable, + .predisable = adxl345_buffer_predisable, +}; + +static int adxl345_get_status(struct adxl345_state *st) +{ + int ret; + unsigned int regval; + + ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); + if (ret < 0) + return ret; + + return (0xff & regval); +} + +static int adxl345_fifo_push(struct iio_dev *indio_dev, + int samples) +{ + struct adxl345_state *st = iio_priv(indio_dev); + int i, ret; + + if (samples <= 0) + return -EINVAL; + + ret = adxl345_fifo_transfer(st, samples); + if (ret) + return ret; + + for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS) { + /* + * To ensure that the FIFO has completely popped, there must be at least 5 + * us between the end of reading the data registers, signified by the + * transition to register 0x38 from 0x37 or the CS pin going high, and the + * start of new reads of the FIFO or reading the FIFO_STATUS register. For + * SPI operation at 1.5 MHz or lower, the register addressing portion of the + * transmission is sufficient delay to ensure the FIFO has completely + * popped. It is necessary for SPI operation greater than 1.5 MHz to + * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us + * at 5 MHz operation. + */ + if (st->fifo_delay && (samples > 1)) + udelay(3); + + iio_push_to_buffers(indio_dev, &st->fifo_buf[i]); + } + + return 0; +} + +/** + * adxl345_event_handler() - Handle events of the ADXL345. + * @irq: The irq being handled. + * @p: The struct iio_device pointer for the device. + * + * Return: The interrupt was handled. + */ +static irqreturn_t adxl345_event_handler(int irq, void *p) +{ + struct iio_dev *indio_dev = p; + struct adxl345_state *st = iio_priv(indio_dev); + u8 int_stat; + int samples; + + int_stat = adxl345_get_status(st); + if (int_stat < 0) + return IRQ_NONE; + + if (int_stat == 0x0) + goto err; + + if (int_stat & ADXL345_INT_OVERRUN) + goto err; + + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { + samples = adxl345_get_samples(st); + if (samples < 0) + goto err; + + if (adxl345_fifo_push(indio_dev, samples) < 0) + goto err; + + } + return IRQ_HANDLED; + +err: + adxl345_fifo_reset(st); + + return IRQ_HANDLED; +} + static const struct iio_info adxl345_info = { .attrs = &adxl345_attrs_group, .read_raw = adxl345_read_raw, .write_raw = adxl345_write_raw, .write_raw_get_fmt = adxl345_write_raw_get_fmt, + .hwfifo_set_watermark = adxl345_set_watermark, }; /** @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | ADXL345_DATA_FORMAT_FULL_RES | ADXL345_DATA_FORMAT_SELF_TEST); + u8 fifo_ctl; int ret; indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, if (ret < 0) return dev_err_probe(dev, ret, "Failed to add action or reset\n"); + if (st->irq > 0) { + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); + + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); + if (ret) + return ret; + + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, + IRQF_SHARED | IRQF_ONESHOT, + indio_dev->name, indio_dev); + if (ret) + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); + + } else { + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); + + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); + + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); + if (ret < 0) + return ret; + } return devm_iio_device_register(dev, indio_dev); } EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
Add a basic setup for FIFO with configurable watermark. Add a handler for watermark interrupt events and extend the channel for the scan_index needed for the iio channel. The sensor is configurable to use a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now a watermark can be configured, or disabled by setting 0. Further features require a working FIFO setup. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++ 1 file changed, 300 insertions(+)