Skip to content

drivers: video: common: introduce CCI utilities #87935

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 21, 2025
Merged

drivers: video: common: introduce CCI utilities #87935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions drivers/video/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ config VIDEO_BUFFER_SMH_ATTRIBUTE
1: SMH_REG_ATTR_NON_CACHEABLE
2: SMH_REG_ATTR_EXTERNAL

config VIDEO_I2C_RETRY_NUM
int "Number of retries after a failed I2C communication"
default 0
help
If set to 0, only a single write attempt will be done with no retry.
The default is to not retry. Board configuration files or user project can then
use the number of retries that matches their situation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for not retry as default?

I2C usage on video is synchronous so a retry presence here would add a level of robustness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this does not matter much, as it can be overridden easily by boards and users.

@avolmat-st I remember you discussing about the RETRY, do you have more insights on that topic?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I agree that this is the good place to put the retry feature but so far I have actually seen no case where such retry is necessary. It is really board dependent I think so what would be the reason for introducing such retry by default ? If there is a HW issue or so leading to those transmit failure, better first notice them and add a retry only if necessary no ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense for me now, there is no point to put a retry without undestand the potential source of failure which is hardware dependent.

Feel free to resolve that @josuah :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense for me now, there is no point to put a retry without undestand the potential source of failure which is hardware dependent.

Feel free to resolve that @josuah :)

Copy link
Collaborator Author

@josuah josuah May 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If putting it the default, then it might be better to put a warning when a retry happens or something to raise awareness of the user that a retry happened (i.e. something else could be going wrong).

It might be possible to start at zero and let the boards decide as they are contributed. If most ends-up wanting a retry, the default better be increased to reduce the amount of config in boards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the strategy of users would be different during development (notice the slightest issue, hardware or software related, with CONFIG_ASSERT=y) and production builds (take some margin to cover for unexpected EMI that might happen in the field far from the engineer supervision)... That sounds like a broader topic than this PR though (i.e. I2C-wise?) as if image sensor I2C communication fails, so might other sensors.


source "drivers/video/Kconfig.esp32_dvp"

source "drivers/video/Kconfig.mcux_csi"
Expand Down
203 changes: 201 additions & 2 deletions drivers/video/video_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
/*
* Copyright (c) 2019, Linaro Limited
* Copyright (c) 2024, tinyVision.ai Inc.
* Copyright (c) 2024-2025, tinyVision.ai Inc.
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <string.h>

#include <zephyr/kernel.h>
#include <zephyr/device.h>
#include <zephyr/drivers/i2c.h>
#include <zephyr/drivers/video.h>
#include <zephyr/kernel.h>
#include <zephyr/logging/log.h>
#include <zephyr/sys/byteorder.h>
#include <zephyr/sys/util.h>

#include "video_common.h"

LOG_MODULE_REGISTER(video_common, CONFIG_VIDEO_LOG_LEVEL);

#if defined(CONFIG_VIDEO_BUFFER_USE_SHARED_MULTI_HEAP)
#include <zephyr/multi_heap/shared_multi_heap.h>
Expand Down Expand Up @@ -166,3 +175,193 @@ void video_closest_frmival(const struct device *dev, struct video_frmival_enum *
}
}
}

static int video_read_reg_retry(const struct i2c_dt_spec *i2c, uint8_t *buf_w, size_t size_w,
uint8_t *buf_r, size_t size_r)
{
int ret;

for (int i = 0;; i++) {
ret = i2c_write_read_dt(i2c, buf_w, size_w, buf_r, size_r);
if (ret == 0) {
break;
}
if (i == CONFIG_VIDEO_I2C_RETRY_NUM) {
LOG_HEXDUMP_ERR(buf_w, size_w, "failed to write-read to I2C register");
return ret;
}

k_sleep(K_MSEC(1));
}

return 0;
}

int video_read_cci_reg(const struct i2c_dt_spec *i2c, uint32_t reg_addr, uint32_t *reg_data)
{
size_t addr_size = FIELD_GET(VIDEO_REG_ADDR_SIZE_MASK, reg_addr);
size_t data_size = FIELD_GET(VIDEO_REG_DATA_SIZE_MASK, reg_addr);
bool big_endian = FIELD_GET(VIDEO_REG_ENDIANNESS_MASK, reg_addr);
uint16_t addr = FIELD_GET(VIDEO_REG_ADDR_MASK, reg_addr);
uint8_t buf_w[sizeof(uint16_t)] = {0};
uint8_t *data_ptr;
int ret;

__ASSERT(addr_size > 0, "The address must have a address size flag");
__ASSERT(data_size > 0, "The address must have a data size flag");

*reg_data = 0;

if (big_endian) {
/* Casting between data sizes in big-endian requires re-aligning */
data_ptr = (uint8_t *)reg_data + sizeof(*reg_data) - data_size;
} else {
/* Casting between data sizes in little-endian is a no-op */
data_ptr = (uint8_t *)reg_data;
}

