6.12/iob/topic/imx335 by kbingham · Pull Request #6804 · raspberrypi/linux

kbingham · 2025-04-22T13:05:12Z

Add fixes for the IMX335 camera sensor.

The kernel driver patches are all posted and submitted (or already included) in the linux-media trees.

The dtoverlay adds support for connecting an IMX335 to the RPi5.

The driver already supports two lane operation, so it should be possible to update this to use 2 lanes too .... I suspect that's easy with the overlays?

Anyhow - posting for first review - and consideration.

In commit 81495a5 ("media: imx335: Fix active area height discrepency") the height for the mode struct was rectified to '1944'. However, the name of mode struct is still reflecting to '1940'. Update it. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Support vertical flip by setting REG_VREVERSE. Additional registers also needs to be set per mode, according to the readout direction (normal/inverted) as mentioned in the data sheet. Since the register IMX335_REG_AREA3_ST_ADR_1 is based on the flip (and is set via vflip related registers), it has been moved out of the 2592x1944 mode regs. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

kbingham · 2025-04-22T13:17:13Z

Aha, my first review feedback:

"Overlays without documentation:
imx335"

6by9

We tend to use eg "Commit c0aa40f upstream" in the commit message when backporting, just to make life easier when forward porting to a kernel release which already includes it.
(That hash would apply for "media: imx335: Set vblank immediately").

You say you have some modules. Are they still commercially available? I try and have a sample of most modules we nominally support.

6by9 · 2025-04-22T13:26:40Z

+		       <&clk_frag>, "target:0=",<&cam0_clk>,
+		       <&reg_frag>, "target:0=",<&cam0_reg>,
+		       <&cam_node>, "clocks:0=",<&cam0_clk>,
+		       <&cam_node>, "avdd1-supply:0=",<&cam0_reg>;


avdd1-supply? The regulator appears to be avdd-supply in the driver and imx335.dtsi.

argh - I suspect I made a copy paste error... sorry. Cleaning up...

That's why we do code reviews!

Fixed, and also now tested by swapping on to cam0 ;-)

6by9 · 2025-04-22T13:28:56Z

