Adxl345 refac and FIFO

[Rubusch]

TL;DR

I tried to refac this driver, fix some of the FIFO related issues and introduce an approach to cover trigger/fetch&get and stream/RTIO bus access.

Long story

In my opinion, the ADXL345 driver in Zephyr still has room for improvement. Having worked on parts of the ADXL345 driver for the Linux kernel, I found it a familiar starting point when getting into Zephyr.

Current State of the ADXL345 Driver in Zephyr:

FIFO Mode Configuration

The driver currently (at least) offers setting up Analog Devices' FIFO_TRIGGER mode. However, this appears to be a misunderstanding of its purpose, especially when viewed in the context of Zephyr’s Trigger API versus a Stream/RTIO-based approach. It's important to note that Analog Devices did not define the FIFO_TRIGGER bit with Zephyr’s trigger model in mind - Analog's FIFO_TRIGGER serves a different function entirely. For typical usage in Zephyr, starting with either FIFO_STREAM or FIFO_BYPASSED modes would be more appropriate, in my opinion.

Broken FIFO Handling

FIFO functionality has been broken for several months and remains so. Currently, a FIFO watermark event is triggered once, but the driver only fetches the most recent measurement(s), rather than reading out the entire FIFO buffer. As a result, no further watermark interrupts are generated after the initial one, since the FIFO is never properly emptied.

Additional Issues

The FIFO watermark should ideally trigger at 2 stored samples, not just 1.

The current FIFO flush mechanism relies on the process described for FIFO_TRIGGER mode in the datasheet. However, when using FIFO_STREAM mode - as recommended - the FIFO should be flushed by reading out all entries manually, as per the datasheet.

Initialization in stream/RTIO mode uses direct bus writes, which makes later fetch/get operations or RTIO usage somewhat redundant. A better approach would be to consolidate these accesses under shared functions that abstract the bus type (RTIO or plain bus).

For register updates where only a few bits need changing, using an _update_bits() function with cached register values could reduce bus traffic. Since configuration registers are not expected to change externally, this kind of caching (similar to the regmap cache mechanism in Linux) could be beneficial. I'm not sure if Zephyr has a general caching mechanism like this, but it could be worth considering.

General Refactoring Needed

Overall, the driver would benefit from a thorough refactoring. There are inconsistencies in naming, instances of dead code, and unnecessarily complex implementations - likely due to the driver evolving without consistent maintenance. In my view, many of these parts are no longer robust or maintainable and need cleanup.

@MaureenHelm : I had a chat with you as the Analog Devices maintainer during OSS in Amsterdam this year. I'm not sure if you remember, anyway you had suggested that I first submit the full series as-is, so that we could later decide how to proceed—whether to split, squash, accept, or drop parts of it. Since it's generally easier to squash commits than to separate them after the fact, I tried to break the series down as much as possible from the beginning. That’s why you're now seeing over 40 commits. I’m also aware that some maintainers prefer this level of granularity.

Please let me know if this approach is excessive or if it's aligned with your expectations. All commits build successfully and pass strict checkpatch. For verification, I used my own test applications for fetch&get, device handler registration, and stream/RTIO integration, as the existing samples didn’t provide enough coverage in those areas. Does the set - all or in pieces - qualify for upstreaming? What should be squashed? What is missing? What needs to be improved? Actually I pretend writing drivers for similar accelerometers by this kind of approach. So, this is kind of a proposal to get a bit into discussion, too.

In a summary of the main topics of this series, I'd like to address:

• refac: comments, simplify variable names, adjust variable types, removal of dead code, remove redundant code

• refac: control measurement on/off by dedicate function and mech

• refac: separate function get_status() into get_fifo_entries() and get_status() for interrupt status

• refac: introduce an api to use common function calls diverting to use underlying either triggered/fetch&get or stream/RTIO implementation

• refac: make interrupt lines configurable over devicetree, and remove rechecking over and over again in the running code for such constant setting

• refac: simplify overly complicated ODR setup, as then also g ranges

• refac: stream handler registration

• fix: change minimum watermark to 2

• fix: stream initialization using direct register write, while partly using RTIO

• fix: stream flush FIFO performs the action documented for reset Analog Device's trigger mode (switch to FIFO BYPASS and back), in Analyg Device's FIFO STREAM mode, entries need actually to be read, to reset fifo status register and interrupt status register (would come much more into play when implementing sensor events)

• fix: fetch&get using FIFO did actually fetch the recent measurements, but did not read out fifo entries

• fix: rework FIFO mode setup - Analog Device's "FIFO TRIGGER" mode seems to be confused with Zephyr's "triggered" config?

• fix/add: let FIFO watermark being configured also dynamically by attribute for fetch&get

• fix/add: overrun and data ready handler, optionally to be overwritten in application

NB: I used AI to elaborate code comments and improve the English descriptions, simply because I'm not a native speaker. I did not use AI / Copilots or the like for sources.

