video: fix dcmipp devicetree #94400
video: fix dcmipp devicetree #94400
Conversation
The driver instance variable name is inst, not n. Signed-off-by: Phi Bang Nguyen <[email protected]>
zephyrbot
added
platform: STM32
zephyrbot
requested review from
avisconti,
avolmat-st,
djiatsaf-st,
erwango,
etienne-lms,
FRASTM,
gautierg-st,
GeorgeCGV,
jfischer-no,
josuah,
kartben,
loicpoulain,
marwaiehm-st and
mathieuchopstm
ngphibang
force-pushed
the
fix_dcmipp_devicetree
branch
from
9dcba5b to
1ea37c5
Compare
dts/arm/st/mp13/stm32mp135.dtsi
Outdated
| pipes { | ||
| compatible = "st,stm32-dcmipp-pipes"; | ||
| #address-cells = <1>; | ||
| #size-cells = <0>; | ||
|
|
||
| port@0 { | ||
| pipe_dump: pipe@0 { | ||
| reg = <0>; | ||
|
|
||
| endpoint { | ||
| remote-endpoint-label = ""; | ||
| bus-type = <VIDEO_BUS_TYPE_PARALLEL>; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
By reviewing #94081, it seems only n6 has multiple pipes and this one has just one pipe ? In this case, pipes {} node is not needed and we can remove compatible string too. Otherwise, need to add an if in the dcmipp driver when retrieving the pipe / pipes:
DT_FOREACH_CHILD_VARGS(DT_INST_CHILD(inst, pipes), DCMIPP_PIPE_INIT_DEFINE, inst);
There was a problem hiding this comment.
Yes, only the N6 has several pipe. Others have single pipe. Is that an issue to keep it as you propose with current patch ?
Aka always have pipes { } and below it one or more pipe@x entries ?
There was a problem hiding this comment.
Both ways works but I think it is more "correct" to remove the pipes {} node if we are sure that it has only one pipe and in the driver we treat it with #if defined(STM32_DCMIPP_HAS_PIXEL_PIPES) as alreadt done in some other places in the driver ?
There was a problem hiding this comment.
Both ways works but I think it is more "correct" to remove the pipes {} node if we are sure that it has only one pipe and in the driver we treat it with
#if defined(STM32_DCMIPP_HAS_PIXEL_PIPES)as alreadt done in some other places in the driver ?
Yes ok for me. Would you update your PR to make this change as well ?
|
Thanks @ngphibang for the patches and sorry for the long delay for review/test/reply. I confirm this looks good to me and is working fine. #define STM32_DCMIPP_INIT(inst) \part is all modified while actually the only modification is related to the DT_FOREACH_CHILD_VARGS, making it hard to notice. |
|
When suggesting to repurpose port/endpoint as support to instantiate the pipes, I think I was not understanding DCMIPP well. After reading the conversations I could grasp it better. The changes make sense to me. |
ngphibang
force-pushed
the
fix_dcmipp_devicetree
branch
from
1ea37c5 to
b417504
Compare
|
Yes, done. I added a 3rd commit "drivers: video: dcmipp: Define video device for each pipe" as well. Otherwise, I encountered a compilation error (due to the 2nd commit): It took me quite a time to debug but still cannot figure out what does it mean this error (I didn't modify these lines at all ...). Will come back to this later or would be glad if you can help :-). |
ngphibang
force-pushed
the
fix_dcmipp_devicetree
branch
from
b417504 to
40b4625
Compare
The issue is the directive |
|
Thanks @ngphibang for the update. LGTM, could you just update based on @etienne-lms comment and that sounds good for me. Tested ok on MP13 & N6 as well. |
The pipe nodes are not video interfaces. Describe them as normal child nodes instead of using port/endpoint. Signed-off-by: Phi Bang Nguyen <[email protected]>
Define a video device for each pipe instead for the main dcmipp device since the pipe is the device that communicates with application (zephyr,camera chosen node), not the dcmipp device. This helps to enable camera controls as well. Signed-off-by: Phi Bang Nguyen <[email protected]>
ngphibang
force-pushed
the
fix_dcmipp_devicetree
branch
from
40b4625 to
6d8550d
Compare
|
Uh oh!
There was an error while loading. Please reload this page.