-
Notifications
You must be signed in to change notification settings - Fork 8k
drivers: video: update ov5640 driver to adapt to generic controller #96411
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: update ov5640 driver to adapt to generic controller #96411
Conversation
drivers/video/ov5640.c
Outdated
msg[1].flags = I2C_MSG_READ | I2C_MSG_STOP | I2C_MSG_RESTART; | ||
|
||
ret = i2c_transfer_dt(spec, msg, 2); | ||
ret = i2c_write_read_dt(spec, &addr_buf[0], sizeof(addr), val, val_size); |
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 fix. Should it be better to use video_read/write_cci_reg()
in video_common.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.
I agree. Recently the OV7670 is also being converted to use the video_read/write_cci helpers.
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.
Done. I have updated the new code using video_read/write_cci_reg()
drivers/video/ov5640.c
Outdated
*/ | ||
uint8_t data_order = 0; | ||
|
||
if (cfg->bus_width == 8 && cfg->data_shift == 0) { |
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 am having difficulties to understand the sensor behavior. I've also checked the datasheet and, I do not really understand why 10-bit and 8-bit / shift 2 need a different setting ?
My point is, if 10bit mode is selected, this will lead to having 10-bits from 9 (MSB) to 0 (LSB)
In 8bit - 0 shift, seems we expect to take 8bits from 7 to 0 right, and since we want to keep the largest range of values from the sensor, it means that we expect the sensor to give us the data shifted to 2, aka previously bit 9 is now moved to bit 7, bit 8 moved to bit 6 etc etc. (and I suspect that, since this 0x4745 register seems to be doing some "rolling", previous bit 1 and bit 0 and now moved to bit 9 and bit 8 (which anyway might not be connected).
Now the 8bit - 2shifted, my understanding is that we take only 8 bits, from bit 9 to 2, is that correct, and we leave the bit 1 / 0 probably unconnected. In such case, how different is that from 10bit mode ?
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.
Sorry, that's more a questioning regarding the sensor behavior and its usage rather that on the source code itself, which seems correct.
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.
@KhanhNguyen-RVC, @thenguyenyf, could you clarify in which format this is being used ?
RGB565 and YUYV are currently seen as 8bit formats I think and that's I think what the OV5640 outputs in term of data. So maybe could this be used for 10-bit bayer format maybe ?
I also see that test-pattern can be output in 10-bit format, but again, I don't really know what this mean for the OV5640 considering RGB565 or YUYV.
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.
Hello @avolmat-st .
Could you clarify in which format this is being used ?
In the case of 10-bit bus width with 2-bit shift, the format used could be YUV or RGB 565 (8 data lines required). The bit shift or rotation, normally not only depend on the format that the sensor given but also depend on the data pins that being used by the capturing controller.
For reference, you can see the board design for the EK-RA8P1 here 'Table 35. Camera Expansion Port Assignments in Parallel mode (SW4-6 ON) - EK-RA8P1 UM'. The CEU uses the DVP sensor's CAM_D9 - CAM_D2 connected to its D7 - D0 pins to capture 8-bit data, so the OV5640's output data must be shifted to match the pin order used by the capture controller.
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. Yes, I understand this case, of shifting to the left. What about the other 2 cases ? 10bit and 8bit without shift (or it is right shift ?) ?
Does this mean that, for a 10-bit output format (btw, which format would that be for the OV5640, only bayer ??), we can either output it in 10bit (aka setting 00) or 8bit, in which case the OV5640 will right shift by 2 to only give the 8 MSB bits on D0-7 ?
My point is that, until now this has not been set and still it seems that we are able to properly capture stuff (I think via D0-7), but we are I think in mode 00 (which is 10-bit mode). My guess is that this is working because we only output 8bit formats anyway (RGB565 / YUYV) and thus, even if mode 10-bit is selected (aka, no rolling), it goes by default to D0-7).
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 can only say that this PR to add a valid setting to for data order on OV5640 which mentioned in the hardware specs.
My point is that, until now this has not been set and still it seems that we are able to properly capture stuff (I think via D0-7), but we are I think in mode 00 (which is 10-bit mode). My guess is that this is working because we only output 8bit formats anyway (RGB565 / YUYV) and thus, even if mode 10-bit is selected (aka, no rolling), it goes by default to D0-7).
As my understanding, it may cause an issue when adding the wrong config for data order that doesn't suite with the output format. Currently, users should properly configure to follow instructions from the sensor HUM .
47c22a0
to
4cfe5da
Compare
Replace the custom I2C read/write functions in the OV5640 driver with the common Video CCI API helpers. - Introduce OV5640_REG8/REG16 macros for register addressing - Convert struct ov5640_reg to struct video_reg16 - Replace ov5640_{read,write,modify}_reg() with CCI API helpers - Replace ov5640_write_multi_regs() with video_write_cci_multiregs16() This aligns the OV5640 driver with other video sensor drivers that already rely on the CCI helpers. Signed-off-by: Khanh Nguyen <[email protected]>
4cfe5da
to
13d68f1
Compare
dts/bindings/video/ovti,ov5640.yaml
Outdated
child-binding: | ||
include: video-interfaces.yaml | ||
|
||
properties: |
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.
Those 2 properties are not specific to the OV5640 so they should be put into the video-interfaces.yaml bindings instead and then this ov5640 should only mention specific parts (such as acceptable values etc)
For reference, this bindings is also available in Linux (and dual licensed GPL / BSD-2 Clause so ok to get it from for applicable parts if necessary)
bus-width:
$ref: /schemas/types.yaml#/definitions/uint32
maximum: 64
description:
Number of data lines actively used, valid for the parallel busses.
data-shift:
$ref: /schemas/types.yaml#/definitions/uint32
maximum: 64
description:
On the parallel data busses, if bus-width is used to specify the number of
data lines, data-shift can be used to specify which data lines are used,
e.g. "bus-width=<8>; data-shift=<2>;" means, that lines 9:2 are used.
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.
After consideration, this change was reverted, because we already have the valid config check in init function
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 update. However, a valid configuration check within the driver doesn't replace the binding description within the yaml file. The yaml is here to describe the property applicable to the device, in addition to performing as well during the compilation phase that the properties written into the device-tree matches with what is described as applicable within the bindings.
It is thus mandatory to have them described within a binding file.
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 description is already added at https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/video/video-interfaces.yaml#L81-L86 and is visible at https://docs.zephyrproject.org/latest/build/dts/api/bindings/video/ovti%2Cov5640.html#grandchild-node-properties
13d68f1
to
a85c3e5
Compare
Hi @thenguyenyf, thanks for the new version. I've made a comment regarding the yaml part. Concerning the implementation of the data-shift / data-width properties, I want to first have a look at how it behave with existing platforms. I have several of them with an OV5640 on it in DVP mode. I understand that, depending on how the sensor to connected to the sensor, this might be needed to perform a shift of data, however I'd like to avoid having a new bindings which could be misleading on how to configure it. For my information, on the reference design you shared previously, which configuration do you need to apply in order to work correctly ? It seems to me that this is same as ST's OV5640 (aka sensor D[9:2] connected to receiver D[7:0]). |
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.
Bindings need to be described
@avolmat-st I have an opening PR that contribute a new OV5640 camera shield for EK-RA8P1 #96414. |
Thanks for sharing the PR. So I understand that in your setup you set data-width=8 and data-shift = 2 with your receiver D[7:0] connected to the sensor output [9:2]. Is that right ? I gave it a try one several platforms on my side, one of them being the STM32H747I-DISCO + ST-B_CAM_OMV (https://www.st.com/en/evaluation-tools/b-cams-omv.html#overview). By default, until now the OV5640 driver has never configured the register 0x4745 in charge of the data order however, the default value is 0x0, aka so called "10-bit mode". I tested on my setup by relying on the test-pattern of the OV5640 sensor, aka setting the register 0x503d to 0x84 (enabled + vertical bars). With that set, I then captured and dumped the 320x240 frame I got in memory and coming from the sensor. With that, here's my findings, with partial hexdump output of the beginning of each frame captured. This is vertical bar test-pattern so same 16bit values are repeated over and over until reaching the next bar area. Normal / current setup of DATA ORDER (0x00):
DATA_ORDER (0x02) - aka BIT(1) set
DATA_ORDER (0x04) - aka BIT(2) set
So basically, what this show is that setting BIT(1) or BIT(2) of the DATA_ORDER register will shift the data to the left or to the right basically. One more thing is, when we are capturing 8-bit data (here it was RGB565), the data given by the OV5640 are already on the D[9-2] of the sensor. So, considering that your board seems to connect the OV5640 in the same way as ours, which is the common way to connect the OV5640 in 8-bit parallel mode, I do not understand why such setting (8-bit - shift) is needed in your case. Moreover, considering that we (usually) connect the sensor for 8-bit capture, I find it very misleading to have to say "10-bit" to make it work and even more avoid saying 8-bit (shift 0 or 2) since this would break the image. |
Hello @avolmat-st . I checked with the board to CAM connection and would correct some information.
The pin connection on some our dev boards with ov5640 is CAM_D9 - CAM_D2 to CEU_D7 - CEU_D0 (don't need to shift) and CAM_D7 - CAM_D0 to CEU_D7 - CEU_D0 (need to shift). So, the default behavior is DVP data will be left alignment (D9 to D2 in case 8bit RGB). But in other case (CAM_D7 - CAM_D0 to CEU_D7 - CEU_D0), we still need the bus-width and data-shift to make it match with the capturing controller bus. So, I'd like to keep the extension for data order configuration, but the comment/description and the condition may need to refine that match with the ov5640 behavior. I will update for it. Below is the image dump from ov5640 test pattern on the board that use CAM_D7 - CAM_D0 to CEU_D7 - CEU_D0 Normal / current setup of DATA ORDER (0x00):
DATA_ORDER (0x02) - aka BIT(1) set - true order setting for this board
DATA_ORDER (0x04) - aka BIT(2) set
|
Hi @thenguyenyf |
a85c3e5
to
786f8e6
Compare
This item has been updated @avolmat-st |
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 update. Modifications sounds great. Just to be sure I will cross-check it as well on the OV5640 based platform I have at hand.
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, just left a small comment on a returned errno. Also tested on RT1170-EVK where ov5640 is in CSI2 mode, no regression was found.
drivers/video/ov5640.c
Outdated
} else { | ||
LOG_ERR("Invalid DVP config: width=%u shift=%u", cfg->bus_width, | ||
cfg->data_shift); | ||
return -EIO; |
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.
return -ENOTSUP;
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 fixed it
Add support in the OV5640 driver to configure the bus-width and data-shift properties from devicetree when operating in DVP (parallel) mode. This allows the number of parallel data lines and the bit shift to be set directly via DTS, ensuring correct configuration across different hardware designs. Signed-off-by: Khanh Nguyen <[email protected]>
The function asserted `!sz && !ctrls`, which is the inverse of the intended precondition. This caused assertion failures on valid inputs. Update the check to `sz && ctrls` so it fails only when size is zero or the control pointer is NULL. Signed-off-by: Khanh Nguyen <[email protected]>
786f8e6
to
aaefa4e
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. Thanks !
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 LGTM. Tested ok on a STM32H747I-DISCO with the ST_B_CAMS_OMV shield.
dts/bindings/video/ovti,ov5640.yaml
to make the bus width and data shift configurable.drivers/video/video_ctrls.c
that cause some test failed