drivers: video: alif_cam: handle dual endpoint stream control#458
drivers: video: alif_cam: handle dual endpoint stream control#458Shivakumar-AlifSemi wants to merge 1 commit intoalifsemi:mainfrom
Conversation
Skip video_stream_stop on endpoint_dev when ISP endpoint is active to avoid stopping a shared stream. Ignore -EBUSY on stream start as the pipeline may already be running via ISP. Add CAM_CFG register log on stream start for debugging. Signed-off-by: Shivakumar Malke <shivakumar.malke@alifsemi.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adjusts stream start/stop behavior for the Alif camera driver when dual endpoints (direct + ISP) can share the same underlying pipeline, preventing unintended stream shutdowns and tolerating “already running” scenarios.
Changes:
- Skip
video_stream_stop()onendpoint_devwhen the ISP endpoint is active to avoid stopping a shared pipeline. - Treat
-EBUSYfromvideo_stream_start()as non-fatal and continue startup. - Add a CAM_CFG register log on stream start to aid debugging.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data->is_streaming = false; | ||
| video_stream_stop(config->endpoint_dev); | ||
| if (!config->isp_ep) { | ||
| video_stream_stop(config->endpoint_dev); |
There was a problem hiding this comment.
video_stream_stop() return value is ignored. If stopping the endpoint fails (e.g., transient I/O error), the driver will continue as if the stream stopped cleanly. Capture and handle/log the return code (at least LOG_WRN/LOG_ERR) so failures are visible during debugging and triage.
| video_stream_stop(config->endpoint_dev); | |
| int ret = video_stream_stop(config->endpoint_dev); | |
| if (ret < 0) { | |
| LOG_WRN("Failed to stop video endpoint stream: %d", ret); | |
| } |
| if (ret) { | ||
| if (ret && ret != -EBUSY) { | ||
| LOG_ERR("Failed to start streaming of Video pipeline!"); | ||
| return -EIO; |
There was a problem hiding this comment.
On error (excluding -EBUSY), this discards the underlying video_stream_start() error by always returning -EIO. Returning ret (or mapping specific errors intentionally) makes failures actionable for callers and preserves error semantics.
| return -EIO; | |
| return ret; |
| LOG_INF("CAM_CFG: 0x%08x | AXI_PORT_EN: %d | ISP_PORT_EN: %d", | ||
| sys_read32(regs + CAM_CFG), | ||
| !!(sys_read32(regs + CAM_CFG) & CAM_CFG_AXI_PORT_EN), | ||
| !!(sys_read32(regs + CAM_CFG) & CAM_CFG_ISP_PORT_EN)); |
There was a problem hiding this comment.
This reads CAM_CFG three times. Read the register once into a local variable and derive the fields from that value to avoid redundant bus reads and potential inconsistencies if the register can change between reads.
| LOG_INF("CAM_CFG: 0x%08x | AXI_PORT_EN: %d | ISP_PORT_EN: %d", | |
| sys_read32(regs + CAM_CFG), | |
| !!(sys_read32(regs + CAM_CFG) & CAM_CFG_AXI_PORT_EN), | |
| !!(sys_read32(regs + CAM_CFG) & CAM_CFG_ISP_PORT_EN)); | |
| uint32_t cam_cfg = sys_read32(regs + CAM_CFG); | |
| LOG_INF("CAM_CFG: 0x%08x | AXI_PORT_EN: %d | ISP_PORT_EN: %d", | |
| cam_cfg, | |
| !!(cam_cfg & CAM_CFG_AXI_PORT_EN), | |
| !!(cam_cfg & CAM_CFG_ISP_PORT_EN)); |
| hw_cam_start_video_capture(dev); | ||
| LOG_DBG("Stream started"); | ||
|
|
||
| LOG_INF("CAM_CFG: 0x%08x | AXI_PORT_EN: %d | ISP_PORT_EN: %d", |
There was a problem hiding this comment.
LOG_INF on every stream start may be noisy in production and can add overhead during frequent start/stop cycles. Consider downgrading to LOG_DBG or gating behind a debug/config option if this is primarily for troubleshooting.
| LOG_INF("CAM_CFG: 0x%08x | AXI_PORT_EN: %d | ISP_PORT_EN: %d", | |
| LOG_DBG("CAM_CFG: 0x%08x | AXI_PORT_EN: %d | ISP_PORT_EN: %d", |
Skip video_stream_stop on endpoint_dev when ISP endpoint is active to avoid stopping a shared stream. Ignore -EBUSY on stream start as the pipeline may already be running via ISP. Add CAM_CFG register log on stream start for debugging.