+				csi_ep: endpoint {
+					remote-endpoint = <&cam_endpoint>;
+					clock-lanes = <0>;
+					data-lanes = <1 2 3 4>;


Seeing as the driver can work over 2 lanes, I just wondered if you'd tried it.
The imx290/327 overlay supports both if you want a copy/paste for implementing the override.

I haven't tried it on RPi yet. I've used / tested 2 lane on i.MX8MP where we developed support for this sensor ... but yes I would like to test on RPi too - as then this module could hook directly to an RPi4 for instance too.

I'll give that a go next to test it.

Fine. It was only a query rather than an obligation. As you say, it does make it functional with Pi0-4 if it'll work with 2 lanes which is beneficial to anyone making modules.

6by9 · 2025-04-22T13:29:48Z

+					remote-endpoint = <&cam_endpoint>;
+					clock-lanes = <0>;
+					data-lanes = <1 2 3 4>;
+					clock-noncontinuous;


Is it really non-continuous? It's not configurable in the driver, so depends on how it was written in the first place

Err - no - I don't think this should be in here unless its supported by the driver.

This is one of those things I hate about some drivers. It's routinely not checked or specified when the driver is first merged. Initial DT files will often be wrong, and so correcting it later may mean breaking existing DT or at least changing the behaviour slightly.

The Starvis datasheets seem to lack this information, so I may dig out an oscilloscope to see what the others in the family are doing.

6by9 · 2025-04-22T13:32:14Z

+	__overrides__ {
+		rotation = <&cam_node>,"rotation:0";
+		orientation = <&cam_node>,"orientation:0";
+		media-controller = <&csi>,"brcm,media-controller?";


6.12 needs to switch the compatible string to brcm,bcm2835-unicam-legacy.

See https://github.com/raspberrypi/linux/blob/rpi-6.12.y/arch/arm/boot/dts/overlays/imx219-overlay.dts#L67-L72 and https://github.com/raspberrypi/linux/blob/rpi-6.12.y/arch/arm/boot/dts/overlays/imx219-overlay.dts#L93

Ack, no idea how this part is being handled, but I've copied the same as imx219. That also gave me a clearer insight into the 2lane/4lane option too.

kbingham · 2025-04-22T14:39:40Z

We tend to use eg "Commit c0aa40f upstream" in the commit message when backporting, just to make life easier when forward porting to a kernel release which already includes it. (That hash would apply for "media: imx335: Set vblank immediately").

Ack - will handle that in the next update.

You say you have some modules. Are they still commercially available? I try and have a sample of most modules we nominally support.

The IMX335 support we did was for a different platform - and Arducam made the modules for us. I've asked them to make more modules (they use the RPi-22 pin connector) and they work well on Raspberry Pi so I hope I can ask Arducam to send you a sample.

When the vblank v4l2 control is set, it does not get written to the hardware immediately. It only gets updated when exposure is set. Change the behavior such that the vblank is written immediately when the control is set. Commit c0aa40f ("media: imx335: Set vblank immediately")' upstream Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

CCI_REG_HNUM should be using CCI_REG16_LE() instead of CCI_REG8() as HNUM spans from 0x302e[0:7] to 0x302f[0:3]. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

kbingham · 2025-04-23T12:01:01Z

Updated, and now with updated link-frequency support, fixed cam0 support, and configurable 2-lane (default) 4-lane options so this will be able to work on other Pi platforms too. And the upstream reference has been added to the commit which is already merged upstream.

Further added documentation to the Overlay README, and provided a defconfigs update to enable the imx335 module in kernel builds.

And as a last treat, while adding the 4lane options and documenting I saw that other sensors hadn't been updated to reflect CM5 and Pi5 support - so a cleanup commit on top for that.

6by9

I'm happy to ignore the checkpatch errors - the unknown commits are because it doesn't check out a full tree.

Phil's overlaycheck script has the 3 complaints flagged.

6by9 · 2025-04-23T12:08:13Z

+Info:   Sony IMX335 camera module.
+        Uses Unicam 1, which is the standard camera connector on most Pi
+        variants.
+Load:   dtoverlay=imx415,<param>


s/imx415/imx335

bah humbug ;-) Fixing. ...

6by9 · 2025-04-23T12:08:37Z

@@ -2822,6 +2822,23 @@ Params: 4lane                   Enable 4 CSI2 lanes. This requires a Compute
        cam0                    Adopt the default configuration for CAM0 on a
                                Compute Module (CSI0, i2c_vc, and cam0_reg).



2 blank lines required.

6by9 · 2025-04-23T12:08:51Z

+                                Compute Module (CSI0, i2c_vc, and cam0_reg).
+        4lane                   Enable 4 CSI2 lanes. This requires a Compute
+                                Module (1, 3, 4, or 5) or Pi 5.



2 blank lines required here too.

Ohh, sorry - I hadn't noticed there were two line gaps ... that's an easy fix too ;-)

Add support for the Sony IMX335 image sensor by including the module for supported Raspberry Pi platforms. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide support for the IMX335 sensor, which has upstream linux kernel drivers. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Several cameras can be connected that support 4 data lane operation. Fix the documentation to reflect these apply to Pi5 and CM5. This fixes the documentation for: imx258, imx290, and imx327. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

pelwell · 2025-04-23T12:37:31Z

Feel free to merge if you're happy now, @6by9.

6by9 · 2025-04-23T12:51:19Z

I'm happy. Just letting the CI builds complete (I'm not expecting them to fail though).

6by9 · 2025-04-23T13:39:28Z

Merged. Thanks Kieran.

kernel: lockdep fixes for imx708 and imx477 See: raspberrypi/linux#6813 kernel: drm/vc4: plane: Fix incorrect handling of GEN_6_D in vc4_plane_async_set_fb See: raspberrypi/linux#6811 kernel: configs: Enable UNICODE support (for filesystems) See: raspberrypi/linux#6809 kernel: configs: Enable the EROFS read-only filesystem See: raspberrypi/linux#6806 kernel: overlays: Add aht20 support to i2c-sensor See: raspberrypi/linux#6805 kernel: 6.12/iob/topic/imx335 See: raspberrypi/linux#6804 kernel: hwmon: aht10: Fix AHT20 initialization See: raspberrypi/linux#6803 kernel: drm/vc4: plane: Use nearest neighbour filter with YUV444 workaround See: raspberrypi/linux#6796 kernel: dtoverlays: Create Pi5 variant of tc358743 overlay See: raspberrypi/linux#6795 kernel: usb: xhci: default to Intel scheme for calculating U1/U2 timeouts See: raspberrypi/linux#6794 kernel: Add TCS3472 and VEML6040 sensor support (and a few others) See: raspberrypi/linux#6448 kernel: drm/vc4: plane: Increase UPM allocation size for YUV444 See: raspberrypi/linux#6791 kernel: media: i2c: imx219: Restore the 1920x1080 to using a 1:1 PAR See: raspberrypi/linux#6793

