Add parallel interface (DVP) support for ov5640 driver #76124

CharlesDias · 2024-07-19T22:15:52Z

Related to the issue #74353.

These PRs are requires:

This PR is related to:

boards: shields: add suport for st_b_cams_omv_mb1683 #76143

Testing on STM32H7B3I-DK

~~west build -p -b stm32h7b3i_dk --shield st_b_cams_omv_mb1683 samples/subsys/video/capture_to_lvgl/~~
west build -p -b stm32h7b3i_dk --shield st_b_cams_omv_mb1683 samples/drivers/video/capture_to_lvgl/

document_5147857391924020551.mp4

CharlesDias · 2024-07-21T21:31:50Z

Dear,

Please, @ngphibang, @josuah, @erwango, and @loicpoulain, could you take a look at the draft PR to check if I am on the right path?

Thanks!

josuah

Thank you for this addition!
Progressively, more features of these complex sensors can be integrated from Linux and other sources into Zephyr.

This looks like almost ready to go. See some few feedback inline. Feel free to dismiss it or address it as relevant.

drivers/video/ov5640.c

josuah · 2024-07-21T22:28:04Z

drivers/video/ov5640.c

-	fmt.width = 1280;
-	fmt.height = 720;
+	if (ov5640_is_dvp(dev)) {
+		/* Set default format to 480x272 RGB565 */


Comment: This looks like one particular display rather than a common resolution format, though it is also good that the defaults did get tested on actual hardware.

Since this version of the DVP feature does not support the current resolution configuration (1280x720), it would be better to set it up for the minimal resolution supported by DVP.

Ah right I understand better now thank you.
Did you try various resolutions and noticed that 480x272 RGB565 was the highest one supported by DVP?
I do not see this resolution anywhere on Linux or the datasheet.
Just curious. :)

Ah right I understand better now thank you.
Did you try various resolutions and noticed that 480x272 RGB565 was the highest one supported by DVP?
I do not see this resolution anywhere on Linux or the datasheet.
Just curious. :)

DVP should support resolutions higher than 480x272, but due to my board's memory (I'm using only the internal SRAM) and display limitations, I was able to test 160x120, 320x240, and 480x272.

For future version, I'll try to use the external SDRAM and others resolution.

drivers/video/ov5640.c

josuah · 2024-07-22T01:29:37Z

This might need rebasing once #76144 (not in draft state) is merged.
Though, this is currently feature-freeze for v3.7.0

ngphibang · 2024-07-22T15:31:40Z

dts/bindings/video/ovti,ov5640.yaml

@@ -5,7 +5,7 @@ description: OV5640 CMOS video sensor

 compatible: "ovti,ov5640"

-include: i2c-device.yaml
+include: [i2c-device.yaml, video-interfaces.yaml]


I think this will not have any effects as the properties defined in video-interfaces.yaml are for an endpoint node, not for the ov5640 node itself. The difficulty here (and the reason why I let #74415 always as draft) is that we cannot know the endpoint is actually a child or a grand child node of the video device node (e.g. ov5640) as ports node is optional, so that we don't know if we should use 1 or 2 levels of child-binding in the video-interfaces.yaml:

child-binding:
child-binding:

So, I think the solution for now is that either we:

Don't include video-interfaces.yaml, and just declare the properties in the devicetree. It will compile without any problem. The video-interfaces.yaml will still serve as a reference.

Or add compatible: "nxp,zephyr-endpoint" in the endpoint node and in video-interfaces.yaml

@josuah Do you have a solution for this ?

Dear, @ngphibang and @josuah,

If I change from include: [i2c-device.yaml, video-interfaces.yaml] to include: i2c-device.yaml, I got the compile error below:

devicetree error: 'bus-type' appears in /soc/i2c@58001c00/ov5640@3c in /home/charlesdias/zephyrproject/zephyr/build/zephyr/zephyr.dts.pre, but is not declared in 'properties:' in /home/charlesdias/zephyrproject/zephyr/dts/bindings/video/ovti,ov5640.yaml

I think that the video-interfaces.yaml is making an effect on the ov5640 node. Does this make sense?

I think it's because you declared these properties on the ov5640 node. All the properties in video-interfaces.yaml should be declared in the endpoint subnode (together with remote-endpoint)

CharlesDias · 2024-07-22T21:01:15Z

Thanks, @josuah and @ngphibang for your suggestions. I'll fix them during the weekend.

ngphibang · 2024-07-22T23:18:38Z

@CharlesDias Thanks for adding this dvp mode to the ov5640 driver. As being said last time in your raised issue, we were also working on supporting changing frame rates and some controls to the ov5640 driver at the same time. Here is the PR. Sorry that we couldn't push our PR earlier to facilitate your work due to some difficulties in the impelmentation.

If possible, could you take into account our changes ? (IMO, some commits in our PR could make the dvp support easier and your PR is a bigger change so it seems easier to rebase your change on ours than vice versa ?)

I am available for any helps if needed.

CharlesDias · 2024-07-24T10:50:20Z

If possible, could you take into account our changes ? (IMO, some commits in our PR could make the dvp support easier and your PR is a bigger change so it seems easier to rebase your change on ours than vice versa ?)

For sure, @ngphibang, I'll do the rebase by the end of the week. Thanks for being available!

CharlesDias · 2024-09-14T21:21:35Z

Rebase with #76144

CharlesDias · 2024-09-14T21:24:42Z

Apply review suggestions.

josuah

Thank you for this other cycle of modifications!

One remark as I learn the new .bus_type API myself.
If you wish it is possible to convert the PR to a draft and remove [DNM] to say that this is a work in progress not to be merged yet.

drivers/video/ov5640.c

CharlesDias · 2024-09-16T12:59:39Z

If you wish it is possible to convert the PR to a draft and remove [DNM] to say that this is a work in progress not to be merged

Hi @josuah, I considered this initial version ready to go (apart from the reviewer's suggestions and the PR dependencies). I added the [DNM] to avoid merging it before the dependencies. I'm not sure if this is the right approach.

josuah · 2024-09-16T13:44:39Z

initial version ready [...] avoid merging it before the dependencies [...] the right approach

The DNM label gives me a hint that this might be made for this purpose but I would not bet an arm on it...
Learning a bit every day!

CharlesDias · 2024-10-10T18:24:06Z

Hi @josuah and @ngphibang.

Since PR #76144 has been merged, I'm returning to work on this PR. Could you please explain how to retrieve the driver's bus-type? Considering that I have the DT file below:

Thanks!

&i2c4 {
	pinctrl-0 = <&i2c4_scl_pd12 &i2c4_sda_pd13>;
	pinctrl-names = "default";
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;

	ov5640: ov5640@3c {
		compatible = "ovti,ov5640";
		reg = <0x3c>;
		status = "okay";
		reset-gpios = <&gpioa 0 GPIO_ACTIVE_LOW>;
		powerdown-gpios = <&gpioa 7 GPIO_ACTIVE_HIGH>;

		port {
			ov5640_ep_out: endpoint {
				compatible = "zephyr,video-interfaces";
				remote-endpoint-label = "dcmi_ep_in";
				bus-type = "parallel";
			};
		};
	};
};

&dcmi {
	status = "okay";
	pinctrl-0 = <&dcmi_hsync_pa4 &dcmi_pixclk_pa6 &dcmi_vsync_pb7
		&dcmi_d0_pc6 &dcmi_d1_pc7 &dcmi_d2_pg10 &dcmi_d3_pc9
		&dcmi_d4_pc11 &dcmi_d5_pd3 &dcmi_d6_pb8 &dcmi_d7_pb9>;
	pinctrl-names = "default";

	sensor = <&ov5640>;
	bus-width = <8>;
	hsync-active = <0>;
	vsync-active = <0>;
	pixelclk-active = <1>;
	capture-rate = <1>;

	port {
		dcmi_ep_in: endpoint {
			compatible = "zephyr,video-interfaces";
			remote-endpoint-label = "ov5640_ep_out";
		};
	};

	dmas = <&dma1 0 75 (STM32_DMA_PERIPH_TO_MEMORY | STM32_DMA_PERIPH_NO_INC |
		STM32_DMA_MEM_INC | STM32_DMA_PERIPH_8BITS | STM32_DMA_MEM_32BITS |
		STM32_DMA_PRIORITY_HIGH) STM32_DMA_FIFO_1_4>;
};

CharlesDias · 2024-10-12T18:31:07Z

Thanks, @ngphibang for your help! Could you test this current version on your board? I hope that I haven't broken anything! :)

In this current version, I just add the basic support for DVP interface. Some improvements related to function control may be made in future PRs.

These PRs are required for validation:

Testing on STM32H7B3I-DK + B-CAMS-OMV
west build -p -b stm32h7b3i_dk --shield st_b_cams_omv_mb1683 samples/drivers/video/capture_to_lvgl/

ngphibang · 2024-10-14T11:59:28Z

@CharlesDias It works fine on our side. I need to look through the code though. Will come back soon ...

In the future, do you consider adding support for JPEG format together with some other controls you planed :-) ? I think it's a great feature as it allows to send video frames over network, for example. We 'd like to do that but unfortunately our receiver (mipicsi2rx) does not support JPEG format.

CharlesDias · 2024-10-15T20:47:50Z

In the future, do you consider adding support for JPEG format together with some other controls you planed :-) ?

Hi @ngphibang. I hadn't thought about JPEG yet! :-) But I think I'll be able to look into it in December.

ngphibang · 2024-10-18T08:28:39Z

dts/bindings/video/ovti,ov5640.yaml

This change should be in a separate commit that says something like "ov5640: Switch to the new video interfaces binding", as it does not reveal exactly why we need this for dvp support and the scope of the change is also larger than just for DVP support.

You need also to change all the overlays that contains the ov5640 nodes (it's one of the reason why CI failed for this PR). You can take this commit if you want as I am not sure which one will be merged first.

This change should be in a separate commit that says something like "ov5640: Switch to the new video interfaces binding", as it does not reveal exactly why we need this for dvp support and the scope of the change is also larger than just for DVP support.

Removed this change from ovti,ov5640.yaml

ngphibang · 2024-10-18T08:31:51Z

drivers/video/ov5640.c

-	fmt.width = 1280;
-	fmt.height = 720;
+	if (ov5640_is_dvp(dev)) {
+		/* Set default format to QQVGA (160x120) RGB565 */


/* Set default resolution to QQVGA */ ?

Fixed. Thanks!

ngphibang · 2024-10-18T08:32:15Z

drivers/video/ov5640.c

+		fmt.width = 160;
+		fmt.height = 120;
+	} else {
+		/* Set default format to 720p RGB565 */


/* Set default resolution to 720p */ ?

ngphibang · 2024-10-18T08:43:31Z

drivers/video/ov5640.c

I think to distinguish more exactly two modes : csi2 vs dvp, all the variables / macro should be named with csi2 rather than mipi as mipi is larger and could cover csi1, csi2, no ?

Changed to csi2.

ngphibang · 2024-10-18T08:49:02Z

drivers/video/ov5640.c

@@ -144,7 +147,7 @@ struct ov5640_data {
 	const struct ov5640_mode_config *cur_mode;
 };

-static const struct ov5640_reg init_params[] = {
+static const struct ov5640_reg init_params_common[] = {


For initialization, it seems for CSI2, we need only init_params_common and for DVP we need init_params_common + init_params_dvp, so dvp seems more complicated to initialize than csi2 ? Did you go through init_params_common registers to check if some settings can be specific only to CSI2 ?

@ngphibang, I didn't have a chance to investigate all the registers because many of them are not present in the datasheet. I took these settings from another OV5640 sample code. init_params_dvp has the register setting that is not present in init_params_common.

check if some settings can be specific only to CSI2

Yes I think.

I believe that there are some registers that are needed to be initialized only for CSI2. And if so, these need to be refactored into init_params_csi2() because leaving them in init_params_common() will be redundant for dvp mode. But well, seeing that it's difficult to check, we can leave this point for the future. The essential thing is it works for both modes so it's ok for me.

ngphibang · 2024-10-18T09:14:54Z

drivers/video/ov5640.c

@@ -897,15 +1127,15 @@ static int ov5640_enum_frmival(const struct device *dev, enum video_endpoint_id
 {


ov5640_enum_frmival() is common for both modes, so we cannot return the results of CSI2 for DVP. I think you need to implement enum_frmival() for DVP by returning the supported framerates for each supported format (I suppose DVP only supports 30 fps for all formats as of now ?).

The same for get_frmival(), it needs to return the current frame rate in DVP modes

For set_frmival(), we can return -ENOTSUP

Maybe branching on ov5640_is_dvp() like you did elsewhere would work well.

@ngphibang and @josuah

As you commented, it looks like that the 30FPs is used as default for DVP. In DVP the framerate depends on pixel clock. Currently, the init_params_dvp configured the SC_PLL_CTRL2_REG with 0x64 (PLL multiplier 100) and SC_PLL_CTRL3_REG with 0x13 (PLL root divided by 2 and PLL pre-divider 3). I'll try to measure the current FPS.

The functions set_frmival, get_frmival, and enum_frmival haven't been implemented on video_stm32_dcmi.c driver. That is the reason for those functions haven't broken when the DVP is used. Check here.

So, since those functions are not implemented on DCMI driver, is this a block to accept this PR? Could this be done in another PR? Thanks!

Thank you for the update. No, it's not a blocker. But as being said in my previous comment, we don't need to implement those functions but is it possible to just return a (hardcode) 30 fps for enum_frmival() and get_frmival() and -ENOTSUP for set_frmival() in DCMI driver ?

Well, I've just rethought. Let the frmival callback not implemented is OK as well (especially now we have the new DEVICE_API, it's safe to do so I think). BTW, should the frmival callback in the ov5640 driver be removed as well if it is not tested (due to frmival in dcmi not implemented) ?

Well, I've just rethought. Let the frmival callback not implemented is OK as well (especially now we have the new DEVICE_API, it's safe to do so I think). BTW, should the frmival callback in the ov5640 driver be removed as well if it is not tested (due to frmival in dcmi not implemented) ?

Hi @ngphibang.

Considering those frmival functions are working with CSI2 driver, I am not sure If it is necessary to remove.

Regarding the DCMI, when those functions are calling the error ENOSYS is returned, as expected.

How about returning -ENOTSUP for DCMI in the meantime they get implemented (compromise)?

@josuah,

Those functions are not implemented in DCMI driver (video_stm32_dcmi.c , see here). In this way, when those functions are called, the VIDEO API returns -ENOSYS. So, to return -ENOTSUP I'll need to change the DCMI driver to add some functions that are not supported yet. I'm not sure about.

BTW, should the frmival callback in the ov5640 driver be removed as well if it is not tested

Oops, sorry, please forget my above nonsense comment, it seems coming from an AI-chatbot not me (maybe I was in hurry while writting this :( ).

My point was just that you can leave the framerate implementation for the future, it's not a blocking point. But since ov5640 driver now supports both dvp and csi2 modes, leaving it as-is will make people think that set/get/enum_frival() is actually working / valid for both modes while it is not the case here.

So I mean we should add

if (ov5640_is_dvp()) { return -ENOTSUP; }

into set/enum_frival() (get_frival() does not need it because it works for both modes I think) to clearly say that it is not implemented for DVP in ov5640 driver. I think it will be not an issue at all if DCMI driver does not implement these callback and the VIDEO API returns -ENOSYS.

Hope it is clear now

drivers/video/ov5640.c

ngphibang · 2024-10-18T09:25:12Z

drivers/video/ov5640.c

@@ -650,6 +875,11 @@ static int ov5640_stream_start(const struct device *dev)
 static int ov5640_stream_stop(const struct device *dev)
 {


For DVP mode, I think we need to implement stream start / stop also. Pipeline should be capable to stop and restart. Could you refer to the Linux implementation here

Sorry, @ngphibang. I've forgotten to comment that this is done.

CharlesDias · 2024-11-30T17:54:48Z

Removed zephyr/dts/bindings/video/ovti,ov5640.yaml changes.

CharlesDias · 2024-11-30T18:06:37Z

Rebase and fixed conflict.

josuah

Some minor remarks that you can ignore as you wish.
The only bug is to have CSI2 framerates returned instead of DVP, though, apparently, that does not break DVP mode.

Thank you for extending the OV5640 support beyond just MIPI! Zephyr is full of small devices and MIPI-capable devices can often run Linux.

josuah · 2024-11-30T21:03:33Z

drivers/video/ov5640.c

@@ -897,15 +1127,15 @@ static int ov5640_enum_frmival(const struct device *dev, enum video_endpoint_id
 {


Maybe branching on ov5640_is_dvp() like you did elsewhere would work well.

drivers/video/ov5640.c

tests/drivers/build_all/video/app.overlay

CharlesDias · 2024-12-01T16:14:01Z

Implemented partial @ngphibang and @josuah's suggestions. I still need to work on ov5640_enum_frmival(), get_frmival(), and set_frmival() implementation. Those function updates are missing!

drivers/video/ov5640.c

josuah · 2025-01-10T21:34:43Z

tests/drivers/build_all/video/mimxrt1064_evk.overlay

+
+				port {
+					ov5640_ep_out: endpoint {
+						remote-endpoint-label = "mipi_csi2rx_ep_in";


Maybe having a video RX peripheral remote-endpoint was the reason why you needed this file.
There is an emulated/fake video RX peripheral which you might be able to use for this purpose if you wish to remain hardware independent.

I do not see how using NXP hardware would be an issue for the build-all test though.

Hi, @josuah ,

could you please share more information about this "emulated/fake video RX peripheral"? Thanks!

@CharlesDias @josuah

Why do you add NXP imxrt1064 overlay to test ov5640 dvp mode ? Does this HW support ov5640 camera sensor ? AFAIU, this should be an ST board (with DCMI), no ?

This is a built test so I think we need to make a real board overlay to involve the actual related drivers and actual code into the test (for imxrt1170 : ov5640 in csi2 mode --> mipicsi2rx --> csi, for ST board : ov5640 in dvp --> DCMI) as the purpose of the test is to find (compilation) bugs in real drivers.

Another reason not to use the emulated video drivers is it will test either csi2 or dvp mode, while we need to test both. We already test csi2 on rt1170 so just need to test dvp on the ST board.

could you please share more information about this "emulated/fake video RX peripheral"? Thanks!

I missed that message sorry. Here is the PR that introduced it for your own curiosity
#79482

Here is an example of integration for testing the API itself:
https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/video/api

This PR got merged a bit fast: I am not against replacing it if something else (i.e. sw_generator) makes more sense!

it will test either csi2 or dvp mode
this should be an ST board (with DCMI)

Not sure why I did not think of it, DCMI as a target sounds ideal.
I will be a bit busy for the next 3 weeks, so cannot offer the resources for it until then.

josuah

I think these are the two unresolved points that could need action. The rest seems ready to land AFAICT.

https://github.com/zephyrproject-rtos/zephyr/pull/76124/files#r1806182604
https://github.com/zephyrproject-rtos/zephyr/pull/76124/files#r1806168527

Thanks once again.

CharlesDias · 2025-01-11T09:25:43Z

I think these are the two unresolved points that could need action. The rest seems ready to land AFAICT.

https://github.com/zephyrproject-rtos/zephyr/pull/76124/files#r1806182604
https://github.com/zephyrproject-rtos/zephyr/pull/76124/files#r1806168527

Hi, @josuah,

The first one is done. I leave some comments in the second. Thanks for your review!

ngphibang · 2025-01-15T18:57:30Z

drivers/video/ov5640.c

@@ -144,7 +147,7 @@ struct ov5640_data {
 	const struct ov5640_mode_config *cur_mode;
 };

-static const struct ov5640_reg init_params[] = {
+static const struct ov5640_reg init_params_common[] = {


I believe that there are some registers that are needed to be initialized only for CSI2. And if so, these need to be refactored into init_params_csi2() because leaving them in init_params_common() will be redundant for dvp mode. But well, seeing that it's difficult to check, we can leave this point for the future. The essential thing is it works for both modes so it's ok for me.

ngphibang · 2025-01-15T19:07:50Z

drivers/video/ov5640.c

@@ -650,6 +875,11 @@ static int ov5640_stream_start(const struct device *dev)
 static int ov5640_stream_stop(const struct device *dev)
 {


ngphibang · 2025-01-15T19:28:27Z

drivers/video/ov5640.c

@@ -897,15 +1127,15 @@ static int ov5640_enum_frmival(const struct device *dev, enum video_endpoint_id
 {


BTW, should the frmival callback in the ov5640 driver be removed as well if it is not tested

Oops, sorry, please forget my above nonsense comment, it seems coming from an AI-chatbot not me (maybe I was in hurry while writting this :( ).

My point was just that you can leave the framerate implementation for the future, it's not a blocking point. But since ov5640 driver now supports both dvp and csi2 modes, leaving it as-is will make people think that set/get/enum_frival() is actually working / valid for both modes while it is not the case here.

So I mean we should add

if (ov5640_is_dvp()) { return -ENOTSUP; }

into set/enum_frival() (get_frival() does not need it because it works for both modes I think) to clearly say that it is not implemented for DVP in ov5640 driver. I think it will be not an issue at all if DCMI driver does not implement these callback and the VIDEO API returns -ENOSYS.

Hope it is clear now

ngphibang · 2025-01-15T19:29:54Z

drivers/video/ov5640.c

ngphibang · 2025-01-15T19:39:49Z

tests/drivers/build_all/video/app.overlay

@@ -53,6 +55,13 @@
 				reg = <0x3>;
 				reset-gpios = <&test_gpio 0 0>;
 				powerdown-gpios = <&test_gpio 1 0>;
+
+				port {


This needs to go into a board-specific overlay, here the ST board (with DCMI) that you tested the ov5640 in dvp mode with all the connection with the DCMI, otherwise it will not be built I think. Just take the imxrt1170 overlay in this same folder as a reference.

BTW to use the port and endpoint, you need to modify the DCMI driver to use the new video-interface bindings (it's already done in another PR if I remember well ?)

@ngphibang, thanks! This test framework is a mysterious for me! :) Do you have any reference material when I can learn more about it?

BTW to use the port and endpoint, you need to modify the DCMI driver to use the new video-interface bindings (it's already done in another PR if I remember well ?)

Correctly. This was made by another PR. I haven't had any need to modify the DCMI driver so far.

@ngphibang and @josuah, I removed the imxrt1064 overlay and created on for stm32h7b3i_dk. However, two twister test failed. The twister-build (2) complained against imxrt1064 and twister-build (3) against native_sim.

Could you please give me some help? Thanks!

@CharlesDias : It's because in the ov5640 driver, you get the bus type with:

.bus_type = DT_PROP(DT_CHILD(DT_INST_CHILD(n, port), endpoint), bus_type),

but when the ov5640 driver is tested on the native_sim (drivers.video.build test) and the mimxrt1064_evk (drivers.video.mcux_csi.build test) platforms, the property bus-type is not defined.

So, to fix it, I think the ov5640 node should be removed in the app.overlay as ov5640 driver tests are already covered in drivers.video.mcux_csi.build, drivers.video.mcux_csi2rx.build and drivers.video.stm32_dcmi.build (the one you will add) tests

ngphibang · 2025-01-15T20:01:08Z

tests/drivers/build_all/video/mimxrt1064_evk.overlay

+
+				port {
+					ov5640_ep_out: endpoint {
+						remote-endpoint-label = "mipi_csi2rx_ep_in";


@CharlesDias @josuah

Why do you add NXP imxrt1064 overlay to test ov5640 dvp mode ? Does this HW support ov5640 camera sensor ? AFAIU, this should be an ST board (with DCMI), no ?

This is a built test so I think we need to make a real board overlay to involve the actual related drivers and actual code into the test (for imxrt1170 : ov5640 in csi2 mode --> mipicsi2rx --> csi, for ST board : ov5640 in dvp --> DCMI) as the purpose of the test is to find (compilation) bugs in real drivers.

Another reason not to use the emulated video drivers is it will test either csi2 or dvp mode, while we need to test both. We already test csi2 on rt1170 so just need to test dvp on the ST board.

Add stm32h7b3i_dk.overlay file for test purpose. Signed-off-by: Charles Dias <[email protected]>

Improve the ov5640 video driver to provide parallel interface (DVP) support Signed-off-by: Charles Dias <[email protected]>

CharlesDias · 2025-01-25T16:17:19Z

Oops, sorry, please forget my above nonsense comment, it seems coming from an AI-chatbot not me (maybe I was in hurry while writting this :( ).

My point was just that you can leave the framerate implementation for the future, it's not a blocking point. But since ov5640 driver now supports both dvp and csi2 modes, leaving it as-is will make people think that set/get/enum_frival() is actually working / valid for both modes while it is not the case here.

@ngphibang, I added the

if (ov5640_is_dvp()) {
    return -ENOTSUP;
}

inside of set/get/enum_frival() to be clear that these functions do not support the DVP. I also added inside the get because I am not sure that the returned value is correct.

ngphibang · 2025-01-26T22:01:47Z

tests/drivers/build_all/video/stm32h7b3i_dk.overlay

+
+				port {
+					ov5640_ep_out: endpoint {
+						remote-endpoint-label = "dvp_ep_in";


Where is the dvp_ep_in ? I think you need to add the receiver (DCMI) node. Moreover, a test case should be added for the dcmi driver in testcase.yaml as well, otherwise it will not be tested (your overlay is not used at all).

Also, people have forgot to add test cases when they added their new drivers and reviewers forgot to mention that too :-). They are:

stm32_dcmi

esp32_dvp

smartdma

ngphibang · 2025-01-26T22:13:29Z

tests/drivers/build_all/video/app.overlay

@@ -53,6 +55,13 @@
 				reg = <0x3>;
 				reset-gpios = <&test_gpio 0 0>;
 				powerdown-gpios = <&test_gpio 1 0>;
+
+				port {


@CharlesDias : It's because in the ov5640 driver, you get the bus type with:

.bus_type = DT_PROP(DT_CHILD(DT_INST_CHILD(n, port), endpoint), bus_type),

but when the ov5640 driver is tested on the native_sim (drivers.video.build test) and the mimxrt1064_evk (drivers.video.mcux_csi.build test) platforms, the property bus-type is not defined.

So, to fix it, I think the ov5640 node should be removed in the app.overlay as ov5640 driver tests are already covered in drivers.video.mcux_csi.build, drivers.video.mcux_csi2rx.build and drivers.video.stm32_dcmi.build (the one you will add) tests

ngphibang · 2025-01-26T22:16:32Z

tests/drivers/build_all/video/stm32h7b3i_dk.overlay

@@ -0,0 +1,49 @@
+/*
+ * Copyright (c) 2024, Charles Dias <[email protected]>


CharlesDias changed the title ~~Ov5640 add parallel interface~~ Improve the ov5640 video driver to provide parallel interface (DVP) support Jul 19, 2024

CharlesDias marked this pull request as draft July 19, 2024 22:16

zephyrbot added the area: Video Video subsystem label Jul 19, 2024

zephyrbot requested a review from loicpoulain July 19, 2024 22:16

CharlesDias mentioned this pull request Jul 19, 2024

Improve the ov5640 video driver to provide parallel interface (DVP) support. #74353

Open

3 tasks

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 635e3a1 to 73e0ea7 Compare July 21, 2024 19:36

CharlesDias changed the title ~~Improve the ov5640 video driver to provide parallel interface (DVP) support~~ Add parallel interface (DVP) support for ov5640 driver Jul 21, 2024

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 73e0ea7 to 77c2eeb Compare July 21, 2024 19:57

josuah reviewed Jul 21, 2024

View reviewed changes

ngphibang reviewed Jul 22, 2024

View reviewed changes

CharlesDias mentioned this pull request Jul 22, 2024

boards: shields: add suport for st_b_cams_omv_mb1683 #76143

Open

josuah mentioned this pull request Jul 27, 2024

drivers: video: image sensor list (from Linux, others) #76378

Open

kartben self-requested a review August 12, 2024 22:14

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 77c2eeb to c0038bd Compare September 14, 2024 21:20

CharlesDias force-pushed the ov5640_add_parallel_interface branch from c0038bd to 7e2dfdf Compare September 14, 2024 21:24

CharlesDias changed the title ~~Add parallel interface (DVP) support for ov5640 driver~~ [DNM] Add parallel interface (DVP) support for ov5640 driver Sep 14, 2024

CharlesDias marked this pull request as ready for review September 14, 2024 21:48

josuah reviewed Sep 15, 2024

View reviewed changes

drivers/video/ov5640.c Outdated Show resolved Hide resolved

drivers/video/ov5640.c Outdated Show resolved Hide resolved

drivers/video/ov5640.c Show resolved Hide resolved

drivers/video/ov5640.c Outdated Show resolved Hide resolved

josuah added the DNM This PR should not be merged (Do Not Merge) label Sep 16, 2024

josuah changed the title ~~[DNM] Add parallel interface (DVP) support for ov5640 driver~~ Add parallel interface (DVP) support for ov5640 driver Sep 29, 2024

CharlesDias marked this pull request as ready for review October 12, 2024 19:34

CharlesDias mentioned this pull request Oct 13, 2024

boards: shields: add suport for weact_ov5640_cam_module #79759

Closed

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 9fd4d1d to 93f9d04 Compare October 15, 2024 20:48

ngphibang reviewed Oct 18, 2024

View reviewed changes

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 93f9d04 to 89e8d76 Compare November 30, 2024 17:53

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 89e8d76 to 222fcde Compare November 30, 2024 18:05

CharlesDias force-pushed the ov5640_add_parallel_interface branch 3 times, most recently from 2960221 to 75980f9 Compare November 30, 2024 21:02

josuah requested changes Nov 30, 2024

View reviewed changes

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 75980f9 to e5814c4 Compare November 30, 2024 21:38

josuah reviewed Dec 1, 2024

View reviewed changes

tests/drivers/build_all/video/app.overlay Outdated Show resolved Hide resolved

CharlesDias force-pushed the ov5640_add_parallel_interface branch from e5814c4 to 52ce484 Compare December 1, 2024 16:08

josuah reviewed Jan 10, 2025

View reviewed changes

drivers/video/ov5640.c Show resolved Hide resolved

josuah reviewed Jan 10, 2025

View reviewed changes

josuah requested changes Jan 10, 2025

View reviewed changes

ngphibang reviewed Jan 15, 2025

View reviewed changes

CharlesDias added 2 commits January 25, 2025 15:56

tests: drivers: build_all: video: add stm32h7b3i_dk overlay

6c8abf9

Add stm32h7b3i_dk.overlay file for test purpose. Signed-off-by: Charles Dias <[email protected]>

drivers: video: add DVP support to ov5640

37229cc

Improve the ov5640 video driver to provide parallel interface (DVP) support Signed-off-by: Charles Dias <[email protected]>

CharlesDias force-pushed the ov5640_add_parallel_interface branch from 52ce484 to 37229cc Compare January 25, 2025 16:04

ngphibang reviewed Jan 26, 2025

View reviewed changes

		@@ -897,15 +1127,15 @@ static int ov5640_enum_frmival(const struct device *dev, enum video_endpoint_id
		{

		@@ -650,6 +875,11 @@ static int ov5640_stream_start(const struct device *dev)
		static int ov5640_stream_stop(const struct device *dev)
		{

		@@ -0,0 +1,49 @@
		/*
		* Copyright (c) 2024, Charles Dias <[email protected]>

Add parallel interface (DVP) support for ov5640 driver #76124

Are you sure you want to change the base?

Add parallel interface (DVP) support for ov5640 driver #76124

Conversation

CharlesDias commented Jul 19, 2024 • edited Loading

CharlesDias commented Jul 21, 2024

josuah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesDias Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

josuah commented Jul 22, 2024

ngphibang Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

CharlesDias Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

ngphibang Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

CharlesDias commented Jul 22, 2024

ngphibang commented Jul 22, 2024

CharlesDias commented Jul 24, 2024

CharlesDias commented Sep 14, 2024

CharlesDias commented Sep 14, 2024

josuah left a comment

Choose a reason for hiding this comment

CharlesDias commented Sep 16, 2024

josuah commented Sep 16, 2024

CharlesDias commented Oct 10, 2024

CharlesDias commented Oct 12, 2024 • edited Loading

ngphibang commented Oct 14, 2024

CharlesDias commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

CharlesDias Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

ngphibang Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesDias Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesDias commented Nov 30, 2024

CharlesDias commented Nov 30, 2024

josuah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesDias commented Dec 1, 2024

Choose a reason for hiding this comment

CharlesDias Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

ngphibang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah left a comment

CharlesDias commented Jul 19, 2024 •

edited

Loading

CharlesDias Sep 16, 2024 •

edited

Loading

ngphibang Jul 22, 2024 •

edited

Loading

CharlesDias Jul 22, 2024 •

edited

Loading

ngphibang Jul 22, 2024 •

edited

Loading

CharlesDias commented Oct 12, 2024 •

edited

Loading

CharlesDias commented Oct 15, 2024 •

edited

Loading

ngphibang Oct 18, 2024 •

edited

Loading

ngphibang Oct 18, 2024 •

edited

Loading

ngphibang Oct 18, 2024 •

edited

Loading

CharlesDias Nov 30, 2024 •

edited

Loading

ngphibang Jan 15, 2025 •

edited

Loading

ngphibang Oct 18, 2024 •

edited

Loading

ngphibang Dec 10, 2024 •

edited

Loading

CharlesDias Dec 21, 2024 •

edited

Loading

ngphibang Jan 15, 2025 •

edited

Loading

CharlesDias Jan 11, 2025 •

edited

Loading

ngphibang Jan 15, 2025 •

edited

Loading

CharlesDias commented Jan 11, 2025 •

edited

Loading

ngphibang Jan 15, 2025 •

edited

Loading

ngphibang Jan 15, 2025 •

edited

Loading

ngphibang Jan 26, 2025 •

edited

Loading

ngphibang Jan 15, 2025 •

edited

Loading

ngphibang Jan 26, 2025 •

edited

Loading

ngphibang Jan 26, 2025 •

edited

Loading