sensor: fix IMX415 latching into an IMX335 misdetection via 0x316A

[widgetii]

Problem

A Sony IMX415 on Hi3516AV300 is reported as imx335_i2c. The streamer trusts that answer and brings the sensor up with the IMX335 register table — a 4K sensor forced into a 5MP IMX335 mode.

This is not a regression from #167. That PR only touches the READ(0x3057) == 0x06 → IMX347 branch, and IMX415 reads 0x3057 == 0x00, so the branch is never entered. #167 is verified still working (see below).

Root cause

0x316A is INCKSEL4 (input-clock select), not an identifier, and the check
(r316A & 0xFC) == 0x7C cannot discriminate. Measured on real silicon:

state	0x316A
IMX415, pristine (cold boot)	0x00
IMX415, after libsns_imx335.so ran	0x7E
real IMX335	0x7E

It self-latches. libsns_imx335.so writes 0x316A = 0x7E in both its linear and WDR init tables (sony_imx335/imx335_sensor_ctl.c:238 and :435, commented INCKSEL4 = 37.125). Streamers ask ipctool which sensor is present, then dlopen that vendor lib. So a single misdetection — from any cause — stamps 0x7E into an IMX415, and from then on ipctool keeps agreeing with itself. The loop survives streamer restarts and soft reboots; only removing power clears it. That is why an affected camera works for months and then "suddenly" breaks and stays broken.

Compounding this, the correct IMX415 test (0x3B00) sat below the 0x316A test, so it was unreachable once latched.

Fix

Rule IMX415 out before concluding IMX335. 0x300B is the right discriminator: neither vendor init table writes it, so it is a true reset default on both parts, and it measured 0xA0 on the IMX415 in both the latched and pristine states.

Only reject IMX335 when IMX415 is positively identified — the same idiom #167 established for IMX347 — so no sensor loses detection if a register drifts. The looser 0x3B00-only test is kept below as a fallback.

Evidence

Full 0x3000–0x40FF dumps (4352 registers) from a real IMX415 and a real IMX335, cross-referenced against the register-address sets each vendor init table writes:

reg	IMX415	IMX335	in imx335 init table?	in imx415 init table?
0x316A	0x7E¹	0x7E	YES (0x7E)	no
0x3B00	0x28²	0x00	no	YES (0x2E)
0x300B	0xA0	0x00	no	no
0x30C0	0x2A	0x00	no	no
0x341C	0x47	0xFF	YES	no
0x3072 / 0x3074	0x00 / 0x19	0x28 / 0xB0	YES (WDR only)	no

¹ latched; pristine 0x00 ² 0x2E once the IMX415 driver has run — the code accepts both, which is necessary.

The dropped HINT claiming 0x30C0 == 0x20 on IMX415 is wrong; it measures 0x2A.

Verification on hardware

• Hi3516AV300 + IMX415, still latched — imx335_i2c → imx415_i2c. The fix works without a power cycle, which is the point.
• Hi3516EV300 + IMX335 — imx335_i2c, unchanged. Its 0x3057 reads 0x06, so it genuinely exercises the sensor: fix IMX335 misdetected as IMX347 after WDR cycle (#157) #167 fall-through rather than skipping it.
• Hi3516AV300 after a cold power-cycle — imx415_i2c → imx415_i2c.ini + libsns_imx415.so → IMX415 4K2K_10BIT_30FPS init succuss!, VI pipe 3840x2160 (was 2592x1944 under the wrong driver), IntCnt +80 over 4s = 20 fps, LostFrame 0, VbFail 0.

Note for future sensor work

Any Sony fingerprint register that appears in a vendor init table is a latch hazard, not an identifier — a wrong answer makes the streamer write that table and cement the error. Screening candidates against the init-table address lists leaves 915 registers that separate IMX415 from IMX335 and are written by neither, so there is no shortage of safe choices.

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

sensor: prevent IMX415 misdetecting as IMX335 by avoiding 0x316A latch

🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Prevent IMX415 being misidentified as IMX335 after IMX335 init latches 0x316A.
• Add positive IMX415 identification (0x3B00 + 0x300B) before accepting IMX335.
• Keep a looser IMX415 fallback check once IMX335 has been ruled out.

Diagram

graph TD
  A["detect_sony_sensor()"] --> B[("Sony sensor via I2C")] --> C["READ 0x316A (INCKSEL4)"] --> D["READ 0x3B00 + 0x300B"] --> E{"IMX415 signature?"}
  E -->|"Yes"| F["Skip IMX335; later returns IMX415"]
  E -->|"No"| G{"316A matches IMX335 pattern?"} -->|"Yes"| H["Set sensor_id=IMX335"]
  G -->|"No"| I["Continue other Sony checks"]

  subgraph Legend
    direction LR
    _fn["Function/step"] ~~~ _dec{"Decision"} ~~~ _hw[("Hardware regs")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Reorder checks to test IMX415 before any 0x316A logic

• ➕ Very small code change (pure reordering)
• ➕ Avoids the specific unreachable-branch issue when 0x316A is latched
• ➖ Doesn’t address that 0x3B00 alone is a weaker discriminator
• ➖ Still leaves the IMX335 decision dependent on a latchable register without an explicit guard

2. Multi-register scoring/whitelist discriminator for IMX335 vs IMX415

• ➕ More robust against any single drifting/latchable register
• ➕ Can be extended to future sensors with similar ambiguity
• ➖ More code and maintenance burden
• ➖ Harder to reason about and validate across all supported sensors

Recommendation: Keep the PR’s current approach: positively identify IMX415 using a reset-default register (0x300B) plus the existing 0x3B00 check, then only conclude IMX335 if IMX415 is ruled out. This directly breaks the latch feedback loop while minimizing the chance of losing IMX335 detection due to unrelated register drift.

Files changed (1) +15 / -7

Bug fix (1) +15 / -7

sensors.cGuard IMX335 detection with positive IMX415 signature (0x3B00 + 0x300B) +15/-7

Guard IMX335 detection with positive IMX415 signature (0x3B00 + 0x300B)

• Updates Sony sensor probing to avoid treating 0x316A (INCKSEL4) as a reliable IMX335 identifier. Adds an 'is_imx415' check based on 0x3B00 and 0x300B, and only returns IMX335 when IMX415 is not positively identified. Also adjusts comments and keeps a looser IMX415 fallback once IMX335 has been ruled out.

src/sensors.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. IMX415 misdetect on read error ✓ Resolved 🐞 Bug ☼ Reliability

Description

detect_sony_sensor() treats a failed READ(0xB) (0x300B) as “not IMX415”, so when r316A matches
the IMX335 heuristic it can still return IMX335 even if r3B00 indicates an IMX415. This means
transient I2C read errors can re-trigger wrong-driver selection and the persistent misdetection loop
the PR is intended to stop.

Code

src/sensors.c[R198-203]

+    int r3B00 = READ(0xB00);
+    int is_imx415 = (r3B00 == 0x2E || r3B00 == 0x28) && READ(0xB) == 0xA0;
+
+    if (r316A > 0 && ((r316A & 0xFC) == 0x7C) && !is_imx415) {
      sprintf(ctx->sensor_id, "IMX335");
      return true;

Evidence

The new logic uses READ(0xB) directly inside is_imx415 and then gates IMX335 detection on
!is_imx415; since register reads return -1 on failure, a read error is treated the same as “not
IMX415” and allows the IMX335 branch to win. The detection framework stops on the first true
return and commits ctx->sensor_id, so a single wrong true result here selects the wrong sensor.

src/sensors.c[156-224]
src/sensors.c[1065-1084]
src/sensors.c[1090-1115]
src/hal/hisi/hal_hisi.c[224-242]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`is_imx415` is computed using `READ(0xB) == 0xA0` without checking for the read-error sentinel (`-1`). If the 0x300B read fails, `is_imx415` becomes false and the code may immediately return `IMX335` when `r316A` matches, even in cases where `r3B00` already matches the IMX415 signature.
## Issue Context
In this codebase, sensor register reads return `-1` on I2C failure. Treating `-1` as a normal “not equal” result makes the IMX415 exclusion unreliable under transient I2C errors.
## Fix Focus Areas
- src/sensors.c[191-224]
## Suggested fix
- Read 0x3B00 and 0x300B into variables.
- If `r3B00` matches the IMX415 signature (0x2E or 0x28) but the 0x300B read returns `-1`, treat detection as inconclusive (e.g., return `false` so other addresses can be tried, or at minimum skip the IMX335 branch and let the later IMX415 fallback decide).
- Optionally apply the same “inconclusive” handling when `r3B00` itself is `-1` (depending on desired behavior).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[widgetii]

1. IMX415 misdetect on read error — is_imx415 is computed using READ(0xB) == 0xA0 without checking for the read-error sentinel (-1).

Valid, and worth fixing — addressed in 46b775c.

Confirmed the sentinel is real rather than theoretical: hisi_gen1_sensor_read_register returns -1 on ioctl failure (src/hal/hisi/hal_hisi.c:238), and the code immediately above already respects it (if (r316A == -1) return false;). So a failed 0x300B read really would have read as "not IMX415" and dropped a latched IMX415 into the IMX335 branch — re-arming the exact loop this PR exists to break. That failure mode is unusually expensive here because clearing the latch needs a physical power cycle, not a reboot, so the asymmetry justifies the guard even though the trigger is rare.

I scoped it more narrowly than the suggested fix:

if (r316A > 0 && ((r316A & 0xFC) == 0x7C) && !is_imx415) {
    if (r3B00 == -1 || r300B == -1)
        return false;
    ...
}

The guard sits inside the IMX335 branch rather than before it, because only that decision is at risk. Hoisting it would make a failed 0x300B read abort detection for every Sony part that legitimately falls through past IMX335 (IMX178, IMX274, IMX185, IMX291/307/327, …), which trades a rare wrong answer for a broad new failure mode. The checks below re-test 0x3B00 on their own, so they are unaffected.

Re-verified on hardware after the change: Hi3516AV300 + IMX415 → imx415_i2c, Hi3516EV300 + IMX335 → imx335_i2c.