-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Add a shield for DVP FPC 24-pins mt9m114 camera modules #72435
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
Add a shield for DVP FPC 24-pins mt9m114 camera modules #72435
Conversation
b29f2e0
to
3858f01
Compare
|
||
port { | ||
csi_ep_in: endpoint { | ||
remote-endpoint = <&mt9m114_ep_out>; |
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.
how is this building, isn't it circular?
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.
The use of port, endpoint and remote-endpoint concepts follows what is done in Linux, see video-interfaces.yaml, but there is currently no backend implementation yet in Zephyr.
But when it is implemented, it has to be buildable
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.
AFAIK circular phandles are not supported by zephyr DT tooling, so this overlay seems invalid for zephyr
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.
@decsny : I think it is misunderstood here. I know that Zephyr tool will check phandle dependencies at compile time and circular phandles are not allowed. But this is not the case. These are not phandles but "endpoints" and "remote-endpoints" as you can see as well, it actually builds.
It is not me who introduced these into Zephyr. In this PR, I just moved existed things from devicetree to an overlay. Please see this commit in this same PR.
In fact, "endpoints" and "remote-endpoints" concepts are adopted into Zephyr and it is not limited to video, it is used for display too, please see @MaureenHelm 's commit message here. The video APIs by @loicpoulain also uses it.
It is a bit out of scope of this PR but "port/endpoint" (circular dependency) is a good concept that needs to be implemented in the future.
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.
okay, I actually don't get what's going on here, I will see what is going on with these overlays because maybe my understanding is wrong, because I would have thought none of these overlays can build
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.
Yes, the overlay actually builds and the mimxrt1064_evk.dts also builds with these nodes defined like that before my commit.
The discussion about endpoint / circular dependency is good but deserves another topic I think.
This tiny / trivial PR just moves the camera nodes which are already defined in the mimxrt1064_evk.dts to a separate overlay because, as said in the commit message:
"The mt9m114 camera module is not always attached to this board. Camera stuffs should go into a seperate overlay / shield. Also, the CSI should be enabled only if there is a camera sensor attached to it."
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.
@decsny : If you think "port/endpoint" concept should not be in Zephyr for the time being (because it is currently not implemented). We can submit a PR to remove them all from Zephyr (video API need to be changed as well because it uses endpoint, I think ?).
BTW, this PR simply moves the existing camera nodes from mimxrt1064_evk.dts to a separate overlay ...
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.
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.
AFAIK circular phandles are not supported by zephyr DT tooling, so this overlay seems invalid for zephyr
This was further characterized recently: #57708
I tried to go to the bottom of the implication of a
remote-endpoint
in Zephyr here: #72311
thanks for this information, it's exactly what I needed I think, I will look into this further soon to see what is going on here
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.
overlay appears invalid for zephyr DT
3858f01
to
b4ba65d
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.
approving because @ngphibang accurately pointed out that the part of the PR that I have a problem with is actually only a move, not new, and it's correct these DT items should be in a shield
@kartben, can you help review this PR. |
Yeah, I think that's accurate. Sort of a tricky problem. Perhaps we prefix the node name with the manufacturer of the module? |
@ngphibang @danieldegrasse I think what's really confusing is that the shield name, and README, refer to it very generically as a "MT9M114 Camera Module" so I am naively (and it looks, wrongly?) assuming this is some kind of generic module for the MT9M114 CMOS chip exposing an FPC connector. If it is in fact a module built for NXP boards specifically, then I would like to ask you to please clarify the naming and README so that it is clearer what actual piece of hardware this is referring to. btw @erwango might be able to chime in here and provide some insights |
@kartben After discussing internally and searching on the internet, it appears that this DVP (Digital Video Port, aka parallel) 24-pins FPC camera interface is generic and not specific to NXP. We have at least another ov7725 camera module that can be plugged into RT10xx boards as a replacement for the MT9M114 module. I also found some camera modules sold on Aliexpress and other pages which have the same form factor and pin function order with ours : https://www.supertekmodule.com/wp-content/uploads/2018/11/ST-OV7725DVP-Datasheet-V2.1.pdf So, I would rename the shield as : dvp_fpc24 with dvp_fpc24_i2c and dvp_fpc24_interface nodes inside it. The naming could be ok now but I have a technical issue that I don't know how to resolve it. The sensor node is specific (it could be mt9m114 or ov7725, etc.), so I have to put them into
And in the dvp_fpc24 shield overlay, we have
But the problem is the board overlay is parsed AFTER the shield overlay so we have an error "undefined cam_module node label" here. Do you know if there is a solution for this ? i.e., something like forward declaration node ... ? Thanks |
0f05313
to
ea181db
Compare
Reworked the PR according to the above comments. The "port / endpoint" part is common and it's better if it is put in dvp_fpc24.overlay rather than a board overlay but I don't know how resolve the issue mentionned in my previous comment. |
I found another board that seems to use that pinout too but a hirose connector instead:
|
I'm confused why we would have a board overlay for this- it seems like the generic connector is the DVP FPC 24 pin interface, and the specific shield is the MT9M114 sensor module. Given this, I think we should define the DVP FPC 24 pin interface at the board level. This is analogous to Arduino shields, where the Arduino interface is defined at the board level (IE in the board devicetree, not as an overlay), then the shield references the nodes defined at the board level (which have standard names) In my mind, a module with the OV7725 sensor should be a different shield definition |
The mt9m114 camera module is not always attached to this board. Camera stuffs should go into a seperate overlay / shield. Also, the CSI should be enabled only if there is a camera sensor attached to it. Signed-off-by: Phi Bang Nguyen <[email protected]>
c93e305
to
ea02aa5
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.
Why is this board overlay needed? Couldn't this be part of the shield definition itself? I'm unclear what makes this RT1064 specific
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.
The "source" property is defined in the DT binding of the CSI which is NXP specific. @kartben said that it must be in a board overlay because it's not generic. Other vendors can use different names.
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.
Ah, this makes sense- in that case, no worries. We should keep this as is. May be worth considering (as a future improvement) if we can standardize the source
property name, since CSI interfaces to camera sensors are somewhat generic IP. We've done this for a lot of the LCD controller timing properties, which allows us to avoid board specific overlays in many cases for this shields
dts/arm/nxp/nxp_rt10xx.dtsi
Outdated
@@ -1160,3 +1160,5 @@ | |||
*/ | |||
status = "disabled"; | |||
}; | |||
|
|||
dvp_fpc24_interface: &csi {}; |
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.
Probably want to put this at the board level- what this line effectively says is "the CSI is broken out to a 24 pin dvp FPC interface" which is only true for the RT10xx EVK boards, not for all RT10xx SOCs
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.
Okay ... so nxp_rt10xx is for all RT10xx SoCs not only EVK boards ... so there is not a "common" place to put common things for RT10xx (and RT11xx) EVKs / EVKB ? As we know that CSI is a common IP for those boards, it is a bit "inconvenient" to duplicate code for each board. But well, I will do as your suggestion then.
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.
No, there isn't unfortunately- although we do duplicate a lot across our RT10xx EVKs, Zephyr does not have any infrastructure I'm aware of for sharing a DTS file describing common board properties across multiple boards
Add a shield for DVP FPC 24-pins mt9m114 camera modules. These camera modules are supported on i.MX RT10xx series, for example. Signed-off-by: Phi Bang Nguyen <[email protected]>
ea02aa5
to
4f39b49
Compare
@danieldegrasse Thanks for the insight ! You are right. But it's a pity that we cannot generalize the shield for other camera modules which use the same connector inteface e.g. ov7725. |
@kartben can you return to this PR please |
This PR adds a shield for DVP FPC 24-pins mt9m114 camera modules which are upported across NXP's i.MX RT10xx series.
It is split from #69810 to ease the review process.
It already got reviewed by @danieldegrasse.