Video: addition of the imx335 driver #88825

avolmat-st · 2025-04-19T21:23:29Z

This PR introduces the CSI IMX335 Sensor driver (5MPix). The sensor supports both 10 and 12 bits bayer format in 2 or 4 csi lanes.
It is based on the CCI helper in order to have a cleaner code.
I kept few TODO within the source code:

For the time being, only 10 bit in 2 data lanes is supported. data-lane DT property isn't checked.
The sensor can be clock using various clock-rate. Register control for the various possible clock rate is in place however check of the DT in order to read the clock rate isn't in place (we could add a fixed-clock within the DT for that purpose). For the time being this is fixed to 24MHz.

This PR depends on #87935

josuah

This is awesome and we can even order the module from here.

I did my best to proofread without the datasheet (as often with image sensors, secret).

A few comments in case these were omissions. If these are intentional, please keep them like so!

josuah · 2025-04-19T22:27:53Z

drivers/video/imx335.c

+	ret = imx335_set_input_clk(dev, MHZ(24));
+	if (ret) {
+		LOG_ERR("Unable to configure INCK");
+		return -EIO;


Suggested change

return -EIO;

return ret;

josuah · 2025-04-19T22:28:05Z

drivers/video/imx335.c

+	ret = video_write_cci_multi(&cfg->i2c, imx335_mode_2l_10b);
+	if (ret) {
+		LOG_ERR("Unable to initialize the sensor");
+		return -EIO;


Suggested change

return -EIO;

return ret;

josuah · 2025-04-19T22:28:13Z

drivers/video/imx335.c

+	ret = video_write_cci_multi(&cfg->i2c, imx335_init_params);
+	if (ret) {
+		LOG_ERR("Unable to initialize the sensor");
+		return -EIO;


Suggested change

return -EIO;

return ret;

josuah · 2025-04-19T22:30:46Z

drivers/video/imx335.c

+		break;
+	default:
+		LOG_ERR("Unsupported inck freq (%d)\n", rate);
+		return -EIO;


Suggested change

return -EIO;

return -EINVAL;

josuah · 2025-04-19T22:43:19Z

If interested in making test of this sensor more easy, it is possible to add the devicetree snippets needed to integrate this sensor with a compatible board via a shield. For instance like these:

Permitting to run west build --board stm32n6... --shield b_cams_imx samples/drivers/video/capture for instance.

avolmat-st · 2025-04-20T12:24:36Z

Thanks for all your comments. I think all of your comments are valid so I will correct the code.
Regarding the shield, I already have it but haven't yet pushed it yet. Reason for that is that, for the N6 this is based on the work done on the DCMIPP so since I haven't pushed it yet I haven't push yet the shield as well. The shield connect to a connector which expose the 22 pins "RPi" type connector.

ngphibang

LGTM in general. Eager for more features coming in the future (chipID, controls, data lanes, resolution, etc.) :-).

Could you add an entry to https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/build_all/video/app.overlay so that we can have a minimal built-only CI test. Without it, we cannot detect even only compilation errors. As you can see, the PRs has "passed" CI although it cannot be compiled because of the dependency on #87935.

Thanks

drivers/video/imx335.c

ngphibang · 2025-04-20T17:18:13Z

drivers/video/imx335.c

+static int imx335_set_ctrl_exposure(const struct device *dev, int value)
+{
+	const struct imx335_config *cfg = dev->config;
+	uint32_t shutter;
+	int ret;
+
+	/* Since we never update VMAX, we can use the default value directly */
+	shutter = (IMX335_VMAX_DEFAULT - (value / IMX335_1H_PERIOD_USEC));
+	if (shutter < IMX335_SHR0_MIN) {
+		return -EINVAL;
+	}
+
+	ret = video_write_cci_reg(&cfg->i2c, IMX335_REGHOLD, 1);
+	if (ret) {
+		return ret;
+	}
+
+	ret = video_write_cci_reg(&cfg->i2c, IMX335_SHR0, shutter);
+	if (ret) {
+		return ret;
+	}
+
+	return video_write_cci_reg(&cfg->i2c, IMX335_REGHOLD, 0);
+}
+
+static int imx335_set_ctrl(const struct device *dev, unsigned int cid, void *value)
+{
+	switch (cid) {
+	case VIDEO_CID_GAIN:
+		return imx335_set_ctrl_gain(dev, (int)(value));
+	case VIDEO_CID_EXPOSURE:
+		return imx335_set_ctrl_exposure(dev, (int)(value));
+	default:
+		return -ENOTSUP;
+	}
+}


Just out of curiosity. In Linux, it seems exposure and gain are clustered (dependent each other). Here, it can be set independently ?

Actually I based this driver on the imx335 driver that is provided with the STM32 HAL.
As far as registers are concerned, gain & exposure aren't really linked to each others. However they both need to be set upon putting the sensor register in hold mode so it can help to set both of them at once to avoid more register access.
I also actually changed another thing which is the range of the exposure. In the previous version I pushed, the exposure was expressed in us, while now it is done in lines, in order to be consistent with the exposure value used in the Linux world.

Thanks for the clarification, just checked in Linux and see they are independent in terms of registers, another interesting usecase of auto-cluster.

josuah · 2025-04-20T18:33:34Z

Converting to draft until the dependency is merged as suggested earlier.

josuah · 2025-04-21T12:12:49Z

I completely forgot to check that the sensor needed to be added to tests/drivers/build_all/video/app.overlay!
Here is an example to save time.

Introduction of bindings for the imx335 sensor. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

avolmat-st · 2025-04-23T20:39:23Z

I push a new version of the driver, all rebased on top of the HEAD, meaning top of new ctrl and video dev frameworks.

Copilot

Pull Request Overview

This PR introduces a new device tree binding for the Sony IMX335 sensor driver, enabling support for the CMOS sensor with 10-bit operation and a dedicated reset-gpios property.

Added a YAML binding file with sensor description, compatibility information, and reset-gpios property.
Defined a child-binding section to include video interface bindings.

Files not reviewed (4)

drivers/video/CMakeLists.txt: Language not supported
drivers/video/Kconfig: Language not supported
drivers/video/Kconfig.imx335: Language not supported
tests/drivers/build_all/video/app.overlay: Language not supported

dts/bindings/video/sony,imx335.yaml

drivers/video/imx335.c

ngphibang · 2025-04-24T05:33:40Z

drivers/video/imx335.c

+static int imx335_set_ctrl_exposure(const struct device *dev, int value)
+{
+	const struct imx335_config *cfg = dev->config;
+	uint32_t shutter;
+	int ret;
+
+	/* Since we never update VMAX, we can use the default value directly */
+	shutter = (IMX335_VMAX_DEFAULT - (value / IMX335_1H_PERIOD_USEC));
+	if (shutter < IMX335_SHR0_MIN) {
+		return -EINVAL;
+	}
+
+	ret = video_write_cci_reg(&cfg->i2c, IMX335_REGHOLD, 1);
+	if (ret) {
+		return ret;
+	}
+
+	ret = video_write_cci_reg(&cfg->i2c, IMX335_SHR0, shutter);
+	if (ret) {
+		return ret;
+	}
+
+	return video_write_cci_reg(&cfg->i2c, IMX335_REGHOLD, 0);
+}
+
+static int imx335_set_ctrl(const struct device *dev, unsigned int cid, void *value)
+{
+	switch (cid) {
+	case VIDEO_CID_GAIN:
+		return imx335_set_ctrl_gain(dev, (int)(value));
+	case VIDEO_CID_EXPOSURE:
+		return imx335_set_ctrl_exposure(dev, (int)(value));
+	default:
+		return -ENOTSUP;
+	}
+}


Thanks for the clarification, just checked in Linux and see they are independent in terms of registers, another interesting usecase of auto-cluster.

drivers/video/imx335.c

avolmat-st · 2025-04-24T08:46:25Z

Actually, doing further tests I noticed an issue with the exposure / analogue_gain control. I might have misunderstood the (auto)-cluster and current code simply lead to not being able to set the exposure correctly. Considering that the application will perform set_ctrl on gain and exposure and their value aren't related, I think they have to be put outside of a cluster.

Add support for the Sony IMX335 CSI sensor. This sensor supports resolution of 2592x1944 in RGGB bayer format either 10 or 12 bits and using 2 or 4 CSI lanes. For the time being only 10 bits on 2 CSI lanes is supported via this commit. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

github-actions bot added area: Video Video subsystem platform: NXP Drivers NXP Semiconductors, drivers labels Apr 19, 2025

github-actions bot requested review from dbaluta, decsny, dleach02, josuah, loicpoulain, manuargue, mmahadevan108, ngphibang and Raymond0225 April 19, 2025 21:24

github-actions bot assigned dleach02 and mmahadevan108 Apr 19, 2025

josuah previously approved these changes Apr 19, 2025

View reviewed changes

avolmat-st dismissed josuah’s stale review via 442eb3e April 20, 2025 13:29

avolmat-st force-pushed the video_imx335_driver branch from 95979fe to 442eb3e Compare April 20, 2025 13:29

josuah previously approved these changes Apr 20, 2025

View reviewed changes

josuah mentioned this pull request Apr 20, 2025

drivers: video: common: introduce CCI utilities #87935

Open

josuah added platform: STM32 ST Micro STM32 area: camera labels Apr 20, 2025

ngphibang reviewed Apr 20, 2025

View reviewed changes

josuah marked this pull request as draft April 20, 2025 18:33

ngphibang mentioned this pull request Apr 21, 2025

Implement video control framework #82158

Merged

josuah self-requested a review April 21, 2025 15:54

dts-bindings: video: imx335: addition of imx335 description

8f594ad

Introduction of bindings for the imx335 sensor. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

avolmat-st dismissed josuah’s stale review via 50bbd8c April 23, 2025 20:39

avolmat-st force-pushed the video_imx335_driver branch from 442eb3e to 50bbd8c Compare April 23, 2025 20:39

kartben requested a review from Copilot April 24, 2025 01:11

Copilot AI reviewed Apr 24, 2025

View reviewed changes

dts/bindings/video/sony,imx335.yaml Show resolved Hide resolved

ngphibang reviewed Apr 24, 2025

View reviewed changes

avolmat-st force-pushed the video_imx335_driver branch from 50bbd8c to f162899 Compare April 24, 2025 06:23

avolmat-st mentioned this pull request Apr 24, 2025

Contributor nomination - Alain Volmat - ST #89005

Closed

avolmat-st force-pushed the video_imx335_driver branch from f162899 to 8290ded Compare April 24, 2025 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Video: addition of the imx335 driver #88825

Video: addition of the imx335 driver #88825

avolmat-st commented Apr 19, 2025 •

edited

Loading

josuah left a comment

josuah Apr 19, 2025

josuah Apr 19, 2025

josuah Apr 19, 2025

josuah Apr 19, 2025

josuah commented Apr 19, 2025

avolmat-st commented Apr 20, 2025

ngphibang left a comment

ngphibang Apr 20, 2025

avolmat-st Apr 23, 2025

ngphibang Apr 24, 2025

josuah commented Apr 20, 2025

josuah commented Apr 21, 2025

avolmat-st commented Apr 23, 2025

Copilot AI left a comment

ngphibang Apr 24, 2025

avolmat-st commented Apr 24, 2025

Video: addition of the imx335 driver #88825

Are you sure you want to change the base?

Video: addition of the imx335 driver #88825

Conversation

avolmat-st commented Apr 19, 2025 • edited Loading

josuah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Apr 19, 2025

avolmat-st commented Apr 20, 2025

ngphibang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Apr 20, 2025

josuah commented Apr 21, 2025

avolmat-st commented Apr 23, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Choose a reason for hiding this comment

avolmat-st commented Apr 24, 2025

avolmat-st commented Apr 19, 2025 •

edited

Loading