-
Notifications
You must be signed in to change notification settings - Fork 7.3k
video: gc2145: add Galaxy Core GC2145 sensor support and basic controls #77770
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
Conversation
Not sure if the compatible should be TI or GC from galaxy core, it seems the company now belongs to TI, happy to hear your suggestions. |
Are you sure? 🤔 My guess is Google's first hit for this IC is what got you confused but I really can't seem to find any mention of the company having been acquired? |
@kartben you are right, I was tricked because the only PDF datasheet I found opened on the Texas Instruments domain, but it is actually in the TI e2e forum, in any case I changed to reflect the Galaxy Core initials the bindings and the compat symbol. |
Thanks @fabiobaltieri , I mostly copied this driver from the OV2640, which seems to be the most complete one. I Will apply the suggestions soon. |
I see, the subsystem is "odd fixes" unfortunately so the code is somewhat incoherent, if you have spare cycles for it there's definitely space for improvements, like adding a common |
73662a8
to
e53bd13
Compare
@fabiobaltieri I pick some spare time, to see what I can improve regarding the log and error handling on the video sensor drivers |
Hi folks, any updates here? Thank you :) |
@fabiobaltieri could you have another look? Also @pillo79 , @loicpoulain can I have your input as well, I already have a temporary branch with complete camera support PoC for the Nicla vision, only depending on merging this sensor driver. Thank you in advance for your efforts. |
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 for your efforts on the Nicla @uLipe! 🥳
I am no expert in this subsystem, but it looks proper to me on the implementation side. I can offer additional nagging on the style though... 🙂
The list below is not complete, there are several similar lines. Please review the whole file and fix the indentations so that:
- all wrapping lines align to the rightmost open brace
- use tabs (set size 8 on your editor) and then spaces for precise aligment
- consider wrapping anything after 80 chars, absolute limit is 100 chars per line including indent.
These are a quite strict convention in Zephyr.
Thanks @pillo79 for your review , it is odd why the compliance check (neither local, nor the remote) are not failling with those style stuff, in general they should check for this and fail in CI |
e53bd13
to
5396dc5
Compare
@pillo79 , I just ran the clang-format and did some small adjustements, also ran the check compliance script locally before push. I'm not sure why those spacing and alignment stuff was not captured, in any case PTAL again when you find time. |
@fabiobaltieri , just letting the device tree to set the speed is sufficient and it is not needed to be called again. I Also tried to document better the magics based on the datasheet I found of the GC2145, PTAL again when you can. |
32356ae
to
d886c7d
Compare
d886c7d
to
07bba8c
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.
LGTM
@fabiobaltieri , @pillo79 , sorry folks to bother you on that again, it is just because I need your okay to proceed on this one and then unblock nicla vision camera full support. |
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.
Here is an unofficial review (I'm not assigned to Video yet).
High time that I implement video_sensor_skeleton.c
to have something
review-ready to use as a starting point!
Thank you for this extra sensor, first of Galaxy Core family!
support and basic controls. Signed-off-by: Felipe Neves <ryukokki.felipe@gmail.com>
07bba8c
to
aa4d0c6
Compare
Thanks for reviewing it, PTAL again @josuah , @loicpoulain |
/* Initiate system reset */ | ||
ret = gc2145_write_reg(&cfg->i2c, GC2145_REG_RESET, GC2145_REG_SW_RESET); | ||
|
||
k_msleep(300); |
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.
Pointing at it in case you wanted to convert all occurrences of k_msleep()
to k_sleep(K_MSEC())
...
Maybe this is where the comment block belongs the most, as it is the longest delay?
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.
Note: my reviews are indicative only (gray mark, not green), but glad if that helps saving some time for when the actual reviews come.
return 0; | ||
} | ||
/* If writing failed wait 5ms before next attempt */ | ||
k_msleep(5); |
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.
Pointing at it in case you wanted to convert all occurrences of k_msleep()
to k_sleep(K_MSEC())
...
return 0; | ||
} | ||
/* If writing failed wait 5ms before next attempt */ | ||
k_msleep(5); |
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.
Pointing at it in case you wanted to convert all occurrences of k_msleep()
to k_sleep(K_MSEC())
...
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 for the review @josuah. Will gladly lend my +1!
@josuah feel very free to send a PR against MAINTAINERS.yml to add yourself as a collaborator :) |
Adding Galaxy Core, GC2145 Image sensor I2C driver, the datasheet documentation is very unclear, and the only two references I found is the linux kernel driver and the driver used in Arducam.
This sensor is the same used in the cameras used by Arduino Nicla Vision, so having this sensor support aims to unblock the full support of the camera for the Nicla Vision board (STM32 DCMI is already present in the main branch).
Unfortunately those two drivers uses lots of hardcoded registers to getting the camera full working, so only some very basic controls and the RGB565 format is supported.
The Arduino Nicla Vision support has been merged recently to the upstream and the camera is one of the outstanding feature that needs to be supported:
#77501