[sonarqubecloud]

Quality Gate passed

Issues
17 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ubieda]

Boy, going over 42 patches was painful, and I can only say this is a first-pass review.

I agree this driver could use some love, and the fact that you took the time to go over the whole thing and make this number of patches (+ filing and re-opening multiple PRs for this driver) says a lot of your interest in contributing/improving this driver. I appreciate it and I hope we can channel it in helping you land a contribution.

However, in order to do that, we need to objectivize the conversation so we filter out the noise. I see read statements both in the body of this PR and in some patches that are stale, unclear or just not true. For instance:

• This PR says the FIFO has been broken for months, yet I see no Bugs filed, and I happen to have used it in the last 6-months (although I will admit it has been only in prototyping). In what way is this driver broken?
• The PR states a burst readout is performed, but it's not implemented (nor I think is possible AFAIK).

I also get a bit of NIH in some of these changes (for instance, changing the way the int1-gpios, int2-gpios dts-props map the corresponding pin to use, or by saying FIFO_WATERMARK=1 is an issue). It's okay to rename variables or reformat code so it's cleaner or more concise/readable, but let's call these things by their name; otherwise we'll get confused and/or look at the other changes (which do objectively provide value) with a hesitant mindset.

All in all, my suggestions to get this moving forward:

1. Simplify the "story" of this PR so it's easier for us to follow-along: What is it fixing, what is a new "feature" and what are just style improvements, nitpicks or formatting changes.
2. Definitely suggest bringing the patch-count down a bit: I'm positive you can summarize some of these changes in less than 20 patches.
3. If you have any questions or would like to discuss specific details in a more direct fashion, feel free to reach out in Discord or join the Sensor WG meetings (every other monday).

[ubieda]

This is merely a way to handle an use-case where both are defined: an alternative is to add a BUILD_ASSERT to prevent both being present. The point is that this driver routes interrupts through one pin, and not both.

I disagree that adding another property to decide which is used when both are defined is better than what's already implemented.

[ubieda]

Is setting fifo-watemark = <1>; an invalid or unsupported configuration by the sensor? I don't understand the value of limiting the driver, beyond its capabilities; irrespective of having a clear-cut use-case.

[ubieda]

I don't see this being helpful, when this can already be inferred based on the existence of either int1-gpios or int2-gpios

[ubieda]

Make instead odr a required dt-property. Let's not double-down on the already questionable arbitrary 25-Hz ODR choice.

[ubieda]

why?

[ubieda]

sensor: adxl345: fix fetch and get to correctly read the FIFO samples
In Analog Devices' FIFO modes (e.g. STREAM), samples are stored in the
sensor's internal buffer, and a watermark interrupt signals when enough
samples are ready. The driver should then fetch all available samples in a
burst read.

Currently, the driver handles interrupts but reads only a single sample
each time, regardless of FIFO mode. This effectively reduces all modes
(FIFO, TRIGGER, STREAM) to BYPASS behavior. Additionally, fetch operations
only returned the last sample repeatedly instead of the full FIFO contents.

This commit restores proper FIFO operation by converting sample storage
back to an array, but fixing the fetch logic to read and return all
configured FIFO entries.

Two things:

1. I don't see the described burst-read change. All I see is code formatted, additional checks and asserts and then the same for-loop performed.
2. Please confirm whether your proposition is even possible: AFAIK - The datasheet specifies the sensor only places the new sample once the previous sample is read. This is the reason why this comment is captured in the driver:
   

   zephyr/drivers/sensor/adi/adxl345/adxl345.c

   Lines 532 to 539 in 6b8fcae

   	/** RTIO SQE/CQE pool size depends on the fifo-watermark because we
   	* can't just burst-read all the fifo data at once. Datasheet specifies
   	* we need to get one frame at a time (through the Data registers),
   	* therefore, we set all the sequence at once to properly pull each
   	* frame, and then end up calling the completion event so the
   	* application receives it).
   	*/
   	#define ADXL345_RTIO_DEFINE(inst) \

RETRIEVING DATA FROM FIFO
The FIFO data is read through the DATAX, DATAY, and DATAZ
registers (Address 0x32 to Address 0x37). When the FIFO is in
FIFO, stream, or trigger mode, reads to the DATAX, DATAY, and
DATAZ registers read data stored in the FIFO. Each time data is
read from the FIFO, the oldest x-, y-, and z-axes data are placed
into the DATAX, DATAY, and DATAZ registers.

[ubieda]

Why from 2X+2 to 4X?

[ubieda]

Isn't iodev_sqe already contained in dev->data? Why are we passing it as another argument?

[ubieda]

I don't think we want to go this ifdefry route. If you want to go the route of everything working over RTIO, I suggest looking into other drivers that define the _bus.h/.c module to abstract this away, OR don't have the fetch-get code depend on RTIO.