Fangzhu fg12-16ch support for JP5.0.2 + JP6.0 - #332
Conversation
03bdb5f to
b2203e2
Compare
b288bcb to
01857bd
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for the Fangzhu FG12-16CH SerDes board with MAX96712 deserializer for both single and dual camera configurations on JetPack 5.0.2 and 6.0. The changes introduce a deserializer abstraction layer to support both MAX9296 and MAX96712 chips, enabling flexible hardware configurations through device tree selection.
Changes:
- Added MAX96712 deserializer driver support with abstraction layer for JP5.0.2 and JP6.0
- Added device tree overlays for FG12-16CH single and dual camera configurations
- Updated build scripts to support FG12-16CH configurations via command-line flags
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| kernel/realsense/d4xx.c | Introduces deserializer abstraction layer and MAX96712 support |
| nvidia-oot/max96712.h | MAX96712 header file for JP6.x |
| kernel/nvidia/max96712.h | MAX96712 header file for JP5.x |
| nvidia-oot/6.x/0004-Adding-max96712-support-for-D4xx.patch | MAX96712 driver implementation for JP6.x |
| kernel/nvidia/5.0.2/0015-Adding-max96712-support-for-D4xx.patch | MAX96712 driver implementation for JP5.x |
| hardware/realsense/tegra234-camera-d4xx-overlay*.dts | Device tree overlays for FG12-16CH configurations |
| hardware/realsense/tegra194-camera-d4xx-*.dtsi | Device tree includes for FG12-16CH on JP5.x |
| apply_patches.sh | Build script updates for FG12-16CH support |
| README_JP5.md, README_JP6.md | Documentation updates for FG12-16CH usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| state->g_ctx.sdev_reg = state->client->addr; | ||
|
|
There was a problem hiding this comment.
The device register address is being set from the client address after it may have already been modified. The override_reg property is read and applied in the probe function after this line executes. This ordering could cause state->g_ctx.sdev_reg to contain the wrong address if override_reg is specified in the device tree. The assignment should occur after the override_reg logic in the probe function.
| state->g_ctx.sdev_reg = state->client->addr; |
| @@ -3179,7 +2909,6 @@ | |||
| } | |||
|
|
|||
| dser_i2c = of_find_i2c_device_by_node(dser_node); | |||
There was a problem hiding this comment.
The removal of of_node_put(dser_node) immediately after finding the deserializer device creates a memory leak risk. The node reference should be released as soon as it's no longer needed, not after additional error checking and initialization. The original placement was correct for early cleanup in error paths.
| + } | ||
| + map_pipe_control[1].val = (vc_id << 6) | data_type1; |
There was a problem hiding this comment.
The conditional override at line 250 sets map_pipe_control[1].val = 0x07 for IMU (when data_type2 == 0x0), but line 252 unconditionally overwrites this value with (vc_id << 6) | data_type1. The conditional assignment has no effect because it's immediately overwritten. The line 252 assignment should be moved inside an else block or the condition logic should be corrected.
| + } | ||
| + map_pipe_control[1].val = (vc_id << 6) | data_type1; |
There was a problem hiding this comment.
The conditional override at line 249 sets map_pipe_control[1].val = 0x07 for IMU (when data_type2 == 0x0), but line 251 unconditionally overwrites this value with (vc_id << 6) | data_type1. The conditional assignment has no effect because it's immediately overwritten. The line 251 assignment should be moved inside an else block or the condition logic should be corrected.
| + } | |
| + map_pipe_control[1].val = (vc_id << 6) | data_type1; | |
| + } else { | |
| + map_pipe_control[1].val = (vc_id << 6) | data_type1; | |
| + } |
| +int max96712_get_available_pipe_id(struct device *dev, int vc_id) | ||
| +{ | ||
| + /* TODO EHUD: Currently vc_id == pipe id but that's definitely | ||
| + * not the end goal if we plan to support multiple cameras | ||
| + */ | ||
| + return vc_id; | ||
| +} | ||
| +EXPORT_SYMBOL(max96712_get_available_pipe_id); | ||
| + | ||
| +int max96712_release_pipe(struct device *dev, int pipe_id) | ||
| +{ | ||
| + /* TODO EHUD: pipe id? */ |
There was a problem hiding this comment.
The max96712_get_available_pipe_id function currently returns vc_id directly without tracking pipe allocation state. This prevents proper support for multiple cameras and could lead to pipe conflicts. The implementation should track allocated pipes and return the next available pipe ID, or clearly document why this simplified approach is temporarily acceptable.
| +int max96712_get_available_pipe_id(struct device *dev, int vc_id) | |
| +{ | |
| + /* TODO EHUD: Currently vc_id == pipe id but that's definitely | |
| + * not the end goal if we plan to support multiple cameras | |
| + */ | |
| + return vc_id; | |
| +} | |
| +EXPORT_SYMBOL(max96712_get_available_pipe_id); | |
| + | |
| +int max96712_release_pipe(struct device *dev, int pipe_id) | |
| +{ | |
| + /* TODO EHUD: pipe id? */ | |
| +/* Simple global pipe allocator. | |
| + * NOTE: This currently tracks pipe allocation at the driver level, not per | |
| + * deserializer instance. This is sufficient for the current use cases and | |
| + * avoids pipe conflicts between concurrent streams, but may need to be | |
| + * revisited if multiple MAX96712 instances are used in the same system. | |
| + */ | |
| +#define MAX96712_MAX_PIPES 4 | |
| + | |
| +static DEFINE_MUTEX(max96712_pipe_lock); | |
| +static unsigned long max96712_pipe_bitmap; | |
| + | |
| +int max96712_get_available_pipe_id(struct device *dev, int vc_id) | |
| +{ | |
| + int pipe_id; | |
| + | |
| + /* For now we ignore vc_id for allocation and simply return the first | |
| + * available pipe. This avoids conflicts when multiple cameras are | |
| + * active. If a stable vc_id-to-pipe_id mapping is required in the | |
| + * future, this function should be extended accordingly. | |
| + */ | |
| + mutex_lock(&max96712_pipe_lock); | |
| + | |
| + for (pipe_id = 0; pipe_id < MAX96712_MAX_PIPES; pipe_id++) { | |
| + if (!(max96712_pipe_bitmap & (1UL << pipe_id))) { | |
| + max96712_pipe_bitmap |= (1UL << pipe_id); | |
| + break; | |
| + } | |
| + } | |
| + | |
| + mutex_unlock(&max96712_pipe_lock); | |
| + | |
| + if (pipe_id == MAX96712_MAX_PIPES) | |
| + return -EBUSY; | |
| + | |
| + return pipe_id; | |
| +} | |
| +EXPORT_SYMBOL(max96712_get_available_pipe_id); | |
| + | |
| +int max96712_release_pipe(struct device *dev, int pipe_id) | |
| +{ | |
| + if (pipe_id < 0 || pipe_id >= MAX96712_MAX_PIPES) | |
| + return -EINVAL; | |
| + | |
| + mutex_lock(&max96712_pipe_lock); | |
| + max96712_pipe_bitmap &= ~(1UL << pipe_id); | |
| + mutex_unlock(&max96712_pipe_lock); | |
| + |
| + for (i = 0; i < 12; i++) { | ||
| + map_pipe_control[i].addr += 0x40 * pipe_id; | ||
| + } | ||
| + map_pipe_control[12].addr += 0x12 * pipe_id; |
There was a problem hiding this comment.
The magic numbers 0x40, 0x12, and hardcoded index 12 represent register address offsets and array indexing for pipe multiplexing, but their meaning is unclear. These should be defined as named constants (e.g., MAX96712_PIPE_REG_OFFSET, MAX96712_VIDEO_RX_OFFSET, and use ARRAY_SIZE(map_pipe_control) - 1 instead of 12) to improve code maintainability and prevent indexing errors.
| override_reg = <0x1a>; | ||
| compatible = "intel,d4xx"; | ||
| vcc-supply = <&p2822_vdd_sys_en>; | ||
| cam-type = "Y8"; |
There was a problem hiding this comment.
Inconsistent indentation: the cam-type property has extra leading whitespace compared to surrounding properties. This should align with the vcc-supply property above it for consistency.
| cam-type = "Y8"; | |
| cam-type = "Y8"; |
| override_reg = <0x1a>; | ||
| compatible = "intel,d4xx"; | ||
| vcc-supply = <&p2822_vdd_sys_en>; | ||
| cam-type = "Y8"; |
There was a problem hiding this comment.
Inconsistent indentation: the cam-type property has extra leading whitespace compared to surrounding properties. This should align with the vcc-supply property above it for consistency.
| cam-type = "Y8"; | |
| cam-type = "Y8"; |
68f4d93 to
e17c38b
Compare
4139b13 to
671917a
Compare
b28137a to
ddd33e3
Compare
Signed-off-by: ejgoldik <ehud.joseph.goldik@intel.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@intel.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@intel.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
- Also added missing max96712.h files Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
ddd33e3 to
f603381
Compare
Signed-off-by: ejgoldik <ehud.joseph.goldik@realsenseai.com>
Added device trees for fg12-16ch support (both single camera and dual camera)
for single camera:
./apply_patches.sh --fg12-16ch 5.0.2
for dual camera on cam0 and cam4:
./apply_patches.sh --fg12-16ch-dual 5.0.2
Added overlays for fg-16ch support for JP6.0 (both single camera and dual camera)
tegra234-camera-d4xx-overlay-fg12-16ch.dts
tegra234-camera-d4xx-overlay-fg12-16ch-dual.dts