add ams-osram image sensors. #6552

fiepfiep · 2024-12-19T16:55:10Z

Add support for the following image sensors:
mira050
mira130
mira220
mira016

pelwell · 2024-12-19T18:13:15Z

Why have you structured your files like this, putting the code in .inl files and including it into .c files? The inclusions are:

drivers/media/i2c/mira016.c:10:#include "mira016.inl"
drivers/media/i2c/mira050.c:10:#include "mira050.inl"
drivers/media/i2c/mira050color.c:10:#include "mira050.inl"
drivers/media/i2c/mira130.c:10:#include "mira130.inl"
drivers/media/i2c/mira220.c:10:#include "mira220.inl"
drivers/media/i2c/mira220color.c:10:#include "mira220.inl"

2 of the .inls are only included in one place - just embed them directly into the .c code.

For the color variants, where 2 .c files share a .inl, the only difference is the driver name, the compatible string, and a way of determining which is being used. This could all be achieved with a single .c file with the .inl embedded, saving the space occupied by a separate module build.

6by9

I've only gone through mira016 as I'm assuming they are all similar.
I'll agree with Phil's comment that the minimal .c file and a huge .inl file is very strange.

I'm reviewing as if you're going to submit this to the mainline linux-media mailing list. I hope you will do in due course as otherwise libcamera won't merge the support for your sensor.

You've chosen an awkward moment to try and add new drivers, as with the switch to 6.12 being imminent, the structure of drivers changes fairly significantly, and the use of the subdev state is mandated. Using 6.12 and basing on something like imx290.c would be a better start point.

General comment of lots of dead or commented out code.

Allowing userspace to generally poke random sensor registers is generally not acceptable as it means requiring a random (probably closed-source) binary to get the sensor working.

Mainline will want a device tree binding. They're a bit of a faff, but not too bad. Various parameters (eg link frequency, and PMIC / LED components) will need to be nailed down before doing that.

6by9 · 2024-12-20T10:51:14Z

arch/arm64/configs/bcm2711_defconfig

@@ -1,4 +1,4 @@
-CONFIG_LOCALVERSION="-v8"
+CONFIG_LOCALVERSION="-v8+"


You shouldn't be changing CONFIG_LOCALVERSION here.

6by9 · 2024-12-20T10:51:21Z

arch/arm64/configs/bcmrpi3_defconfig

@@ -1,4 +1,4 @@
-CONFIG_LOCALVERSION="-v8"
+CONFIG_LOCALVERSION="-v8+"


You shouldn't be changing CONFIG_LOCALVERSION here.

6by9 · 2024-12-20T10:56:57Z

drivers/media/i2c/mira016.inl

+ * Introduce new v4l2 control
+ */
+#include <linux/v4l2-controls.h>
+#define AMS_CAMERA_CID_BASE (V4L2_CTRL_CLASS_CAMERA | 0x2000)


It's more normal to claim a range above V4L2_CID_USER_BASE, and then use those.

6by9 · 2024-12-20T11:00:20Z

drivers/media/i2c/mira016.inl

+#define MIRA016_TEST_PATTERN_2D_GRADIENT 0x02
+
+/* Embedded metadata stream structure */
+#define MIRA016_EMBEDDED_LINE_WIDTH 0


If you have no embedded data, then you've chosen a bad base driver to base yours on. There is no point in having the metadata pad at all, so it should be dropped.

Using a second pad for metadata is also not the way that mainline are doing it, and we're about to switch to that approach.

6by9 · 2024-12-20T11:05:22Z

drivers/media/i2c/mira016.inl

+// converted_Draco_i2c_configuration_sequence_hex_10bit_1x_360fps_Version3
+static const struct mira016_reg full_400_400_100fps_10b_1lane_reg_pre_soft_reset[] = {
+	// Sensor Operating Mode
+	{57344, 0},


The use of decimal here looks odd, particularly seeing as you converted something that included hex in the name. 57344 = 0xe000.

Can any of this be documented with register names?

6by9 · 2024-12-20T11:38:29Z

drivers/media/i2c/mira016.inl

+
+	/* Current mode */
+	const struct mira016_mode *mode;
+	/* current bit depth, may defer from mode->bit_depth */


Typo for differ.

And I see no route in the code where it can differ. Can you point it out to me?

6by9 · 2024-12-20T11:39:23Z

drivers/media/i2c/mira016.inl

+			ret = mira016_write(mira016, MIRA016_TEST_PATTERN_REG,
+								mira016_test_pattern_val[ctrl->val]);
+			break;
+		case V4L2_CID_HFLIP:


If not implemented, then don't add the control.

6by9 · 2024-12-20T11:42:22Z

drivers/media/i2c/mira016.inl

+// 0.9Gb/s * 2 * 1 / 12 = 157286400
+// 1.5 Gbit/s * 2 * 1 / 12 = 250 000 000
+#define MIRA016_PIXEL_RATE (400000000)
+/* Should match device tree link freq */


It should also be correct for the sensor. Below you reference MIRA016_DATA_RATE 1500 //Mbit/s, which would imply the link frequency is 750MHz

NB technically this exceeds the spec for the Pi0-4 CSI2 receiver, however you may get away with it.

6by9 · 2024-12-20T11:47:12Z

drivers/media/i2c/mira016.inl

+
+	{
+		printk(KERN_INFO "[MIRA016]: Init PMIC and uC and led driver.\n");
+		mira016->pmic_client = i2c_new_dummy_device(client->adapter,


If you have a separate PMIC device, please write a regulator driver for it, and configure device tree accordingly. Particularly if it is common to all these sensors.

6by9 · 2024-12-20T11:49:35Z

drivers/media/i2c/mira016.inl

+												  MIRA016UC_I2C_ADDR);
+		if (IS_ERR(mira016->uc_client))
+			return PTR_ERR(mira016->uc_client);
+		mira016->led_client = i2c_new_dummy_device(client->adapter,


Other than a couple of init commands, what does the kernel do with this LED driver?

6by9 · 2024-12-20T12:06:36Z

For the color variants, where 2 .c files share a .inl, the only difference is the driver name, the compatible string, and a way of determining which is being used. This could all be achieved with a single .c file with the .inl embedded, saving the space occupied by a separate module build.

There are already image sensor drivers that are a single driver with multiple compatibles to select between colour and mono variants, and also slight variations in the sensor. The best example is imx290 which currently supports mono and colour imx290, colour imx327, and support will be in 6.14 for mono and colour imx462 as well.

Thinking about the overlays, are we better off having one overlay per sensor, and then have an override for mono (or colour) which just changes the compatible? Again we have that already with imx290 (imx327 also shares the same overlay include to reduce the duplication).

add ams-osram image sensors.

928de88

6by9 reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ams-osram image sensors. #6552

add ams-osram image sensors. #6552

fiepfiep commented Dec 19, 2024

pelwell commented Dec 19, 2024

6by9 left a comment

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 Dec 20, 2024

6by9 commented Dec 20, 2024

		@@ -1,4 +1,4 @@
		CONFIG_LOCALVERSION="-v8"
		CONFIG_LOCALVERSION="-v8+"

add ams-osram image sensors. #6552

Are you sure you want to change the base?

add ams-osram image sensors. #6552

Conversation

fiepfiep commented Dec 19, 2024

pelwell commented Dec 19, 2024

6by9 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

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

Choose a reason for hiding this comment

6by9 commented Dec 20, 2024