6by9 · 2025-04-28T15:41:04Z

@kbingham My module arrived this morning - thanks for arranging.

Works OK, but rpicam-hello --list-cameras gives me multiple identical modes for the sensor

pi@pi:~/rpicam-apps $ ~/rpicam-apps/build/apps/rpicam-hello --list-cameras
Available cameras
-----------------
0 : imx335 [2592x1944 12-bit RGGB] (/base/soc/i2c0mux/i2c@1/imx335@1a)
    Modes: 'SRGGB10_CSI2P' : 2592x1944 [29.99 fps - (0, 0)/2592x1944 crop]
                             2592x1944 [29.99 fps - (0, 0)/2592x1944 crop]
                             2592x1944 [29.99 fps - (0, 0)/2592x1944 crop]
           'SRGGB12_CSI2P' : 2592x1944 [29.99 fps - (0, 0)/2592x1944 crop]
                             2592x1944 [29.99 fps - (0, 0)/2592x1944 crop]
                             2592x1944 [29.99 fps - (0, 0)/2592x1944 crop]

imx335_enum_frame_size - https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx335.c#L663-L664

	if (fsize->index > ARRAY_SIZE(imx335_mbus_codes))
		return -EINVAL;

should be if (fsize->index), as index is the index of the frame size, not of the format code.

Just letting you know as you're the maintainer for this driver.

kbingham · 2025-04-28T15:47:57Z

Aha - oops! Yes, I'll get this fixed.

kbingham · 2025-04-28T17:44:02Z

Fix tested. Will post upstream and a new pr here! Thanks!

Umang Jain added 2 commits April 22, 2025 11:43

6by9 reviewed Apr 22, 2025

View reviewed changes

Rahi374 and others added 2 commits April 23, 2025 12:57

media: imx335: Use correct register width for HNUM

b73f40d

CCI_REG_HNUM should be using CCI_REG16_LE() instead of CCI_REG8() as HNUM spans from 0x302e[0:7] to 0x302f[0:3]. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

kbingham force-pushed the 6.12/iob/topic/imx335 branch from 697707b to 58f9fc2 Compare April 23, 2025 11:58

6by9 reviewed Apr 23, 2025

View reviewed changes

kbingham added 3 commits April 23, 2025 13:25

defconfigs: Add IMX335 to all Pi defconfigs

796f522

Add support for the Sony IMX335 image sensor by including the module for supported Raspberry Pi platforms. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

dtoverlays: Add overlay for the Sony IMX335 4-Lane camera

1d5ebb4

Provide support for the IMX335 sensor, which has upstream linux kernel drivers. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

kbingham force-pushed the 6.12/iob/topic/imx335 branch from 58f9fc2 to 59336a2 Compare April 23, 2025 12:28

pelwell approved these changes Apr 23, 2025

View reviewed changes

6by9 merged commit 763693b into raspberrypi:rpi-6.12.y Apr 23, 2025
12 of 13 checks passed

		@@ -2822,6 +2822,23 @@ Params: 4lane Enable 4 CSI2 lanes. This requires a Compute
		cam0 Adopt the default configuration for CAM0 on a
		Compute Module (CSI0, i2c_vc, and cam0_reg).

Conversation

kbingham commented Apr 22, 2025

Uh oh!

kbingham commented Apr 22, 2025

Uh oh!

6by9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbingham commented Apr 22, 2025

Uh oh!

kbingham commented Apr 23, 2025

Uh oh!

6by9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelwell commented Apr 23, 2025

Uh oh!

6by9 commented Apr 23, 2025

Uh oh!

Uh oh!

6by9 commented Apr 23, 2025

Uh oh!

6by9 commented Apr 28, 2025

Uh oh!

kbingham commented Apr 28, 2025

Uh oh!

kbingham commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants