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

[josuah]

No dependency. 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