Skip to content

Koror ram capture support - #3448

Open
stefpopa wants to merge 2 commits into
mainfrom
staging/koror_ram_capture
Open

Koror ram capture support#3448
stefpopa wants to merge 2 commits into
mainfrom
staging/koror_ram_capture

Conversation

@stefpopa

Copy link
Copy Markdown
Collaborator

PR Description

  • Add debugfs interface for quick single-shot RX/ORx RAM captures
  • Add full IIO device (adrv904x-ramc) with I/Q channel support and buffer infrastructure

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have compiled my changes, including the documentation
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly
  • I have provided links for the relevant upstream lore

@github-actions

Copy link
Copy Markdown
Contributor

LLM review

This series adds a debugfs interface and a dedicated IIO device (adrv904x-ramc) for
triggering RX/ORx capture-RAM snapshots on the ADRV904X transceiver.

run: 29838719279

dff75231057fb - Add IIO device for RX/ORx RAM capture

Build failure: adrv904x_ramc_probe() calls devm_iio_kfifo_buffer_setup_ext(), but
ADRV904X in drivers/iio/trx-rf/Kconfig only selects CF_AXI_ADC (which pulls in
IIO_BUFFER/IIO_BUFFER_HW_CONSUMER/IIO_BUFFER_DMAENGINE, not IIO_KFIFO_BUF). Reproduced
with set_arch gcc_arm + auto_set_kconfig + make:

ld: vmlinux.o: in function `adrv904x_ramc_probe':
undefined reference to `devm_iio_kfifo_buffer_setup_ext'

Adding select IIO_KFIFO_BUF fixes the link, and a full ARCH=arm build then succeeds.

Data-correctness bug: adrv904x_ramc_capture_and_push() always splits the capture
buffer into two equal halves (offset = word_count / 2) to demux I/Q data. That matches the
RX/DPD capture RAM layout (adrv904x_datainterface.c: bankSize = wordCount >> 1, comment
"dpd capture memory 2 banks 8k each"), but the ORx capture RAM uses a different, fixed
layout (comment "ORx capture ram 3 banks 4k each": up to three fixed 4K-word banks depending
on the requested size). The halved split is bank-aligned for ORx only when
word_count == 8K; for every other ORx-valid sample_count (32-4096, or 12288) the demux
mixes data from the wrong physical banks and silently returns corrupted I/Q data on the
ORX0/ORX1 channels, without any error being reported to userspace.

CI warnings

The gcc_fanalyzer/clang_analyzer warnings for the dead-store struct device *dev in
adrv904x.c (lines ~1599-1948) all trace back to commit cb241a9d869ab, which predates this
review range, so they are pre-existing and not introduced by this series.

Verification

  • Built with set_arch gcc_arm + auto_set_kconfig + make -j$(nproc) -C . O=build;
    reproduced the devm_iio_kfifo_buffer_setup_ext link error, then confirmed a
    select IIO_KFIFO_BUF fix produces a complete arch/arm/boot/zImage.
  • Read adrv904x_datainterface.c's private adrv904x_RxOrxDataCaptureRead() to confirm the
    RX/DPD vs. ORx bank-layout difference (no ADRV904X hardware available to reproduce the
    ORx corruption at runtime).
  • checkpatch.pl is clean on both commits, aside from the generic "does MAINTAINERS need
    updating?" notice, which is not actionable (this driver has no MAINTAINERS entry at all).
  • Built the two touched files with W=1: no new warnings introduced by this series.

Suggested patches

Two --fixup=dff75231057fb commits, verified to git apply --check cleanly and to build
successfully for ARCH=arm:

  1. select IIO_KFIFO_BUF in drivers/iio/trx-rf/Kconfig.
  2. Restrict ORx sample_count to 8K (the only value for which the current 2-bank demux is
    correct) at sysfs-store time and defensively inside the capture path, instead of silently
    returning garbled data. Proper multi-bank ORx demux is left to the driver author to
    implement and validate against real hardware.

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 29838719279
Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh"      -o ~/.local/bin/apply-patches.sh &&   grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

@stefpopa
stefpopa force-pushed the staging/koror_ram_capture branch from dff7523 to 2b7e61a Compare July 22, 2026 09:16
Add a debugfs interface for triggering single-shot RX/ORx data captures
from the ADRV904x capture RAM. This provides a simple mechanism for
debugging and signal analysis without requiring full IIO buffer setup.

The rx_data_capture debugfs file accepts channel and capture length:
  echo "<channel> <length>" > rx_data_capture
  cat rx_data_capture

Channel mapping: 0-7 for RX0-RX7, 8-9 for ORX0-ORX1.
Capture length must be a hardware-supported size (32 to 32768 samples).
ORX channels are limited to 12288 samples maximum.

The captured data is stored as raw 32-bit values (one per line) and
contains interleaved I/Q samples in hardware format.

Memory allocation is performed before taking the device mutex to avoid
sleeping with locks held.

Signed-off-by: Stefan Popa <stefan.popa@analog.com>
@stefpopa
stefpopa force-pushed the staging/koror_ram_capture branch from 2b7e61a to 104691a Compare July 31, 2026 06:03
Add a dedicated IIO device (adrv904x-ramc) for RX/ORx data capture with
proper I/Q channel representation and standard IIO buffer interface.

Features:
- 10 channel pairs (RX0-7, ORX0-1) with I/Q modifiers
- Channels appear as voltage0_i/voltage0_q through voltage9_i/voltage9_q
- Hardware constraint enforced via available_scan_masks: only one I/Q
  pair can be captured at a time
- Single-shot capture: enabling the buffer triggers one capture that
  fills the buffer, then stops. This matches the hardware behavior
  (capture RAM is a snapshot mechanism, not continuous streaming)
- Configurable capture size via sample_count attribute (32-32768)
- 28-bit two's complement data using sign_extend32()
- Bank interleaving and Q negation per hardware specification

The single-shot behavior is intentional: continuous RX data is available
through the JESD streaming interface (axi-adrv904x-rx-hpc). RAM capture
is designed for debugging, calibration verification, and signal analysis
where point-in-time snapshots are needed.

Usage with iio_readdev:
  iio_readdev -s 1024 adrv904x-ramc voltage0_i voltage0_q > capture.bin

Signed-off-by: Stefan Popa <stefan.popa@analog.com>
@stefpopa
stefpopa force-pushed the staging/koror_ram_capture branch from 104691a to f066077 Compare July 31, 2026 06:12

@nunojsa nunojsa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said, not sure I'm convinced about the debugfs added value but if you choose to keep it and, as a nit, swap the patches. Make the real feature (the iio buffer) be the first patch and the debug thing a follow up. Makes more sense to me


ret = devm_add_action(&spi->dev, adrv904x_free_capture_data, phy);
if (ret)
return ret;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely be in adrv904x_ramc_probe(). Another oddity is why aren't we using devm_add_action_or_reset()?


/* Scan buffer: I + Q + timestamp */
u8 scan_data[ALIGN(2 * sizeof(s32), sizeof(s64)) + sizeof(s64)]
__aligned(IIO_DMA_MINALIGN);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to the last in the struct! Also please check if we have this helpers:

https://elixir.bootlin.com/linux/v7.2-rc5/source/include/linux/iio/iio.h#L935

If not, might be worth it to backport them (if easy enough)

* re-enable the buffer (or use iio_readdev which handles this automatically).
*/

#include "adrv904x.h"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go after the "system" includes

.write = adrv904x_ramc_sample_count_store,
.shared = IIO_SHARED_BY_ALL,
},
{},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a space and not need for comma here

i_val = sign_extend_28bit(bank1[2 * i + 1]);
scan_s32[0] = i_val;
scan_s32[1] = q_val;
iio_push_to_buffers_with_timestamp(st->indio_dev,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dev_err(st->dev, "Failed to capture channel 0x%lx: %d\n",
active_channel, ret);
return -EIO;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to lock the whole thing or just adi_adrv904x_RxOrxDataCaptureStart(). This also makes me wonder if we shouldn't just use the device lock for the the whole ram device

/* Allocate buffer: up to 12 chars per value (including newline) */
buf_size = phy->rx_capture_len * 12 + 1;
out_buf = kvzalloc(buf_size, GFP_KERNEL);
if (!out_buf)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use __free(kvfree) and avoid some leaks. Also what sizes are talking about here? IIRC, kvalloc() is for things in the megabyte range

result = simple_read_from_buffer(userbuf, count, ppos, out_buf, pos);
kvfree(out_buf);
return result;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer to move this into an helper function

}

kvfree(phy->rx_capture_data);
phy->rx_capture_data = capture_buf;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we have phy->rx_capture_data. So the action in the other patch is just wrong (needs to be in the same patch as debugfs). Also not sure if all of this handling in terms of allocation makes the debugfs thing worth it! What's the big advantaged when compared with using IIO buffering?

Another side note is maybe kvrealloc() would also make this simpler?

@nunojsa nunojsa mentioned this pull request Aug 5, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llm review Request a review from a LLM Reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants