Skip to content

Staging/xlnx/ad9084 cfir sparse mode support#3254

Merged
nunojsa merged 5 commits intomainfrom
staging/xlnx/ad9084-cfir-sparse-mode-support
Apr 24, 2026
Merged

Staging/xlnx/ad9084 cfir sparse mode support#3254
nunojsa merged 5 commits intomainfrom
staging/xlnx/ad9084-cfir-sparse-mode-support

Conversation

@mhennerich
Copy link
Copy Markdown
Contributor

PR Description

Two commits touching ad9088_parse_cfilt() (the parser behind the cfir_config sysfs bin attribute):

  1. Fix CFIR parser bugs and document blob format
    - enable: and coeff_transfer: both claimed BIT(6) in read_mask, silently dropping one when both were present and running the enable-path with uninitialized locals. Moved enable: to BIT(8) and widened read_mask to u16.
    - gain: was read into a u32 via %d, breaking negative-dB values. Use a dedicated s32.
    - Bound %s conversions in enable: / selection_mode: (cfir + pfilt) to %15s.
    - Rewrote the kernel-doc to enumerate every directive, its value set, the read_mask bit, the Apollo API it feeds, and the post-parse programming order.
  2. Add CFIR sparse-mode programming
    - The blob accepted sparse_filt_en but never called adi_apollo_cfir_sparse_coeff_sel_pgm() / adi_apollo_cfir_sparse_mem_sel_pgm(), so sparse mode silently used stale HW state.
    - When sparse_filt_en: 1 precedes the coefficient block, each tap line is now (I/Q columns unchanged, hsel 0..63 appended); exactly 16 taps required.
    - New optional sparse_mem_sel: directive (each 0..3) for explicit Dstore0/2/5/7 spacing.
    - Programming sequence mirrors the Apollo fullchip_sparse_cfir() reference.
    - kdoc documents the Dstore/hsel model and the UG §6389 hsel=15/31/47/63 limitation.

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)

Three small correctness fixes in ad9088_parse_cfilt() plus a thorough
kernel-doc rewrite so the cfir_config sysfs blob format is actually
discoverable:

  - enable: and coeff_transfer: both claimed BIT(6) in read_mask, so
    a blob containing both silently dropped one of them and ran the
    enable-path with uninitialized enable/enable_profile. Move enable
    to BIT(8) and widen read_mask to u16.
  - gain: was parsed into a u32 via %d, breaking negative-dB values
    that ad9088_cfir_gain_enc() expects (-18/-12/-6). Use a dedicated
    s32.
  - Bound the %s conversions in "enable: " and "selection_mode: " to
    the destination buffer size (%15s). Also applied to the matching
    pfilt parser.

The new kdoc enumerates every directive, its value set, the bit it
claims in read_mask, the Apollo API call it feeds, and the post-parse
programming order.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
@github-actions
Copy link
Copy Markdown
Contributor

Automated review run: https://github.com/analogdevicesinc/linux/actions/runs/24449775287


This series (6dacbb616..3bc9ff877) fixes parser bugs in the AD9088 CFIR/PFILT filter config blobs and adds CFIR sparse-mode programming. Two of the three commits are correct; one has an ordering bug.

6dacbb616 — Fix CFIR parser bugs

All three fixes look correct:

  • BIT(6) collision between enable: and coeff_transfer:enable: moved to BIT(8), read_mask widened to u16
  • Negative dB gain (-18/-12/-6) silently lost through u32 → dedicated s32 gain_db
  • Unbounded %s in enable: / selection_mode:%15s

⚠️ 5e42d5eb5 — Add CFIR sparse-mode programming

One ordering bug: the sparse coefficient count is validated after adi_apollo_cfir_coeff_pgm() has already been called with uninitialized array tail entries.

adi_apollo_cfir_coeff_pgm() always programs exactly 16 taps regardless of the len argument (verified in public/src/adi_apollo_cfir.c:66). If fewer than 16 <i> <q> <hsel> lines are provided in sparse mode, uninitialized stack garbage is written to hardware before the count check fires:

// ad9088.c — coeff_pgm runs FIRST, with uninitialized cfir_coeff_i/q[i..15]
ret = adi_apollo_cfir_coeff_pgm(..., cfir_coeff_i, cfir_coeff_q, i);
if (ret < 0)
    goto out1;

