Skip to content

kernel/hi3516cv200: recognize the gc2023_mipi sensor type - #207

Open
phedoreanu wants to merge 1 commit into
OpenIPC:mainfrom
phedoreanu:gc2023-mipi
Open

kernel/hi3516cv200: recognize the gc2023_mipi sensor type#207
phedoreanu wants to merge 1 commit into
OpenIPC:mainfrom
phedoreanu:gc2023-mipi

Conversation

@phedoreanu

Copy link
Copy Markdown

What

Add a gc2023_mipi arm to the hi3516cv200 open_sys_config sensor table: I2C0 pinmux + 24 MHz sensor clock, parallel VI pads left alone so the MIPI PHY keeps the pins. Mechanical mirror of the existing ov2710_mipi arm.

Why

Companion to OpenIPC/firmware#2248, which adds the gc2023_mipi identity to load_hisilicon and a gc2023_mipi_1080p.ini. open_sys_config falls through to sensor_type '%s' is error!!! for names it does not know; that is non-fatal (the script's devmem writes run afterwards and win), but it leaves an alarming line in every boot log and lets the two pinmux tables drift. The OV2710 MIPI addition updated both tables; this restores that symmetry for GC2023.

Raised by @widgetii in the review of OpenIPC/firmware#2248.

Verification

The register writes are hardware-verified in their devmem form: they are exactly what the load_hisilicon arm issues on a VStarcam Hi3518EV200 with the GC2023 wired to MIPI (I2C reaches the sensor, MIPI PHY delivers frames with the vendor MIPI libsns — full bring-up record in OpenIPC/firmware#2243). This kernel arm is a byte-for-byte mirror of the ov2710_mipi arm apart from the name and comment; I have not built the cv200 kernel tree standalone.

Mirror of the ov2710_mipi arm: I2C0 pinmux plus 24MHz sensor clock,
with the parallel VI pads left alone so the MIPI PHY keeps the pins.
Companion to OpenIPC/firmware#2248, which adds the gc2023_mipi identity
to load_hisilicon and a gc2023_mipi_1080p.ini; without this arm a
gc2023_mipi boot logs "sensor_type 'gc2023_mipi' is error!!!" before
the script's devmem writes apply the same values.

The register writes themselves are hardware-verified: they are the ones
the load_hisilicon arm issues via devmem on a VStarcam Hi3518EV200
(see OpenIPC/firmware#2243 for the bring-up record).
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

hi3516cv200: add gc2023_mipi sensor handling to open_sys_config

🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

Grey Divider

AI Description

• Recognize the "gc2023_mipi" sensor type in hi3516cv200 sys_config.
• Apply I2C0 pinmux and 24MHz sensor clock without touching parallel VI pads.
• Prevent misleading "sensor_type ... is error" boot logs and keep tables in sync.
Diagram

graph TD
  A([Boot]) --> B["open_sys_config: insert_sns()"] --> C{Sensor type?}
  C -->|"gc2023_mipi"| D["Pinmux: I2C0 SCL/SDA"] --> E["Reg: 24MHz clk + unreset"] --> F["MIPI PHY owns pads"]
  C -->|other/unknown| G["Fallback: log error / other arms"]
  
  subgraph Legend
    direction LR
    _start([Entry]) ~~~ _proc["Function/step"] ~~~ _dec{Decision}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Table-driven sensor config map
  • ➕ Reduces duplication across near-identical MIPI/parallel sensor arms
  • ➕ Makes it harder for sensor tables to drift over time
  • ➕ Simplifies adding new sensor names to a single data structure
  • ➖ Refactor risk in boot-critical, hardware-facing code
  • ➖ May be undesirable in kernel/vendor trees where minimal diffs are preferred
2. Share common MIPI helper for I2C0+clock
  • ➕ Keeps changes small while still reducing copy/paste conditionals
  • ➕ Encapsulates register programming for review and reuse
  • ➖ Still leaves a growing dispatch chain of sensor names
  • ➖ Requires touching more lines/files than the minimal mirror approach

Recommendation: The PR’s “mirror ov2710_mipi” approach is the lowest-risk fix and appropriate for a vendor-style sys_config tree. If more MIPI sensor variants are expected, consider a follow-up that centralizes common MIPI pinmux/clock setup (helper or table-driven mapping) to prevent future drift and copy/paste errors.

Files changed (1) +10 / -0

Bug fix (1) +10 / -0
sys_config.cAdd gc2023_mipi branch mirroring ov2710_mipi pinmux/clock setup +10/-0

Add gc2023_mipi branch mirroring ov2710_mipi pinmux/clock setup

• Adds a new sensor-name conditional for "gc2023_mipi" in insert_sns(). The branch programs I2C0 SCL/SDA pinmux and the 24MHz sensor clock/unreset register while leaving parallel VI pads unchanged to preserve MIPI PHY ownership.

kernel/sys_config/hi3516cv200/sys_config.c

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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.

1 participant