drivers: video: ov7670: Implement missing video API functions, controls and update init regs. #84229
Conversation
| /* PCLK does not toggle during horizontal blank, one PCLK, one pixel */ | ||
| {OV7670_COM10, 0x20}, /* COM10 */ | ||
| /* Free running PCLK, default VSYNC, HSYNC and PCLK */ | ||
| {OV7670_COM10, 0x00}, /* COM10 */ |
There was a problem hiding this comment.
I will try to see if I can find the original platform used for testing this.
There was a problem hiding this comment.
FRDM-MCXN947: I have that board with me! If I find some time to test it, that would make things easy.
There was a problem hiding this comment.
Free-running clock shouldn't have any effect. The other change I wanted to make is set VSYNC/HSYNC active high (COM10 0x03), so we wouldn't need to change the polarity in the overlay when switching between OV to say GC. I don't know if this was the plan or not, but it would be a good idea if all sensors are made consistent (all active high or all active low). Also, it will be one less thing to worry about if/when selecting sensors at runtime is supported.
There was a problem hiding this comment.
Thank you for commenting further on this.
Good idea about unifying the signals polarity. An issue about it sounds like a good first step.
There was a problem hiding this comment.
cc @danieldegrasse just for visibility as you did the original driver.
I think it should just not fail if stream start and stop are not provided. As far as I know there's no way to stop the stream, at least not for this sensor, once powered it just starts outputting frames. Yeah I see the kernel power_on/off but if we power down the sensor that way then any registers set will be lost. Either way is fine with me, if you allow these ops to be NULL, I can just remove the dummy start/stop. EDIT: Actually now I remember that these OV sensors had a sleep function, which we could use instead, but I don't think I2C works in sleep state, it probably does. @josuah |
There was a problem hiding this comment.
Thank you for clarifying the different points and making sure that the implications were intended.
WRT start()/stop() missing, it seems like the return status needs to be "it did nothing" rather than "it failed because it had nothing to be done". return 0; is one way to do that. If many sensors need that, then this could become part of the API in the future.
iabdalkader
force-pushed
the
ov7670_fixes
branch
4 times, most recently
from
9091416 to
70c87d2
Compare
|
Can I fix this too while we're at it? Line 256 in 9a236f8 This doesn't fix YUV output, it still looks corrupted, but at least RGB won't have to be swapped in software. |
|
Always good to get more things to work!
Is this related to the image being swapped left/right per chance? This swaps the endianness for some sensors. |
of course :) however it's usually not a bad idea to split things in multiple PRs to get incremental fixes/improvements approved (like, e.g. this PR here is now good to go, and you will be losing approvals if you push updates, and then folks might not be available to review again :) ) |
Yes this swaps the endianness. The current setting is set to UYVY. It seems to also swap RGB565 output.
@kartben Got it, will send any other fixes in another PR. |
iabdalkader
force-pushed
the
ov7670_fixes
branch
3 times, most recently
from
a369ab0 to
e795693
Compare
Add the missing stream_start and stream_stop API functions (the driver doesn't work without them) and implement video controls for horizontal mirror (hmirror) and vertical flip. Signed-off-by: Ibrahim Abdalkader <[email protected]>
The STM32 DCMI capture seems to stop when the pixel clock is disabled during horizontal blank. This patch switches pixel clock to free-running, which shouldn't have any effect on other capture devices. Signed-off-by: Ibrahim Abdalkader <[email protected]>
TSLB[3] swaps the YUYV/RGB565 output. Signed-off-by: Ibrahim Abdalkader <[email protected]>
iabdalkader
force-pushed
the
ov7670_fixes
branch
from
e795693 to
08e5319
Compare
Add the missing
stream_startandstream_stopAPI functions (the driver doesn't work without them) and implement video controls for horizontal mirror (hmirror) and vertical flip. Additionally, I've changed PCLK to free-running as the STM32 DCMI doesn't seem to work without it.I would also like to change the default VSYNC/HSYNC polarity to high (0x03) to make this driver consistent with others. This change would allow us to reuse the same overlay without modifying vsync-active. Since this driver wasn't functioning at all, I assume it is safe to make this change as it won't break anything else?