if (cfir_pgm.cfir_sparse_filt_en) {
    if (i != ADI_APOLLO_CFIR_COEFF_NUM) {  // ← too late
        ...
        goto out1;  // ← still returns 'size' (success) to userspace

Additionally, sscanf(line, "sparse_mem_sel: %i %i %i", &m0, &m1, &m2) uses %i with u32 * (UB for negative input per C standard); same for hsel. Should be %u/%u respectively.

A fixup patch is suggested (attached to this run):

fixup! iio: trx-rf: ad9088: Add CFIR sparse-mode programming

- Move sparse coefficient count check BEFORE adi_apollo_cfir_coeff_pgm()
  and use goto out (returns -EINVAL) instead of goto out1 (returns size).
- Fix sparse_mem_sel: scanner from %i to %u for unsigned m0/m1/m2.
- Fix hsel scanner from %i %i %i to %i %i %u for unsigned hsel.

Patch stored at: /tmp/tmp.rhzMn4knoq/0001-fixup-iio-trx-rf-ad9088-Add-CFIR-sparse-mode-program.patch

3bc9ff877 — Fix PFILT parser bugs

Both fixes are correct:

  • pfilt_mask%u / bank_mask%u now scanned from pre-extracted substrings p/b instead of the full line
  • Missing dest: now returns -EINVAL (matching CFIR parser) ✅

CI Warnings

All clang_analyzer warnings (uninitialized val at lines 1573/1599, dead stores at 1564/1569/1582/1593) and coccicheck unneeded-semicolon warnings at lines 64/65 are pre-existing and not introduced by this series. The checkpatch subject-line warning for commit 5e42d5eb5 is a false positive — the subject describes the feature, not a tool.

Build verified clean with gcc_aarch64 and llvm_x86 for ad9088.c.

The cfir_config sysfs blob accepts sparse_filt_en but never programs
the sparse-mode control values, so enabling sparse mode leaves hsel
and mem_sel at stale HW/profile defaults and the filter silently
produces garbage.

Extend ad9088_parse_cfilt() to:

  - switch the coefficient-line scanner to "<i> <q> <hsel>" when
    sparse_filt_en: 1 has already been parsed (I/Q columns stay in the
    same position as the non-sparse layout, hsel 0..63 appended);
  - accept an optional "sparse_mem_sel: <m0> <m1> <m2>" directive
    (each 0..3) for explicit Dstore0/2/5/7 spacing;
  - require exactly ADI_APOLLO_CFIR_COEFF_NUM (16) taps in sparse mode;
  - push the values down via adi_apollo_cfir_sparse_coeff_sel_pgm()
    and adi_apollo_cfir_sparse_mem_sel_pgm() right after the ordinary
    coefficient load, before cfir_pgm() flips the sparse-enable bit.

The kdoc now documents both the Dstore/hsel model
and the UG sec. 6389 limitation on hsel=15/31/47/63 when Dstores are
non-contiguous.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Two real fixes plus a thorough kernel-doc rewrite for the pfilt_config
sysfs blob format:

  - The pfilt_mask / bank_mask forms in the "dest:" directive scanned
    the full line ("dest: rx pfilt_maskN bank_X") with a format string
    anchored at "pfilt_mask%u" / "bank_mask%u", so the mask values never
    matched and whatever stale value was in the local was used instead.
    Scan the pre-extracted pfilt/bank substrings.
  - When "dest:" was absent the debug prints and the two Apollo API
    calls that followed ran with uninitialized terminal/pfilt_sel/
    bank_sel. Error out with -EINVAL like the CFIR parser does.

The new kdoc enumerates every directive and its value set, maps the
gain slots to the UG matrix-mode Ga/Gb/Gc/Gd labelling, documents the
scalar_gain encoding, explains when mode_switch / hc_delay apply, lists
the per-mode coefficient-count requirement, and spells out the
selection_mode variants (including the 2-GPIO config-0/config-1 split
that CFIR lacks).

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
@mhennerich mhennerich force-pushed the staging/xlnx/ad9084-cfir-sparse-mode-support branch from 3bc9ff8 to 81d51e8 Compare April 15, 2026 12:48
@mhennerich
Copy link
Copy Markdown
Contributor Author

Fixed:

  • Move sparse coefficient count check BEFORE adi_apollo_cfir_coeff_pgm()
    and use goto out (returns -EINVAL) instead of goto out1 (returns size).

Did not fix:

  • Fix sparse_mem_sel: scanner from %i to %u for unsigned m0/m1/m2.
  • Fix hsel scanner from %i %i %i to %i %i %u for unsigned hsel.

%i parses decimals as well as hex values - that's what we need.

Copy link
Copy Markdown
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

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

Just one remark from me

}
} else {
ret = sscanf(line, "%i %i", &sval_i, &sval_q);
if (ret == 2) {
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.

Should we ignore errors? Same for all the abov e

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look

Replace generic "malformed filter file" messages with specific per-
directive errors that name the offending keyword/value. Route all Apollo
API return values through ad9088_check_apollo_error() for consistent
human-readable error strings and proper errno mapping.

Key changes:
- Validate sysfs_match_string() results before storing (PFILT mode:
  previously stored negative returns as enum values)
- Report which directive and value failed (e.g. "dest: unknown terminal
  'foo'", "mode: unknown Q mode 'bar'")
- Log coefficient overflow with the max count
- API failures now return error codes instead of silently returning
  size (success)
- Remove goto out/out1 spaghetti in favour of direct returns
- Skip empty lines alongside comment lines
- Move selection_mode debug print inside the BIT(9) guard to avoid
  printing uninitialized data
- Capture return value of adi_apollo_cfir_profile_sel() which was
  previously unchecked

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Change ad9088_iiochan_to_cfir() from void to int so callers can detect
failure. Previously, if ad9088_get_chan_map() returned NULL or the
switch hit the default case, the output parameters (terminal, cfir_sel,
dp_sel) were left uninitialized and passed straight into Apollo API
calls — flagged by clang's core.CallAndMessage checker.

Return -EINVAL on both the NULL-map and unhandled-FDDC paths, and check
the return value at all four call sites.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
@nunojsa nunojsa merged commit 760ed29 into main Apr 24, 2026
32 checks passed
@nunojsa nunojsa deleted the staging/xlnx/ad9084-cfir-sparse-mode-support branch April 24, 2026 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants