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

drivers: video: esp32s3: add support for cam interface #77709

Merged
merged 1 commit into from
Sep 6, 2024
Merged

drivers: video: esp32s3: add support for cam interface #77709

merged 1 commit into from
Sep 6, 2024

Conversation

LucasTambor
Copy link
Collaborator

Add support for the esp32s3 LCD_CAM peripheral.

@zephyrbot zephyrbot added area: Video Video subsystem area: Xtensa Xtensa Architecture platform: ESP32 Espressif ESP32 labels Aug 28, 2024
@LucasTambor
Copy link
Collaborator Author

This commit is a shared work between @epc-ake and me. Mostly based on his PR (#75331) and some internal discussions.

uLipe
uLipe previously approved these changes Aug 28, 2024
View reviewed changes
Copy link
Collaborator

@uLipe uLipe left a comment

Choose a reason for hiding this comment

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

LGTM so far, there is just a small nit, but nothing blocker at all, feel free to skip it.

drivers/video/video_esp32_dvp.c
{
int ret = 0;

ret = pinctrl_apply_state(esp32_config.pcfg, PINCTRL_STATE_DEFAULT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think since this is not related to clock, it is better to be moved to the global esp32_cam_init?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I need this guy here to configure the pin that generates the clock for the cam. But it is duplicated, I'll fix it and change the function name.

Copy link

Choose a reason for hiding this comment

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

Maybe a little comment about this would help future maintenance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer self-explanatory code like a function or something, I'm afraid a comment here may get outdated silently until is not reflecting the actual code.

@LucasTambor LucasTambor force-pushed the esp32s3/lcd_cam branch 2 times, most recently from f89f7a3 to 7a87208 Compare August 28, 2024 17:07
@zephyrbot
Copy link
Collaborator

zephyrbot commented Aug 28, 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_espressif DNM This PR should not be merged (Do Not Merge) labels Aug 28, 2024
@LucasTambor LucasTambor force-pushed the esp32s3/lcd_cam branch 2 times, most recently from 9d3963e to a404c18 Compare August 28, 2024 19:52
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Aug 28, 2024
@epc-ake epc-ake mentioned this pull request Sep 2, 2024
Closed
wmrsouza
wmrsouza previously approved these changes Sep 3, 2024
View reviewed changes
sylvioalves
sylvioalves reviewed Sep 3, 2024
View reviewed changes
west.yml Outdated Show resolved Hide resolved
raffarost
raffarost previously approved these changes Sep 3, 2024
View reviewed changes
drivers/video/video_esp32_dvp.c Outdated Show resolved Hide resolved
drivers/video/video_esp32_dvp.c Show resolved Hide resolved
dts/xtensa/espressif/esp32s3/esp32s3_common.dtsi Show resolved Hide resolved
drivers/video/video_esp32_dvp.c Outdated Show resolved Hide resolved
drivers/video/video_esp32_dvp.c Outdated Show resolved Hide resolved
drivers/video/video_esp32_dvp.c Outdated Show resolved Hide resolved
drivers/video/video_esp32_dvp.c Show resolved Hide resolved
drivers/video/video_esp32_dvp.c Outdated Show resolved Hide resolved
drivers/video/video_esp32_dvp.c Outdated Show resolved Hide resolved
uLipe
uLipe previously approved these changes Sep 4, 2024
View reviewed changes
loicpoulain
loicpoulain previously approved these changes Sep 4, 2024
View reviewed changes
@marekmatej
Copy link

Need to address those comments, otherwise LGTM.

@epc-ake @LucasTambor
drivers: video: esp32s3: add support for cam interface
a63db63 
Adding support for the esp32s3 LCD_CAM peripheral.

Signed-off-by: Armin Kessler <[email protected]>
@nashif nashif merged commit 576fc20 into zephyrproject-rtos:main Sep 6, 2024
25 checks passed
@josuah josuah mentioned this pull request Nov 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raffarost raffarost raffarost approved these changes

@wmrsouza wmrsouza wmrsouza approved these changes

@sylvioalves sylvioalves sylvioalves approved these changes

@uLipe uLipe uLipe approved these changes

@andyross andyross Awaiting requested review from andyross

@ceolin ceolin Awaiting requested review from ceolin

@dcpleung dcpleung Awaiting requested review from dcpleung

@nashif nashif Awaiting requested review from nashif

@marekmatej marekmatej Awaiting requested review from marekmatej

@loicpoulain loicpoulain Awaiting requested review from loicpoulain

Assignees

@sylvioalves sylvioalves

Labels
area: Video Video subsystem area: Xtensa Xtensa Architecture platform: ESP32 Espressif ESP32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@LucasTambor @zephyrbot @marekmatej @uLipe @sylvioalves @loicpoulain @wmrsouza @raffarost @nashif @epc-ake