-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Add support for Renesas RA CEU (Capture Engine Unit) for RA8M1 and RA8D1 #92146
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
base: main
Are you sure you want to change the base?
Add support for Renesas RA CEU (Capture Engine Unit) for RA8M1 and RA8D1 #92146
Conversation
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
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 implementing the Renesas video driver!
The driver appears in a good shape WRT video APIs, with a few key points to modify (marked with :x), and a few proposal to improve it further, non-blocking (i.e. could be improved over time eventually, although appreciated if modified now).
Let me know if anything needs clarifications.
[blocking ❌] |
} | ||
} | ||
|
||
static int video_renesas_ra_ceu_get_format(const struct device *dev, struct video_format *fmt) |
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.
Function name is rather long, could it be shorten like ra_ceu_get_format
?. Idem for others
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.
@thaoluonguw : This is a small point but do you think it's better to shorten the function name a bit ? The prefix video_renesas_
seems not needed as all these functions are static, no worry that other functions will have the same name
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.
Update hal_renesas to support CEU on RA SoCs Signed-off-by: Duy Vo <[email protected]> Signed-off-by: Khanh Nguyen <[email protected]>
92a4b3b
to
03bdbce
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 modifications!
I have "folded" the points it does resolve to allow you to keep track of feedback.
A few items left might need some adjustment (i.e. for CI), but I am looking forward to see this merged!
03bdbce
to
c5ed982
Compare
Add support for the Renesas RA Capture Engine Unit (CEU), including driver source files, Kconfig options, and DTS bindings. - Add initial implementation of the RA CEU driver - Add dedicated Kconfig and CMake integration - Provide Devicetree bindings for the RA CEU - Update module Kconfig to include the new driver This enables image capture functionality using the CEU peripheral on Renesas RA series MCUs. Signed-off-by: Duy Vo <[email protected]> Signed-off-by: Khanh Nguyen <[email protected]>
Add pin definitions required by the RA Capture Engine Unit Signed-off-by: Duy Vo <[email protected]> Signed-off-by: Khanh Nguyen <[email protected]>
Add CEU to r7fa8d1xh.dtsi and r7fa8m1xh.dtsi Signed-off-by: Khanh Nguyen <[email protected]>
- Add CEU pin configuration to ek_ra8d1-pinctrl.dtsi - Enable CEU node and Arducam 20-pin connector in ek_ra8d1.dts - Configure PWM3 as external XCLK via pwm-clock node - Update board YAML to declare video support Signed-off-by: Duy Vo <[email protected]> Signed-off-by: Khanh Nguyen <[email protected]>
Support OV7670 DVP 20-pin shield on the EK-RA8D1 board Signed-off-by: Khanh Nguyen <[email protected]>
Add EK-RA8D1 board support for capture and capture_to_lvgl samples Signed-off-by: Khanh Nguyen <[email protected]>
Add SW1 configuration for using Camera Exapansion Port (J59) Signed-off-by: Thao Luong <[email protected]>
c5ed982
to
356db7a
Compare
|
@josuah : Thank you so much for your review. And I am so sorry for my late responding to you. |
No problem, it is possible to pause activity on PRs for as long as you need. The last item I think is missing would be adding it to the Here is an example of how it can be done for [EDIT: see more recent version below] diff --git a/tests/drivers/build_all/video/testcase.yaml b/tests/drivers/build_all/video/testcase.yaml
[ see more recent version below ]
|
} | ||
} | ||
|
||
static int video_renesas_ra_ceu_get_format(const struct device *dev, struct video_format *fmt) |
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.
@thaoluonguw : This is a small point but do you think it's better to shorten the function name a bit ? The prefix video_renesas_
seems not needed as all these functions are static, no worry that other functions will have the same name
#include <soc.h> | ||
#include <errno.h> |
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 better to sort the headers byte alphabetical order ?
/* Common header */
#include <errno.h>
#include <soc.h>
/* Zephyr header */
/* Local header */
#include "r_ceu.h"
#include "video_device.h"
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.
Could you add a test case in build_all so that the driver build can be triggered by CI ?
diff --git a/tests/drivers/build_all/video/testcase.yaml b/tests/drivers/build_all/video/testcase.yaml
index c7a5e07161e..f8c03137bfa 100644
--- a/tests/drivers/build_all/video/testcase.yaml
+++ b/tests/drivers/build_all/video/testcase.yaml
@@ -36,3 +36,8 @@ tests:
- stm32n6570_dk/stm32n657xx/sb
extra_args:
- platform:stm32n6570_dk/stm32n657xx/sb:SHIELD=st_b_cams_imx_mb1854
+ drivers.video.renesas_ra_ceu.build:
+ platform_allow:
+ - ek_ra8d1/r7fa8d1bhecbd
+ extra_args:
+ - platform:ek_ra8d1/r7fa8d1bhecbd:SHIELD="dvp_20pin_ov7670;rtkmipilcdb00000be"
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.
I have some comments regarding camera support on the board and in the shield layer
&pwm3 { | ||
pinctrl-0 = <&pwm3_default>; | ||
pinctrl-names = "default"; | ||
interrupts = <51 12>, <52 12>; | ||
interrupt-names = "gtioca", "overflow"; | ||
|
||
cam_clock: pwmclock { | ||
compatible = "pwm-clock"; | ||
status = "disabled"; | ||
#clock-cells = <1>; | ||
clock-frequency = <24000000>; | ||
pwms = <&pwm3 0 PWM_KHZ(24000) PWM_POLARITY_NORMAL>; | ||
}; | ||
}; | ||
|
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.
Should we define this pwm3
and cam_clock
in the shield layer? I feel it's wasting two interrupt lines on the board when they're not used, and the cam_clock properties are specific to each shield.
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.
As long as there is status = "disabled";
, then the driver instance will not be generated, and the resources will not be used.
The clock-frequency
belongs to the shield I agree. A new standard label name need to be chosen for it. For instance, dvp_20pin_xclk:
instead of cam_clock
for instance.
This way this devkit can be compatible with any future camera shield contributed, which only specify this:
&dvp_20pin_xclk {
clock-frequency = <24000000>;
};
Thank you for raising this point!
ceu_default: ceu_default { | ||
group1 { | ||
/* CEU */ | ||
psels = <RA_PSEL(RA_PSEL_CEU, 4, 0)>, /* VIO_D0 */ | ||
<RA_PSEL(RA_PSEL_CEU, 4, 1)>, /* VIO_D1 */ | ||
<RA_PSEL(RA_PSEL_CEU, 4, 5)>, /* VIO_D2 */ | ||
<RA_PSEL(RA_PSEL_CEU, 4, 6)>, /* VIO_D3 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 0)>, /* VIO_D4 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 1)>, /* VIO_D5 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 2)>, /* VIO_D6 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 3)>, /* VIO_D7 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 8)>, /* VIO_CLK */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 9)>, /* VIO_HD */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 10)>; /* VIO_VD */ | ||
drive-strength = "high"; | ||
}; | ||
}; |
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.
Also, since the connector is already defined in the board dts, should we move this to shield layer?
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 shield connector is nexus GPIO mapping, not related to this pinctrl define.
As my opinion, this pinctrl should be keeped at board layer due to it is board specific and the policy encourage enabling all the standard connector on board by default.
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.
If this pinctrl definition is placed in the board layer, I think it would be nice to include /omit-if-no-ref/
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 not necessary in this case. /omit-if-no-ref/
just have meaning in case you have multiple configuration for the pinctrl and actually use one in application. But with EK-RA8D1, the camera shield should only use the dvp_20pin connector on board.
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.
/omit-if-no-ref/ just have meaning in case you have multiple configuration for the pinctrl and actually use one in application
From my view, the main reason is as below image - hardware/pinctrl page:

