-
Notifications
You must be signed in to change notification settings - Fork 8.3k
drivers: video: Add ov5642 camera driver #97106
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.
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
drivers: video: Add ov5642 camera driver #97106
Conversation
|
Hello @hardevsinh-1, and thank you very much for your first pull request to the Zephyr project! |
|
Thanks @hardevsinh-1 for this PR. There is already an existing driver for the OV5640 sensor. Having very very rapidly checked the difference between the OV5642 and the OV5640, it seems that the OV5642 has less features than the OV5640, having for example only DVP support and limited to 720p maximum frames. Do you know the differences between the OV5640 and the OV5642 and the difference in the way to control it ? Looking very rapidly, it seems that at least on some aspects it is very similar to the OV5640. Of course if the sensor are very different or if the way to control them is very different, then it makes sense to have a different driver. |
@avolmat-st, Thanks for the review.
Given these differences, what would you recommend ? |
Hi @avolmat-st, did you get a chance to review my comparison between the OV5640 and OV5642 sensors? |
josuah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this new driver!
Image sensor drivers are almost just configuration data.
I agree that it is worth considering de-duplicating all the code with ov5640.cvideo_common.c.
It might be possible to reduce the driver by a fair 50% once all video APIs are in place.
It is maybe surprising that copying an existing image sensor driver as much as possible leads to a lot of requests for modifications, but this is because image sensor currently in Zephyr almost all need a round of refactoring and I am very late on this!
Finally, you would need to add an extra commit to add this sensor to the build_all test:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/build_all/video/app.overlay
@josuah , Thanks for the reviewing. I will update the same. |
596b096 to
f7d02c4
Compare
josuah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes!
It seems almost ready, with just a few minor coding style tuning but otherwise seems ready to me.
One more cycle to go!
f7d02c4 to
dfcf0fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the edits!
I think there might be one more round needed as I changed my mind after doing other reviews and gave wrong instructions last time (sorry!).
Maybe other reviewers will find other things after that but for my part I think that's all of it after this round.
dfcf0fe to
3d83017
Compare
Thanks @josuah , I’m interested in adjusting these lines. I’ve made changes based on your suggestions. Could you please review them? |
josuah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the edits! The driver looks ready now.
But there is a "merge commit" that appeared, and you need to remove it.
Then this driver can be merged I think.
drivers/video/ov5642.c
Outdated
|
|
||
| #define PI 3.141592654 | ||
|
|
||
| #define ABS(a, b) (a > b ? a - b : b - a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you are now using the libc <stdlib.h> version abs() now, so you do not need this define.
052f7ce to
42ae093
Compare
|
Good catch for I think you still need to remove the merge commits: If you wish to catch-up with latest Zephyr Now you would need to remove them, for instance with Feel free to reach on the Discord chat if you wanted to troubleshoot this interactively https://discord.com/invite/Ck7jw53nU2 |
42ae093 to
49a9390
Compare
673ed08 to
0bee337
Compare
0bee337 to
4f83c1c
Compare
| case VIDEO_EXPOSURE_AUTO: | ||
| ret = video_modify_cci_reg(&cfg->i2c, | ||
| AEC_PK_CCI, BIT(0), 0); | ||
|
|
||
| break; | ||
| case VIDEO_EXPOSURE_MANUAL: | ||
| ret = video_modify_cci_reg(&cfg->i2c, | ||
| AEC_PK_CCI, BIT(0), BIT(0)); | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two cases can be folded and may add a TODO to remember adding support for manual mode ?
case VIDEO_EXPOSURE_AUTO:
case VIDEO_EXPOSURE_MANUAL:
/* TODO: Add support for manual exposure mode */
ret = video_modify_cci_reg(&cfg->i2c, AEC_PK_CCI, BIT(0), value == VIDEO_EXPOSURE_AUTO ? BIT(0), 0);
drivers/video/ov5642.c
Outdated
| default: | ||
| return -EINVAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two cases need to be added as "unsupported" ?
case:
VIDEO_EXPOSURE_SHUTTER_PRIORITY:
VIDEO_EXPOSURE_APERTURE_PRIORITY:
return -ENOTSUP;
then the default case:
default:
CODE_UNREACHABLE;
as the video framework already handles values out of the control value range.
4f83c1c to
dc90d33
Compare
| case VIDEO_EXPOSURE_AUTO: | ||
| case VIDEO_EXPOSURE_MANUAL: | ||
| /* TODO: Add support for manual exposure mode */ | ||
| ret = video_modify_cci_reg(&cfg->i2c, AEC_PK_CCI, BIT(0), | ||
| value == VIDEO_EXPOSURE_AUTO ? 0 : BIT(0)); | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that VIDEO_EXPOSURE_AUTO = 0 and VIDEO_EXPOSURE_MANUAL = 1, this can be simplified a bit more ?
`ret = video_modify_cci_reg(&cfg->i2c, AEC_PK_CCI, BIT(0), value);`
dc90d33 to
5917aae
Compare
ngphibang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks !
|
@avolmat-st and @josuah, could you please review again? |
|
@avolmat-st and @josuah, please review this PR again and let me know your input if I need to update anything. |
avolmat-st
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
sorry for the delay. This looks good to me. Few multi-lines calls could be done in single line but that's really nitpicking so I am all fine with the current code as well.
Thanks.
drivers/video/ov5642.c
Outdated
| {SDE_CTRL2_REG, abs(sin_coef)} | ||
| }; | ||
|
|
||
| ret = video_modify_cci_reg(&cfg->i2c, SDE_CTRL10_CCI, 0x7F, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: can fit in one line.
drivers/video/ov5642.c
Outdated
| return ret; | ||
| } | ||
|
|
||
| return video_write_cci_multiregs16(&cfg->i2c, hue_params, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: can fit in one line.
| return ret; | ||
| } | ||
|
|
||
| ret = video_modify_cci_reg(&cfg->i2c, SDE_CTRL10_CCI, BIT(2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: can fit in one line.
drivers/video/ov5642.c
Outdated
| return ret; | ||
| } | ||
|
|
||
| return video_write_cci_reg(&cfg->i2c, SDE_CTRL9_CCI, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: can fit in one line.
drivers/video/ov5642.c
Outdated
|
|
||
| switch (id) { | ||
| case VIDEO_CID_HFLIP: | ||
| return video_modify_cci_reg(&cfg->i2c, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: can fit in two lines ? same below ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avolmat-st , Thanks for the suggestions. I have updated the PR to resolve alignment according to the suggestions. No other changes related to logic.
@avolmat-st, @ngphibang and @josuah, Please review it again.
c467b40
c467b40 to
439ae27
Compare
avolmat-st
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Approved.
439ae27 to
1023925
Compare
drivers/video/CMakeLists.txt
Outdated
| zephyr_library_sources_ifdef(CONFIG_VIDEO_STM32_JPEG video_stm32_jpeg.c) | ||
| zephyr_library_sources_ifdef(CONFIG_VIDEO_HIMAX_HM01B0 hm01b0.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ngphibang , It was a rebase issue. I resolved it.
Add driver to support ov5642 camera sensor Co-developed-by: Rutvij Trivedi <rutvij.trivedi@siliconsignals.io> Signed-off-by: Rutvij Trivedi <rutvij.trivedi@siliconsignals.io> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
1023925 to
c50c81f
Compare
|
|
Hi @Hardevsinh-Palaniya! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |




Add driver to support ov5642 camera sensor