for (int i = 0; i < data_size; i++) {
if (addr_size == 1) {
buf_w[0] = addr + i;
} else {
sys_put_be16(addr + i, &buf_w[0]);
}

ret = video_read_reg_retry(i2c, buf_w, addr_size, &data_ptr[i], 1);
if (ret < 0) {
LOG_ERR("Failed to read from register 0x%x", addr + i);
return ret;
}

LOG_HEXDUMP_DBG(buf_w, addr_size, "Data written to the I2C device...");
LOG_HEXDUMP_DBG(&data_ptr[i], 1, "... data read back from the I2C device");
}

*reg_data = big_endian ? sys_be32_to_cpu(*reg_data) : sys_le32_to_cpu(*reg_data);

return 0;
}

static int video_write_reg_retry(const struct i2c_dt_spec *i2c, uint8_t *buf_w, size_t size)
{
int ret;

for (int i = 0;; i++) {
ret = i2c_write_dt(i2c, buf_w, size);
if (ret == 0) {
break;
}
if (i == CONFIG_VIDEO_I2C_RETRY_NUM) {
LOG_HEXDUMP_ERR(buf_w, size, "failed to write to I2C register");
return ret;
}

k_sleep(K_MSEC(1));
}

return 0;
}

int video_write_cci_reg(const struct i2c_dt_spec *i2c, uint32_t reg_addr, uint32_t reg_data)
{
size_t addr_size = FIELD_GET(VIDEO_REG_ADDR_SIZE_MASK, reg_addr);
size_t data_size = FIELD_GET(VIDEO_REG_DATA_SIZE_MASK, reg_addr);
bool big_endian = FIELD_GET(VIDEO_REG_ENDIANNESS_MASK, reg_addr);
uint16_t addr = FIELD_GET(VIDEO_REG_ADDR_MASK, reg_addr);
uint8_t buf_w[sizeof(uint16_t) + sizeof(uint32_t)] = {0};
uint8_t *data_ptr;
int ret;

__ASSERT(addr_size > 0, "The address must have a address size flag");
__ASSERT(data_size > 0, "The address must have a data size flag");

if (big_endian) {
/* Casting between data sizes in big-endian requires re-aligning */
reg_data = sys_cpu_to_be32(reg_data);
data_ptr = (uint8_t *)&reg_data + sizeof(reg_data) - data_size;
} else {
/* Casting between data sizes in little-endian is a no-op */
reg_data = sys_cpu_to_le32(reg_data);
data_ptr = (uint8_t *)&reg_data;
}

for (int i = 0; i < data_size; i++) {
/* The address is always big-endian as per CCI standard */
if (addr_size == 1) {
buf_w[0] = addr + i;
} else {
sys_put_be16(addr + i, &buf_w[0]);
}

buf_w[addr_size] = data_ptr[i];

LOG_HEXDUMP_DBG(buf_w, addr_size + 1, "Data written to the I2C device");

ret = video_write_reg_retry(i2c, buf_w, addr_size + 1);
if (ret < 0) {
LOG_ERR("Failed to write to register 0x%x", addr + i);
return ret;
}
}

return 0;
}

int video_modify_cci_reg(const struct i2c_dt_spec *i2c, uint32_t reg_addr, uint32_t field_mask,
uint32_t field_value)
{
uint32_t reg;
int ret;

ret = video_read_cci_reg(i2c, reg_addr, &reg);
if (ret < 0) {
return ret;
}

return video_write_cci_reg(i2c, reg_addr, (reg & ~field_mask) | field_value);
}

int video_write_cci_multiregs(const struct i2c_dt_spec *i2c, const struct video_reg *regs,
size_t num_regs)
{
int ret;

for (int i = 0; i < num_regs; i++) {
ret = video_write_cci_reg(i2c, regs[i].addr, regs[i].data);
if (ret < 0) {
return ret;
}
}

return 0;
}

int video_write_cci_multiregs8(const struct i2c_dt_spec *i2c, const struct video_reg8 *regs,
size_t num_regs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for doing differently between the multiregs and multiregs8/16 ? In case of multiregs, this mandate to have a last null entry then iterate until reaching it while in case of multiregs8/16 an ARRAY_SIZE has to be provided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multireg can have a {0, 0} terminator as there is always at some flag that prevents the last field to be zero.
multireg8/multireg16 do not.

It is true that this can be confusing and lead to bug if there are different APIs... I will update it so that ARRAY_SIZE() can be used in both case...

{
int ret;

for (int i = 0; i < num_regs; i++) {
ret = video_write_cci_reg(i2c, regs[i].addr | VIDEO_REG_ADDR8_DATA8, regs[i].data);
if (ret < 0) {
return ret;
}
}

return 0;
}

int video_write_cci_multiregs16(const struct i2c_dt_spec *i2c, const struct video_reg16 *regs,
size_t num_regs)
{
int ret;

for (int i = 0; i < num_regs; i++) {
ret = video_write_cci_reg(i2c, regs[i].addr | VIDEO_REG_ADDR16_DATA8, regs[i].data);
if (ret < 0) {
return ret;
}
}

return 0;
}
Loading