Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for camera on i.MX RT11XX #69810

Closed
wants to merge 24 commits into from
Closed

Add support for camera on i.MX RT11XX #69810

wants to merge 24 commits into from

Conversation

ngphibang
Copy link
Contributor

This PR adds support to enable camera on i.MX RT11XX series. It adds

  • ov5640 camera driver
  • MIPI CSI-2 Rx driver
  • Modify CSI driver to support both RT11XX and RT10XX
  • Improve the video capture sample to display frames on LCD panel instead of just discarding them. For this to work, another PR should be merged first.

@zephyrbot zephyrbot added platform: NXP NXP area: Shields Shields (add-on boards) area: Devicetree Binding PR modifies or adds a Device Tree binding area: Samples Samples platform: NXP Drivers NXP Semiconductors, drivers area: Video Video subsystem labels Mar 5, 2024
This was referenced Mar 5, 2024
Merged
Merged
@zephyrbot
Copy link
Collaborator

zephyrbot commented Mar 7, 2024 •
edited
Loading

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Mar 7, 2024
@ngphibang
Copy link
Contributor Author

ngphibang commented Mar 8, 2024 •
edited
Loading

Hi @danieldegrasse , @decsny , @DerekSnell, could you please advise on the naming of the MIPI CSI-2 Rx driver ?

Currently, I named it "mipi_csi2rx_nxp" because I remembered @DerekSnell has said that :

We should not use imx in the name. We reuse our IP everywhere across SOC families. So we should not include SOC family names in the drivers or bindings.

@decsny also proposed "CONFIG_VIDEO_MCUX_MIPI_CSI2RX". I think it is correct too. But frankly, I am a bit confused between "mcux" and "imx" (imx seems covers both mcu and mpu ?). So, I hesitated too much.

Is it ok for you if we name them as below ?

  • CONFIG_VIDEO_MCUX_MIPI_CSI2RX
  • video_mcux_mipi_csi2rx.c
  • nxp,mipi-csi2rx.yaml
  • Kconfig.mcux_mipi_csi2rx

Thanks

@ngphibang
tests: drivers: build_all: video: Add entry for ov5640
41f5802 
Add entry for ov5640 and add mimxrt1170_evk to the test platforms

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
@ngphibang
boards: nxp: mimxrt1170_evk: Enable video feature
1325e3e 
Enable video feature on cm7 evk and evkb which are tested with ov5640
camera.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
@ngphibang
Copy link
Contributor Author

ngphibang commented Apr 15, 2024 •
edited
Loading

@danieldegrasse I updated the PR which addressed all of your comments in my possibility (for some comments that I cannot make the changes, I left my arguments).

@loicpoulain , @danieldegrasse , @dleach02 : Really need your helps to make this PR moving forward as it has been here for quite a while.

Thanks !

@loicpoulain
Copy link
Collaborator

@ngphibang could you split this PR in different logical PRs (like one for misc fixes, one for new camera driver, etc...), it will be easier for you to have them reviewed and integrated.

@ngphibang
Copy link
Contributor Author

@loicpoulain Okay, I will do it. But the problem is the next PR will depend on all previous PRs and Zephyr requires that each commit / PR is bisectable and buildable.

Would you mind if I include all the commits of the previous PRs into another PR ? For example :

PR 1
PR 2 = its commits + commits of PR 1
PR 3 = its commits + all PR2

This was referenced Apr 24, 2024
Merged
Merged
Merged
@ngphibang
Copy link
Contributor Author

@loicpoulain The first 3 splitted PRs which are independent each others are pushed.
Waiting for your reviews and integration before continuing to push the next ones which depend on these.

This was referenced May 7, 2024
Merged
Merged
Merged
Merged
Merged
Closed
Merged
mmahadevan108
mmahadevan108 reviewed May 14, 2024
View reviewed changes
drivers/video/video_mcux_csi.c

if (!bpp || ep != VIDEO_EP_OUT) {
return -EINVAL;
}

data->pixelformat = fmt->pixelformat;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be deleted from video_mcux_csi_data

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this commit can be squashed with the previous commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is addressed and merged.

mmahadevan108
mmahadevan108 reviewed May 14, 2024
View reviewed changes
dts/arm/nxp/nxp_rt11xx.dtsi
@@ -1230,3 +1230,7 @@
*/
status = "disabled";
};

zephyr_cam_i2c: &lpi2c6 {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems board specific and probably belongs in the board overlay file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is pointed out by Daniel and I addressed it in the split PR.

mmahadevan108
mmahadevan108 reviewed May 14, 2024
View reviewed changes
boards/nxp/mimxrt1064_evk/mimxrt1064_evk.dts
csi_ep_in: endpoint {
remote-endpoint = <&mt9m114_ep_out>;
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add the labels related to the new shield you have created for 1064?

@ngphibang
Copy link
Contributor Author

@mmahadevan108 : Thank you for your reviews ! But this PR is split into smaller one as requested by Loic. Could you review them instead ? I don't know if we can add a label "do not review" or "stale" or something like that on this PR ?

@DerekSnell DerekSnell removed their request for review May 14, 2024 21:51
mmahadevan108
mmahadevan108 reviewed May 14, 2024
View reviewed changes
samples/subsys/video/capture/README.rst
@@ -7,9 +7,8 @@
Description
***********
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been tested in 1064_evk after the changes to the sample? Do we need a boards overlay file for 1064_evk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's tested. The board overlay for 1064 evk is in the same PR.

@danieldegrasse
Copy link
Collaborator

@ngphibang is this PR still needed? My understanding was that all the components of this PR had been split into smaller PRs. If we don't need this one open, let's go ahead and close it out to avoid additional review noise

@ngphibang
Copy link
Contributor Author

I thought that we should keep it as a reference and just close it when all the split PRs merged.
I close it now, sorry.

@ngphibang
Copy link
Contributor Author

Close to avoid additional review noise

@ngphibang ngphibang closed this May 14, 2024
@danieldegrasse
Copy link
Collaborator

I thought that we should keep it as a reference and just close it when all the split PRs merged.
I close it now, sorry.

No worries- just don't want anyone reviewing the enablement to get confused by this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmahadevan108 mmahadevan108 mmahadevan108 left review comments

@dleach02 dleach02 dleach02 left review comments

@danieldegrasse danieldegrasse danieldegrasse left review comments

@avisconti avisconti Awaiting requested review from avisconti

@dbaluta dbaluta Awaiting requested review from dbaluta

@decsny decsny Awaiting requested review from decsny

@EmilioCBen EmilioCBen Awaiting requested review from EmilioCBen

@erwango erwango Awaiting requested review from erwango

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@kartben kartben Awaiting requested review from kartben

@loicpoulain loicpoulain Awaiting requested review from loicpoulain

@manuargue manuargue Awaiting requested review from manuargue

@nashif nashif Awaiting requested review from nashif

@yvanderv yvanderv Awaiting requested review from yvanderv

Assignees

@dleach02 dleach02

Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Samples Samples area: Shields Shields (add-on boards) area: Video Video subsystem platform: NXP Drivers NXP Semiconductors, drivers platform: NXP NXP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ngphibang @zephyrbot @decsny @DerekSnell @loicpoulain @dleach02 @danieldegrasse @mmahadevan108