drivers: video: common: introduce CCI utilities

[josuah]

No dependency. Split out of:

• drivers: video: imagers/cci/imx219: easier image sensor contribution #87004

Downstream:

• drivers: video: emul-imager: use the emulated I2C bus #87937
• Video: addition of the imx335 driver #88825
• video: st_mipid02: addition of ST MIPID02 CSI bridge #89764

Add a library for the Camera Common Interface, part of the MIPI CSI protocol standard defining methods to configure a camera device over I2C, such as which size for the register address/data.

This is applied to the GC2145 image sensor. Ping @uLipe @iabdalkader who last worked with it.
Also tested with an IMX219 and the emulated video imager, not part of this PR.

[josuah]

Force-push:

• Rebase on main after drivers: video: gc2145: Add Chip ID 0x2145 #87872

[ngphibang]

Thanks ! It will help drivers not need to define their own read / write register functions. Each drivers has different type of register address and value:

• Some are 16-bit address registers and need to read/write 8-bits data
• Some are 16-bit address registers and need to read/write 16-bit data

Does the CCI library will work in all these cases ? Per my observation, Linux also has CCI library but I don't know why (video) drivers still define and use their own read / write function (?).

Will we have also modify register functions (to modify only some bit through a mask) ?

[josuah]

Yes the goal is to offer a replacement for all the custom write functions for compatible sensors.

Supported address sizes:

• 8-bit address
• 16-bit address

Supported Data sizes:

• 8-bit data
• 16-bit data (big/little endian) sent one byte at a time
• 24-bit data (big/little endian) sent one byte at a time (sensors define arbitrary size for their register)
• 32-bit data (big/little endian) sent one byte at a time (the max supported, 64-bit registers needs to be split)

Supported operations:

• Read one register into a 32-bit variable (32-bit, 24-bit, 16-bit, 8-bit alike, like in Linux)
• Write one register from a 32-bit variable (32-bit, 24-bit, 16-bit, 8-bit alike, like in Linux)
• Write multiple registers using a struct video_reg table
• Write one bitfield described by a value and a mask. Should it be named video_modify_cci_reg() instead of video_write_cci_field()?

[ngphibang]

LGTM (I didn't test the GC2145 part), thanks !

[ngphibang]

BTW, in order to use the CCI read/write/modify functions, we need to convert all the register addresses to 32-bit:

#define IMX335_REG8(addr)  ((addr) | VIDEO_REG_ADDR16_DATA8)
#define IMX335_REG16(addr)  ((addr) | VIDEO_REG_ADDR16_DATA16_LE)
#define IMX335_REG24(addr)  ((addr) | VIDEO_REG_ADDR16_DATA24_LE)

For drivers that uses dozens or hundreds of registers, it seems rather a significant amount of memory. Should it be better if we take these additional information as function parameters (maybe separate functions for 8bit, 16bit, 24 bit registers) and/or as a config (LE or BE does not change on a platform) instead of encoding all of them in the address parameter ?

[josuah]

we need to convert all the register addresses to 32-bit

The flags are forced o be 32-bit here but good to clear any doubt through an obvious syntax.

dozens or hundreds of registers

For instance, for the galaxycore,gc2145 driver, I skipped the CCI functions for this purpose. On Linux, it is always 32-bit address and 32-bit data fields, but ROM is typically more abundant on a Linux-capable chip than a microcontroller.

The first implementation (unpublished) used 7 variants (write8, write16le, write16be, write24le, write24be, write32le, write32le), although this does not permit to define table of registers with many .

Switching to another strategy that implementation was wrapping the "table" declaration like this:

#define VIDEO_REG_ADDR16_DATA24(addr, reg) \
        { (addr) + 0, (reg) >> 16 }, \
        { (addr) + 1, (reg) >> 8 }, \
        { (addr) + 2, (reg) >> 0 }

struct { uint16_t addr; uint8_t data; } init_regs[] = {
        VIDEO_REG_ADDR16_DATA24(0x3008, 0xaabbcc),
        IMX335_REG8(0x3016, 0),
        IMX335_REG16(0x3014, 8),
        {0},
};

For 16-bit addresses and 24-bit registers:

before: 8 bytes

[ addr0 | addr1 |      |      | data0 | data1 | data2 |       ]  4 + 4

after: 16 bytes:

[ addr0 | addr1 | data0 |      ]  2 + 2
[ addr0 | addr1 | data1 |      ]  2 + 2
[ addr0 | addr1 | data2 |      ]  2 + 2
[ addr0 | addr1 | data3 |      ]  2 + 2

For 8-bit addresses and 8-bit registers

A frequent pathological case for small image sensors (8-bit address) with big tables of undocumented register...

before: 8 bytes:

[ addr0 | addr1 |      |      | data0 |      |      |      ]  4 + 4

after: 2 bytes:

[ addr0 | data0 ]  1 + 1

[josuah]

Currently, the struct video_reg is uint32_t addr; uint32_t data;.

There could be variants to cover the compromises to wrap these frequent "big data tables" use-cases, this is only 2 extra types to add:

struct video_reg16 { uint16_t addr; uint8_t data; }; -> 4 bytes, 2× better
for 16-bit addresses (same as uint16_t addr; uint16_t data; in practice due to alignment)

struct video_reg8 { uint8_t addr; uint8_t data; }; -> 2 bytes, 4× better
for 8-bit addresses

[josuah]

config (LE or BE does not change on a platform) instead of encoding all of them in the address parameter

It was a compromise to reduce the API size non-V4L2 embedded developers need to face to avoid too much domain-specific knowledge required for writing video drivers:

1. In #define SENSORNAME_REG16(addr) (addr | FLAGS_THEY_DECIDE), the addition of the "type flags" is defined by the developer themselves. A trick to actively push the undestanding of "CCI magic" to non-V4L2 developers.
2. By having only one flag, there is no room for mistake, only valid flags are defined, and not so many to maintain.
3. A 16-bit address that encodes a 2-bit "address size" flag becomes a 32-bit variable, with 14-bit unused anyway.

If another compromise is possible, glad to hear about it, I probably did not think about it.
I tried to find an 'OK' balance but I do not know what the best answer is!

[uLipe]

Nice work, just a first pass :)

[uLipe]

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.

[josuah]

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?

[avolmat-st]

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 ?

[uLipe]

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 :)

[uLipe]

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 :)

[josuah]

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.

[josuah]

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.

[avolmat-st]

@kartben could you have a look at this PR so that we could merge it if ok since it is a dependency of several other PR. Thanks ;)

[Copilot]

Pull Request Overview

This PR introduces Camera Control Interface (CCI) utilities in the video driver module to support I2C-based configuration of camera sensors. Key changes include:

• Addition of a new header (video_common.h) defining CCI register and flag macros, plus API prototypes.
• Implementation of CCI functions in video_common.c with retry logic and endianness handling.
• Addition of a Kconfig option (VIDEO_I2C_RETRY_NUM) for configuring I2C retry attempts.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
drivers/video/video_common.h	Introduces CCI register structures, macros, and function prototypes for camera control.
drivers/video/video_common.c	Implements the CCI read, write, modify, and multi-register operations with retry logic.
drivers/video/Kconfig	Adds a new configuration option controlling the number of I2C communication retries.

Comments suppressed due to low confidence (2)

drivers/video/video_common.c:263

• The parameter name 'data' in the implementation does not match the header's 'reg_value'. Consider renaming it to 'reg_value' for consistency.

int video_write_cci_reg(const struct i2c_dt_spec *i2c, uint32_t reg_addr, uint32_t data)

drivers/video/video_common.c:198

• The parameter name 'data' differs from the header's 'reg_value'. Aligning these names would improve clarity.

int video_read_cci_reg(const struct i2c_dt_spec *i2c, uint32_t reg_addr, uint32_t *data)

[Copilot]

Consider inserting a space to read 'the value' instead of 'thevalue' for improved readability in the comment.

Suggested change

	* thevalue.
	* the value.

[josuah]

The blocking PR was about a copilot review, which we can trigger independently, so let's go with it then dismiss the stale change request once Copilot review is addressed. :)

If no progress is made, the assignee (maintainer) has the right to dismiss stale, unrelated or irrelevant change requests by reviewers giving the reviewers a minimum of 1 business day to respond and revisit their initial change requests or start the escalation process.
-- https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-technical-escalation

[josuah]

Force-push:

• Just rebasing on latest main for now.

[josuah]

Force-push:

• Apply Copilot proposed typo-fixes

Tested via #87937 just rebased on top of it.
Politely dismissing kartben review as I assume this is addressed by now.

The change request was based on Copilot feedback. This was addressed and Copilot feedback was requested again, and new changes were addressed again.

I suppose this means your change request was addressed (+3 days) and a new change request is welcome for new topics.

[avolmat-st]

Very very minor typo, spotted while checking the very last update via the compare.

endinaness -> endianness

[josuah]

Thank you, much easier to fix now than in subsequent PRs.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud