load_goke: opt-in sensor_dvp / sensor_mclk gate for DVP-wired boards - #2276
Open
bneigher wants to merge 1 commit into
Open
load_goke: opt-in sensor_dvp / sensor_mclk gate for DVP-wired boards#2276bneigher wants to merge 1 commit into
bneigher wants to merge 1 commit into
Conversation
open_sys_config picks MIPI or DVP pad routing from its chip= and g_cmos_yuv_flag= arguments. On a board that wires the sensor to the DVP pads, the MIPI arguments mux the i2c controller to pads the sensor is not connected to, so every address NACKs and no sensor is ever detected. Gated on an env var rather than $CHIP_TYPE deliberately: both wirings exist on gk7202v300, so keying this off the SoC name would fix DVP boards by breaking every MIPI one. Profiles opt in with 'fw_setenv sensor_dvp 1'; unset means the current behaviour is unchanged. sensor_mclk is gated the same way. open_sys_config reads MCLK only from its module parameters and defaults to 27 MHz at this chip=, so a sensor init table tuned for 24 MHz runs against the wrong clock -- and the ini's MCLK key is never consulted on this path, which makes sweeping it look like a ruled-out cause. Refs: OpenIPC#2074
PR Summary by Qodoload_goke: opt-in sensor_dvp/sensor_mclk gates for DVP-wired boards
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Silent MCLK write failure
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+188
to
+190
| case "$(fw_printenv -n sensor_mclk 2>/dev/null)" in | ||
| 24) devmem 0x120100F0 32 0x0000000D ;; | ||
| esac |
There was a problem hiding this comment.
1. Silent mclk write failure 🐞 Bug ☼ Reliability
insert_ko adds a devmem write when sensor_mclk=24 but does not check whether devmem exists or whether the write succeeded, so the script can continue with the wrong MCLK while giving no diagnostic. This can make sensor bring-up fail with little/no signal in logs.
Agent Prompt
### Issue description
A new `devmem`-based MCLK override is executed without checking for tool presence or write success. If the write fails, the script continues silently, leaving the system misconfigured.
### Issue Context
This is a shared boot-time module loader script; failures should be explicit to avoid hard-to-debug field breakage.
### Fix Focus Areas
- general/package/goke-osdrv-gk7205v200/files/script/load_goke[184-190]
### Suggested fix
- Check `command -v devmem` before use.
- Check `devmem` exit status; on failure, emit a `logger` message (daemon.err) and either `report_error` or explicitly continue with a warning (but do not fail silently).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+38
| if [ "$(fw_printenv -n sensor_dvp 2>/dev/null)" = "1" ]; then | ||
| CHIP_TYPE=gk7205v200 | ||
| YUV_TYPE0=1 | ||
| fi |
There was a problem hiding this comment.
2. Chip_type role conflation 🐞 Bug ⚙ Maintainability
The sensor_dvp opt-in block overwrites the detected CHIP_TYPE globally, conflating the actual SoC identity with the value intended to steer open_sys_config routing. This makes later CHIP_TYPE-based logic (present now or added later) harder to reason about and increases the risk of unintended behavior changes when extending the script.
Agent Prompt
### Issue description
`sensor_dvp=1` currently mutates `CHIP_TYPE` (the detected SoC name) for the remainder of the script. This mixes two concepts:
- detected SoC (used for chip-specific branching)
- desired `open_sys_config chip=` selector (used for pad routing selection)
### Issue Context
This script is shared across multiple Goke SoCs/boards; minimizing global state mutation reduces future regressions.
### Fix Focus Areas
- general/package/goke-osdrv-gk7205v200/files/script/load_goke[4-10]
- general/package/goke-osdrv-gk7205v200/files/script/load_goke[24-38]
- general/package/goke-osdrv-gk7205v200/files/script/load_goke[109-115]
- general/package/goke-osdrv-gk7205v200/files/script/load_goke[179-184]
### Suggested fix
- Introduce e.g. `SYS_CONFIG_CHIP="$CHIP_TYPE"` after detection.
- In the `sensor_dvp` block, set `SYS_CONFIG_CHIP=gk7205v200` (and keep `CHIP_TYPE` unchanged).
- Pass `chip=$SYS_CONFIG_CHIP` to `modprobe open_sys_config` in both `insert_detect` and `insert_ko`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an opt-in env-var gate so DVP-wired boards can select the DVP pad
routing, without changing behaviour for anything that does not ask for it.
open_sys_configpicks MIPI or DVP pad routing from itschip=andg_cmos_yuv_flag=arguments. On a board that routes the sensor's parallel data,sync, PCLK and i2c to the SoC's DVP pads, the MIPI arguments mux the i2c
controller to pads the sensor is not connected to — every address NACKs, no chip
ID is ever read, and no video is possible.
Per @widgetii's review note on #2074, this is keyed off an env
var rather than
$CHIP_TYPE:So a profile opts in with
fw_setenv sensor_dvp 1. Unset — every existing board —takes exactly the path it takes today.
sensor_mclkis gated the same way.open_sys_configreads MCLK only from itsmodule parameters and defaults to 27 MHz at this
chip=, so a sensor init tabletuned for 24 MHz (the GC2053 ForCar tables are) runs against the wrong clock.
Worth noting for anyone who hits this: the
.iniMCLK key is not consulted onthis path, so sweeping it produces identical results at every value — which is
indistinguishable from having ruled the cause out. That cost me a while.
Companion to the
gk7202v300_lite_w7_8mdevice profile in OpenIPC/builder,which is where the board-specific bring-up lives.
Verified on three GK-W7 boards (GK7202V300, 8 MB NOR, GC2053 in DVP mode with
SID strapped high): with
sensor_dvp=1andsensor_mclk=24the i2c bus comesup, the sensor answers at 7-bit
0x3fwith chip ID0x2053, and the pipelinedelivers 1920x1080 H.264 over RTSP at 25 fps with
FrmErrCnt 0. With the varsunset the script is byte-for-byte equivalent in behaviour to before.
sh -nclean.Refs: #2074