This ensures that when the ek_ra8d1 is built without the shield, the CEU pinctrl is not included. To minimize the DTS content, /omit-if-no-ref/
should be included.
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 CEU should have the default pinctrl, even with or without the shield
ceu: ceu@40348000 { | ||
compatible = "renesas,ra-ceu"; | ||
reg = <0x40348000 0x8000>; | ||
clocks = <&pclka MSTPC 16>; |
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.
I think it's missing clock-names
here
+-------------+-------------+--------------+------------+------------+------------+-------------+-----------+ | ||
| SW1-1 PMOD1 | SW1-2 TRACE | SW1-3 CAMERA | SW1-4 ETHA | SW1-5 ETHB | SW1-6 GLCD | SW1-7 SDRAM | SW1-8 I3C | | ||
+-------------+-------------+--------------+------------+------------+------------+-------------+-----------+ | ||
| OFF | OFF | ON | OFF | OFF | OFF | ON | OFF | | ||
+-------------+-------------+--------------+------------+------------+------------+-------------+-----------+ |
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.
Just a comment for nit. Others LGTM.
group2 { | ||
/* GTIOC3B */ | ||
psels = <RA_PSEL(RA_PSEL_GPT1, 4, 4)>; | ||
drive-strength = "medium"; | ||
}; |
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.
Because there is only channel 0 to use with camera clock, the definition for GTIOC3B pincfg could be removed
ceu_default: ceu_default { | ||
group1 { | ||
/* CEU */ | ||
psels = <RA_PSEL(RA_PSEL_CEU, 4, 0)>, /* VIO_D0 */ | ||
<RA_PSEL(RA_PSEL_CEU, 4, 1)>, /* VIO_D1 */ | ||
<RA_PSEL(RA_PSEL_CEU, 4, 5)>, /* VIO_D2 */ | ||
<RA_PSEL(RA_PSEL_CEU, 4, 6)>, /* VIO_D3 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 0)>, /* VIO_D4 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 1)>, /* VIO_D5 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 2)>, /* VIO_D6 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 3)>, /* VIO_D7 */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 8)>, /* VIO_CLK */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 9)>, /* VIO_HD */ | ||
<RA_PSEL(RA_PSEL_CEU, 7, 10)>; /* VIO_VD */ | ||
drive-strength = "high"; | ||
}; | ||
}; |
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 shield connector is nexus GPIO mapping, not related to this pinctrl define.
As my opinion, this pinctrl should be keeped at board layer due to it is board specific and the policy encourage enabling all the standard connector on board by default.
FYI, to summarize what allows this PR to be merged in practice:
Then of course letting time for PR author to check all comments. |
Add support for Renesas RA CEU (Capture Engine Unit):
It works with OV7670 camera, and MIPI graphics expansion board included in the Kit