drivers: video: emul-imager: use the emulated I2C bus

[josuah]

Dependency:

• drivers: video: common: introduce CCI utilities #87935

Split out of:

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

Downstream:

• drivers: video: imager: move the boilerplate in a library #88003

Replace the ad-hoc register emulation by the dedicated I2C emulator, making it usable with the same APIs as every other image sensors.

[ngphibang]

The emul imager now uses the emul i2c which is initialized with CONFIG_EMUL_INIT_PRIORITY = CONFIG_VIDEO_INIT_PRIORITY= 60 while we need i2c initialized BEFORE the imager. This may work now by chance due to linker order but I think the init priorrities should be explicitly arranged to be sure all is in order, similar to this fix

config EMUL_INIT_PRIORITY
	int "Init priority"
	default 60
	help
	  Emulation device driver initialisation priority.

[josuah]

Force-push:

• Rebased on latest main
• Apply a variant of 1b501e5

I went with CONFIG_VIDEO_IMAGER_INIT_PRIORITY=65 by default for all image sensors, allowing CSI/DVP receivers to use i.e. 61/62/63/64 to be initialized before, or 66/67/68/69 to be initialized after the image sensors.

I can switch to i.e. CONFIG_VIDEO_EMUL_IMAGER_INIT_PRIORITY=XYZ if this is better.

[ngphibang]

Increase the CONFIG_VIDEO_IMAGER_INIT_PRIORITY to 65 resolves the issue but it is a bit unusual as in general, all image sensors are initialized with priority CONFIG_VIDEO_INIT_PRIORITY = 60.

I see that there is the I2C_EMUL driver, why can't we just use this instead of making a customized one ? I suppose you need some different behavior for the transfer_i2c function that i2c_emul cannot support ? If yes, can we emulate the read/write register so that it can be used with i2c_emul ?

Another thought is that I2C EMUL is an emulated driver but surprisingly, it can set its init priority to 50 (equal to the real i2c init priority), so I wonder if we can set the emul imager i2c the same way so that we don't need to change the imager priority as currently ?

Just some thoughts for us to dig deeper.

[josuah]

see below instead.

why can't we just use this instead of making a customized one?

Maybe I missed something... Current situation:

zephyr/drivers/i2c/i2c_emul.c

Lines 338 to 339 in c47926e

	I2C_DEVICE_DT_INST_DEFINE(n, i2c_emul_init, NULL, &i2c_emul_data_##n, &i2c_emul_cfg_##n, \
	POST_KERNEL, CONFIG_I2C_INIT_PRIORITY, &i2c_emul_api);

CONFIG_I2C_INIT_PRIORITY -> CONFIG_KERNEL_INIT_PRIORITY_DEVICE -> 50

zephyr/drivers/video/video_emul_imager.c

Lines 428 to 430 in c47926e

	DEVICE_DT_INST_DEFINE(inst, &emul_imager_init, NULL, &emul_imager_data_##inst, \
	&emul_imager_cfg_##inst, POST_KERNEL, CONFIG_VIDEO_INIT_PRIORITY, \
	&emul_imager_driver_api);

CONFIG_VIDEO_INIT_PRIORITY -> 60

zephyr/drivers/video/video_emul_rx.c

Lines 298 to 299 in c47926e

	DEVICE_DT_INST_DEFINE(n, &emul_rx_init, NULL, &emul_rx_data_##n, &emul_rx_cfg_##n, \
	POST_KERNEL, CONFIG_VIDEO_INIT_PRIORITY, &emul_rx_driver_api);

CONFIG_VIDEO_INIT_PRIORITY -> 60

Between external sensors and on-chip video peripheral, the priority is sometimes in one direction, sometimes in the other direction?

• On-chip video RX initialized before the external chip. I.e. to provide them a clock required for I2C I/O
• On-chip video RX initialized after the external chip. I.e. check device_is_ready(cfg->source_dev)(?)

The assumption is that the on-chip video RX will know whether it is initialized before or after, and the external chip is less likely to have this requirement. A solution for this would be splitting CONFIG_VIDEO_INIT_PRIORITY like this:

• CONFIG_I2C_INIT_PRIORITY -> 50
• CONFIG_VIDEO_BEFORE_INIT_PRIORITY -> 60 (used by video RX drivers that provide a clock)
• CONFIG_VIDEO_IMAGER_INIT_PRIORITY -> 65 (used by all imagers)
• CONFIG_VIDEO_AFTER_INIT_PRIORITY -> 70 (used by video RX drivers that check the imager status)

I am not a big fan of my own proposal, as it's much nicer to have just one or two init priorities.
I will happily apply some better compromise!

[josuah]

you need some different behavior for the transfer_i2c function that i2c_emul cannot support ?

No such requirement thankfully!

I2C EMUL is an emulated driver but surprisingly, it can set its init priority to 50

It seems like it is already set to 50 after all? 🤔

[ngphibang]

It seems like it is already set to 50 after all?

Yes, the i2c_emul driver is clearly initialized with CONFIG_I2C_INIT_PRIORITY = 50

	I2C_DEVICE_DT_INST_DEFINE(n, i2c_emul_init, NULL, &i2c_emul_data_##n, &i2c_emul_cfg_##n,   \
				  POST_KERNEL, CONFIG_I2C_INIT_PRIORITY, &i2c_emul_api);

But in the emul imager, the emul imager i2c is defined as:

static int emul_imager_init_i2c(const struct emul *target, const struct device *dev)
{
	return 0;
}

#define EMUL_I2C_DEFINE(inst)                                                                      \
	EMUL_DT_INST_DEFINE(inst, &emul_imager_init_i2c, NULL, NULL, &emul_imager_i2c_api, NULL);

which does not specifiy an init priority, that made me think it will use the CONFIG_EMUL_INIT_PRIORITY = 60 for an emul driver instead of CONFIG_I2C_INIT_PRIORITY = 50 of the i2c_emul ?

Maybe we can check this in the build/ ?

Maybe I am wrong, if so, appologize for the mistake !

[josuah]

Maybe we can check this in the build/ ?

Good idea!

west build -b native_sim tests/drivers/video/api/

Max priority level (999 got introduced)

$ west build -p -b native_sim tests/drivers/video/api/ -t initlevels -- -DCONFIG_EMUL_INIT_PRIORITY=999

EARLY
PRE_KERNEL_1
  __init_statics_init_pre: statics_init(NULL)
  __init_posix_arch_console_init: posix_arch_console_init(NULL)
PRE_KERNEL_2
  __init_sys_clock_driver_init: sys_clock_driver_init(NULL)
POST_KERNEL
  __init_enable_logger: enable_logger(NULL)
  __init_k_sys_work_q_init: k_sys_work_q_init(NULL)
  __init___device_dts_ord_31: NULL(__device_dts_ord_31)
  __init___device_dts_ord_32: NULL(__device_dts_ord_32)
  __init___device_dts_ord_39: NULL(__device_dts_ord_39)
APPLICATION
SMP

Min priority level (0)

$ west build -p -b native_sim tests/drivers/video/api/ -t initlevels -- -DCONFIG_EMUL_INIT_PRIORITY=0

EARLY
PRE_KERNEL_1
  __init_statics_init_pre: statics_init(NULL)
  __init_posix_arch_console_init: posix_arch_console_init(NULL)
PRE_KERNEL_2
  __init_sys_clock_driver_init: sys_clock_driver_init(NULL)
POST_KERNEL
  __init_enable_logger: enable_logger(NULL)
  __init_k_sys_work_q_init: k_sys_work_q_init(NULL)
  __init___device_dts_ord_31: NULL(__device_dts_ord_31)
  __init___device_dts_ord_32: NULL(__device_dts_ord_32)
  __init___device_dts_ord_39: NULL(__device_dts_ord_39)
APPLICATION
SMP

From build/zephyr/include/generated/zephyr/devicetree_generated.h:

* Node dependency ordering (ordinal and path):

 *   0   /
 *   1   /adc
 *   2   /aliases
 *   3   /bt_hci_userchan
 *   4   /can
 *   5   /can_loopback0
 *   6   /chosen
 *   7   /counter
 *   8   /dma
 *   9   /eeprom
 *   10  /espi@300
 *   11  /sdl_dc
 *   12  /input-sdl-touch
 *   13  /lvgl_pointer
 *   14  /mspi@400
 *   15  /rng
 *   16  /rtc
 *   17  /spi@200
 *   18  /uart
 *   19  /uart_1
 *   20  /udc0
 *   21  /cpus
 *   22  /cpus/cpu@0
 *   23  /flash-controller@0
 *   24  /flash-controller@0/flash@0
 *   25  /flash-controller@0/flash@0/partitions
 *   26  /flash-controller@0/flash@0/partitions/partition@0
 *   27  /flash-controller@0/flash@0/partitions/partition@c000
 *   28  /flash-controller@0/flash@0/partitions/partition@75000
 *   29  /flash-controller@0/flash@0/partitions/partition@de000
 *   30  /flash-controller@0/flash@0/partitions/partition@fc000
 *   31  /i2c@100
 *   32  /i2c@100/video_emul_imager@6
 *   33  /i2c@100/video_emul_imager@6/port
 *   34  /i2c@100/video_emul_imager@6/port/endpoint
 *   35  /gpio_emul
 *   36  /leds
 *   37  /leds/led_0
 *   38  /test
 *   39  /test/video_emul_rx@10003000
 *   40  /test/video_emul_rx@10003000/port
 *   41  /test/video_emul_rx@10003000/port/endpoint@0
 *   42  /test/video_emul_rx@10003000/port/endpoint@1

No change... Is it even called anywhere?

$ grep -r EMUL_INIT_PRIORITY *
subsys/emul/Kconfig:config EMUL_INIT_PRIORITY

Not anywhere... Maybe a submodule somewhere else?

https://github.com/search?q=CONFIG_EMUL_INIT_PRIORITY&type=code

Pan-Github search shows nothing. Which is surprising... Maybe 3rd party private repositories use it for something...
But we might be init-level-safe for this time.

Good to know this patch will not randomly break an unrelated pull-request due to unstable init priorities.

Maybe I am wrong, if so, apologize for the mistake !

I am glad you are helping me for this PR!

[josuah]

Cherry-picking your commit with CONFIG_VIDEO_EMUL_RX_INIT_PRIORITY, which is a lot simpler than the crazy _BEFORE_/_AFTER_ scheme I plotted.

All drivers (including imagers) use CONFIG_VIDEO_INIT_PRIORITY like they do now. Whenever a driver needs a different priority level (to be before or after an image sensor), they can define their own priority level.

It took me long enough to get that but I think now we are good. 😅

[ngphibang]

I didn't see any usage of CONFIG_EMUL_INIT_PRIORITY neither.

Anyways, I think with :

#define EMUL_I2C_DEFINE(inst)                                                                      \
	EMUL_DT_INST_DEFINE(inst, &emul_imager_init_i2c, NULL, NULL, &emul_imager_i2c_api, NULL);

DT_INST_FOREACH_STATUS_OKAY(EMUL_I2C_DEFINE)

it will take the inst of the i2c0 node of the native_sim.dts which is drived by the i2c_emul driver which is initialized with priority 50. So, everything is fine.

	i2c0: i2c@100 {
		status = "okay";
		compatible = "zephyr,i2c-emul-controller"; => this is i2c_emul driver.

I was a bit confused because of CONFIG_EMUL_INIT_PRIORITY (which has no role here) and because in video_emul_imager.c there are two DT_INST_FOREACH_STATUS_OKAY() calls so I was not sure which instant is used (the i2c or the imager) ...

Thanks and sorry for my confusion.

[josuah]

Better check twice than cause random CI failure for everyone 👍

[Copilot]

Pull Request Overview

This PR updates the emulated image sensor drivers to use a dedicated I2C emulator rather than ad-hoc register emulation. Key changes include:

• Adjusting the initialization priority in the video emulation receiver driver.
• Replacing hard-coded register accesses in the imagers with proper I2C calls.
• Introducing an emulated I2C transfer function and related API for the image sensor.

Reviewed Changes

Copilot reviewed 2 out of 6 changed files in this pull request and generated no comments.

File	Description
drivers/video/video_emul_rx.c	Update of the initialization macro to use a dedicated init priority.
drivers/video/video_emul_imager.c	Replacement of ad-hoc register emulation with I2C read/write operations and addition of a simulated I2C bus.

Files not reviewed (4)

• drivers/video/Kconfig.emul_imager: Language not supported
• drivers/video/Kconfig.emul_rx: Language not supported
• tests/drivers/video/api/app.overlay: Language not supported
• tests/drivers/video/api/prj.conf: Language not supported

[josuah]

Force-push:

• Rebase on top of drivers: video: common: introduce CCI utilities #87935
• Slight implementation change to fit the changes of CCI (ARRAY_SIZE() rather than {0} sentinel).

[avolmat-st]

This emul imager is all based on 8bit registers as far as I see, so why not using the 8bits variants of accessor ?
so video_reg8 etc so there is no need to change all.

Reason why I am pointing this is the code at the very end of the driver

static uint8_t fake_regs[UINT8_MAX] = {
	[EMUL_IMAGER_REG_SENSOR_ID] = EMUL_IMAGER_SENSOR_ID,			[EMUL_IMAGER_REG_SENSOR_ID & VIDEO_REG_ADDR_MASK] = EMUL_IMAGER_SENSOR_ID,
};		};

in which there is this mask using VIDEO_REG_ADDR_MASK in order to just keep the address. I feel that VIDEO_REG_ADDR_MASK is part if the CCI helper more than all drivers and normally drivers might not have to use this macro which is here for the helper to "construct" the addresses.

[josuah]

If I understand correctly:

a. The imaginary hardware needs to cover as many edge cases as possible
b. On top of this model, the implementation needs to be as straightforward as possible.

Rather than forcing _REG8() macros in the implementation (b.), I must tune the hardware model (a.) so that it covers the edge cases where the CCI library helps (so it can be tested). Otherwise this test driver will be a bit clumsy like now. :D

normally drivers might not have to use this macro

Real drivers would not have any of the code below /* Simulated I2C bus */ but this part still needs to be improved you are right!

[avolmat-st]

I am referring to basically instead of using struct video_reg { }, use the struct video_reg8 { }.
The on a per register access, use:
video_write_cci_reg(&cfg->i2c, SENSORNAME_REG8(EMUL_IMAGER_REG_CUSTOM), data->ctrls.custom.val);
and
video_write_cci_multiregs8 instead of video_write_cci_multiregs

[josuah]

video_write_cci_multiregs8 instead of video_write_cci_multiregs

I agree and am currently converting it. Would you rather do it for only the initial registers (the vendor blobs)? Or all the extra documented regsiters?

video_write_cci_reg(&cfg->i2c, SENSORNAME_REG8(EMUL_IMAGER_REG_CUSTOM), data->ctrls.custom.val);

Could I ask your opinion on this:

First we make a model that permits us to to forget about the register size anywhere in the driver.
Second we do not use it and still use a manual API.

We do not necessarily need to impose a "greater model" that we do not want to use, maybe the simplicity of...

#define EMUL_IMAGER_REG_CUSTOM 0x12

video_write_reg(&cfg->i2c, EMUL_IMAGER_REG8, EMUL_IMAGER_REG_CUSTOM, data->ctrls.custom.val);

... is what we always wanted?

Or was it a special case where all registers are 8-bit, where a "degraded" model is better, for the sake of simplicity and avoiding casting:

#define EMUL_IMAGER_REG_CUSTOM EMUL_IMAGER_REG8(0x12)

struct video_reg8 init_regs[] = {
        {(uint8_t)EMUL_IMAGER_REG_CUSTOM, 123},
        {...},
};

[josuah]

The practical issue I saw with...

video_write_cci_reg(&cfg->i2c, SENSORNAME_REG8(EMUL_IMAGER_REG_CUSTOM), data->ctrls.custom.val);

... is that in such case:

• some drivers use #define SENSORNAME_REG_YYY 0x203 only,
• some drivers use #define SENSORNAME_REG_YYY SENSORNAME_REG8(0x203) only,
• some dirvers use a mix of both depending if they are included on a multireg8() or not

The CCI library does not help abstracting the register sizes in such case.

So maybe this is not the right model and the CCI library abstracts too much and comes in the way rather than takes the complexity out of the way?

Note that even after it's merged it is possible to fine-tune the API for some use-cases I suppose...

[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