* [PATCH 0/2] Add bindings and driver for Sony IMX908
@ 2026-07-30 2:15 Lachlan Michael
2026-07-30 2:15 ` [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings Lachlan Michael
2026-07-30 2:15 ` [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver Lachlan Michael
0 siblings, 2 replies; 16+ messages in thread
From: Lachlan Michael @ 2026-07-30 2:15 UTC (permalink / raw)
To: mchehab, sakari.ailus, hverkuil+cisco, laurent.pinchart,
linux-media, devicetree
Cc: robh, krzk+dt, conor+dt, kieran.bingham, jai.luthra,
Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird, Kazumi.A.Sato,
linux-kernel, Lachlan Michael
Hi,
This series adds device tree bindings and a V4L2 sub-device driver
for the Sony IMX908 image sensor.
The IMX908 is an 8.39 MP CMOS image sensor with a MIPI CSI-2 output
interface. The driver supports RAW10 and RAW12 output formats,
exposure and analogue gain controls, image flipping, blanking
controls, window cropping and test pattern generation.
HDR modes and RAW16 output are not currently supported.
Comments and feedback are welcome.
Lachlan Michael (2):
media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
media: i2c: Add Sony IMX908 image sensor driver
.../bindings/media/i2c/sony,imx908.yaml | 107 ++
MAINTAINERS | 7 +
drivers/media/i2c/Kconfig | 11 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/imx908.c | 1593 +++++++++++++++++
5 files changed, 1719 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
create mode 100644 drivers/media/i2c/imx908.c
--
2.47.3
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
2026-07-30 2:15 [PATCH 0/2] Add bindings and driver for Sony IMX908 Lachlan Michael
@ 2026-07-30 2:15 ` Lachlan Michael
2026-07-30 3:24 ` Rob Herring (Arm)
2026-07-30 9:06 ` Krzysztof Kozlowski
2026-07-30 2:15 ` [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver Lachlan Michael
1 sibling, 2 replies; 16+ messages in thread
From: Lachlan Michael @ 2026-07-30 2:15 UTC (permalink / raw)
To: mchehab, sakari.ailus, hverkuil+cisco, laurent.pinchart,
linux-media, devicetree
Cc: robh, krzk+dt, conor+dt, kieran.bingham, jai.luthra,
Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird, Kazumi.A.Sato,
linux-kernel, Lachlan Michael
The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor
with a MIPI CSI-2 output interface.
Add device tree bindings for the sensor.
Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
---
.../bindings/media/i2c/sony,imx908.yaml | 107 ++++++++++++++++++
MAINTAINERS | 7 ++
2 files changed, 114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
new file mode 100644
index 000000000000..6ab382d1cf34
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2026 Sony Semiconductor Solutions
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx908.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX908 CMOS Image Sensor
+
+maintainers:
+ - Lachlan Michael <Lachlan.Michael@sony.com>
+
+description:
+ Sony IMX908 diagonal 6.42 mm (Type 1/2.8) CMOS active pixel type
+ solid-state image sensor with a square pixel array and 8.39 M
+ (3856x2176) pixels, supporting MIPI CSI-2 output with
+ RAW10 and RAW12 formats, configured as either 2 or 4 data lanes.
+ The I2C address is fixed to 0x1A as per sensor data sheet.
+
+properties:
+ compatible:
+ const: sony,imx908
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ description: Input clock (24 MHz, 27 MHz, 37.125 MHz, 72 MHz or 74.25 MHz)
+ maxItems: 1
+
+ dvdd-supply:
+ description: Digital supply voltage (1.1V)
+
+ ovdd-supply:
+ description: Interface supply voltage (1.8V)
+
+ avdd-supply:
+ description: Analog supply voltage (3.3V)
+
+ reset-gpios:
+ description: Sensor reset (XCLR) GPIO. Active low.
+ maxItems: 1
+
+ port:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ additionalProperties: false
+
+ properties:
+ data-lanes:
+ oneOf:
+ - items:
+ - const: 1
+ - const: 2
+ - items:
+ - const: 1
+ - const: 2
+ - const: 3
+ - const: 4
+
+ link-frequencies: true
+
+ required:
+ - data-lanes
+ - link-frequencies
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - dvdd-supply
+ - ovdd-supply
+ - avdd-supply
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ camera@1a {
+ compatible = "sony,imx908";
+ reg = <0x1a>;
+ clocks = <&cdce913_cam0 0>;
+ dvdd-supply = <&cam_dvdd_1r1v>;
+ ovdd-supply = <&cam_ovdd_1r8v>;
+ avdd-supply = <&cam_avdd_3r3v>;
+ reset-gpios = <&tca6416_cam0 5 GPIO_ACTIVE_LOW>;
+
+ port {
+ imx908_out: endpoint {
+ data-lanes = <1 2 3 4>;
+ link-frequencies = /bits/ 64 <720000000>;
+ remote-endpoint = <&csi0_in>;
+ };
+ };
+ };
+ };
+
diff --git a/MAINTAINERS b/MAINTAINERS
index 1ab8736850ea..41b39aa24bda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25264,6 +25264,13 @@ T: git git://linuxtv.org/media.git
F: Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
F: drivers/media/i2c/imx415.c
+SONY IMX908 SENSOR DRIVER
+M: Lachlan Michael <lachlan.michael@sony.com>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
+F: drivers/media/i2c/imx908.c
+
SONY MEMORYSTICK SUBSYSTEM
M: Maxim Levitsky <maximlevitsky@gmail.com>
M: Alex Dubov <oakad@yahoo.com>
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-07-30 2:15 [PATCH 0/2] Add bindings and driver for Sony IMX908 Lachlan Michael
2026-07-30 2:15 ` [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings Lachlan Michael
@ 2026-07-30 2:15 ` Lachlan Michael
2026-07-30 9:09 ` Krzysztof Kozlowski
2026-08-03 0:53 ` Laurent Pinchart
1 sibling, 2 replies; 16+ messages in thread
From: Lachlan Michael @ 2026-07-30 2:15 UTC (permalink / raw)
To: mchehab, sakari.ailus, hverkuil+cisco, laurent.pinchart,
linux-media, devicetree
Cc: robh, krzk+dt, conor+dt, kieran.bingham, jai.luthra,
Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird, Kazumi.A.Sato,
linux-kernel, Lachlan Michael
Add a V4L2 sub-device driver for the Sony IMX908 CMOS image sensor.
The driver supports:
- RAW10 and RAW12 output formats
- Exposure and analogue gain controls
- Horizontal and vertical image flipping
- HBLANK and VBLANK controls
- Full-resolution and window-cropping modes
- Test pattern generation
HDR modes and RAW16 output are not currently supported.
The driver has been tested with libcamera on Raspberry Pi 5 based
hardware.
Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
---
drivers/media/i2c/Kconfig | 11 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/imx908.c | 1593 ++++++++++++++++++++++++++++++++++++
3 files changed, 1605 insertions(+)
create mode 100644 drivers/media/i2c/imx908.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 5d173e0ecf42..8e521f21414b 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -287,6 +287,17 @@ config VIDEO_IMX415
To compile this driver as a module, choose M here: the
module will be called imx415.
+config VIDEO_IMX908
+ tristate "Sony IMX908 sensor support"
+ depends on GPIOLIB
+ select V4L2_CCI_I2C
+ help
+ This is a Video4Linux2 sensor driver for the Sony
+ IMX908 CMOS image sensor.
+
+ To compile this driver as a module, choose M here: the
+ module will called imx908.
+
config VIDEO_MAX9271_LIB
tristate
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index e45359efe0e4..3ccf6d29bbfd 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o
obj-$(CONFIG_VIDEO_IMX355) += imx355.o
obj-$(CONFIG_VIDEO_IMX412) += imx412.o
obj-$(CONFIG_VIDEO_IMX415) += imx415.o
+obj-$(CONFIG_VIDEO_IMX908) += imx908.o
obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o
obj-$(CONFIG_VIDEO_KS0127) += ks0127.o
diff --git a/drivers/media/i2c/imx908.c b/drivers/media/i2c/imx908.c
new file mode 100644
index 000000000000..6ec1322f9c16
--- /dev/null
+++ b/drivers/media/i2c/imx908.c
@@ -0,0 +1,1593 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * V4L2 driver for Sony IMX908
+ *
+ * Diagonal 6.42 mm (Type 1/2.8) CMOS image sensor with 8.39 M pixels.
+ *
+ * Copyright 2026 Sony Semiconductor Solutions Corporation
+ *
+ */
+
+#include <linux/align.h>
+#include <linux/array_size.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mediabus.h>
+#include <media/v4l2-rect.h>
+#include <media/v4l2-subdev.h>
+
+/* ---- IMX908 Registers --------- */
+#define IMX908_REG_STANDBY CCI_REG8(0x3000)
+#define IMX908_STANDBY_EN BIT(0) /* Standby mode */
+#define IMX908_STANDBY_CANCEL 0x00 /* Operating mode */
+#define IMX908_REG_XMSTA CCI_REG8(0x3002) /* [0] Init 1h */
+#define IMX908_START_CONTROLLER 0x00 /* Controller mode operation start */
+#define IMX908_STOP_CONTROLLER 0x01 /* Controller mode operation stop */
+#define IMX908_REG_XMASTER CCI_REG8(0x3003) /* [0] Init 0h */
+#define IMX908_CONTROLLER_MODE 0x00
+#define IMX908_REG_INCK_SEL CCI_REG8(0x3014) /* 0:74.25 1:37.125 2:72 3:27 4:24 */
+#define IMX908_REG_DATARATE_SEL CCI_REG8(0x3015)
+
+/* ---- Crop */
+#define IMX908_REG_WINMODE CCI_REG8(0x3018) /* [3:0] */
+#define IMX908_WINMODE_ALLPIX 0x00 /* All-pixel readout mode */
+#define IMX908_WINMODE_CROP 0x04 /* Window cropping mode */
+
+/* ---- Flip */
+#define IMX908_REG_HREVERSE CCI_REG8(0x3020) /* 0:Normal, 1:Inverted */
+#define IMX908_REG_VREVERSE CCI_REG8(0x3021) /* 0:Normal, 1:Inverted */
+
+/* ---- Internal Analog to Digital conversion bit width */
+#define IMX908_REG_ADBIT CCI_REG8(0x3022)
+#define IMX908_ADBIT_10BIT 0x00 /* 10-bit */
+#define IMX908_ADBIT_12BIT 0x01 /* 11-bit + digital dither */
+
+#define IMX908_REG_MDBIT CCI_REG8(0x3023)
+#define IMX908_MDBIT_RAW10 0x00
+#define IMX908_MDBIT_RAW12 0x01
+
+/* ---- VMAX Frame length in lines */
+#define IMX908_REG_VMAX CCI_REG24_LE(0x3028) /* [19:0] */
+#define IMX908_VMAX_MAX 0xfffff
+#define IMX908_VMAX_DEFAULT 2250U
+/*
+ * Window cropping mode restrictions:
+ * VMAX >= PIX_VWIDTH + 70
+ * VMAX >= 1206
+ */
+#define IMX908_VMAX_CROP_MIN_MARGIN 70
+#define IMX908_VMAX_CROP_MIN_LIMIT 1206
+
+/* ---- HMAX Line length in clocks */
+#define IMX908_REG_HMAX CCI_REG16_LE(0x302c) /* [15:0] */
+#define IMX908_HMAX_MAX 0xffff
+#define IMX908_HMAX_DEFAULT 1100U
+
+/* ---- Cropping related */
+#define IMX908_REG_PIX_HST CCI_REG16_LE(0x303c) /* [12:0] Init 0h */
+#define IMX908_REG_PIX_HWIDTH CCI_REG16_LE(0x303e) /* [12:0] Init 0F10h */
+#define IMX908_REG_LANEMODE CCI_REG8(0x3040) /* 1:2-lane, 3:4-lane */
+#define IMX908_REG_PIX_VST CCI_REG16_LE(0x3044) /* [12:0] Init 0h */
+#define IMX908_REG_PIX_VWIDTH CCI_REG16_LE(0x3046) /* [12:0] Init 0884h */
+#define IMX908_CROP_ALIGN_VSTART 4U
+#define IMX908_CROP_ALIGN_VWIDTH 4U
+#define IMX908_CROP_ALIGN_HSTART 4U
+#define IMX908_CROP_ALIGN_HWIDTH 16U
+#define IMX908_CROP_MIN_HEIGHT 1136U
+#define IMX908_CROP_MIN_WIDTH 1040U
+
+/* ---- Shutter */
+#define IMX908_REG_SHR0 CCI_REG24_LE(0x3050) /* [19:0] */
+#define IMX908_MIN_SHR0 7U
+
+/*
+ * Analogue gain control
+ * Range is from 0 to 100 (0dB - 30dB) with 0.3dB step size
+ * Values from 101 to 240 are valid but correspond to additional digital gain
+ * (0.3dB - 42dB) so don't expose it to userspace
+ */
+#define IMX908_REG_GAIN CCI_REG16_LE(0x3070) /* [10:0] */
+#define IMX908_ANA_GAIN_MAX 100
+#define IMX908_ANA_GAIN_MIN 0
+#define IMX908_ANA_GAIN_STEP 1
+#define IMX908_ANA_GAIN_DEFAULT 0
+
+/* ---- XVS/XHS output mode */
+#define IMX908_XXS_DRV CCI_REG8(0x30a6) /* [1:0] XVS_DRV [3:2] XHS_DRV */
+
+#define IMX908_REG_BLKLEVEL CCI_REG16_LE(0x30dc) /* [15:0] black level */
+
+/* ---- Test Pattern Generator (TPG) registers */
+#define IMX908_REG_TPG_EN CCI_REG8(0x30e0)
+#define IMX908_TPG_EN_BIT BIT(0)
+#define IMX908_REG_TPG_PATSEL CCI_REG8(0x30e2) /* [4:0] pattern idx */
+#define IMX908_REG_TESTCLKEN CCI_REG8(0x4900)
+#define IMX908_TESTCLKEN_BIT BIT(3)
+
+#define IMX908_REG_TYPE_ID CCI_REG16_LE(0x4c0c)
+#define IMX908_CHIP_ID 0x038c /* 908 */
+
+/* ---- IMX908 Common Register List ---- */
+static const struct cci_reg_sequence imx908_common_regs[] = {
+ { IMX908_XXS_DRV, IMX908_CONTROLLER_MODE },
+ { CCI_REG8(0x039c), 0x03 },
+ { CCI_REG8(0x3416), 0x20 },
+ { CCI_REG8(0x3417), 0x00 },
+ { CCI_REG8(0x3456), 0xf4 },
+ { CCI_REG8(0x345d), 0x01 },
+ { CCI_REG8(0x3460), 0x00 },
+ { CCI_REG8(0x3461), 0x0b },
+ { CCI_REG8(0x3471), 0x00 },
+ { CCI_REG8(0x3472), 0x23 },
+ { CCI_REG8(0x347b), 0x02 },
+ { CCI_REG8(0x3481), 0x01 },
+ { CCI_REG8(0x380c), 0x00 },
+ { CCI_REG8(0x380f), 0x0c },
+ { CCI_REG8(0x381c), 0x11 },
+ { CCI_REG8(0x3820), 0x22 },
+ { CCI_REG8(0x3824), 0x33 },
+ { CCI_REG8(0x3828), 0x22 },
+ { CCI_REG8(0x382c), 0x33 },
+ { CCI_REG8(0x3830), 0x33 },
+ { CCI_REG8(0x3838), 0x05 },
+ { CCI_REG8(0x383c), 0x07 },
+ { CCI_REG8(0x3840), 0x06 },
+ { CCI_REG8(0x3848), 0x17 },
+ { CCI_REG8(0x384c), 0x0b },
+ { CCI_REG8(0x3850), 0x10 },
+ { CCI_REG8(0x3854), 0x12 },
+ { CCI_REG8(0x385c), 0x20 },
+ { CCI_REG8(0x38c4), 0x64 },
+ { CCI_REG8(0x38c5), 0x64 },
+ { CCI_REG8(0x38c6), 0x64 },
+ { CCI_REG8(0x3c4a), 0x15 },
+ { CCI_REG8(0x3c4c), 0x13 },
+ { CCI_REG8(0x3c4d), 0x13 },
+ { CCI_REG8(0x3c4e), 0x13 },
+ { CCI_REG8(0x3c50), 0x77 },
+ { CCI_REG8(0x3c51), 0x07 },
+ { CCI_REG8(0x3db4), 0x00 },
+ { CCI_REG8(0x4419), 0x06 },
+ { CCI_REG8(0x441c), 0x00 },
+ { CCI_REG8(0x4426), 0x00 },
+ { CCI_REG8(0x4538), 0x20 },
+ { CCI_REG8(0x4539), 0x19 },
+ { CCI_REG8(0x453a), 0x19 },
+ { CCI_REG8(0x453b), 0x19 },
+ { CCI_REG8(0x453c), 0x19 },
+ { CCI_REG8(0x453d), 0x19 },
+ { CCI_REG8(0x453e), 0x19 },
+ { CCI_REG8(0x453f), 0x19 },
+ { CCI_REG8(0x4540), 0x19 },
+ { CCI_REG8(0x4544), 0x11 },
+ { CCI_REG8(0x4545), 0x11 },
+ { CCI_REG8(0x4546), 0x11 },
+ { CCI_REG8(0x463c), 0x20 },
+ { CCI_REG8(0x465e), 0xcf },
+ { CCI_REG8(0x4684), 0x20 },
+ { CCI_REG8(0x46a6), 0xcf },
+ { CCI_REG8(0x46e2), 0xf3 },
+};
+
+static const u32 tuning_regs[] = {
+ CCI_REG8(0x3d78),
+ CCI_REG8(0x3d79),
+ CCI_REG8(0x3d80),
+ CCI_REG8(0x3d81),
+ CCI_REG8(0x3d88),
+ CCI_REG8(0x3d89),
+ CCI_REG8(0x3d90),
+ CCI_REG8(0x3d91),
+};
+
+#define IMX908_EXPOSURE_MIN 1
+#define IMX908_EXPOSURE_STEP 1
+
+/* ---- Speed of internal clock */
+#define IMX908_XHS_HZ 74250000ULL
+
+/* ---- Subdev Pads */
+#define IMX908_SOURCE_PAD 0
+
+#define IMX908_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
+
+/* IMX908 total area includes effective area plus
+ * 10 effective OB pixels in vertical direction,
+ * 10 pixels OB side ignored area in height
+ */
+static const struct v4l2_rect imx908_total_area = {
+ .top = 0,
+ .left = 0,
+ .width = 3856,
+ .height = 2200,
+};
+
+static const struct v4l2_rect imx908_active_area = {
+ .top = 0,
+ .left = 0,
+ .width = 3856,
+ .height = 2176,
+};
+
+/* Recommended 4K recording area centered within the active area */
+static const struct v4l2_rect imx908_recording_area = {
+ .top = 8,
+ .left = 8,
+ .width = 3840,
+ .height = 2160,
+};
+
+static const char *const imx908_supply_names[] = {
+ "avdd", /* Analog (3.3V) supply */
+ "dvdd", /* Digital Core (1.1V) supply */
+ "ovdd", /* IF (1.8V) supply */
+};
+
+static const u32 imx908_mbus_codes[] = {
+ MEDIA_BUS_FMT_SRGGB10_1X10, /* RAW10 */
+ MEDIA_BUS_FMT_SRGGB12_1X12, /* RAW12 */
+};
+
+enum {
+ IMX908_LINK_FREQ_297MHZ,
+ IMX908_LINK_FREQ_360MHZ,
+ IMX908_LINK_FREQ_445MHZ,
+ IMX908_LINK_FREQ_594MHZ,
+ IMX908_LINK_FREQ_720MHZ,
+ IMX908_LINK_FREQ_891MHZ,
+ IMX908_LINK_FREQ_1039MHZ,
+ IMX908_LINK_FREQ_1188MHZ,
+};
+
+static const u64 imx908_link_freqs[] = {
+ [IMX908_LINK_FREQ_297MHZ] = 297000000ULL,
+ [IMX908_LINK_FREQ_360MHZ] = 360000000ULL,
+ [IMX908_LINK_FREQ_445MHZ] = 445000000ULL,
+ [IMX908_LINK_FREQ_594MHZ] = 594000000ULL,
+ [IMX908_LINK_FREQ_720MHZ] = 720000000ULL,
+ [IMX908_LINK_FREQ_891MHZ] = 891000000ULL,
+ [IMX908_LINK_FREQ_1039MHZ] = 1039500000ULL,
+ [IMX908_LINK_FREQ_1188MHZ] = 1188000000ULL,
+};
+
+/* DDR lane_rate (Mbps/lane) = 2 x link_freq (Hz) / 1e6 */
+static const u8 imx908_datarate_sel[] = {
+ [IMX908_LINK_FREQ_297MHZ] = 0x07, /* 594 Mbps */
+ [IMX908_LINK_FREQ_360MHZ] = 0x06, /* 720 Mbps */
+ [IMX908_LINK_FREQ_445MHZ] = 0x05, /* 891 Mbps */
+ [IMX908_LINK_FREQ_594MHZ] = 0x04, /* 1188 Mbps */
+ [IMX908_LINK_FREQ_720MHZ] = 0x03, /* 1440 Mbps */
+ [IMX908_LINK_FREQ_891MHZ] = 0x02, /* 1782 Mbps */
+ [IMX908_LINK_FREQ_1039MHZ] = 0x01, /* 2079 Mbps */
+ [IMX908_LINK_FREQ_1188MHZ] = 0x00, /* 2376 Mbps */
+};
+
+struct imx908_inck_cfg {
+ u32 xclk_hz;
+ u8 inck_sel;
+};
+
+/* Allowed clock values in Hz */
+static const u32 imx908_inck_table[] = {
+ 74250000,
+ 37125000,
+ 72000000,
+ 27000000,
+ 24000000,
+};
+
+struct imx908_mode {
+ /* Sensor timing */
+ u16 hmax; /* clocks per line */
+ u32 vmax; /* lines per frame */
+
+ /* Derived values */
+ u64 pixel_rate;
+};
+
+struct imx908 {
+ /* Core V4L2 objects */
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct device *dev;
+
+ struct regmap *cci;
+
+ /* Hardware resources */
+ struct clk *xclk;
+ struct gpio_desc *reset_gpio;
+ struct regulator_bulk_data supplies[ARRAY_SIZE(imx908_supply_names)];
+
+ /* Clock configuration */
+ u32 xclk_freq;
+ u8 inck_sel;
+
+ /* Link/bus configuration */
+ u8 num_lanes;
+ unsigned long link_freq_bitmap;
+ unsigned int link_freq_idx;
+
+ struct imx908_mode mode; /* Current mode */
+
+ /* V4L2 controls */
+ struct {
+ struct v4l2_ctrl_handler handler;
+
+ struct v4l2_ctrl *pixel_rate;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *vblank;
+ struct v4l2_ctrl *hblank;
+ struct v4l2_ctrl *test_pattern;
+ } ctrls;
+};
+
+static inline struct imx908 *to_imx908(struct v4l2_subdev *_sd)
+{
+ return container_of(_sd, struct imx908, sd);
+}
+
+/* ----------------------- Basic Helper Functions ---------------------------- */
+
+/* Compute pixel-rate from link-freq/lanes/bpp */
+static inline u64 imx908_pixel_rate(u8 lanes, u8 bpp, u64 linkfreq_hz)
+{
+ u64 total = lanes * linkfreq_hz * 2; /* DDR: 2 * linkfreq */
+
+ return div_u64(total, bpp);
+}
+
+static int imx908_link_freq_to_datarate_sel(struct imx908 *imx, u64 link_freq_hz, u8 *sel)
+{
+ for (unsigned int i = 0; i < ARRAY_SIZE(imx908_link_freqs); i++) {
+ if (imx908_link_freqs[i] == link_freq_hz) {
+ *sel = imx908_datarate_sel[i];
+ return 0;
+ }
+ }
+ dev_err(imx->dev, "Unsupported link frequency %llu Hz\n", link_freq_hz);
+ return -EINVAL;
+}
+
+static bool imx908_mbus_code_supported(struct imx908 *imx, u32 code)
+{
+ for (unsigned int i = 0; i < ARRAY_SIZE(imx908_mbus_codes); i++) {
+ if (imx908_mbus_codes[i] == code)
+ return true;
+ }
+
+ return false;
+}
+
+static u16 imx908_calc_hmax(u32 width, u32 hblank, u64 pixel_rate)
+{
+ u64 hmax;
+
+ /* HMAX is expressed in cycles of the internal 74.25 MHz clock */
+ hmax = DIV_ROUND_UP_ULL((width + hblank) * IMX908_XHS_HZ, pixel_rate);
+
+ return min_t(u64, hmax, IMX908_HMAX_MAX);
+}
+
+static u32 imx908_calc_vmax(u32 height, u32 vblank)
+{
+ return min_t(u32, height + vblank, IMX908_VMAX_MAX);
+}
+
+static u16 imx908_calc_min_hmax(u32 width, u64 pixel_rate)
+{
+ /*
+ * Calculate the minimum HMAX that guarantees:
+ * line_length_pixels >= width
+ * where:
+ * line_length_pixels =
+ * ceil(HMAX * pixel_rate / IMX908_XHS_HZ)
+ */
+ return DIV_ROUND_UP((u64)width * IMX908_XHS_HZ, pixel_rate);
+}
+
+static u32 imx908_calc_min_vmax(const struct v4l2_rect *crop)
+{
+ /* In window crop mode constrain VMAX (from datasheet) */
+ if (!v4l2_rect_equal(crop, &imx908_active_area))
+ return max(crop->height + IMX908_VMAX_CROP_MIN_MARGIN,
+ IMX908_VMAX_CROP_MIN_LIMIT);
+
+ return IMX908_VMAX_DEFAULT;
+}
+
+static u32 imx908_calc_min_vblank(const struct v4l2_rect *crop)
+{
+ u32 min_vmax = imx908_calc_min_vmax(crop);
+
+ if (min_vmax <= crop->height)
+ return 0;
+
+ return min_vmax - crop->height;
+}
+
+static u32 imx908_calc_max_vblank(u32 height)
+{
+ return IMX908_VMAX_MAX - height;
+}
+
+static int imx908_set_exposure_lines(struct imx908 *imx, u32 exposure_lines)
+{
+ u32 max_lines = imx->mode.vmax - IMX908_MIN_SHR0;
+
+ exposure_lines = clamp_t(u32, exposure_lines, 1, max_lines);
+
+ /* Calculate the shutter register value based on current VMAX */
+ u32 shr0 = imx->mode.vmax - exposure_lines;
+
+ return cci_write(imx->cci, IMX908_REG_SHR0, shr0, NULL);
+}
+
+static u8 imx908_bits_per_pixel(u32 code)
+{
+ switch (code) {
+ case MEDIA_BUS_FMT_SRGGB12_1X12:
+ return 12;
+ case MEDIA_BUS_FMT_SRGGB10_1X10:
+ default:
+ return 10;
+ }
+}
+
+static int imx908_sync_pixel_rate_ctrl(struct imx908 *imx)
+{
+ s64 pixel_rate = imx->mode.pixel_rate;
+ int ret;
+
+ if (!imx->ctrls.pixel_rate)
+ return 0;
+
+ /* Update control range so pixel_rate is fixed (min = max = default) */
+ __v4l2_ctrl_modify_range(imx->ctrls.pixel_rate, pixel_rate, pixel_rate,
+ 1, pixel_rate);
+
+ /* Update the current control value to match the new mode pixel_rate */
+ ret = __v4l2_ctrl_s_ctrl_int64(imx->ctrls.pixel_rate, pixel_rate);
+ if (ret) {
+ dev_err(imx->dev, "failed to update pixel rate control: %d\n", ret);
+ return ret;
+ }
+
+ return ret;
+}
+
+static u32 imx908_hmax_to_hblank(u16 hmax, u64 pixel_rate, u32 width)
+{
+ /* Convert HMAX to line length in pixels */
+ u64 line_len_pixels = div_u64((u64)hmax * pixel_rate, IMX908_XHS_HZ);
+
+ if (line_len_pixels <= width)
+ return 0;
+
+ /* Subtract active image width to obtain horizontal blanking interval */
+ return line_len_pixels - width;
+}
+
+static void imx908_update_hblank_limits(struct imx908 *imx, u32 width, u64 pixel_rate)
+{
+ u16 min_hmax = imx908_calc_min_hmax(width, pixel_rate);
+ u32 min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate, width);
+ u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate, width);
+ u32 hblank = imx908_hmax_to_hblank(imx->mode.hmax, pixel_rate, width);
+
+ hblank = clamp_t(u32, hblank, min_hblank, max_hblank);
+
+ __v4l2_ctrl_modify_range(imx->ctrls.hblank, min_hblank, max_hblank, 1, hblank);
+
+ if (imx->ctrls.hblank->val != hblank)
+ __v4l2_ctrl_s_ctrl(imx->ctrls.hblank, hblank);
+}
+
+static void imx908_update_vblank_limits(struct imx908 *imx,
+ const struct v4l2_rect *crop)
+{
+ u32 min_vblank;
+ u32 max_vblank;
+ u32 vblank;
+
+ if (!imx->ctrls.vblank)
+ return;
+
+ min_vblank = imx908_calc_min_vblank(crop);
+ max_vblank = imx908_calc_max_vblank(crop->height);
+ vblank = clamp_t(u32, imx->ctrls.vblank->val,
+ min_vblank, max_vblank);
+
+ __v4l2_ctrl_modify_range(imx->ctrls.vblank,
+ min_vblank,
+ max_vblank,
+ 1,
+ vblank);
+}
+
+/* ------------------ Pattern Generator (TPG) ----------------------- */
+static const char * const imx908_tpg_menu[] = {
+ "Disabled",
+ "All 000h",
+ "All FFFh",
+ "All 555h",
+ "All AAAh",
+ "555h / AAAh Toggle",
+ "AAAh / 555h Toggle",
+ "000h / 555h Toggle",
+ "555h / 000h Toggle",
+ "000h / FFFh Toggle",
+ "FFFh / 000h Toggle",
+ "Horiz Color Bars",
+ "Vert Color Bars",
+};
+
+static int imx908_update_test_pattern(struct imx908 *imx, u32 index)
+{
+ int ret = 0;
+
+ if (index == 0) {
+ /* Picture (not TPG) output setting */
+ cci_update_bits(imx->cci, IMX908_REG_TPG_EN, IMX908_TPG_EN_BIT,
+ 0x00, &ret);
+
+ /* Disable TESTCLKEN (write-only register) */
+ cci_write(imx->cci, IMX908_REG_TESTCLKEN, 0x00, &ret);
+
+ return ret;
+ }
+
+ cci_write(imx->cci, IMX908_REG_TESTCLKEN, IMX908_TESTCLKEN_BIT, &ret);
+ cci_write(imx->cci, IMX908_REG_TPG_PATSEL, index - 1, &ret);
+ cci_update_bits(imx->cci, IMX908_REG_TPG_EN, IMX908_TPG_EN_BIT,
+ IMX908_TPG_EN_BIT, &ret);
+
+ return ret;
+}
+
+/* -------------------- Mode Building ----------------------- */
+static void imx908_mode_build(struct imx908 *imx,
+ const struct v4l2_mbus_framefmt *fmt,
+ const struct v4l2_rect *crop)
+{
+ struct imx908_mode *m = &imx->mode;
+ u8 bpp = imx908_bits_per_pixel(fmt->code);
+ u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
+
+ m->pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
+
+ /* --- H timing (HMAX) --- */
+ if (v4l2_rect_equal(crop, &imx908_active_area)) {
+ /* Update HBLANK HMAX in all pixel mode */
+ u16 min_hmax = imx908_calc_min_hmax(fmt->width, m->pixel_rate);
+ u32 min_hblank = imx908_hmax_to_hblank(min_hmax, m->pixel_rate,
+ fmt->width);
+ u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX,
+ m->pixel_rate, fmt->width);
+ u32 hblank = clamp_t(u32, imx->ctrls.hblank->val, min_hblank,
+ max_hblank);
+ m->hmax = imx908_calc_hmax(fmt->width, hblank, m->pixel_rate);
+ } else if (!m->hmax) {
+ u32 hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT,
+ m->pixel_rate,
+ imx908_active_area.width);
+
+ m->hmax = imx908_calc_hmax(fmt->width, hblank, m->pixel_rate);
+ }
+
+ /* --- V timing (VMAX) --- */
+ u32 min_vblank = imx908_calc_min_vblank(crop);
+ u32 max_vblank = imx908_calc_max_vblank(fmt->height);
+
+ u32 vblank = clamp_t(u32,
+ imx->ctrls.vblank->val,
+ min_vblank,
+ max_vblank);
+
+ m->vmax = imx908_calc_vmax(fmt->height, vblank);
+
+ u64 line_length_pixels = div_u64((u64)m->hmax * m->pixel_rate, IMX908_XHS_HZ);
+
+ if (line_length_pixels < fmt->width)
+ dev_warn(imx->dev, "Invalid timing (line=%llu < width=%u)\n",
+ line_length_pixels, fmt->width);
+}
+
+static void imx908_update_framing_limits(struct imx908 *imx, struct v4l2_subdev_state *state)
+{
+ const struct v4l2_mbus_framefmt *fmt;
+ const struct v4l2_rect *crop;
+
+ fmt = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD);
+ crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
+
+ u8 bpp = imx908_bits_per_pixel(fmt->code);
+ u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
+ u64 pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
+
+ /* Only update HBLANK in all pixel mode */
+ if (v4l2_rect_equal(crop, &imx908_active_area))
+ imx908_update_hblank_limits(imx, fmt->width, pixel_rate);
+
+ imx908_update_vblank_limits(imx, crop);
+
+ /* Build mode so cached timing uses current control values */
+ imx908_mode_build(imx, fmt, crop);
+
+ /* Exposure handling */
+ __v4l2_ctrl_modify_range(imx->ctrls.exposure,
+ 1,
+ imx->mode.vmax - IMX908_MIN_SHR0,
+ 1,
+ min_t(u32, imx->ctrls.exposure->val,
+ imx->mode.vmax - IMX908_MIN_SHR0));
+}
+
+/* ----------------------- HW Programming ---------------------- */
+
+static int imx908_mode_apply(struct imx908 *imx,
+ const struct v4l2_mbus_framefmt *fmt)
+{
+ int ret = 0;
+ u8 adbit, mdbit;
+
+ switch (fmt->code) {
+ case MEDIA_BUS_FMT_SRGGB12_1X12:
+ adbit = IMX908_ADBIT_12BIT;
+ mdbit = IMX908_MDBIT_RAW12;
+ break;
+
+ case MEDIA_BUS_FMT_SRGGB10_1X10:
+ default:
+ adbit = IMX908_ADBIT_10BIT;
+ mdbit = IMX908_MDBIT_RAW10;
+ break;
+ }
+
+ cci_write(imx->cci, IMX908_REG_XMASTER, IMX908_CONTROLLER_MODE, &ret);
+ cci_write(imx->cci, IMX908_REG_ADBIT, adbit, &ret);
+ cci_write(imx->cci, IMX908_REG_MDBIT, mdbit, &ret);
+ if (ret) {
+ dev_err(imx->dev, "Mode register write failed: %d\n", ret);
+ return ret;
+ }
+
+ /* Tuning registers that need to be changed for RAW10/10-bit */
+ u16 value = adbit == IMX908_ADBIT_10BIT &&
+ mdbit == IMX908_MDBIT_RAW10 ? 0x0c : 0x05;
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(tuning_regs); ++i)
+ cci_write(imx->cci, tuning_regs[i], value, &ret);
+
+ if (ret) {
+ dev_err(imx->dev, "Tuning register write failed: %d\n", ret);
+ return ret;
+ }
+
+ /* INCK */
+ cci_write(imx->cci, IMX908_REG_INCK_SEL, imx->inck_sel, &ret);
+ if (ret) {
+ dev_err(imx->dev, "INCK register write failed: %d\n", ret);
+ return ret;
+ }
+
+ /* MIPI lane rate selections */
+ u8 datarate_sel;
+ u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
+
+ ret = imx908_link_freq_to_datarate_sel(imx, link_freq, &datarate_sel);
+ if (ret) {
+ dev_err(imx->dev, "Unsupported link_freq=%llu for DATARATE_SEL\n",
+ link_freq);
+ return ret;
+ }
+ cci_write(imx->cci, IMX908_REG_DATARATE_SEL, datarate_sel, &ret);
+
+ /* Lane config */
+ cci_write(imx->cci, IMX908_REG_LANEMODE, imx->num_lanes - 1, &ret);
+ if (ret) {
+ dev_err(imx->dev, "Clock/lane configuration failed: %d\n", ret);
+ return ret;
+ }
+
+ /* Recommended black level offset is 50 in 10-bit, 200 for others */
+ u16 blklevel = (mdbit == IMX908_MDBIT_RAW10) ? 50 : 200;
+
+ cci_write(imx->cci, IMX908_REG_BLKLEVEL, blklevel, &ret);
+
+ return ret;
+}
+
+static int imx908_program_window(struct imx908 *imx, const struct v4l2_rect *crop)
+{
+ bool all_pixel_mode = v4l2_rect_equal(crop, &imx908_active_area);
+ int ret = 0;
+
+ cci_write(imx->cci, IMX908_REG_WINMODE,
+ all_pixel_mode ? IMX908_WINMODE_ALLPIX : IMX908_WINMODE_CROP,
+ &ret);
+
+ /* Update window size in window crop mode */
+ if (!all_pixel_mode) {
+ cci_write(imx->cci, IMX908_REG_PIX_HST, crop->left, &ret);
+ cci_write(imx->cci, IMX908_REG_PIX_HWIDTH, crop->width, &ret);
+ cci_write(imx->cci, IMX908_REG_PIX_VST, crop->top, &ret);
+ cci_write(imx->cci, IMX908_REG_PIX_VWIDTH, crop->height, &ret);
+ }
+
+ return ret;
+}
+
+/* ----------------------- Start/Stop streaming ---------------------- */
+
+static int imx908_start_streaming(struct imx908 *imx)
+{
+ struct v4l2_subdev_state *sd_state;
+ const struct v4l2_rect *crop;
+ const struct v4l2_mbus_framefmt *format;
+ int ret = 0;
+
+ /* Get active state */
+ sd_state = v4l2_subdev_get_locked_active_state(&imx->sd);
+ crop = v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD);
+ format = v4l2_subdev_state_get_format(sd_state, IMX908_SOURCE_PAD);
+
+ /* Build the mode based on active state before streaming */
+ imx908_mode_build(imx, format, crop);
+
+ /* Write common registers: static sensor tuning only */
+ cci_multi_reg_write(imx->cci, imx908_common_regs,
+ ARRAY_SIZE(imx908_common_regs), &ret);
+ if (ret)
+ return ret;
+
+ /* HW write all other registers (which depend on mode) */
+ ret = imx908_mode_apply(imx, format);
+ if (ret)
+ return ret;
+
+ /* Geometry */
+ ret = imx908_program_window(imx, crop);
+ if (ret)
+ return ret;
+
+ /* Apply the current control values */
+ ret = __v4l2_ctrl_handler_setup(imx->sd.ctrl_handler);
+ if (ret)
+ return ret;
+
+ /* Cancel standby state */
+ cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, &ret);
+ msleep(24); /* SRM >=24 ms regulator stabilization after standby cancel */
+
+ /* Start controller mode */
+ cci_write(imx->cci, IMX908_REG_XMSTA, IMX908_START_CONTROLLER, &ret);
+ return ret;
+}
+
+static int imx908_stop_streaming(struct imx908 *imx)
+{
+ int ret = 0;
+
+ cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, &ret);
+ cci_write(imx->cci, IMX908_REG_XMSTA, IMX908_STOP_CONTROLLER, &ret);
+ return ret;
+}
+
+/* --------------------------- V4L2 controls ------------------------------ */
+
+static int imx908_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct imx908 *imx = container_of(ctrl->handler, struct imx908, ctrls.handler);
+ struct v4l2_subdev_state *state;
+ struct i2c_client *client = v4l2_get_subdevdata(&imx->sd);
+ const struct v4l2_mbus_framefmt *format;
+ int ret = 0;
+
+ /* Retrieve the active, locked subdev state format dimensions */
+ state = v4l2_subdev_get_locked_active_state(&imx->sd);
+ format = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD);
+
+ /* Applying V4L2 controls only if powered up */
+ if (!pm_runtime_get_if_in_use(&client->dev))
+ return 0;
+
+ switch (ctrl->id) {
+ case V4L2_CID_EXPOSURE: {
+ u32 lines = clamp_t(u32, ctrl->val, 1,
+ imx->mode.vmax - IMX908_MIN_SHR0);
+
+ ret = imx908_set_exposure_lines(imx, lines);
+ break;
+ }
+
+ case V4L2_CID_ANALOGUE_GAIN:
+
+ u32 reg = clamp_t(u32, ctrl->val, IMX908_ANA_GAIN_MIN,
+ IMX908_ANA_GAIN_MAX);
+
+ cci_write(imx->cci, IMX908_REG_GAIN, reg, &ret);
+ break;
+
+ case V4L2_CID_VBLANK: {
+ const struct v4l2_rect *crop;
+
+ crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
+
+ u32 min_vblank = imx908_calc_min_vblank(crop);
+ u32 max_vblank = imx908_calc_max_vblank(format->height);
+ u32 vblank = clamp_t(u32, ctrl->val, min_vblank, max_vblank);
+
+ imx->mode.vmax = imx908_calc_vmax(format->height, vblank);
+
+ u32 exposure = min_t(u32, imx->ctrls.exposure->val,
+ imx->mode.vmax - IMX908_MIN_SHR0);
+
+ __v4l2_ctrl_modify_range(imx->ctrls.exposure,
+ IMX908_EXPOSURE_MIN,
+ imx->mode.vmax - IMX908_MIN_SHR0,
+ IMX908_EXPOSURE_STEP, exposure);
+
+ ret = cci_write(imx->cci, IMX908_REG_VMAX,
+ imx->mode.vmax, NULL);
+ if (!ret)
+ ret = imx908_set_exposure_lines(imx, exposure);
+
+ break;
+ }
+
+ case V4L2_CID_HBLANK: {
+ u16 hmax;
+ u32 width = format->width;
+ u64 pixel_rate = imx->mode.pixel_rate;
+ const struct v4l2_rect *crop;
+
+ crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
+
+ /* Ignore HBLANK update in window crop mode */
+ if (!v4l2_rect_equal(crop, &imx908_active_area))
+ break;
+
+ /* Skip timing calculation when not initialized */
+ if (!pixel_rate || !width)
+ break;
+
+ /* Recompute hmax based on HBLANK */
+ hmax = imx908_calc_hmax(width, ctrl->val, pixel_rate);
+ imx->mode.hmax = hmax;
+ cci_write(imx->cci, IMX908_REG_HMAX, hmax, &ret);
+ break;
+ }
+
+ case V4L2_CID_PIXEL_RATE:
+ case V4L2_CID_LINK_FREQ:
+ break;
+
+ case V4L2_CID_TEST_PATTERN:
+ ret = imx908_update_test_pattern(imx, ctrl->val);
+ break;
+
+ case V4L2_CID_HFLIP:
+ cci_write(imx->cci, IMX908_REG_HREVERSE, ctrl->val, &ret);
+ break;
+
+ case V4L2_CID_VFLIP:
+ cci_write(imx->cci, IMX908_REG_VREVERSE, ctrl->val, &ret);
+ break;
+
+ default:
+ dev_warn(&client->dev,
+ "ctrl(id:0x%x,val:0x%x) is not handled\n",
+ ctrl->id, ctrl->val);
+ break;
+ }
+
+ pm_runtime_put(&client->dev);
+ return ret;
+}
+
+static const struct v4l2_ctrl_ops imx908_ctrl_ops = {
+ .s_ctrl = imx908_set_ctrl,
+};
+
+/* ------------------------ Pad/video ops ------------------ */
+
+static int imx908_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ if (code->index >= ARRAY_SIZE(imx908_mbus_codes))
+ return -EINVAL;
+ code->code = imx908_mbus_codes[code->index];
+ return 0;
+}
+
+static int imx908_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ const struct v4l2_rect *crop = v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD);
+ struct imx908 *imx = to_imx908(sd);
+
+ if (fse->index > 0)
+ return -EINVAL;
+
+ if (!imx908_mbus_code_supported(imx, fse->code))
+ return -EINVAL;
+
+ fse->min_width = crop->width;
+ fse->max_width = crop->width;
+ fse->min_height = crop->height;
+ fse->max_height = crop->height;
+
+ return 0;
+}
+
+static int imx908_set_pad_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *fmt)
+{
+ struct imx908 *imx = to_imx908(sd);
+ struct v4l2_mbus_framefmt *format;
+ int ret = 0;
+
+ if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && v4l2_subdev_is_streaming(sd))
+ return -EBUSY;
+
+ format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
+
+ if (imx908_mbus_code_supported(imx, fmt->format.code))
+ format->code = fmt->format.code;
+ else
+ format->code = IMX908_DEFAULT_MBUS_CODE;
+
+ /* Fully define metadata */
+ format->field = V4L2_FIELD_NONE;
+ format->colorspace = V4L2_COLORSPACE_RAW;
+ format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+ format->xfer_func = V4L2_XFER_FUNC_NONE;
+
+ /* Update pixel rate range and hblank (ACTIVE ONLY) */
+ if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ imx908_update_framing_limits(imx, sd_state);
+ ret = imx908_sync_pixel_rate_ctrl(imx);
+ if (ret)
+ return ret;
+ }
+
+ /* Return final applied format to userspace */
+ fmt->format = *format;
+
+ return 0;
+}
+
+static int imx908_set_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_selection *sel)
+{
+ struct imx908 *imx = to_imx908(sd);
+ struct v4l2_rect *crop; /* current crop rectangle in subdev state */
+ struct v4l2_rect *r = &sel->r;
+ struct v4l2_mbus_framefmt *format;
+
+ if (sel->target != V4L2_SEL_TGT_CROP)
+ return -EINVAL;
+
+ if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && v4l2_subdev_is_streaming(sd))
+ return -EBUSY;
+
+ crop = v4l2_subdev_state_get_crop(sd_state, sel->pad);
+ format = v4l2_subdev_state_get_format(sd_state, sel->pad);
+
+ /*
+ * Round the crop rectangle size down to the hardware alignment
+ * constraints, then clamp it to the supported size range.
+ */
+ r->width = clamp_t(s32, ALIGN_DOWN(r->width, IMX908_CROP_ALIGN_HWIDTH),
+ IMX908_CROP_MIN_WIDTH, imx908_active_area.width);
+ r->height = clamp_t(s32, ALIGN_DOWN(r->height, IMX908_CROP_ALIGN_VWIDTH),
+ IMX908_CROP_MIN_HEIGHT, imx908_active_area.height);
+
+ /*
+ * Round and clamp the crop position similarly, with the maximum value
+ * chosen so that the crop rectangle remains fully inside the active
+ * pixel array.
+ */
+ r->left = clamp_t(s32, ALIGN_DOWN(r->left, IMX908_CROP_ALIGN_HSTART), 0,
+ imx908_active_area.width - r->width);
+ r->top = clamp_t(s32, ALIGN_DOWN(r->top, IMX908_CROP_ALIGN_VSTART), 0,
+ imx908_active_area.height - r->height);
+
+ /* Update the crop rectangle in subdev state */
+ *crop = *r;
+
+ /* Binning is not supported in IMX908 sensor */
+ format->width = crop->width;
+ format->height = crop->height;
+
+ if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ imx908_update_framing_limits(imx, sd_state);
+
+ return 0;
+}
+
+static int imx908_get_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_selection *sel)
+{
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP:
+ sel->r = *v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD);
+ return 0;
+
+ case V4L2_SEL_TGT_NATIVE_SIZE:
+ sel->r = imx908_total_area;
+ return 0;
+
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ sel->r = imx908_recording_area;
+ return 0;
+
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ sel->r = imx908_active_area;
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int imx908_enable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ u32 pad,
+ u64 streams_mask)
+{
+ struct imx908 *imx = to_imx908(sd);
+ int ret;
+
+ ret = pm_runtime_resume_and_get(imx->dev);
+ if (ret)
+ return ret;
+
+ ret = imx908_start_streaming(imx);
+ if (ret) {
+ pm_runtime_mark_last_busy(imx->dev);
+ pm_runtime_put_autosuspend(imx->dev);
+ }
+
+ return ret;
+}
+
+static int imx908_disable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ u32 pad,
+ u64 streams_mask)
+{
+ struct imx908 *imx = to_imx908(sd);
+ int ret;
+
+ ret = imx908_stop_streaming(imx);
+
+ pm_runtime_mark_last_busy(imx->dev);
+ pm_runtime_put_autosuspend(imx->dev);
+
+ return ret;
+}
+
+static int imx908_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state)
+{
+ int ret = 0;
+ struct v4l2_subdev_selection sel = {
+ .which = V4L2_SUBDEV_FORMAT_TRY,
+ .pad = IMX908_SOURCE_PAD,
+ .target = V4L2_SEL_TGT_CROP,
+ .r = imx908_active_area,
+ };
+ struct v4l2_subdev_format fmt = {
+ .which = V4L2_SUBDEV_FORMAT_TRY,
+ .pad = IMX908_SOURCE_PAD,
+ .format = {
+ .code = IMX908_DEFAULT_MBUS_CODE,
+ .width = imx908_active_area.width,
+ .height = imx908_active_area.height,
+ },
+ };
+
+ ret = imx908_set_selection(sd, sd_state, &sel);
+ ret = imx908_set_pad_format(sd, sd_state, &fmt);
+
+ return ret;
+}
+
+static const struct v4l2_subdev_core_ops imx908_core_ops = {
+ .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+ .unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_video_ops imx908_video_ops = {
+ .s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops imx908_pad_ops = {
+ .enum_mbus_code = imx908_enum_mbus_code,
+ .enum_frame_size = imx908_enum_frame_size,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = imx908_set_pad_format,
+ .get_selection = imx908_get_selection,
+ .set_selection = imx908_set_selection,
+ .enable_streams = imx908_enable_streams,
+ .disable_streams = imx908_disable_streams,
+};
+
+static const struct v4l2_subdev_internal_ops imx908_internal_ops = {
+ .init_state = imx908_init_state,
+};
+
+static const struct v4l2_subdev_ops imx908_subdev_ops = {
+ .core = &imx908_core_ops,
+ .video = &imx908_video_ops,
+ .pad = &imx908_pad_ops,
+};
+
+/* ----------------------- Power management ---------------------- */
+
+static int imx908_power_on(struct imx908 *imx)
+{
+ int ret;
+
+ /* Power Rails */
+ ret = regulator_bulk_enable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
+ if (ret) {
+ dev_err(imx->dev, "%s: failed to enable regulators\n", __func__);
+ return ret;
+ }
+ msleep(200); /* IMX908 power ok after 200ms */
+
+ if (imx->reset_gpio) {
+ gpiod_set_value_cansleep(imx->reset_gpio, 1); /* XCLR low */
+ udelay(1); /* >= 500ns T_low */
+ gpiod_set_value_cansleep(imx->reset_gpio, 0); /* Sensor starts */
+ }
+
+ /* Start INCK */
+ ret = clk_prepare_enable(imx->xclk);
+ if (ret) {
+ dev_err(imx->dev, "failed to enable xclk: %d\n", ret);
+ goto err_reset;
+ }
+
+ /* T_1 >=20us delay before initial SDA/SCL */
+ usleep_range(20, 25);
+
+ return 0;
+
+err_reset:
+ gpiod_set_value_cansleep(imx->reset_gpio, 1); /* assert reset */
+ regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
+ return ret;
+}
+
+static void imx908_power_off(struct imx908 *imx)
+{
+ gpiod_set_value_cansleep(imx->reset_gpio, 1);
+ clk_disable_unprepare(imx->xclk);
+ regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
+}
+
+static int imx908_runtime_resume(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct imx908 *imx = to_imx908(sd);
+
+ return imx908_power_on(imx);
+}
+
+static int imx908_runtime_suspend(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct imx908 *imx = to_imx908(sd);
+
+ imx908_power_off(imx);
+ return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(imx908_pm_ops,
+ imx908_runtime_suspend,
+ imx908_runtime_resume, NULL);
+
+/* ------------------------------- Probe/remove --------------------------- */
+
+static int imx908_get_inck_sel(struct imx908 *imx)
+{
+ for (unsigned int i = 0; i < ARRAY_SIZE(imx908_inck_table); i++) {
+ if (imx908_inck_table[i] == imx->xclk_freq) {
+ imx->inck_sel = i;
+ return 0;
+ }
+ }
+
+ return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n", imx->xclk_freq);
+}
+
+static int imx908_get_regulators(struct imx908 *imx)
+{
+ for (unsigned int i = 0; i < ARRAY_SIZE(imx908_supply_names); i++)
+ imx->supplies[i].supply = imx908_supply_names[i];
+
+ return devm_regulator_bulk_get(imx->dev,
+ ARRAY_SIZE(imx908_supply_names),
+ imx->supplies);
+}
+
+static int imx908_parse_fwnode(struct imx908 *imx)
+{
+ struct fwnode_handle *ep;
+ struct v4l2_fwnode_endpoint bus_cfg = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY
+ };
+ int ret = 0;
+
+ /* Get firmware node from device tree */
+ ep = fwnode_graph_get_next_endpoint(dev_fwnode(imx->dev), NULL);
+ if (!ep) {
+ dev_err(imx->dev, "Failed to find endpoint in device tree\n");
+ return -ENXIO;
+ }
+
+ /* Parse properties: data-lanes, link-frequencies */
+ ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+ fwnode_handle_put(ep);
+ if (ret)
+ return ret;
+
+ imx->num_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
+
+ if (imx->num_lanes != 2 && imx->num_lanes != 4) {
+ dev_err(imx->dev, "only 2 or 4 CSI-2 data lanes are supported (got %u)\n",
+ imx->num_lanes);
+ ret = -EINVAL;
+ goto out_free;
+ }
+
+ ret = v4l2_link_freq_to_bitmap(imx->dev,
+ bus_cfg.link_frequencies,
+ bus_cfg.nr_of_link_frequencies,
+ imx908_link_freqs,
+ ARRAY_SIZE(imx908_link_freqs),
+ &imx->link_freq_bitmap);
+ if (ret) {
+ dev_err(imx->dev, "Failed to parse link frequencies from DT\n");
+ goto out_free;
+ }
+
+ if (bitmap_empty(&imx->link_freq_bitmap, ARRAY_SIZE(imx908_link_freqs))) {
+ dev_err(imx->dev,
+ "No common link frequencies between driver and DT\n");
+ ret = -EINVAL;
+ goto out_free;
+ }
+
+ imx->link_freq_idx = __ffs(imx->link_freq_bitmap);
+
+out_free:
+ v4l2_fwnode_endpoint_free(&bus_cfg);
+ return ret;
+}
+
+static int imx908_init_controls(struct imx908 *imx)
+{
+ struct v4l2_ctrl_handler *hdl = &imx->ctrls.handler;
+ struct v4l2_ctrl *link_freq_ctl;
+ struct v4l2_fwnode_device_properties props;
+ int ret;
+ s32 hblank, min_hblank;
+ u32 max_exp, max_hblank, min_vblank, max_vblank;
+ u16 min_hmax;
+
+ ret = v4l2_ctrl_handler_init(hdl, 11);
+ if (ret)
+ return ret;
+
+ /* Initialize the pixel_rate control */
+ u8 bpp = imx908_bits_per_pixel(IMX908_DEFAULT_MBUS_CODE);
+ u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
+ u64 pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
+
+ imx->ctrls.pixel_rate = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_PIXEL_RATE,
+ pixel_rate, pixel_rate, 1, pixel_rate);
+ if (imx->ctrls.pixel_rate)
+ imx->ctrls.pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ /* Set link Frequency */
+ link_freq_ctl = v4l2_ctrl_new_int_menu(hdl, &imx908_ctrl_ops,
+ V4L2_CID_LINK_FREQ,
+ ARRAY_SIZE(imx908_link_freqs) - 1,
+ imx->link_freq_idx,
+ imx908_link_freqs);
+
+ if (link_freq_ctl)
+ link_freq_ctl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ /* Initialize HMAX and VMAX */
+ imx->mode.hmax = IMX908_HMAX_DEFAULT;
+ imx->mode.vmax = IMX908_VMAX_DEFAULT;
+
+ /* Set vblank */
+ min_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height;
+ max_vblank = imx908_calc_max_vblank(imx908_active_area.height);
+
+ imx->ctrls.vblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
+ V4L2_CID_VBLANK,
+ min_vblank,
+ max_vblank,
+ 1,
+ min_vblank);
+
+ /* Set hblank */
+ min_hmax = imx908_calc_min_hmax(imx908_active_area.width, pixel_rate);
+ min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate,
+ imx908_active_area.width);
+ max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate,
+ imx908_active_area.width);
+ hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, pixel_rate,
+ imx908_active_area.width);
+
+ imx->ctrls.hblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
+ V4L2_CID_HBLANK,
+ min_hblank,
+ max_hblank,
+ 1,
+ hblank);
+ max_exp = imx->mode.vmax - IMX908_MIN_SHR0;
+
+ imx->ctrls.exposure = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
+ V4L2_CID_EXPOSURE,
+ IMX908_EXPOSURE_MIN,
+ max_exp,
+ IMX908_EXPOSURE_STEP,
+ max_exp / 2);
+
+ v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+ IMX908_ANA_GAIN_MIN, IMX908_ANA_GAIN_MAX,
+ IMX908_ANA_GAIN_STEP, IMX908_ANA_GAIN_DEFAULT);
+
+ /* Set test pattern. Menu (13 entries: Disabled + 12 patterns) */
+ imx->ctrls.test_pattern = v4l2_ctrl_new_std_menu_items(hdl,
+ &imx908_ctrl_ops,
+ V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(imx908_tpg_menu) - 1,
+ 0,
+ 0,
+ imx908_tpg_menu);
+ if (imx->ctrls.test_pattern)
+ imx->ctrls.test_pattern->flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
+
+ v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
+
+ /* Pick up rotation and orientation if defined in overlay */
+ ret = v4l2_fwnode_device_parse(imx->dev, &props);
+ if (ret)
+ return ret;
+
+ ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx908_ctrl_ops, &props);
+ if (ret)
+ return ret;
+
+ if (hdl->error) {
+ ret = hdl->error;
+ v4l2_ctrl_handler_free(hdl);
+ return ret;
+ }
+
+ imx->sd.ctrl_handler = hdl;
+
+ return 0;
+}
+
+/* Confirm the ID of the HW device */
+static int imx908_identify_model(struct imx908 *imx)
+{
+ int ret;
+ int err;
+ u64 val;
+ u16 chip_id;
+
+ /*
+ * The TYPE_ID registers are not accessible after power-up while
+ * the device remains in standby. Exit standby and wait for the
+ * required stabilization period before reading the chip ID.
+ */
+ ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, NULL);
+ if (ret)
+ return ret;
+
+ msleep(24); /* Regulator stabilization after standby cancel. */
+
+ ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, &err);
+ if (ret || err)
+ return ret ? ret : err;
+
+ chip_id = val;
+ dev_info(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id);
+
+ if (chip_id != IMX908_CHIP_ID) {
+ dev_err(imx->dev, "Unexpected chip ID 0x%04x (expected 0x%04x)\n",
+ chip_id, IMX908_CHIP_ID);
+ return -ENXIO;
+ }
+
+ /* Set to standby mode */
+ ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL);
+ if (ret)
+ dev_err(imx->dev, "failed to enter standby state: %d\n", ret);
+
+ return 0;
+}
+
+static int imx908_probe(struct i2c_client *client)
+{
+ struct imx908 *imx;
+ int ret;
+
+ /* Allocate Memory */
+ imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
+ if (!imx)
+ return dev_err_probe(&client->dev, -ENOMEM,
+ "failed to allocate IMX908 device structure\n");
+ imx->dev = &client->dev;
+
+ /* Initialize V4L2 subdevice */
+ v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
+ imx->sd.internal_ops = &imx908_internal_ops;
+
+ /* Register access initialization. Set 2-byte (16-bit) addresses */
+ imx->cci = devm_cci_regmap_init_i2c(client, 16);
+ if (IS_ERR(imx->cci))
+ return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
+ "CCI regmap init failed\n");
+
+ /* Get mandatory input clock from DT (INCK) */
+ imx->xclk = devm_clk_get(imx->dev, "xclk");
+ if (IS_ERR(imx->xclk))
+ return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), "xclk\n");
+
+ /* Get clock frequency and check against acceptable HW values */
+ imx->xclk_freq = clk_get_rate(imx->xclk);
+ ret = imx908_get_inck_sel(imx);
+ if (ret)
+ return ret;
+
+ /* GPIO reset acquisition */
+ imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
+ GPIOD_OUT_HIGH);
+
+ if (IS_ERR(imx->reset_gpio))
+ return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
+ "reset gpio\n");
+
+ /* Link to power supplies */
+ ret = imx908_get_regulators(imx);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "regulator get failed\n");
+
+ /* Parse Device Tree endpoint */
+ ret = imx908_parse_fwnode(imx);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "device tree parse failed\n");
+
+ /* Power on IMX908 image sensor */
+ ret = imx908_power_on(imx);
+ if (ret)
+ return dev_err_probe(&client->dev, ret, "power-on failed\n");
+
+ /* Read IMX908 device ID */
+ ret = imx908_identify_model(imx);
+ if (ret) {
+ dev_err(imx->dev, "failed to identify model: %d\n", ret);
+ goto err_power_off;
+ }
+
+ pm_runtime_set_active(imx->dev);
+ pm_runtime_enable(imx->dev);
+
+ ret = imx908_init_controls(imx);
+ if (ret)
+ goto err_pm_disable;
+
+ /* Initialize V4L2 subdevice, media entity, and pad */
+ imx->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+ imx->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+ imx->pad.flags = MEDIA_PAD_FL_SOURCE;
+ ret = media_entity_pads_init(&imx->sd.entity, 1, &imx->pad);
+ if (ret)
+ goto err_hdl;
+
+ /* Use ctrl handler mutex for centrally managed subdev state */
+ imx->sd.state_lock = imx->ctrls.handler.lock;
+
+ ret = v4l2_subdev_init_finalize(&imx->sd);
+ if (ret)
+ goto err_entity;
+
+ pm_runtime_set_autosuspend_delay(imx->dev, 1000);
+ pm_runtime_use_autosuspend(imx->dev);
+ pm_runtime_mark_last_busy(imx->dev);
+
+ ret = v4l2_async_register_subdev_sensor(&imx->sd);
+ if (ret)
+ goto err_pm_disable;
+
+ return 0;
+
+err_pm_disable:
+ pm_runtime_disable(imx->dev);
+ pm_runtime_set_suspended(imx->dev);
+ v4l2_subdev_cleanup(&imx->sd);
+err_entity:
+ media_entity_cleanup(&imx->sd.entity);
+err_hdl:
+ v4l2_ctrl_handler_free(imx->sd.ctrl_handler);
+err_power_off:
+ imx908_power_off(imx);
+
+ return ret;
+}
+
+static void imx908_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx908 *imx = to_imx908(sd);
+
+ v4l2_async_unregister_subdev(sd);
+ v4l2_subdev_cleanup(sd);
+ media_entity_cleanup(&sd->entity);
+ v4l2_ctrl_handler_free(sd->ctrl_handler);
+
+ pm_runtime_disable(imx->dev);
+ if (!pm_runtime_status_suspended(imx->dev))
+ imx908_power_off(imx);
+ pm_runtime_set_suspended(imx->dev);
+}
+
+static const struct of_device_id imx908_of_ids[] = {
+ { .compatible = "sony,imx908" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx908_of_ids);
+
+static struct i2c_driver imx908_i2c_driver = {
+ .driver = {
+ .name = "imx908",
+ .pm = pm_ptr(&imx908_pm_ops),
+ .of_match_table = imx908_of_ids,
+ },
+ .probe = imx908_probe,
+ .remove = imx908_remove,
+};
+module_i2c_driver(imx908_i2c_driver);
+
+MODULE_AUTHOR("Lachlan Michael <Lachlan.Michael@sony.com>");
+MODULE_DESCRIPTION("Sony IMX908 Sensor Driver");
+MODULE_LICENSE("GPL");
--
2.47.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
2026-07-30 2:15 ` [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings Lachlan Michael
@ 2026-07-30 3:24 ` Rob Herring (Arm)
2026-07-30 9:06 ` Krzysztof Kozlowski
1 sibling, 0 replies; 16+ messages in thread
From: Rob Herring (Arm) @ 2026-07-30 3:24 UTC (permalink / raw)
To: Lachlan Michael
Cc: conor+dt, Ryuichi.Tadano, Kazumi.A.Sato, Tim.Bird, Kengo.Hayasaka,
laurent.pinchart, kieran.bingham, mchehab, hverkuil+cisco,
linux-kernel, linux-media, jai.luthra, devicetree, krzk+dt,
sakari.ailus
On Thu, 30 Jul 2026 11:15:24 +0900, Lachlan Michael wrote:
> The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor
> with a MIPI CSI-2 output interface.
>
> Add device tree bindings for the sensor.
>
> Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
> ---
> .../bindings/media/i2c/sony,imx908.yaml | 107 ++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 114 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/media/i2c/sony,imx908.example.dtb: camera@1a (sony,imx908): port:endpoint: 'remote-endpoint' does not match any of the regexes: '^pinctrl-[0-9]+$'
from schema $id: http://devicetree.org/schemas/media/i2c/sony,imx908.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.kernel.org/project/devicetree/patch/20260730021525.166811-2-lachlan.michael@sony.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
2026-07-30 2:15 ` [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings Lachlan Michael
2026-07-30 3:24 ` Rob Herring (Arm)
@ 2026-07-30 9:06 ` Krzysztof Kozlowski
2026-08-02 23:03 ` Laurent Pinchart
1 sibling, 1 reply; 16+ messages in thread
From: Krzysztof Kozlowski @ 2026-07-30 9:06 UTC (permalink / raw)
To: Lachlan Michael
Cc: mchehab, sakari.ailus, hverkuil+cisco, laurent.pinchart,
linux-media, devicetree, robh, krzk+dt, conor+dt, kieran.bingham,
jai.luthra, Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird,
Kazumi.A.Sato, linux-kernel
On Thu, Jul 30, 2026 at 11:15:24AM +0900, Lachlan Michael wrote:
> The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor
> with a MIPI CSI-2 output interface.
>
> Add device tree bindings for the sensor.
>
> Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
You need to test code before you send it, not with our infra.
To spare you one sending cycle:
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23
> ---
> .../bindings/media/i2c/sony,imx908.yaml | 107 ++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 114 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
...
> +++ b/MAINTAINERS
> @@ -25264,6 +25264,13 @@ T: git git://linuxtv.org/media.git
> F: Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
> F: drivers/media/i2c/imx415.c
>
> +SONY IMX908 SENSOR DRIVER
> +M: Lachlan Michael <lachlan.michael@sony.com>
> +L: linux-media@vger.kernel.org
Drop
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
> +F: drivers/media/i2c/imx908.c
No such file.
> +
> SONY MEMORYSTICK SUBSYSTEM
> M: Maxim Levitsky <maximlevitsky@gmail.com>
> M: Alex Dubov <oakad@yahoo.com>
> --
> 2.47.3
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-07-30 2:15 ` [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver Lachlan Michael
@ 2026-07-30 9:09 ` Krzysztof Kozlowski
2026-08-05 9:23 ` Lachlan.Michael
2026-08-03 0:53 ` Laurent Pinchart
1 sibling, 1 reply; 16+ messages in thread
From: Krzysztof Kozlowski @ 2026-07-30 9:09 UTC (permalink / raw)
To: Lachlan Michael
Cc: mchehab, sakari.ailus, hverkuil+cisco, laurent.pinchart,
linux-media, devicetree, robh, krzk+dt, conor+dt, kieran.bingham,
jai.luthra, Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird,
Kazumi.A.Sato, linux-kernel
On Thu, Jul 30, 2026 at 11:15:25AM +0900, Lachlan Michael wrote:
> + msleep(24); /* Regulator stabilization after standby cancel. */
> +
> + ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, &err);
> + if (ret || err)
> + return ret ? ret : err;
> +
> + chip_id = val;
> + dev_info(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id);
Drivers should be silent on success. Drop or dev_dbg.
> +
> + if (chip_id != IMX908_CHIP_ID) {
> + dev_err(imx->dev, "Unexpected chip ID 0x%04x (expected 0x%04x)\n",
> + chip_id, IMX908_CHIP_ID);
> + return -ENXIO;
> + }
> +
> + /* Set to standby mode */
> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL);
> + if (ret)
> + dev_err(imx->dev, "failed to enter standby state: %d\n", ret);
> +
> + return 0;
> +}
> +
> +static int imx908_probe(struct i2c_client *client)
> +{
> + struct imx908 *imx;
> + int ret;
> +
> + /* Allocate Memory */
Really?
> + imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
> + if (!imx)
> + return dev_err_probe(&client->dev, -ENOMEM,
> + "failed to allocate IMX908 device structure\n");
> + imx->dev = &client->dev;
> +
> + /* Initialize V4L2 subdevice */
Obvious.
> + v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
> + imx->sd.internal_ops = &imx908_internal_ops;
> +
> + /* Register access initialization. Set 2-byte (16-bit) addresses */
> + imx->cci = devm_cci_regmap_init_i2c(client, 16);
> + if (IS_ERR(imx->cci))
> + return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
> + "CCI regmap init failed\n");
> +
> + /* Get mandatory input clock from DT (INCK) */
Obvious.
> + imx->xclk = devm_clk_get(imx->dev, "xclk");
> + if (IS_ERR(imx->xclk))
> + return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), "xclk\n");
> +
> + /* Get clock frequency and check against acceptable HW values */
> + imx->xclk_freq = clk_get_rate(imx->xclk);
> + ret = imx908_get_inck_sel(imx);
> + if (ret)
> + return ret;
> +
> + /* GPIO reset acquisition */
Please drop obvious comments. Can devm_gpiod_get_optional() be anything
else than GPIO reset acquisition? No.
Redundant comments bloat the code and make it more difficult to actually
spot important things.
> + imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
> + GPIOD_OUT_HIGH);
> +
> + if (IS_ERR(imx->reset_gpio))
> + return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
> + "reset gpio\n");
> +
> + /* Link to power supplies */
> + ret = imx908_get_regulators(imx);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "regulator get failed\n");
> +
> + /* Parse Device Tree endpoint */
Obvious
> + ret = imx908_parse_fwnode(imx);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "device tree parse failed\n");
> +
> + /* Power on IMX908 image sensor */
What if imx908_power_on() does power off of the sensor?
> + ret = imx908_power_on(imx);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "power-on failed\n");
> +
> + /* Read IMX908 device ID */
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
2026-07-30 9:06 ` Krzysztof Kozlowski
@ 2026-08-02 23:03 ` Laurent Pinchart
2026-08-03 6:16 ` Krzysztof Kozlowski
0 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2026-08-02 23:03 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Lachlan Michael, mchehab, sakari.ailus, hverkuil+cisco,
linux-media, devicetree, robh, krzk+dt, conor+dt, kieran.bingham,
jai.luthra, Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird,
Kazumi.A.Sato, linux-kernel
On Thu, Jul 30, 2026 at 11:06:09AM +0200, Krzysztof Kozlowski wrote:
> On Thu, Jul 30, 2026 at 11:15:24AM +0900, Lachlan Michael wrote:
> > The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor
> > with a MIPI CSI-2 output interface.
> >
> > Add device tree bindings for the sensor.
> >
> > Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
>
> You need to test code before you send it, not with our infra.
I have provided guidance to Lachlan on how to test DT bindings (and have
submitted a fix to dt-schema as part of that work) before he submitted
this patch series. Unfortunately, we missed an issue in the
configuration of his development environment which resulted in the DT
bindings check not being run. Mistakes happen, it will work better next
time :-)
> To spare you one sending cycle:
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23
>
> > ---
> > .../bindings/media/i2c/sony,imx908.yaml | 107 ++++++++++++++++++
> > MAINTAINERS | 7 ++
> > 2 files changed, 114 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
>
> ...
>
> > +++ b/MAINTAINERS
> > @@ -25264,6 +25264,13 @@ T: git git://linuxtv.org/media.git
> > F: Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
> > F: drivers/media/i2c/imx415.c
> >
> > +SONY IMX908 SENSOR DRIVER
> > +M: Lachlan Michael <lachlan.michael@sony.com>
> > +L: linux-media@vger.kernel.org
>
> Drop
Why so ?
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
> > +F: drivers/media/i2c/imx908.c
>
> No such file.
Lachlan, that line belongs to patch 2/2.
> > +
> > SONY MEMORYSTICK SUBSYSTEM
> > M: Maxim Levitsky <maximlevitsky@gmail.com>
> > M: Alex Dubov <oakad@yahoo.com>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-07-30 2:15 ` [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver Lachlan Michael
2026-07-30 9:09 ` Krzysztof Kozlowski
@ 2026-08-03 0:53 ` Laurent Pinchart
2026-08-03 17:37 ` Dave Stevenson
2026-08-05 10:08 ` Lachlan.Michael
1 sibling, 2 replies; 16+ messages in thread
From: Laurent Pinchart @ 2026-08-03 0:53 UTC (permalink / raw)
To: Lachlan Michael
Cc: mchehab, sakari.ailus, hverkuil+cisco, linux-media, devicetree,
robh, krzk+dt, conor+dt, kieran.bingham, jai.luthra,
Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird, Kazumi.A.Sato,
linux-kernel
Hi Lachlan,
Thank you for the patch.
On Thu, Jul 30, 2026 at 11:15:25AM +0900, Lachlan Michael wrote:
> Add a V4L2 sub-device driver for the Sony IMX908 CMOS image sensor.
>
> The driver supports:
> - RAW10 and RAW12 output formats
> - Exposure and analogue gain controls
> - Horizontal and vertical image flipping
> - HBLANK and VBLANK controls
> - Full-resolution and window-cropping modes
> - Test pattern generation
>
> HDR modes and RAW16 output are not currently supported.
>
> The driver has been tested with libcamera on Raspberry Pi 5 based
> hardware.
>
> Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
> ---
> drivers/media/i2c/Kconfig | 11 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/imx908.c | 1593 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 1605 insertions(+)
> create mode 100644 drivers/media/i2c/imx908.c
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 5d173e0ecf42..8e521f21414b 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -287,6 +287,17 @@ config VIDEO_IMX415
> To compile this driver as a module, choose M here: the
> module will be called imx415.
>
> +config VIDEO_IMX908
> + tristate "Sony IMX908 sensor support"
> + depends on GPIOLIB
> + select V4L2_CCI_I2C
> + help
> + This is a Video4Linux2 sensor driver for the Sony
> + IMX908 CMOS image sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will called imx908.
> +
> config VIDEO_MAX9271_LIB
> tristate
>
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index e45359efe0e4..3ccf6d29bbfd 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o
> obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> obj-$(CONFIG_VIDEO_IMX412) += imx412.o
> obj-$(CONFIG_VIDEO_IMX415) += imx415.o
> +obj-$(CONFIG_VIDEO_IMX908) += imx908.o
> obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
> obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o
> obj-$(CONFIG_VIDEO_KS0127) += ks0127.o
> diff --git a/drivers/media/i2c/imx908.c b/drivers/media/i2c/imx908.c
> new file mode 100644
> index 000000000000..6ec1322f9c16
> --- /dev/null
> +++ b/drivers/media/i2c/imx908.c
> @@ -0,0 +1,1593 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * V4L2 driver for Sony IMX908
> + *
> + * Diagonal 6.42 mm (Type 1/2.8) CMOS image sensor with 8.39 M pixels.
> + *
> + * Copyright 2026 Sony Semiconductor Solutions Corporation
> + *
> + */
> +
> +#include <linux/align.h>
> +#include <linux/array_size.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <media/media-entity.h>
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mediabus.h>
> +#include <media/v4l2-rect.h>
> +#include <media/v4l2-subdev.h>
> +
> +/* ---- IMX908 Registers --------- */
> +#define IMX908_REG_STANDBY CCI_REG8(0x3000)
> +#define IMX908_STANDBY_EN BIT(0) /* Standby mode */
> +#define IMX908_STANDBY_CANCEL 0x00 /* Operating mode */
> +#define IMX908_REG_XMSTA CCI_REG8(0x3002) /* [0] Init 1h */
> +#define IMX908_START_CONTROLLER 0x00 /* Controller mode operation start */
> +#define IMX908_STOP_CONTROLLER 0x01 /* Controller mode operation stop */
> +#define IMX908_REG_XMASTER CCI_REG8(0x3003) /* [0] Init 0h */
> +#define IMX908_CONTROLLER_MODE 0x00
> +#define IMX908_REG_INCK_SEL CCI_REG8(0x3014) /* 0:74.25 1:37.125 2:72 3:27 4:24 */
> +#define IMX908_REG_DATARATE_SEL CCI_REG8(0x3015)
> +
> +/* ---- Crop */
> +#define IMX908_REG_WINMODE CCI_REG8(0x3018) /* [3:0] */
> +#define IMX908_WINMODE_ALLPIX 0x00 /* All-pixel readout mode */
> +#define IMX908_WINMODE_CROP 0x04 /* Window cropping mode */
> +
> +/* ---- Flip */
> +#define IMX908_REG_HREVERSE CCI_REG8(0x3020) /* 0:Normal, 1:Inverted */
> +#define IMX908_REG_VREVERSE CCI_REG8(0x3021) /* 0:Normal, 1:Inverted */
> +
> +/* ---- Internal Analog to Digital conversion bit width */
> +#define IMX908_REG_ADBIT CCI_REG8(0x3022)
> +#define IMX908_ADBIT_10BIT 0x00 /* 10-bit */
> +#define IMX908_ADBIT_12BIT 0x01 /* 11-bit + digital dither */
> +
> +#define IMX908_REG_MDBIT CCI_REG8(0x3023)
> +#define IMX908_MDBIT_RAW10 0x00
> +#define IMX908_MDBIT_RAW12 0x01
> +
> +/* ---- VMAX Frame length in lines */
> +#define IMX908_REG_VMAX CCI_REG24_LE(0x3028) /* [19:0] */
> +#define IMX908_VMAX_MAX 0xfffff
> +#define IMX908_VMAX_DEFAULT 2250U
> +/*
> + * Window cropping mode restrictions:
> + * VMAX >= PIX_VWIDTH + 70
> + * VMAX >= 1206
> + */
> +#define IMX908_VMAX_CROP_MIN_MARGIN 70
> +#define IMX908_VMAX_CROP_MIN_LIMIT 1206
> +
> +/* ---- HMAX Line length in clocks */
> +#define IMX908_REG_HMAX CCI_REG16_LE(0x302c) /* [15:0] */
> +#define IMX908_HMAX_MAX 0xffff
> +#define IMX908_HMAX_DEFAULT 1100U
> +
> +/* ---- Cropping related */
> +#define IMX908_REG_PIX_HST CCI_REG16_LE(0x303c) /* [12:0] Init 0h */
> +#define IMX908_REG_PIX_HWIDTH CCI_REG16_LE(0x303e) /* [12:0] Init 0F10h */
> +#define IMX908_REG_LANEMODE CCI_REG8(0x3040) /* 1:2-lane, 3:4-lane */
> +#define IMX908_REG_PIX_VST CCI_REG16_LE(0x3044) /* [12:0] Init 0h */
> +#define IMX908_REG_PIX_VWIDTH CCI_REG16_LE(0x3046) /* [12:0] Init 0884h */
> +#define IMX908_CROP_ALIGN_VSTART 4U
> +#define IMX908_CROP_ALIGN_VWIDTH 4U
> +#define IMX908_CROP_ALIGN_HSTART 4U
> +#define IMX908_CROP_ALIGN_HWIDTH 16U
> +#define IMX908_CROP_MIN_HEIGHT 1136U
> +#define IMX908_CROP_MIN_WIDTH 1040U
> +
> +/* ---- Shutter */
> +#define IMX908_REG_SHR0 CCI_REG24_LE(0x3050) /* [19:0] */
> +#define IMX908_MIN_SHR0 7U
> +
> +/*
> + * Analogue gain control
> + * Range is from 0 to 100 (0dB - 30dB) with 0.3dB step size
> + * Values from 101 to 240 are valid but correspond to additional digital gain
> + * (0.3dB - 42dB) so don't expose it to userspace
> + */
> +#define IMX908_REG_GAIN CCI_REG16_LE(0x3070) /* [10:0] */
> +#define IMX908_ANA_GAIN_MAX 100
> +#define IMX908_ANA_GAIN_MIN 0
> +#define IMX908_ANA_GAIN_STEP 1
> +#define IMX908_ANA_GAIN_DEFAULT 0
> +
> +/* ---- XVS/XHS output mode */
> +#define IMX908_XXS_DRV CCI_REG8(0x30a6) /* [1:0] XVS_DRV [3:2] XHS_DRV */
> +
> +#define IMX908_REG_BLKLEVEL CCI_REG16_LE(0x30dc) /* [15:0] black level */
> +
> +/* ---- Test Pattern Generator (TPG) registers */
> +#define IMX908_REG_TPG_EN CCI_REG8(0x30e0)
> +#define IMX908_TPG_EN_BIT BIT(0)
> +#define IMX908_REG_TPG_PATSEL CCI_REG8(0x30e2) /* [4:0] pattern idx */
> +#define IMX908_REG_TESTCLKEN CCI_REG8(0x4900)
> +#define IMX908_TESTCLKEN_BIT BIT(3)
> +
> +#define IMX908_REG_TYPE_ID CCI_REG16_LE(0x4c0c)
> +#define IMX908_CHIP_ID 0x038c /* 908 */
> +
> +/* ---- IMX908 Common Register List ---- */
> +static const struct cci_reg_sequence imx908_common_regs[] = {
> + { IMX908_XXS_DRV, IMX908_CONTROLLER_MODE },
> + { CCI_REG8(0x039c), 0x03 },
> + { CCI_REG8(0x3416), 0x20 },
> + { CCI_REG8(0x3417), 0x00 },
> + { CCI_REG8(0x3456), 0xf4 },
> + { CCI_REG8(0x345d), 0x01 },
> + { CCI_REG8(0x3460), 0x00 },
> + { CCI_REG8(0x3461), 0x0b },
> + { CCI_REG8(0x3471), 0x00 },
> + { CCI_REG8(0x3472), 0x23 },
> + { CCI_REG8(0x347b), 0x02 },
> + { CCI_REG8(0x3481), 0x01 },
> + { CCI_REG8(0x380c), 0x00 },
> + { CCI_REG8(0x380f), 0x0c },
> + { CCI_REG8(0x381c), 0x11 },
> + { CCI_REG8(0x3820), 0x22 },
> + { CCI_REG8(0x3824), 0x33 },
> + { CCI_REG8(0x3828), 0x22 },
> + { CCI_REG8(0x382c), 0x33 },
> + { CCI_REG8(0x3830), 0x33 },
> + { CCI_REG8(0x3838), 0x05 },
> + { CCI_REG8(0x383c), 0x07 },
> + { CCI_REG8(0x3840), 0x06 },
> + { CCI_REG8(0x3848), 0x17 },
> + { CCI_REG8(0x384c), 0x0b },
> + { CCI_REG8(0x3850), 0x10 },
> + { CCI_REG8(0x3854), 0x12 },
> + { CCI_REG8(0x385c), 0x20 },
> + { CCI_REG8(0x38c4), 0x64 },
> + { CCI_REG8(0x38c5), 0x64 },
> + { CCI_REG8(0x38c6), 0x64 },
> + { CCI_REG8(0x3c4a), 0x15 },
> + { CCI_REG8(0x3c4c), 0x13 },
> + { CCI_REG8(0x3c4d), 0x13 },
> + { CCI_REG8(0x3c4e), 0x13 },
> + { CCI_REG8(0x3c50), 0x77 },
> + { CCI_REG8(0x3c51), 0x07 },
> + { CCI_REG8(0x3db4), 0x00 },
> + { CCI_REG8(0x4419), 0x06 },
> + { CCI_REG8(0x441c), 0x00 },
> + { CCI_REG8(0x4426), 0x00 },
> + { CCI_REG8(0x4538), 0x20 },
> + { CCI_REG8(0x4539), 0x19 },
> + { CCI_REG8(0x453a), 0x19 },
> + { CCI_REG8(0x453b), 0x19 },
> + { CCI_REG8(0x453c), 0x19 },
> + { CCI_REG8(0x453d), 0x19 },
> + { CCI_REG8(0x453e), 0x19 },
> + { CCI_REG8(0x453f), 0x19 },
> + { CCI_REG8(0x4540), 0x19 },
> + { CCI_REG8(0x4544), 0x11 },
> + { CCI_REG8(0x4545), 0x11 },
> + { CCI_REG8(0x4546), 0x11 },
> + { CCI_REG8(0x463c), 0x20 },
> + { CCI_REG8(0x465e), 0xcf },
> + { CCI_REG8(0x4684), 0x20 },
> + { CCI_REG8(0x46a6), 0xcf },
> + { CCI_REG8(0x46e2), 0xf3 },
> +};
> +
> +static const u32 tuning_regs[] = {
> + CCI_REG8(0x3d78),
> + CCI_REG8(0x3d79),
> + CCI_REG8(0x3d80),
> + CCI_REG8(0x3d81),
> + CCI_REG8(0x3d88),
> + CCI_REG8(0x3d89),
> + CCI_REG8(0x3d90),
> + CCI_REG8(0x3d91),
> +};
> +
> +#define IMX908_EXPOSURE_MIN 1
> +#define IMX908_EXPOSURE_STEP 1
> +
> +/* ---- Speed of internal clock */
> +#define IMX908_XHS_HZ 74250000ULL
> +
> +/* ---- Subdev Pads */
> +#define IMX908_SOURCE_PAD 0
> +
> +#define IMX908_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
> +
> +/* IMX908 total area includes effective area plus
This should be
/*
* IMX908 total area includes effective area plus
> + * 10 effective OB pixels in vertical direction,
> + * 10 pixels OB side ignored area in height
> + */
> +static const struct v4l2_rect imx908_total_area = {
> + .top = 0,
> + .left = 0,
> + .width = 3856,
> + .height = 2200,
> +};
> +
> +static const struct v4l2_rect imx908_active_area = {
> + .top = 0,
> + .left = 0,
> + .width = 3856,
> + .height = 2176,
> +};
> +
> +/* Recommended 4K recording area centered within the active area */
> +static const struct v4l2_rect imx908_recording_area = {
> + .top = 8,
> + .left = 8,
> + .width = 3840,
> + .height = 2160,
> +};
> +
> +static const char *const imx908_supply_names[] = {
> + "avdd", /* Analog (3.3V) supply */
> + "dvdd", /* Digital Core (1.1V) supply */
> + "ovdd", /* IF (1.8V) supply */
> +};
> +
> +static const u32 imx908_mbus_codes[] = {
> + MEDIA_BUS_FMT_SRGGB10_1X10, /* RAW10 */
> + MEDIA_BUS_FMT_SRGGB12_1X12, /* RAW12 */
> +};
> +
> +enum {
> + IMX908_LINK_FREQ_297MHZ,
> + IMX908_LINK_FREQ_360MHZ,
> + IMX908_LINK_FREQ_445MHZ,
> + IMX908_LINK_FREQ_594MHZ,
> + IMX908_LINK_FREQ_720MHZ,
> + IMX908_LINK_FREQ_891MHZ,
> + IMX908_LINK_FREQ_1039MHZ,
> + IMX908_LINK_FREQ_1188MHZ,
> +};
> +
> +static const u64 imx908_link_freqs[] = {
> + [IMX908_LINK_FREQ_297MHZ] = 297000000ULL,
> + [IMX908_LINK_FREQ_360MHZ] = 360000000ULL,
> + [IMX908_LINK_FREQ_445MHZ] = 445000000ULL,
> + [IMX908_LINK_FREQ_594MHZ] = 594000000ULL,
> + [IMX908_LINK_FREQ_720MHZ] = 720000000ULL,
> + [IMX908_LINK_FREQ_891MHZ] = 891000000ULL,
> + [IMX908_LINK_FREQ_1039MHZ] = 1039500000ULL,
> + [IMX908_LINK_FREQ_1188MHZ] = 1188000000ULL,
> +};
> +
> +/* DDR lane_rate (Mbps/lane) = 2 x link_freq (Hz) / 1e6 */
> +static const u8 imx908_datarate_sel[] = {
> + [IMX908_LINK_FREQ_297MHZ] = 0x07, /* 594 Mbps */
> + [IMX908_LINK_FREQ_360MHZ] = 0x06, /* 720 Mbps */
> + [IMX908_LINK_FREQ_445MHZ] = 0x05, /* 891 Mbps */
> + [IMX908_LINK_FREQ_594MHZ] = 0x04, /* 1188 Mbps */
> + [IMX908_LINK_FREQ_720MHZ] = 0x03, /* 1440 Mbps */
> + [IMX908_LINK_FREQ_891MHZ] = 0x02, /* 1782 Mbps */
> + [IMX908_LINK_FREQ_1039MHZ] = 0x01, /* 2079 Mbps */
> + [IMX908_LINK_FREQ_1188MHZ] = 0x00, /* 2376 Mbps */
> +};
> +
> +struct imx908_inck_cfg {
> + u32 xclk_hz;
> + u8 inck_sel;
> +};
> +
> +/* Allowed clock values in Hz */
> +static const u32 imx908_inck_table[] = {
> + 74250000,
> + 37125000,
> + 72000000,
> + 27000000,
> + 24000000,
> +};
> +
> +struct imx908_mode {
> + /* Sensor timing */
> + u16 hmax; /* clocks per line */
> + u32 vmax; /* lines per frame */
> +
> + /* Derived values */
> + u64 pixel_rate;
> +};
This structure should be dropped. It will be easier to do with a
hardcoded pixel_rate value (see below), but should be done even if we
decide to keep the pixel rate dynamic. I can give it a try based on v2
of the driver if you're having trouble.
> +
> +struct imx908 {
> + /* Core V4L2 objects */
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct device *dev;
> +
> + struct regmap *cci;
> +
> + /* Hardware resources */
> + struct clk *xclk;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[ARRAY_SIZE(imx908_supply_names)];
> +
> + /* Clock configuration */
> + u32 xclk_freq;
> + u8 inck_sel;
> +
> + /* Link/bus configuration */
> + u8 num_lanes;
> + unsigned long link_freq_bitmap;
> + unsigned int link_freq_idx;
> +
> + struct imx908_mode mode; /* Current mode */
> +
> + /* V4L2 controls */
> + struct {
> + struct v4l2_ctrl_handler handler;
> +
> + struct v4l2_ctrl *pixel_rate;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *test_pattern;
> + } ctrls;
> +};
> +
> +static inline struct imx908 *to_imx908(struct v4l2_subdev *_sd)
> +{
> + return container_of(_sd, struct imx908, sd);
> +}
> +
> +/* ----------------------- Basic Helper Functions ---------------------------- */
> +
> +/* Compute pixel-rate from link-freq/lanes/bpp */
> +static inline u64 imx908_pixel_rate(u8 lanes, u8 bpp, u64 linkfreq_hz)
> +{
> + u64 total = lanes * linkfreq_hz * 2; /* DDR: 2 * linkfreq */
> +
> + return div_u64(total, bpp);
In his IMX678 driver, Jai hardcodes the pixel rate to 594MHz, which is
8 * 74.25MHz (IMX908_XHS_HZ). The rationale is that HMAX is expressed in
units of the 74.25 MHz internal clock, so we assumed that the pixel
array is not read out using the link clock but using a clock derived
from the internal clock. The multiplier was chosen to ensure that the
minimum HMAX value corresponds to a positive HBLANK, as negative HBLANK
would confuse userspace (and developers). Having a fixed pixel rate
greatly simplifies the driver, as you don't have to update the pixel
rate control, and HMAX calculation from HBLANK becomes a division by 8.
We're not sure how the pixel array of the IMX678 is clocked exactly, so
I don't know if the above corresponds to reality, neither for the IMX678
nor the IMX908. If you could provide more information about the pixel
array readout, that would be greatly appreciated. Note that the pixel
rate value does not have to match the physical readout clock, as it is
only used by userspace to calculate timings (to convert between h/v
blank and fps).
Jai can provide more information if needed.
> +}
> +
> +static int imx908_link_freq_to_datarate_sel(struct imx908 *imx, u64 link_freq_hz, u8 *sel)
> +{
> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_link_freqs); i++) {
> + if (imx908_link_freqs[i] == link_freq_hz) {
> + *sel = imx908_datarate_sel[i];
> + return 0;
> + }
> + }
A blank line would be nice here.
> + dev_err(imx->dev, "Unsupported link frequency %llu Hz\n", link_freq_hz);
The caller prints a similar error message, you can drop this one.
> + return -EINVAL;
> +}
> +
> +static bool imx908_mbus_code_supported(struct imx908 *imx, u32 code)
> +{
> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_mbus_codes); i++) {
> + if (imx908_mbus_codes[i] == code)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static u16 imx908_calc_hmax(u32 width, u32 hblank, u64 pixel_rate)
> +{
> + u64 hmax;
> +
> + /* HMAX is expressed in cycles of the internal 74.25 MHz clock */
> + hmax = DIV_ROUND_UP_ULL((width + hblank) * IMX908_XHS_HZ, pixel_rate);
> +
> + return min_t(u64, hmax, IMX908_HMAX_MAX);
> +}
> +
> +static u32 imx908_calc_vmax(u32 height, u32 vblank)
> +{
> + return min_t(u32, height + vblank, IMX908_VMAX_MAX);
> +}
> +
> +static u16 imx908_calc_min_hmax(u32 width, u64 pixel_rate)
> +{
> + /*
> + * Calculate the minimum HMAX that guarantees:
> + * line_length_pixels >= width
> + * where:
> + * line_length_pixels =
> + * ceil(HMAX * pixel_rate / IMX908_XHS_HZ)
> + */
> + return DIV_ROUND_UP((u64)width * IMX908_XHS_HZ, pixel_rate);
> +}
> +
> +static u32 imx908_calc_min_vmax(const struct v4l2_rect *crop)
> +{
> + /* In window crop mode constrain VMAX (from datasheet) */
> + if (!v4l2_rect_equal(crop, &imx908_active_area))
> + return max(crop->height + IMX908_VMAX_CROP_MIN_MARGIN,
> + IMX908_VMAX_CROP_MIN_LIMIT);
> +
> + return IMX908_VMAX_DEFAULT;
> +}
> +
> +static u32 imx908_calc_min_vblank(const struct v4l2_rect *crop)
> +{
> + u32 min_vmax = imx908_calc_min_vmax(crop);
> +
> + if (min_vmax <= crop->height)
> + return 0;
> +
> + return min_vmax - crop->height;
> +}
> +
> +static u32 imx908_calc_max_vblank(u32 height)
> +{
> + return IMX908_VMAX_MAX - height;
> +}
> +
> +static int imx908_set_exposure_lines(struct imx908 *imx, u32 exposure_lines)
> +{
> + u32 max_lines = imx->mode.vmax - IMX908_MIN_SHR0;
> +
> + exposure_lines = clamp_t(u32, exposure_lines, 1, max_lines);
> +
> + /* Calculate the shutter register value based on current VMAX */
> + u32 shr0 = imx->mode.vmax - exposure_lines;
> +
> + return cci_write(imx->cci, IMX908_REG_SHR0, shr0, NULL);
> +}
> +
> +static u8 imx908_bits_per_pixel(u32 code)
> +{
> + switch (code) {
> + case MEDIA_BUS_FMT_SRGGB12_1X12:
> + return 12;
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + default:
> + return 10;
> + }
> +}
> +
> +static int imx908_sync_pixel_rate_ctrl(struct imx908 *imx)
> +{
> + s64 pixel_rate = imx->mode.pixel_rate;
> + int ret;
> +
> + if (!imx->ctrls.pixel_rate)
> + return 0;
> +
> + /* Update control range so pixel_rate is fixed (min = max = default) */
> + __v4l2_ctrl_modify_range(imx->ctrls.pixel_rate, pixel_rate, pixel_rate,
> + 1, pixel_rate);
> +
> + /* Update the current control value to match the new mode pixel_rate */
> + ret = __v4l2_ctrl_s_ctrl_int64(imx->ctrls.pixel_rate, pixel_rate);
> + if (ret) {
> + dev_err(imx->dev, "failed to update pixel rate control: %d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static u32 imx908_hmax_to_hblank(u16 hmax, u64 pixel_rate, u32 width)
> +{
> + /* Convert HMAX to line length in pixels */
> + u64 line_len_pixels = div_u64((u64)hmax * pixel_rate, IMX908_XHS_HZ);
> +
> + if (line_len_pixels <= width)
> + return 0;
> +
> + /* Subtract active image width to obtain horizontal blanking interval */
> + return line_len_pixels - width;
> +}
> +
> +static void imx908_update_hblank_limits(struct imx908 *imx, u32 width, u64 pixel_rate)
> +{
> + u16 min_hmax = imx908_calc_min_hmax(width, pixel_rate);
> + u32 min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate, width);
> + u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate, width);
> + u32 hblank = imx908_hmax_to_hblank(imx->mode.hmax, pixel_rate, width);
> +
> + hblank = clamp_t(u32, hblank, min_hblank, max_hblank);
> +
> + __v4l2_ctrl_modify_range(imx->ctrls.hblank, min_hblank, max_hblank, 1, hblank);
> +
> + if (imx->ctrls.hblank->val != hblank)
> + __v4l2_ctrl_s_ctrl(imx->ctrls.hblank, hblank);
> +}
> +
> +static void imx908_update_vblank_limits(struct imx908 *imx,
> + const struct v4l2_rect *crop)
> +{
> + u32 min_vblank;
> + u32 max_vblank;
> + u32 vblank;
> +
> + if (!imx->ctrls.vblank)
> + return;
> +
> + min_vblank = imx908_calc_min_vblank(crop);
> + max_vblank = imx908_calc_max_vblank(crop->height);
> + vblank = clamp_t(u32, imx->ctrls.vblank->val,
> + min_vblank, max_vblank);
> +
> + __v4l2_ctrl_modify_range(imx->ctrls.vblank,
> + min_vblank,
> + max_vblank,
> + 1,
> + vblank);
> +}
> +
> +/* ------------------ Pattern Generator (TPG) ----------------------- */
> +static const char * const imx908_tpg_menu[] = {
> + "Disabled",
> + "All 000h",
> + "All FFFh",
> + "All 555h",
> + "All AAAh",
> + "555h / AAAh Toggle",
> + "AAAh / 555h Toggle",
> + "000h / 555h Toggle",
> + "555h / 000h Toggle",
> + "000h / FFFh Toggle",
> + "FFFh / 000h Toggle",
> + "Horiz Color Bars",
> + "Vert Color Bars",
> +};
> +
> +static int imx908_update_test_pattern(struct imx908 *imx, u32 index)
> +{
> + int ret = 0;
> +
> + if (index == 0) {
> + /* Picture (not TPG) output setting */
> + cci_update_bits(imx->cci, IMX908_REG_TPG_EN, IMX908_TPG_EN_BIT,
> + 0x00, &ret);
> +
> + /* Disable TESTCLKEN (write-only register) */
> + cci_write(imx->cci, IMX908_REG_TESTCLKEN, 0x00, &ret);
> +
> + return ret;
> + }
> +
> + cci_write(imx->cci, IMX908_REG_TESTCLKEN, IMX908_TESTCLKEN_BIT, &ret);
> + cci_write(imx->cci, IMX908_REG_TPG_PATSEL, index - 1, &ret);
> + cci_update_bits(imx->cci, IMX908_REG_TPG_EN, IMX908_TPG_EN_BIT,
> + IMX908_TPG_EN_BIT, &ret);
> +
> + return ret;
> +}
> +
> +/* -------------------- Mode Building ----------------------- */
> +static void imx908_mode_build(struct imx908 *imx,
> + const struct v4l2_mbus_framefmt *fmt,
> + const struct v4l2_rect *crop)
> +{
> + struct imx908_mode *m = &imx->mode;
> + u8 bpp = imx908_bits_per_pixel(fmt->code);
> + u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
> +
> + m->pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
> +
> + /* --- H timing (HMAX) --- */
> + if (v4l2_rect_equal(crop, &imx908_active_area)) {
> + /* Update HBLANK HMAX in all pixel mode */
> + u16 min_hmax = imx908_calc_min_hmax(fmt->width, m->pixel_rate);
> + u32 min_hblank = imx908_hmax_to_hblank(min_hmax, m->pixel_rate,
> + fmt->width);
> + u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX,
> + m->pixel_rate, fmt->width);
> + u32 hblank = clamp_t(u32, imx->ctrls.hblank->val, min_hblank,
> + max_hblank);
> + m->hmax = imx908_calc_hmax(fmt->width, hblank, m->pixel_rate);
> + } else if (!m->hmax) {
> + u32 hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT,
> + m->pixel_rate,
> + imx908_active_area.width);
> +
> + m->hmax = imx908_calc_hmax(fmt->width, hblank, m->pixel_rate);
> + }
> +
> + /* --- V timing (VMAX) --- */
> + u32 min_vblank = imx908_calc_min_vblank(crop);
> + u32 max_vblank = imx908_calc_max_vblank(fmt->height);
> +
> + u32 vblank = clamp_t(u32,
> + imx->ctrls.vblank->val,
> + min_vblank,
> + max_vblank);
> +
> + m->vmax = imx908_calc_vmax(fmt->height, vblank);
> +
> + u64 line_length_pixels = div_u64((u64)m->hmax * m->pixel_rate, IMX908_XHS_HZ);
> +
> + if (line_length_pixels < fmt->width)
> + dev_warn(imx->dev, "Invalid timing (line=%llu < width=%u)\n",
> + line_length_pixels, fmt->width);
> +}
> +
> +static void imx908_update_framing_limits(struct imx908 *imx, struct v4l2_subdev_state *state)
Please wrap the line.
> +{
> + const struct v4l2_mbus_framefmt *fmt;
> + const struct v4l2_rect *crop;
> +
> + fmt = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD);
> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
> +
> + u8 bpp = imx908_bits_per_pixel(fmt->code);
> + u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
> + u64 pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
> +
> + /* Only update HBLANK in all pixel mode */
> + if (v4l2_rect_equal(crop, &imx908_active_area))
> + imx908_update_hblank_limits(imx, fmt->width, pixel_rate);
> +
> + imx908_update_vblank_limits(imx, crop);
> +
> + /* Build mode so cached timing uses current control values */
> + imx908_mode_build(imx, fmt, crop);
> +
> + /* Exposure handling */
> + __v4l2_ctrl_modify_range(imx->ctrls.exposure,
> + 1,
> + imx->mode.vmax - IMX908_MIN_SHR0,
> + 1,
> + min_t(u32, imx->ctrls.exposure->val,
> + imx->mode.vmax - IMX908_MIN_SHR0));
> +}
> +
> +/* ----------------------- HW Programming ---------------------- */
> +
> +static int imx908_mode_apply(struct imx908 *imx,
> + const struct v4l2_mbus_framefmt *fmt)
> +{
> + int ret = 0;
> + u8 adbit, mdbit;
> +
> + switch (fmt->code) {
> + case MEDIA_BUS_FMT_SRGGB12_1X12:
> + adbit = IMX908_ADBIT_12BIT;
> + mdbit = IMX908_MDBIT_RAW12;
> + break;
> +
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + default:
> + adbit = IMX908_ADBIT_10BIT;
> + mdbit = IMX908_MDBIT_RAW10;
> + break;
> + }
> +
> + cci_write(imx->cci, IMX908_REG_XMASTER, IMX908_CONTROLLER_MODE, &ret);
> + cci_write(imx->cci, IMX908_REG_ADBIT, adbit, &ret);
> + cci_write(imx->cci, IMX908_REG_MDBIT, mdbit, &ret);
> + if (ret) {
> + dev_err(imx->dev, "Mode register write failed: %d\n", ret);
> + return ret;
> + }
You can drop the error check here.
> +
> + /* Tuning registers that need to be changed for RAW10/10-bit */
> + u16 value = adbit == IMX908_ADBIT_10BIT &&
> + mdbit == IMX908_MDBIT_RAW10 ? 0x0c : 0x05;
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(tuning_regs); ++i)
> + cci_write(imx->cci, tuning_regs[i], value, &ret);
> +
> + if (ret) {
> + dev_err(imx->dev, "Tuning register write failed: %d\n", ret);
> + return ret;
> + }
And this one too.
> +
> + /* INCK */
> + cci_write(imx->cci, IMX908_REG_INCK_SEL, imx->inck_sel, &ret);
> + if (ret) {
> + dev_err(imx->dev, "INCK register write failed: %d\n", ret);
> + return ret;
> + }
> +
> + /* MIPI lane rate selections */
> + u8 datarate_sel;
> + u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
> +
> + ret = imx908_link_freq_to_datarate_sel(imx, link_freq, &datarate_sel);
> + if (ret) {
> + dev_err(imx->dev, "Unsupported link_freq=%llu for DATARATE_SEL\n",
> + link_freq);
> + return ret;
> + }
The link frequency is validated at probe time, so the
imx908_link_freq_to_datarate_sel() can never return an error. Drop error
checking here too.
> + cci_write(imx->cci, IMX908_REG_DATARATE_SEL, datarate_sel, &ret);
> +
> + /* Lane config */
> + cci_write(imx->cci, IMX908_REG_LANEMODE, imx->num_lanes - 1, &ret);
> + if (ret) {
> + dev_err(imx->dev, "Clock/lane configuration failed: %d\n", ret);
> + return ret;
> + }
Drop this error check here too.
> +
> + /* Recommended black level offset is 50 in 10-bit, 200 for others */
> + u16 blklevel = (mdbit == IMX908_MDBIT_RAW10) ? 50 : 200;
> +
> + cci_write(imx->cci, IMX908_REG_BLKLEVEL, blklevel, &ret);
> +
> + return ret;
The error is propagated through the chained cci_write() calls and
handled here. That's all you need, cci_write() will print a message on
failure, with the register address and value.
> +}
> +
> +static int imx908_program_window(struct imx908 *imx, const struct v4l2_rect *crop)
> +{
> + bool all_pixel_mode = v4l2_rect_equal(crop, &imx908_active_area);
> + int ret = 0;
> +
> + cci_write(imx->cci, IMX908_REG_WINMODE,
> + all_pixel_mode ? IMX908_WINMODE_ALLPIX : IMX908_WINMODE_CROP,
> + &ret);
> +
> + /* Update window size in window crop mode */
> + if (!all_pixel_mode) {
> + cci_write(imx->cci, IMX908_REG_PIX_HST, crop->left, &ret);
> + cci_write(imx->cci, IMX908_REG_PIX_HWIDTH, crop->width, &ret);
> + cci_write(imx->cci, IMX908_REG_PIX_VST, crop->top, &ret);
> + cci_write(imx->cci, IMX908_REG_PIX_VWIDTH, crop->height, &ret);
> + }
> +
> + return ret;
> +}
> +
> +/* ----------------------- Start/Stop streaming ---------------------- */
> +
> +static int imx908_start_streaming(struct imx908 *imx)
> +{
> + struct v4l2_subdev_state *sd_state;
> + const struct v4l2_rect *crop;
> + const struct v4l2_mbus_framefmt *format;
> + int ret = 0;
> +
> + /* Get active state */
> + sd_state = v4l2_subdev_get_locked_active_state(&imx->sd);
The caller already has the active state, pass it as an argument to the
imx908_start_streaming() function.
> + crop = v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD);
> + format = v4l2_subdev_state_get_format(sd_state, IMX908_SOURCE_PAD);
> +
> + /* Build the mode based on active state before streaming */
> + imx908_mode_build(imx, format, crop);
> +
> + /* Write common registers: static sensor tuning only */
> + cci_multi_reg_write(imx->cci, imx908_common_regs,
> + ARRAY_SIZE(imx908_common_regs), &ret);
> + if (ret)
> + return ret;
> +
> + /* HW write all other registers (which depend on mode) */
> + ret = imx908_mode_apply(imx, format);
> + if (ret)
> + return ret;
> +
> + /* Geometry */
> + ret = imx908_program_window(imx, crop);
> + if (ret)
> + return ret;
> +
> + /* Apply the current control values */
> + ret = __v4l2_ctrl_handler_setup(imx->sd.ctrl_handler);
> + if (ret)
> + return ret;
> +
> + /* Cancel standby state */
> + cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, &ret);
> + msleep(24); /* SRM >=24 ms regulator stabilization after standby cancel */
> +
> + /* Start controller mode */
> + cci_write(imx->cci, IMX908_REG_XMSTA, IMX908_START_CONTROLLER, &ret);
> + return ret;
> +}
> +
> +static int imx908_stop_streaming(struct imx908 *imx)
> +{
> + int ret = 0;
> +
> + cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, &ret);
> + cci_write(imx->cci, IMX908_REG_XMSTA, IMX908_STOP_CONTROLLER, &ret);
> + return ret;
> +}
> +
> +/* --------------------------- V4L2 controls ------------------------------ */
> +
> +static int imx908_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct imx908 *imx = container_of(ctrl->handler, struct imx908, ctrls.handler);
> + struct v4l2_subdev_state *state;
> + struct i2c_client *client = v4l2_get_subdevdata(&imx->sd);
> + const struct v4l2_mbus_framefmt *format;
> + int ret = 0;
> +
> + /* Retrieve the active, locked subdev state format dimensions */
> + state = v4l2_subdev_get_locked_active_state(&imx->sd);
> + format = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD);
> +
> + /* Applying V4L2 controls only if powered up */
> + if (!pm_runtime_get_if_in_use(&client->dev))
if (!pm_runtime_get_if_in_use(imx->dev))
and similarly below, and drop the client local variable.
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_EXPOSURE: {
> + u32 lines = clamp_t(u32, ctrl->val, 1,
> + imx->mode.vmax - IMX908_MIN_SHR0);
Drop this, the control framework already clamps to the limits.
> +
> + ret = imx908_set_exposure_lines(imx, lines);
> + break;
> + }
> +
> + case V4L2_CID_ANALOGUE_GAIN:
> +
Drop this blank line.
> + u32 reg = clamp_t(u32, ctrl->val, IMX908_ANA_GAIN_MIN,
> + IMX908_ANA_GAIN_MAX);
Drop this, the control framework already clamps to the limits.
> +
> + cci_write(imx->cci, IMX908_REG_GAIN, reg, &ret);
> + break;
> +
> + case V4L2_CID_VBLANK: {
> + const struct v4l2_rect *crop;
> +
> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
> +
> + u32 min_vblank = imx908_calc_min_vblank(crop);
> + u32 max_vblank = imx908_calc_max_vblank(format->height);
> + u32 vblank = clamp_t(u32, ctrl->val, min_vblank, max_vblank);
> +
> + imx->mode.vmax = imx908_calc_vmax(format->height, vblank);
> +
> + u32 exposure = min_t(u32, imx->ctrls.exposure->val,
> + imx->mode.vmax - IMX908_MIN_SHR0);
> +
> + __v4l2_ctrl_modify_range(imx->ctrls.exposure,
> + IMX908_EXPOSURE_MIN,
> + imx->mode.vmax - IMX908_MIN_SHR0,
> + IMX908_EXPOSURE_STEP, exposure);
All the the above should go before the pm_runtime_get_if_in_use() call,
as the exposure control limits needs to be updated even if the sensor
isn't streaming. You can add a
if (ctrl->id == V4L2_CID_VBLANK) {
...
}
block there.
> +
> + ret = cci_write(imx->cci, IMX908_REG_VMAX,
> + imx->mode.vmax, NULL);
> + if (!ret)
> + ret = imx908_set_exposure_lines(imx, exposure);
> +
> + break;
> + }
> +
> + case V4L2_CID_HBLANK: {
> + u16 hmax;
> + u32 width = format->width;
> + u64 pixel_rate = imx->mode.pixel_rate;
> + const struct v4l2_rect *crop;
> +
> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
> +
> + /* Ignore HBLANK update in window crop mode */
> + if (!v4l2_rect_equal(crop, &imx908_active_area))
> + break;
> +
> + /* Skip timing calculation when not initialized */
> + if (!pixel_rate || !width)
> + break;
> +
> + /* Recompute hmax based on HBLANK */
> + hmax = imx908_calc_hmax(width, ctrl->val, pixel_rate);
> + imx->mode.hmax = hmax;
> + cci_write(imx->cci, IMX908_REG_HMAX, hmax, &ret);
> + break;
> + }
> +
> + case V4L2_CID_PIXEL_RATE:
> + case V4L2_CID_LINK_FREQ:
> + break;
> +
> + case V4L2_CID_TEST_PATTERN:
> + ret = imx908_update_test_pattern(imx, ctrl->val);
> + break;
> +
> + case V4L2_CID_HFLIP:
> + cci_write(imx->cci, IMX908_REG_HREVERSE, ctrl->val, &ret);
> + break;
> +
> + case V4L2_CID_VFLIP:
> + cci_write(imx->cci, IMX908_REG_VREVERSE, ctrl->val, &ret);
> + break;
> +
> + default:
> + dev_warn(&client->dev,
> + "ctrl(id:0x%x,val:0x%x) is not handled\n",
> + ctrl->id, ctrl->val);
> + break;
> + }
> +
> + pm_runtime_put(&client->dev);
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops imx908_ctrl_ops = {
> + .s_ctrl = imx908_set_ctrl,
> +};
> +
> +/* ------------------------ Pad/video ops ------------------ */
> +
> +static int imx908_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->index >= ARRAY_SIZE(imx908_mbus_codes))
> + return -EINVAL;
> + code->code = imx908_mbus_codes[code->index];
> + return 0;
> +}
> +
> +static int imx908_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + const struct v4l2_rect *crop = v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD);
> + struct imx908 *imx = to_imx908(sd);
> +
> + if (fse->index > 0)
> + return -EINVAL;
> +
> + if (!imx908_mbus_code_supported(imx, fse->code))
> + return -EINVAL;
> +
> + fse->min_width = crop->width;
> + fse->max_width = crop->width;
> + fse->min_height = crop->height;
> + fse->max_height = crop->height;
> +
> + return 0;
> +}
> +
> +static int imx908_set_pad_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct imx908 *imx = to_imx908(sd);
> + struct v4l2_mbus_framefmt *format;
> + int ret = 0;
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && v4l2_subdev_is_streaming(sd))
> + return -EBUSY;
> +
> + format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> +
> + if (imx908_mbus_code_supported(imx, fmt->format.code))
> + format->code = fmt->format.code;
> + else
> + format->code = IMX908_DEFAULT_MBUS_CODE;
> +
> + /* Fully define metadata */
> + format->field = V4L2_FIELD_NONE;
> + format->colorspace = V4L2_COLORSPACE_RAW;
> + format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> + format->xfer_func = V4L2_XFER_FUNC_NONE;
> +
> + /* Update pixel rate range and hblank (ACTIVE ONLY) */
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + imx908_update_framing_limits(imx, sd_state);
> + ret = imx908_sync_pixel_rate_ctrl(imx);
> + if (ret)
> + return ret;
> + }
> +
> + /* Return final applied format to userspace */
> + fmt->format = *format;
> +
> + return 0;
> +}
> +
> +static int imx908_set_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> +{
> + struct imx908 *imx = to_imx908(sd);
> + struct v4l2_rect *crop; /* current crop rectangle in subdev state */
> + struct v4l2_rect *r = &sel->r;
> + struct v4l2_mbus_framefmt *format;
> +
> + if (sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> +
> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && v4l2_subdev_is_streaming(sd))
> + return -EBUSY;
> +
> + crop = v4l2_subdev_state_get_crop(sd_state, sel->pad);
> + format = v4l2_subdev_state_get_format(sd_state, sel->pad);
> +
> + /*
> + * Round the crop rectangle size down to the hardware alignment
> + * constraints, then clamp it to the supported size range.
> + */
> + r->width = clamp_t(s32, ALIGN_DOWN(r->width, IMX908_CROP_ALIGN_HWIDTH),
> + IMX908_CROP_MIN_WIDTH, imx908_active_area.width);
> + r->height = clamp_t(s32, ALIGN_DOWN(r->height, IMX908_CROP_ALIGN_VWIDTH),
> + IMX908_CROP_MIN_HEIGHT, imx908_active_area.height);
> +
> + /*
> + * Round and clamp the crop position similarly, with the maximum value
> + * chosen so that the crop rectangle remains fully inside the active
> + * pixel array.
> + */
> + r->left = clamp_t(s32, ALIGN_DOWN(r->left, IMX908_CROP_ALIGN_HSTART), 0,
> + imx908_active_area.width - r->width);
> + r->top = clamp_t(s32, ALIGN_DOWN(r->top, IMX908_CROP_ALIGN_VSTART), 0,
> + imx908_active_area.height - r->height);
> +
> + /* Update the crop rectangle in subdev state */
> + *crop = *r;
> +
> + /* Binning is not supported in IMX908 sensor */
> + format->width = crop->width;
> + format->height = crop->height;
> +
> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + imx908_update_framing_limits(imx, sd_state);
> +
> + return 0;
> +}
> +
> +static int imx908_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> +{
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + sel->r = *v4l2_subdev_state_get_crop(sd_state, IMX908_SOURCE_PAD);
> + return 0;
> +
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + sel->r = imx908_total_area;
> + return 0;
> +
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + sel->r = imx908_recording_area;
> + return 0;
> +
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r = imx908_active_area;
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int imx908_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + u32 pad,
> + u64 streams_mask)
> +{
> + struct imx908 *imx = to_imx908(sd);
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(imx->dev);
> + if (ret)
> + return ret;
> +
> + ret = imx908_start_streaming(imx);
> + if (ret) {
> + pm_runtime_mark_last_busy(imx->dev);
> + pm_runtime_put_autosuspend(imx->dev);
I would add
return ret;
> + }
> +
> + return ret;
and here write
return 0;
to clearly isolate the error path from the success path.
That's a personal preference though, up to you.
> +}
> +
> +static int imx908_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + u32 pad,
> + u64 streams_mask)
> +{
> + struct imx908 *imx = to_imx908(sd);
> + int ret;
> +
> + ret = imx908_stop_streaming(imx);
> +
> + pm_runtime_mark_last_busy(imx->dev);
> + pm_runtime_put_autosuspend(imx->dev);
> +
> + return ret;
> +}
> +
> +static int imx908_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state)
> +{
> + int ret = 0;
> + struct v4l2_subdev_selection sel = {
> + .which = V4L2_SUBDEV_FORMAT_TRY,
> + .pad = IMX908_SOURCE_PAD,
> + .target = V4L2_SEL_TGT_CROP,
> + .r = imx908_active_area,
> + };
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_TRY,
> + .pad = IMX908_SOURCE_PAD,
> + .format = {
> + .code = IMX908_DEFAULT_MBUS_CODE,
> + .width = imx908_active_area.width,
> + .height = imx908_active_area.height,
> + },
> + };
> +
> + ret = imx908_set_selection(sd, sd_state, &sel);
> + ret = imx908_set_pad_format(sd, sd_state, &fmt);
Those two functions return errors only when called with invalid
arguments. Invalid .which, .pad or .target values in sel or fmt would be
driver bugs, so I think it's safe to not check for errors. You shouldn't
assign ret, and you can
return 0;
> +
> + return ret;
> +}
> +
> +static const struct v4l2_subdev_core_ops imx908_core_ops = {
> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_video_ops imx908_video_ops = {
> + .s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx908_pad_ops = {
> + .enum_mbus_code = imx908_enum_mbus_code,
> + .enum_frame_size = imx908_enum_frame_size,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .set_fmt = imx908_set_pad_format,
> + .get_selection = imx908_get_selection,
> + .set_selection = imx908_set_selection,
> + .enable_streams = imx908_enable_streams,
> + .disable_streams = imx908_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx908_internal_ops = {
> + .init_state = imx908_init_state,
> +};
> +
> +static const struct v4l2_subdev_ops imx908_subdev_ops = {
> + .core = &imx908_core_ops,
> + .video = &imx908_video_ops,
> + .pad = &imx908_pad_ops,
> +};
> +
> +/* ----------------------- Power management ---------------------- */
> +
> +static int imx908_power_on(struct imx908 *imx)
> +{
> + int ret;
> +
> + /* Power Rails */
> + ret = regulator_bulk_enable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
> + if (ret) {
> + dev_err(imx->dev, "%s: failed to enable regulators\n", __func__);
You can drop the "%s: " prefix, the error message is clear enough and
doesn't need the function name.
> + return ret;
> + }
> + msleep(200); /* IMX908 power ok after 200ms */
> +
> + if (imx->reset_gpio) {
> + gpiod_set_value_cansleep(imx->reset_gpio, 1); /* XCLR low */
> + udelay(1); /* >= 500ns T_low */
> + gpiod_set_value_cansleep(imx->reset_gpio, 0); /* Sensor starts */
> + }
> +
> + /* Start INCK */
> + ret = clk_prepare_enable(imx->xclk);
> + if (ret) {
> + dev_err(imx->dev, "failed to enable xclk: %d\n", ret);
> + goto err_reset;
> + }
> +
> + /* T_1 >=20us delay before initial SDA/SCL */
> + usleep_range(20, 25);
> +
> + return 0;
> +
> +err_reset:
> + gpiod_set_value_cansleep(imx->reset_gpio, 1); /* assert reset */
> + regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
> + return ret;
> +}
> +
> +static void imx908_power_off(struct imx908 *imx)
> +{
> + gpiod_set_value_cansleep(imx->reset_gpio, 1);
> + clk_disable_unprepare(imx->xclk);
> + regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
> +}
> +
> +static int imx908_runtime_resume(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct imx908 *imx = to_imx908(sd);
> +
> + return imx908_power_on(imx);
> +}
> +
> +static int imx908_runtime_suspend(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct imx908 *imx = to_imx908(sd);
> +
> + imx908_power_off(imx);
> + return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx908_pm_ops,
> + imx908_runtime_suspend,
> + imx908_runtime_resume, NULL);
> +
> +/* ------------------------------- Probe/remove --------------------------- */
> +
> +static int imx908_get_inck_sel(struct imx908 *imx)
> +{
> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_inck_table); i++) {
> + if (imx908_inck_table[i] == imx->xclk_freq) {
> + imx->inck_sel = i;
> + return 0;
> + }
> + }
> +
> + return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n", imx->xclk_freq);
return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n",
imx->xclk_freq);
> +}
> +
> +static int imx908_get_regulators(struct imx908 *imx)
> +{
> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_supply_names); i++)
> + imx->supplies[i].supply = imx908_supply_names[i];
> +
> + return devm_regulator_bulk_get(imx->dev,
> + ARRAY_SIZE(imx908_supply_names),
> + imx->supplies);
> +}
> +
> +static int imx908_parse_fwnode(struct imx908 *imx)
> +{
> + struct fwnode_handle *ep;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> + int ret = 0;
> +
> + /* Get firmware node from device tree */
> + ep = fwnode_graph_get_next_endpoint(dev_fwnode(imx->dev), NULL);
> + if (!ep) {
> + dev_err(imx->dev, "Failed to find endpoint in device tree\n");
> + return -ENXIO;
> + }
> +
> + /* Parse properties: data-lanes, link-frequencies */
> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> + fwnode_handle_put(ep);
> + if (ret)
> + return ret;
> +
> + imx->num_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
> +
> + if (imx->num_lanes != 2 && imx->num_lanes != 4) {
> + dev_err(imx->dev, "only 2 or 4 CSI-2 data lanes are supported (got %u)\n",
> + imx->num_lanes);
> + ret = -EINVAL;
> + goto out_free;
> + }
> +
> + ret = v4l2_link_freq_to_bitmap(imx->dev,
> + bus_cfg.link_frequencies,
> + bus_cfg.nr_of_link_frequencies,
> + imx908_link_freqs,
> + ARRAY_SIZE(imx908_link_freqs),
> + &imx->link_freq_bitmap);
> + if (ret) {
> + dev_err(imx->dev, "Failed to parse link frequencies from DT\n");
> + goto out_free;
> + }
> +
> + if (bitmap_empty(&imx->link_freq_bitmap, ARRAY_SIZE(imx908_link_freqs))) {
> + dev_err(imx->dev,
> + "No common link frequencies between driver and DT\n");
> + ret = -EINVAL;
> + goto out_free;
> + }
> +
> + imx->link_freq_idx = __ffs(imx->link_freq_bitmap);
> +
> +out_free:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> + return ret;
> +}
> +
> +static int imx908_init_controls(struct imx908 *imx)
> +{
> + struct v4l2_ctrl_handler *hdl = &imx->ctrls.handler;
> + struct v4l2_ctrl *link_freq_ctl;
> + struct v4l2_fwnode_device_properties props;
> + int ret;
> + s32 hblank, min_hblank;
> + u32 max_exp, max_hblank, min_vblank, max_vblank;
> + u16 min_hmax;
We tend to follow a "reverse christmas tree" order for variable
declaration (it's one of those coding style rules that aim at increasing
consistency but are otherwise completely arbitrary).
u32 max_exp, max_hblank, min_vblank, max_vblank;
s32 hblank, min_hblank;
u16 min_hmax;
int ret;
I think hblank and min_hblank can be u32, as they're calculated below
with imx908_hmax_to_hblank() that returns a u32.
> +
> + ret = v4l2_ctrl_handler_init(hdl, 11);
> + if (ret)
> + return ret;
> +
> + /* Initialize the pixel_rate control */
> + u8 bpp = imx908_bits_per_pixel(IMX908_DEFAULT_MBUS_CODE);
> + u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
> + u64 pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
> +
> + imx->ctrls.pixel_rate = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_PIXEL_RATE,
> + pixel_rate, pixel_rate, 1, pixel_rate);
> + if (imx->ctrls.pixel_rate)
> + imx->ctrls.pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + /* Set link Frequency */
> + link_freq_ctl = v4l2_ctrl_new_int_menu(hdl, &imx908_ctrl_ops,
> + V4L2_CID_LINK_FREQ,
> + ARRAY_SIZE(imx908_link_freqs) - 1,
> + imx->link_freq_idx,
> + imx908_link_freqs);
> +
> + if (link_freq_ctl)
> + link_freq_ctl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + /* Initialize HMAX and VMAX */
> + imx->mode.hmax = IMX908_HMAX_DEFAULT;
> + imx->mode.vmax = IMX908_VMAX_DEFAULT;
> +
> + /* Set vblank */
> + min_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height;
> + max_vblank = imx908_calc_max_vblank(imx908_active_area.height);
As you declare the bpp, link_freq and pixel_rate variables just above
the code that uses them, instead of at the beginning of the function, I
would do the same here:
u32 min_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height;
u32 max_vblank = imx908_calc_max_vblank(imx908_active_area.height);
Unless Sakari has a stricter rule about declaring all variables at the
beginning of the function ?
> +
> + imx->ctrls.vblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
> + V4L2_CID_VBLANK,
> + min_vblank,
> + max_vblank,
> + 1,
> + min_vblank);
> +
> + /* Set hblank */
> + min_hmax = imx908_calc_min_hmax(imx908_active_area.width, pixel_rate);
> + min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate,
> + imx908_active_area.width);
> + max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate,
> + imx908_active_area.width);
> + hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, pixel_rate,
> + imx908_active_area.width);
Same here.
u16 min_hmax = imx908_calc_min_hmax(imx908_active_area.width, pixel_rate);
u32 min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate,
imx908_active_area.width);
u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate,
imx908_active_area.width);
u32 hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, pixel_rate,
imx908_active_area.width);
> +
> + imx->ctrls.hblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
> + V4L2_CID_HBLANK,
> + min_hblank,
> + max_hblank,
> + 1,
> + hblank);
> + max_exp = imx->mode.vmax - IMX908_MIN_SHR0;
u32 max_exp = imx->mode.vmax - IMX908_MIN_SHR0;
or better
u32 max_exp = IMX908_VMAX_DEFAULT - IMX908_MIN_SHR0;
as that's more explicit.
> +
> + imx->ctrls.exposure = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + IMX908_EXPOSURE_MIN,
> + max_exp,
> + IMX908_EXPOSURE_STEP,
> + max_exp / 2);
> +
> + v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> + IMX908_ANA_GAIN_MIN, IMX908_ANA_GAIN_MAX,
> + IMX908_ANA_GAIN_STEP, IMX908_ANA_GAIN_DEFAULT);
> +
> + /* Set test pattern. Menu (13 entries: Disabled + 12 patterns) */
> + imx->ctrls.test_pattern = v4l2_ctrl_new_std_menu_items(hdl,
> + &imx908_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(imx908_tpg_menu) - 1,
> + 0,
> + 0,
> + imx908_tpg_menu);
> + if (imx->ctrls.test_pattern)
> + imx->ctrls.test_pattern->flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +
> + v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> + /* Pick up rotation and orientation if defined in overlay */
> + ret = v4l2_fwnode_device_parse(imx->dev, &props);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx908_ctrl_ops, &props);
> + if (ret)
> + return ret;
You need to call v4l2_ctrl_handler_free() in the error path here.
Better, just drop error checking. The function sets hdl->error upon
failure, so the error check just below is enough.
> +
> + if (hdl->error) {
> + ret = hdl->error;
> + v4l2_ctrl_handler_free(hdl);
> + return ret;
> + }
> +
> + imx->sd.ctrl_handler = hdl;
> +
> + return 0;
> +}
> +
> +/* Confirm the ID of the HW device */
> +static int imx908_identify_model(struct imx908 *imx)
> +{
> + int ret;
> + int err;
> + u64 val;
> + u16 chip_id;
> +
> + /*
> + * The TYPE_ID registers are not accessible after power-up while
> + * the device remains in standby. Exit standby and wait for the
> + * required stabilization period before reading the chip ID.
> + */
> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, NULL);
> + if (ret)
> + return ret;
> +
> + msleep(24); /* Regulator stabilization after standby cancel. */
> +
> + ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, &err);
> + if (ret || err)
> + return ret ? ret : err;
You don't need to check both ret and err. The err pointer to cci_read()
(and cci_write()) is meant to chain calls with error checking at the
end. Just check ret and pass NULL as the last argument to cci_read().
> +
> + chip_id = val;
> + dev_info(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id);
This should be a dev_dbg() message, drivers should be silent at probe
time when everything goes fine to avoid slowing down the boot process.
> +
> + if (chip_id != IMX908_CHIP_ID) {
> + dev_err(imx->dev, "Unexpected chip ID 0x%04x (expected 0x%04x)\n",
> + chip_id, IMX908_CHIP_ID);
> + return -ENXIO;
> + }
> +
> + /* Set to standby mode */
> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL);
> + if (ret)
> + dev_err(imx->dev, "failed to enter standby state: %d\n", ret);
This error shouldn't be ignored:
return ret;
}
> +
> + return 0;
> +}
> +
> +static int imx908_probe(struct i2c_client *client)
> +{
> + struct imx908 *imx;
> + int ret;
> +
> + /* Allocate Memory */
> + imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
> + if (!imx)
> + return dev_err_probe(&client->dev, -ENOMEM,
> + "failed to allocate IMX908 device structure\n");
I would simply
return -ENOMEM;
here. kzalloc() failures are logged by the allocator.
> + imx->dev = &client->dev;
> +
> + /* Initialize V4L2 subdevice */
> + v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
> + imx->sd.internal_ops = &imx908_internal_ops;
> +
> + /* Register access initialization. Set 2-byte (16-bit) addresses */
> + imx->cci = devm_cci_regmap_init_i2c(client, 16);
> + if (IS_ERR(imx->cci))
> + return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
> + "CCI regmap init failed\n");
> +
> + /* Get mandatory input clock from DT (INCK) */
> + imx->xclk = devm_clk_get(imx->dev, "xclk");
The DT bindings don't specify a clock name, use NULL instead of "xclk".
> + if (IS_ERR(imx->xclk))
> + return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), "xclk\n");
That's a bit short as an error message.
return dev_err_probe(imx->dev, PTR_ERR(imx->xclk),
"failed to get clock\n");
> +
> + /* Get clock frequency and check against acceptable HW values */
> + imx->xclk_freq = clk_get_rate(imx->xclk);
imx->xclk_freq is used in imx908_get_inck_sel() only. You can drop the
field from the structure, and pass it as an argument to the function.
> + ret = imx908_get_inck_sel(imx);
> + if (ret)
> + return ret;
> +
> + /* GPIO reset acquisition */
> + imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
> + GPIOD_OUT_HIGH);
> +
> + if (IS_ERR(imx->reset_gpio))
> + return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
> + "reset gpio\n");
Same here, "failed to get reset GPIO\n".
> +
> + /* Link to power supplies */
> + ret = imx908_get_regulators(imx);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "regulator get failed\n");
And for consistency, "failed to get regulators\n". Same comment for the
messages below.
> +
> + /* Parse Device Tree endpoint */
> + ret = imx908_parse_fwnode(imx);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "device tree parse failed\n");
> +
> + /* Power on IMX908 image sensor */
> + ret = imx908_power_on(imx);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "power-on failed\n");
> +
> + /* Read IMX908 device ID */
> + ret = imx908_identify_model(imx);
> + if (ret) {
> + dev_err(imx->dev, "failed to identify model: %d\n", ret);
> + goto err_power_off;
> + }
> +
> + pm_runtime_set_active(imx->dev);
> + pm_runtime_enable(imx->dev);
> +
> + ret = imx908_init_controls(imx);
> + if (ret)
> + goto err_pm_disable;
> +
> + /* Initialize V4L2 subdevice, media entity, and pad */
> + imx->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> + imx->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + imx->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&imx->sd.entity, 1, &imx->pad);
> + if (ret)
> + goto err_hdl;
> +
> + /* Use ctrl handler mutex for centrally managed subdev state */
> + imx->sd.state_lock = imx->ctrls.handler.lock;
> +
> + ret = v4l2_subdev_init_finalize(&imx->sd);
> + if (ret)
> + goto err_entity;
> +
> + pm_runtime_set_autosuspend_delay(imx->dev, 1000);
> + pm_runtime_use_autosuspend(imx->dev);
> + pm_runtime_mark_last_busy(imx->dev);
> +
> + ret = v4l2_async_register_subdev_sensor(&imx->sd);
> + if (ret)
> + goto err_pm_disable;
> +
> + return 0;
> +
> +err_pm_disable:
> + pm_runtime_disable(imx->dev);
> + pm_runtime_set_suspended(imx->dev);
> + v4l2_subdev_cleanup(&imx->sd);
> +err_entity:
> + media_entity_cleanup(&imx->sd.entity);
> +err_hdl:
> + v4l2_ctrl_handler_free(imx->sd.ctrl_handler);
> +err_power_off:
> + imx908_power_off(imx);
> +
> + return ret;
> +}
> +
> +static void imx908_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct imx908 *imx = to_imx908(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(sd->ctrl_handler);
> +
> + pm_runtime_disable(imx->dev);
> + if (!pm_runtime_status_suspended(imx->dev))
> + imx908_power_off(imx);
> + pm_runtime_set_suspended(imx->dev);
> +}
> +
> +static const struct of_device_id imx908_of_ids[] = {
> + { .compatible = "sony,imx908" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx908_of_ids);
> +
> +static struct i2c_driver imx908_i2c_driver = {
> + .driver = {
> + .name = "imx908",
> + .pm = pm_ptr(&imx908_pm_ops),
> + .of_match_table = imx908_of_ids,
> + },
> + .probe = imx908_probe,
> + .remove = imx908_remove,
> +};
> +module_i2c_driver(imx908_i2c_driver);
> +
> +MODULE_AUTHOR("Lachlan Michael <Lachlan.Michael@sony.com>");
> +MODULE_DESCRIPTION("Sony IMX908 Sensor Driver");
> +MODULE_LICENSE("GPL");
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
2026-08-02 23:03 ` Laurent Pinchart
@ 2026-08-03 6:16 ` Krzysztof Kozlowski
2026-08-03 19:18 ` Laurent Pinchart
2026-08-05 9:15 ` Lachlan.Michael
0 siblings, 2 replies; 16+ messages in thread
From: Krzysztof Kozlowski @ 2026-08-03 6:16 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Lachlan Michael, mchehab, sakari.ailus, hverkuil+cisco,
linux-media, devicetree, robh, krzk+dt, conor+dt, kieran.bingham,
jai.luthra, Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird,
Kazumi.A.Sato, linux-kernel
On 03/08/2026 01:03, Laurent Pinchart wrote:
> On Thu, Jul 30, 2026 at 11:06:09AM +0200, Krzysztof Kozlowski wrote:
>> On Thu, Jul 30, 2026 at 11:15:24AM +0900, Lachlan Michael wrote:
>>> The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor
>>> with a MIPI CSI-2 output interface.
>>>
>>> Add device tree bindings for the sensor.
>>>
>>> Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
>>
>> You need to test code before you send it, not with our infra.
>
> I have provided guidance to Lachlan on how to test DT bindings (and have
> submitted a fix to dt-schema as part of that work) before he submitted
> this patch series. Unfortunately, we missed an issue in the
> configuration of his development environment which resulted in the DT
> bindings check not being run. Mistakes happen, it will work better next
> time :-)
>
>> To spare you one sending cycle:
>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23
>>
>>> ---
>>> .../bindings/media/i2c/sony,imx908.yaml | 107 ++++++++++++++++++
>>> MAINTAINERS | 7 ++
>>> 2 files changed, 114 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
>>
>> ...
>>
>>> +++ b/MAINTAINERS
>>> @@ -25264,6 +25264,13 @@ T: git git://linuxtv.org/media.git
>>> F: Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
>>> F: drivers/media/i2c/imx415.c
>>>
>>> +SONY IMX908 SENSOR DRIVER
>>> +M: Lachlan Michael <lachlan.michael@sony.com>
>>> +L: linux-media@vger.kernel.org
>>
>> Drop
>
> Why so ?
It's just redundant and bloat. The address will ALWAYS appear through
the subsystem maintainers entry, thus duplicating exactly the same list
is not necessary. Many drivers make the same mistake (I fixed Samsung
now) resulting in quite big amount of redundant data.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-08-03 0:53 ` Laurent Pinchart
@ 2026-08-03 17:37 ` Dave Stevenson
2026-08-03 19:12 ` Laurent Pinchart
2026-08-04 1:00 ` Lachlan.Michael
2026-08-05 10:08 ` Lachlan.Michael
1 sibling, 2 replies; 16+ messages in thread
From: Dave Stevenson @ 2026-08-03 17:37 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Lachlan Michael, mchehab, sakari.ailus, hverkuil+cisco,
linux-media, devicetree, robh, krzk+dt, conor+dt, kieran.bingham,
jai.luthra, Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird,
Kazumi.A.Sato, linux-kernel
Hi Lachlan and Laurent.
It's brilliant that Sony are getting involved in upstreaming drivers!
<snip>
>
> In his IMX678 driver, Jai hardcodes the pixel rate to 594MHz, which is
> 8 * 74.25MHz (IMX908_XHS_HZ). The rationale is that HMAX is expressed in
> units of the 74.25 MHz internal clock, so we assumed that the pixel
> array is not read out using the link clock but using a clock derived
> from the internal clock. The multiplier was chosen to ensure that the
> minimum HMAX value corresponds to a positive HBLANK, as negative HBLANK
> would confuse userspace (and developers). Having a fixed pixel rate
> greatly simplifies the driver, as you don't have to update the pixel
> rate control, and HMAX calculation from HBLANK becomes a division by 8.
>
> We're not sure how the pixel array of the IMX678 is clocked exactly, so
> I don't know if the above corresponds to reality, neither for the IMX678
> nor the IMX908. If you could provide more information about the pixel
> array readout, that would be greatly appreciated. Note that the pixel
> rate value does not have to match the physical readout clock, as it is
> only used by userspace to calculate timings (to convert between h/v
> blank and fps).
>
> Jai can provide more information if needed.
In case you'd missed it, I'm trying to convert Jai's IMX678 driver
into a generic Starvis 2 driver[1].
IMX662 that I'm interested in is streaming with a total of 7 changes
to defines or const structs in the current imx678 driver.
IMX675 (5MPix Starvis2) frames with a similar number of changes but
currently isn't giving me images (test patterns work).
IMX585 looks like it may fit the same pattern, but I'm waiting on a
module for that one.
I'm therefore looking to parameterise those diffs but otherwise keep
it all common. This avoids the hassle of having to convert potentially
4 (or more) drivers to the Common Raw Sensor Model once it is
finalised.
IMX908 here looks to very much follow the same pattern, so unless
there are additional diffs between Starvis 2 and Starvis 3 it would
potentially fit into the same framework.
I'm hoping to get an RFC out this week as I'm then on holiday for a
week. It'll be interesting to see if IMX908 will fit with the same
pattern.
Lachlan: Are there any particular people we should contact for details
of IMX908, or just go through our normal contact?
Thanks
Dave
[1] https://lore.kernel.org/linux-media/20260312150437.1091195-1-eagle.alexander923@gmail.com/T/#m6072f85b03811932427597d8ffe5da008d33246a
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-08-03 17:37 ` Dave Stevenson
@ 2026-08-03 19:12 ` Laurent Pinchart
2026-08-04 1:00 ` Lachlan.Michael
1 sibling, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2026-08-03 19:12 UTC (permalink / raw)
To: Dave Stevenson
Cc: Lachlan Michael, mchehab, sakari.ailus, hverkuil+cisco,
linux-media, devicetree, robh, krzk+dt, conor+dt, kieran.bingham,
jai.luthra, Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird,
Kazumi.A.Sato, linux-kernel
Hi Dave,
On Mon, Aug 03, 2026 at 06:37:47PM +0100, Dave Stevenson wrote:
> Hi Lachlan and Laurent.
>
> It's brilliant that Sony are getting involved in upstreaming drivers!
>
> <snip>
> >
> > In his IMX678 driver, Jai hardcodes the pixel rate to 594MHz, which is
> > 8 * 74.25MHz (IMX908_XHS_HZ). The rationale is that HMAX is expressed in
> > units of the 74.25 MHz internal clock, so we assumed that the pixel
> > array is not read out using the link clock but using a clock derived
> > from the internal clock. The multiplier was chosen to ensure that the
> > minimum HMAX value corresponds to a positive HBLANK, as negative HBLANK
> > would confuse userspace (and developers). Having a fixed pixel rate
> > greatly simplifies the driver, as you don't have to update the pixel
> > rate control, and HMAX calculation from HBLANK becomes a division by 8.
> >
> > We're not sure how the pixel array of the IMX678 is clocked exactly, so
> > I don't know if the above corresponds to reality, neither for the IMX678
> > nor the IMX908. If you could provide more information about the pixel
> > array readout, that would be greatly appreciated. Note that the pixel
> > rate value does not have to match the physical readout clock, as it is
> > only used by userspace to calculate timings (to convert between h/v
> > blank and fps).
> >
> > Jai can provide more information if needed.
>
> In case you'd missed it, I'm trying to convert Jai's IMX678 driver
> into a generic Starvis 2 driver[1].
Just a quick message to tell you that I plan to reply to that mail
thread. I'm on vacation this week, so there will be a delay.
> IMX662 that I'm interested in is streaming with a total of 7 changes
> to defines or const structs in the current imx678 driver.
> IMX675 (5MPix Starvis2) frames with a similar number of changes but
> currently isn't giving me images (test patterns work).
> IMX585 looks like it may fit the same pattern, but I'm waiting on a
> module for that one.
>
> I'm therefore looking to parameterise those diffs but otherwise keep
> it all common. This avoids the hassle of having to convert potentially
> 4 (or more) drivers to the Common Raw Sensor Model once it is
> finalised.
>
> IMX908 here looks to very much follow the same pattern, so unless
> there are additional diffs between Starvis 2 and Starvis 3 it would
> potentially fit into the same framework.
>
> I'm hoping to get an RFC out this week as I'm then on holiday for a
> week. It'll be interesting to see if IMX908 will fit with the same
> pattern.
>
> Lachlan: Are there any particular people we should contact for details
> of IMX908, or just go through our normal contact?
>
> Thanks
> Dave
>
> [1] https://lore.kernel.org/linux-media/20260312150437.1091195-1-eagle.alexander923@gmail.com/T/#m6072f85b03811932427597d8ffe5da008d33246a
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
2026-08-03 6:16 ` Krzysztof Kozlowski
@ 2026-08-03 19:18 ` Laurent Pinchart
2026-08-05 9:15 ` Lachlan.Michael
1 sibling, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2026-08-03 19:18 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Lachlan Michael, mchehab, sakari.ailus, hverkuil+cisco,
linux-media, devicetree, robh, krzk+dt, conor+dt, kieran.bingham,
jai.luthra, Ryuichi.Tadano, Kengo.Hayasaka, Tim.Bird,
Kazumi.A.Sato, linux-kernel
On Mon, Aug 03, 2026 at 08:16:10AM +0200, Krzysztof Kozlowski wrote:
> On 03/08/2026 01:03, Laurent Pinchart wrote:
> > On Thu, Jul 30, 2026 at 11:06:09AM +0200, Krzysztof Kozlowski wrote:
> >> On Thu, Jul 30, 2026 at 11:15:24AM +0900, Lachlan Michael wrote:
> >>> The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor
> >>> with a MIPI CSI-2 output interface.
> >>>
> >>> Add device tree bindings for the sensor.
> >>>
> >>> Signed-off-by: Lachlan Michael <lachlan.michael@sony.com>
> >>
> >> You need to test code before you send it, not with our infra.
> >
> > I have provided guidance to Lachlan on how to test DT bindings (and have
> > submitted a fix to dt-schema as part of that work) before he submitted
> > this patch series. Unfortunately, we missed an issue in the
> > configuration of his development environment which resulted in the DT
> > bindings check not being run. Mistakes happen, it will work better next
> > time :-)
> >
> >> To spare you one sending cycle:
> >>
> >> A nit, subject: drop second/last, redundant "bindings". The
> >> "dt-bindings" prefix is already stating that these are bindings.
> >> See also:
> >> https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23
> >>
> >>> ---
> >>> .../bindings/media/i2c/sony,imx908.yaml | 107 ++++++++++++++++++
> >>> MAINTAINERS | 7 ++
> >>> 2 files changed, 114 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx908.yaml
> >>
> >> ...
> >>
> >>> +++ b/MAINTAINERS
> >>> @@ -25264,6 +25264,13 @@ T: git git://linuxtv.org/media.git
> >>> F: Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
> >>> F: drivers/media/i2c/imx415.c
> >>>
> >>> +SONY IMX908 SENSOR DRIVER
> >>> +M: Lachlan Michael <lachlan.michael@sony.com>
> >>> +L: linux-media@vger.kernel.org
> >>
> >> Drop
> >
> > Why so ?
>
> It's just redundant and bloat. The address will ALWAYS appear through
> the subsystem maintainers entry, thus duplicating exactly the same list
> is not necessary. Many drivers make the same mistake (I fixed Samsung
> now) resulting in quite big amount of redundant data.
Good point, thanks for the explanation. I'm fine with that. I'd like
Sakari's opinion though. If he disagrees, I'll let the two of you debate
this, and Lachlan can then drop or keep the L: line based on the
conclusion :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-08-03 17:37 ` Dave Stevenson
2026-08-03 19:12 ` Laurent Pinchart
@ 2026-08-04 1:00 ` Lachlan.Michael
1 sibling, 0 replies; 16+ messages in thread
From: Lachlan.Michael @ 2026-08-04 1:00 UTC (permalink / raw)
To: Dave Stevenson, laurent.pinchart@ideasonboard.com
Cc: mchehab@kernel.org, sakari.ailus@linux.intel.com,
hverkuil+cisco@kernel.org, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, kieran.bingham@ideasonboard.com,
jai.luthra@ideasonboard.com, Ryuichi.Tadano@sony.com,
Kengo.Hayasaka@sony.com, Bird, Tim, Kazumi.A.Sato@sony.com,
linux-kernel@vger.kernel.org
Dear Dave,
>In case you'd missed it, I'm trying to convert Jai's IMX678 driver
>into a generic Starvis 2 driver[1].
Thanks for looping me in.
>IMX662 that I'm interested in is streaming with a total of 7 changes
>to defines or const structs in the current imx678 driver.
>IMX675 (5MPix Starvis2) frames with a similar number of changes but
>currently isn't giving me images (test patterns work).
>IMX585 looks like it may fit the same pattern, but I'm waiting on a
>module for that one.
>
>I'm therefore looking to parameterise those diffs but otherwise keep
>it all common. This avoids the hassle of having to convert potentially
>4 (or more) drivers to the Common Raw Sensor Model once it is
>finalised.
We will be following your work and also plan to convert IMX908 to the
Common Raw Sensor Model at the appropriate time.
>IMX908 here looks to very much follow the same pattern, so unless
>there are additional diffs between Starvis 2 and Starvis 3 it would
>potentially fit into the same framework.
Currently the IMX908 driver is SDR only. We plan to add Clear HDR3 [1] support
in a future patch. Clear HDR3 does the image combining internally in the HW
and outputs one combined HDR image, reducing artifacts for moving objects.
>I'm hoping to get an RFC out this week as I'm then on holiday for a
>week. It'll be interesting to see if IMX908 will fit with the same
>pattern.
>
>Lachlan: Are there any particular people we should contact for details
>of IMX908, or just go through our normal contact?
For details of HW, please go through your normal contacts.
If anything to do with the driver, please contact me.
Best Regards,
Lachlan
[1] https://www.sony-semicon.com/en/products/is/security/security/IMX908.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings
2026-08-03 6:16 ` Krzysztof Kozlowski
2026-08-03 19:18 ` Laurent Pinchart
@ 2026-08-05 9:15 ` Lachlan.Michael
1 sibling, 0 replies; 16+ messages in thread
From: Lachlan.Michael @ 2026-08-05 9:15 UTC (permalink / raw)
To: Krzysztof Kozlowski, laurent.pinchart@ideasonboard.com
Cc: mchehab@kernel.org, sakari.ailus@linux.intel.com,
hverkuil+cisco@kernel.org, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, kieran.bingham@ideasonboard.com,
jai.luthra@ideasonboard.com, Ryuichi.Tadano@sony.com,
Kengo.Hayasaka@sony.com, Bird, Tim, Kazumi.A.Sato@sony.com,
linux-kernel@vger.kernel.org
Dear Kyrsztof,
Thank-you for your review.
<snip>
>>> To spare you one sending cycle:
>>>
>>> A nit, subject: drop second/last, redundant "bindings". The
>>> "dt-bindings" prefix is already stating that these are bindings.
Ok, will fix this in v2.
<snip>
>>>> +SONY IMX908 SENSOR DRIVER
>>>> +M: Lachlan Michael <lachlan.michael@sony.com>
>>>> +L: linux-media@vger.kernel.org
>>>
>>> Drop
>>
>> Why so ?
>
>It's just redundant and bloat. The address will ALWAYS appear through
>the subsystem maintainers entry, thus duplicating exactly the same list
>is not necessary. Many drivers make the same mistake (I fixed Samsung
>now) resulting in quite big amount of redundant data.
Ok, I will drop this line in v2.
Best Regards,
Lachlan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-07-30 9:09 ` Krzysztof Kozlowski
@ 2026-08-05 9:23 ` Lachlan.Michael
0 siblings, 0 replies; 16+ messages in thread
From: Lachlan.Michael @ 2026-08-05 9:23 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mchehab@kernel.org, sakari.ailus@linux.intel.com,
hverkuil+cisco@kernel.org, laurent.pinchart@ideasonboard.com,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
kieran.bingham@ideasonboard.com, jai.luthra@ideasonboard.com,
Ryuichi.Tadano@sony.com, Kengo.Hayasaka@sony.com, Bird, Tim,
Kazumi.A.Sato@sony.com, linux-kernel@vger.kernel.org
Dear Krzysztof,
Thank-you for your review.
<snip>
On Thu, Jul 30, 2026 at 11:15:25AM +0900, Lachlan Michael wrote:
>> + msleep(24); /* Regulator stabilization after standby cancel. */
>> +
>> + ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, &err);
>> + if (ret || err)
>> + return ret ? ret : err;
>> +
>> + chip_id = val;
>> + dev_info(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id);
>
>Drivers should be silent on success. Drop or dev_dbg.
I will dev_dbg in v2.
>> + if (chip_id != IMX908_CHIP_ID) {
>> + dev_err(imx->dev, "Unexpected chip ID 0x%04x (expected 0x%04x)\n",
>> + chip_id, IMX908_CHIP_ID);
>> + return -ENXIO;
>> + }
>> +
>> + /* Set to standby mode */
>> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL);
>> + if (ret)
>> + dev_err(imx->dev, "failed to enter standby state: %d\n", ret);
>> +
>> + return 0;
>> +}
>> +
>> +static int imx908_probe(struct i2c_client *client)
>> +{
>> + struct imx908 *imx;
>> + int ret;
>> +
>> + /* Allocate Memory */
>
>Really?
I will review all comments for the v2 submission and remove / reword them to be
more in line with the kernel style.
>> + imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
>> + if (!imx)
>> + return dev_err_probe(&client->dev, -ENOMEM,
>> + "failed to allocate IMX908 device structure\n");
>> + imx->dev = &client->dev;
>> +
>> + /* Initialize V4L2 subdevice */
>
>Obvious.
Will remove in v2.
>> + v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
>> + imx->sd.internal_ops = &imx908_internal_ops;
>> +
>> + /* Register access initialization. Set 2-byte (16-bit) addresses */
>> + imx->cci = devm_cci_regmap_init_i2c(client, 16);
>> + if (IS_ERR(imx->cci))
>> + return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
>> + "CCI regmap init failed\n");
>> +
>> + /* Get mandatory input clock from DT (INCK) */
>
>Obvious.
Will remove in v2.
>> + imx->xclk = devm_clk_get(imx->dev, "xclk");
>> + if (IS_ERR(imx->xclk))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), "xclk\n");
>> +
>> + /* Get clock frequency and check against acceptable HW values */
>> + imx->xclk_freq = clk_get_rate(imx->xclk);
>> + ret = imx908_get_inck_sel(imx);
>> + if (ret)
>> + return ret;
>> +
>> + /* GPIO reset acquisition */
>
>Please drop obvious comments. Can devm_gpiod_get_optional() be anything
>else than GPIO reset acquisition? No.
>
>Redundant comments bloat the code and make it more difficult to actually
>spot important things.
Understood.
>> + imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
>> + GPIOD_OUT_HIGH);
>> +
>> + if (IS_ERR(imx->reset_gpio))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
>> + "reset gpio\n");
>> +
>> + /* Link to power supplies */
>> + ret = imx908_get_regulators(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "regulator get failed\n");
>> +
>> + /* Parse Device Tree endpoint */
>
>Obvious
:-)
>> + ret = imx908_parse_fwnode(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "device tree parse failed\n");
>> +
>> + /* Power on IMX908 image sensor */
>
>What if imx908_power_on() does power off of the sensor?
Will remove these comments for v2.
>> + ret = imx908_power_on(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret, "power-on failed\n");
>> +
>> + /* Read IMX908 device ID */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver
2026-08-03 0:53 ` Laurent Pinchart
2026-08-03 17:37 ` Dave Stevenson
@ 2026-08-05 10:08 ` Lachlan.Michael
1 sibling, 0 replies; 16+ messages in thread
From: Lachlan.Michael @ 2026-08-05 10:08 UTC (permalink / raw)
To: laurent.pinchart@ideasonboard.com
Cc: mchehab@kernel.org, sakari.ailus@linux.intel.com,
hverkuil+cisco@kernel.org, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, kieran.bingham@ideasonboard.com,
jai.luthra@ideasonboard.com, Ryuichi.Tadano@sony.com,
Kengo.Hayasaka@sony.com, Bird, Tim, Kazumi.A.Sato@sony.com,
linux-kernel@vger.kernel.org
Dear Laurent,
Thank-you for your review.
<snip>
>> +#define IMX908_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
>> +
>> +/* IMX908 total area includes effective area plus
>
>This should be
>
>/*
>* IMX908 total area includes effective area plus
>
>> + * 10 effective OB pixels in vertical direction,
>> + * 10 pixels OB side ignored area in height
>> + */
I will review the comments for alignment in v2.
<snip>
>> +struct imx908_mode {
>> + /* Sensor timing */
>> + u16 hmax; /* clocks per line */
>> + u32 vmax; /* lines per frame */
>> +
>> + /* Derived values */
>> + u64 pixel_rate;
>> +};
>
>This structure should be dropped. It will be easier to do with a
>hardcoded pixel_rate value (see below), but should be done even if we
>decide to keep the pixel rate dynamic. I can give it a try based on v2
>of the driver if you're having trouble.
In v2 I have
- hardcoded the pixel_rate
- dropped the pixel_rate from the structure
- moved hmax and vmax to the main struct imx908 and deleted struct imx908_mode
<snip>
>> +/* Compute pixel-rate from link-freq/lanes/bpp */
>> +static inline u64 imx908_pixel_rate(u8 lanes, u8 bpp, u64 linkfreq_hz)
>> +{
>> + u64 total = lanes * linkfreq_hz * 2; /* DDR: 2 * linkfreq */
>> +
>> + return div_u64(total, bpp);
>
>In his IMX678 driver, Jai hardcodes the pixel rate to 594MHz, which is
>8 * 74.25MHz (IMX908_XHS_HZ). The rationale is that HMAX is expressed in
>units of the 74.25 MHz internal clock, so we assumed that the pixel
>array is not read out using the link clock but using a clock derived
>from the internal clock. The multiplier was chosen to ensure that the
>minimum HMAX value corresponds to a positive HBLANK, as negative HBLANK
>would confuse userspace (and developers). Having a fixed pixel rate
>greatly simplifies the driver, as you don't have to update the pixel
>rate control, and HMAX calculation from HBLANK becomes a division by 8.
>
>We're not sure how the pixel array of the IMX678 is clocked exactly, so
>I don't know if the above corresponds to reality, neither for the IMX678
>nor the IMX908. If you could provide more information about the pixel
>array readout, that would be greatly appreciated. Note that the pixel
>rate value does not have to match the physical readout clock, as it is
>only used by userspace to calculate timings (to convert between h/v
>blank and fps).
>
>Jai can provide more information if needed.
After a little bit of wavering in v2 I have hardcoded the pixel rate as per IMX678.
In parallel I have tried to get more information from the engineers internally,
however this is still on-going.
I have also tweaked the constraints on the minimum HMAX, you can see this
in v2.
>> +static int imx908_link_freq_to_datarate_sel(struct imx908 *imx, u64 link_freq_hz, u8 *sel)
>> +{
>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_link_freqs); i++) {
>> + if (imx908_link_freqs[i] == link_freq_hz) {
>> + *sel = imx908_datarate_sel[i];
>> + return 0;
>> + }
>> + }
>
>A blank line would be nice here.
Ok
>> + dev_err(imx->dev, "Unsupported link frequency %llu Hz\n", link_freq_hz);
>
>The caller prints a similar error message, you can drop this one.
Ok.
>> + return -EINVAL;
>> +}
>> +
<snip>
>> +
>> +static void imx908_update_framing_limits(struct imx908 *imx, struct v4l2_subdev_state *state)
>
>Please wrap the line.
Ok.
<snip>
>> + if (ret) {
>> + dev_err(imx->dev, "Mode register write failed: %d\n", ret);
>> + return ret;
>> + }
>
>You can drop the error check here.
Removed in v2.
> > + for (unsigned int i = 0; i < ARRAY_SIZE(tuning_regs); ++i)
> > + cci_write(imx->cci, tuning_regs[i], value, &ret);
> > +
> > + if (ret) {
> > + dev_err(imx->dev, "Tuning register write failed: %d\n", ret);
> > + return ret;
> > + }
>
> And this one too.
Removed in v2.
<snip>
>> + ret = imx908_link_freq_to_datarate_sel(imx, link_freq, &datarate_sel);
>> + if (ret) {
>> + dev_err(imx->dev, "Unsupported link_freq=%llu for DATARATE_SEL\n",
>> + link_freq);
>> + return ret;
>> + }
>
>The link frequency is validated at probe time, so the
>imx908_link_freq_to_datarate_sel() can never return an error. Drop error
>checking here too.
Ok, understood.
>> + cci_write(imx->cci, IMX908_REG_DATARATE_SEL, datarate_sel, &ret);
>> +
>> + /* Lane config */
>> + cci_write(imx->cci, IMX908_REG_LANEMODE, imx->num_lanes - 1, &ret);
>> + if (ret) {
>> + dev_err(imx->dev, "Clock/lane configuration failed: %d\n", ret);
>> + return ret;
>> + }
>
>Drop this error check here too.
Will remove in v2.
>> +
>> + /* Recommended black level offset is 50 in 10-bit, 200 for others */
>> + u16 blklevel = (mdbit == IMX908_MDBIT_RAW10) ? 50 : 200;
>> +
>> + cci_write(imx->cci, IMX908_REG_BLKLEVEL, blklevel, &ret);
>> +
>> + return ret;
>
>The error is propagated through the chained cci_write() calls and
>handled here. That's all you need, cci_write() will print a message on
>failure, with the register address and value.
Understood, will fix this section in v2.
<snip>
>> + /* Get active state */
>> + sd_state = v4l2_subdev_get_locked_active_state(&imx->sd);
>
>The caller already has the active state, pass it as an argument to the
>imx908_start_streaming() function.
Ok.
<snip>
>> +/* --------------------------- V4L2 controls ------------------------------ */
>> +
>> +static int imx908_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct imx908 *imx = container_of(ctrl->handler, struct imx908, ctrls.handler);
>> + struct v4l2_subdev_state *state;
>> + struct i2c_client *client = v4l2_get_subdevdata(&imx->sd);
>> + const struct v4l2_mbus_framefmt *format;
>> + int ret = 0;
>> +
>> + /* Retrieve the active, locked subdev state format dimensions */
>> + state = v4l2_subdev_get_locked_active_state(&imx->sd);
>> + format = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD);
>> +
>> + /* Applying V4L2 controls only if powered up */
>> + if (!pm_runtime_get_if_in_use(&client->dev))
>
> if (!pm_runtime_get_if_in_use(imx->dev))
>
>and similarly below, and drop the client local variable.
Understood.
>> + return 0;
>> +
>> + switch (ctrl->id) {
>> + case V4L2_CID_EXPOSURE: {
>> + u32 lines = clamp_t(u32, ctrl->val, 1,
>> + imx->mode.vmax - IMX908_MIN_SHR0);
>
>Drop this, the control framework already clamps to the limits.
Fixed in v2.
>> + ret = imx908_set_exposure_lines(imx, lines);
>> + break;
>> + }
>> +
>> + case V4L2_CID_ANALOGUE_GAIN:
>> +
>
>Drop this blank line.
Ok.
>> + u32 reg = clamp_t(u32, ctrl->val, IMX908_ANA_GAIN_MIN,
>> + IMX908_ANA_GAIN_MAX);
>
>Drop this, the control framework already clamps to the limits.
Fixed in v2.
>> +
>> + cci_write(imx->cci, IMX908_REG_GAIN, reg, &ret);
>> + break;
>> +
>> + case V4L2_CID_VBLANK: {
>> + const struct v4l2_rect *crop;
>> +
>> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
>> +
>> + u32 min_vblank = imx908_calc_min_vblank(crop);
>> + u32 max_vblank = imx908_calc_max_vblank(format->height);
>> + u32 vblank = clamp_t(u32, ctrl->val, min_vblank, max_vblank);
>> +
>> + imx->mode.vmax = imx908_calc_vmax(format->height, vblank);
>> +
>> + u32 exposure = min_t(u32, imx->ctrls.exposure->val,
>> + imx->mode.vmax - IMX908_MIN_SHR0);
>> +
>> + __v4l2_ctrl_modify_range(imx->ctrls.exposure,
>> + IMX908_EXPOSURE_MIN,
>> + imx->mode.vmax - IMX908_MIN_SHR0,
>> + IMX908_EXPOSURE_STEP, exposure);
>
>All the the above should go before the pm_runtime_get_if_in_use() call,
>as the exposure control limits needs to be updated even if the sensor
>isn't streaming. You can add a
>
> if (ctrl->id == V4L2_CID_VBLANK) {
> ...
> }
>
>block there.
This has been modified per your suggestions in v2.
<snip>
>> + ret = imx908_start_streaming(imx);
>> + if (ret) {
>> + pm_runtime_mark_last_busy(imx->dev);
>> + pm_runtime_put_autosuspend(imx->dev);
>
>I would add
>
> return ret;
>
>> + }
>> +
>> + return ret;
>
>and here write
>
> return 0;
>
>to clearly isolate the error path from the success path.
>
>That's a personal preference though, up to you.
I will follow your suggestion.
<snip>
>> + ret = imx908_set_selection(sd, sd_state, &sel);
>> + ret = imx908_set_pad_format(sd, sd_state, &fmt);
>
>Those two functions return errors only when called with invalid
>arguments. Invalid .which, .pad or .target values in sel or fmt would be
>driver bugs, so I think it's safe to not check for errors. You shouldn't
>assign ret, and you can
>
> return 0;
Ok.
<snip>
>> + ret = regulator_bulk_enable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
>> + if (ret) {
>> + dev_err(imx->dev, "%s: failed to enable regulators\n", __func__);
>
>You can drop the "%s: " prefix, the error message is clear enough and
>doesn't need the function name.
Fixed in v2.
<snip>
>> + return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n", imx->xclk_freq);
>
> return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n",
> imx->xclk_freq);
I have updated the driver in v2 with stricter 80 column lin-breaking.
<snip>
>> +static int imx908_init_controls(struct imx908 *imx)
>> +{
>> + struct v4l2_ctrl_handler *hdl = &imx->ctrls.handler;
>> + struct v4l2_ctrl *link_freq_ctl;
>> + struct v4l2_fwnode_device_properties props;
>> + int ret;
>> + s32 hblank, min_hblank;
>> + u32 max_exp, max_hblank, min_vblank, max_vblank;
>> + u16 min_hmax;
>
>We tend to follow a "reverse christmas tree" order for variable
>declaration (it's one of those coding style rules that aim at increasing
>consistency but are otherwise completely arbitrary).
>
> u32 max_exp, max_hblank, min_vblank, max_vblank;
> s32 hblank, min_hblank;
> u16 min_hmax;
> int ret;
>
>I think hblank and min_hblank can be u32, as they're calculated below
>with imx908_hmax_to_hblank() that returns a u32.
Understood about the indenting and fixed the assignment for these variables.
>> +
>> + ret = v4l2_ctrl_handler_init(hdl, 11);
>> + if (ret)
>> + return ret;
>> +
>> + /* Initialize the pixel_rate control */
>> + u8 bpp = imx908_bits_per_pixel(IMX908_DEFAULT_MBUS_CODE);
>> + u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
>> + u64 pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
>> +
>> + imx->ctrls.pixel_rate = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_PIXEL_RATE,
>> + pixel_rate, pixel_rate, 1, pixel_rate);
>> + if (imx->ctrls.pixel_rate)
>> + imx->ctrls.pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + /* Set link Frequency */
>> + link_freq_ctl = v4l2_ctrl_new_int_menu(hdl, &imx908_ctrl_ops,
>> + V4L2_CID_LINK_FREQ,
>> + ARRAY_SIZE(imx908_link_freqs) - 1,
>> + imx->link_freq_idx,
>> + imx908_link_freqs);
>> +
>> + if (link_freq_ctl)
>> + link_freq_ctl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + /* Initialize HMAX and VMAX */
>> + imx->mode.hmax = IMX908_HMAX_DEFAULT;
>> + imx->mode.vmax = IMX908_VMAX_DEFAULT;
>> +
>> + /* Set vblank */
>> + min_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height;
>> + max_vblank = imx908_calc_max_vblank(imx908_active_area.height);
>
>As you declare the bpp, link_freq and pixel_rate variables just above
>the code that uses them, instead of at the beginning of the function, I
>would do the same here:
>
> u32 min_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height;
> u32 max_vblank = imx908_calc_max_vblank(imx908_active_area.height);
>
>Unless Sakari has a stricter rule about declaring all variables at the
>beginning of the function ?
In v2 I have mostly tried to go with the inline declarations but if it is better
at the beginning of the function please comment again when I submit v2.
>> +
>> + imx->ctrls.vblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
>> + V4L2_CID_VBLANK,
>> + min_vblank,
>> + max_vblank,
>> + 1,
>> + min_vblank);
>> +
>> + /* Set hblank */
>> + min_hmax = imx908_calc_min_hmax(imx908_active_area.width, pixel_rate);
>> + min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate,
>> + imx908_active_area.width);
>> + max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate,
>> + imx908_active_area.width);
>> + hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, pixel_rate,
>> + imx908_active_area.width);
>
>Same here.
>
> u16 min_hmax = imx908_calc_min_hmax(imx908_active_area.width, pixel_rate);
> u32 min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate,
> imx908_active_area.width);
> u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate,
> imx908_active_area.width);
> u32 hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, pixel_rate,
> imx908_active_area.width);
Ok.
>> +
>> + imx->ctrls.hblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
>> + V4L2_CID_HBLANK,
>> + min_hblank,
>> + max_hblank,
>> + 1,
>> + hblank);
>> + max_exp = imx->mode.vmax - IMX908_MIN_SHR0;
>
> u32 max_exp = imx->mode.vmax - IMX908_MIN_SHR0;
>
>or better
>
> u32 max_exp = IMX908_VMAX_DEFAULT - IMX908_MIN_SHR0;
>
>as that's more explicit.
Modified in v2.
<snip>
>> + /* Pick up rotation and orientation if defined in overlay */
>> + ret = v4l2_fwnode_device_parse(imx->dev, &props);
>> + if (ret)
>> + return ret;
>> +
>> + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx908_ctrl_ops, &props);
>> + if (ret)
>> + return ret;
>
>You need to call v4l2_ctrl_handler_free() in the error path here.
>Better, just drop error checking. The function sets hdl->error upon
>failure, so the error check just below is enough.
Ok.
>> +
>> + if (hdl->error) {
>> + ret = hdl->error;
>> + v4l2_ctrl_handler_free(hdl);
>> + return ret;
>> + }
>> +
>> + imx->sd.ctrl_handler = hdl;
>> +
>> + return 0;
>> +}
>> +
>> +/* Confirm the ID of the HW device */
>> +static int imx908_identify_model(struct imx908 *imx)
>> +{
>> + int ret;
>> + int err;
>> + u64 val;
>> + u16 chip_id;
>> +
>> + /*
>> + * The TYPE_ID registers are not accessible after power-up while
>> + * the device remains in standby. Exit standby and wait for the
>> + * required stabilization period before reading the chip ID.
>> + */
>> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + msleep(24); /* Regulator stabilization after standby cancel. */
>> +
>> + ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, &err);
>> + if (ret || err)
> + return ret ? ret : err;
>
>You don't need to check both ret and err. The err pointer to cci_read()
>(and cci_write()) is meant to chain calls with error checking at the
>end. Just check ret and pass NULL as the last argument to cci_read().
Understood.
>> +
>> + chip_id = val;
>> + dev_info(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id);
>
>This should be a dev_dbg() message, drivers should be silent at probe
>time when everything goes fine to avoid slowing down the boot process.
It will be dev_dbg in v2.
>> +
>> + if (chip_id != IMX908_CHIP_ID) {
>> + dev_err(imx->dev, "Unexpected chip ID 0x%04x (expected 0x%04x)\n",
>> + chip_id, IMX908_CHIP_ID);
>> + return -ENXIO;
>> + }
>> +
>> + /* Set to standby mode */
>> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL);
>> + if (ret)
>> + dev_err(imx->dev, "failed to enter standby state: %d\n", ret);
>
>This error shouldn't be ignored:
>
> return ret;
> }
Ok.
>> +
>> + return 0;
>> +}
>> +
>> +static int imx908_probe(struct i2c_client *client)
>> +{
>> + struct imx908 *imx;
>> + int ret;
>> +
>> + /* Allocate Memory */
>> + imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
>> + if (!imx)
>> + return dev_err_probe(&client->dev, -ENOMEM,
>> + "failed to allocate IMX908 device structure\n");
>
>I would simply
>
> return -ENOMEM;
>
>here. kzalloc() failures are logged by the allocator.
Fixed per your suggestion in v2.
>> + imx->dev = &client->dev;
>> +
>> + /* Initialize V4L2 subdevice */
>> + v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
>> + imx->sd.internal_ops = &imx908_internal_ops;
>> +
>> + /* Register access initialization. Set 2-byte (16-bit) addresses */
>> + imx->cci = devm_cci_regmap_init_i2c(client, 16);
>> + if (IS_ERR(imx->cci))
>> + return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
>> + "CCI regmap init failed\n");
>> +
>> + /* Get mandatory input clock from DT (INCK) */
>> + imx->xclk = devm_clk_get(imx->dev, "xclk");
>
>The DT bindings don't specify a clock name, use NULL instead of "xclk".
Ok.
>> + if (IS_ERR(imx->xclk))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), "xclk\n");
>
>That's a bit short as an error message.
>
> return dev_err_probe(imx->dev, PTR_ERR(imx->xclk),
> "failed to get clock\n");
I'm mostly too verbose but good to see I can swing both ways :-)
>> +
>> + /* Get clock frequency and check against acceptable HW values */
>> + imx->xclk_freq = clk_get_rate(imx->xclk);
>
>imx->xclk_freq is used in imx908_get_inck_sel() only. You can drop the
>field from the structure, and pass it as an argument to the function.
Done in v2.
>> + ret = imx908_get_inck_sel(imx);
>> + if (ret)
>> + return ret;
>> +
>> + /* GPIO reset acquisition */
>> + imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
>> + GPIOD_OUT_HIGH);
>> +
>> + if (IS_ERR(imx->reset_gpio))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
>> + "reset gpio\n");
>
>Same here, "failed to get reset GPIO\n".
Ok.
>> +
>> + /* Link to power supplies */
>> + ret = imx908_get_regulators(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "regulator get failed\n");
>
>And for consistency, "failed to get regulators\n". Same comment for the
>messages below.
Understood.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-08-05 10:08 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-30 2:15 [PATCH 0/2] Add bindings and driver for Sony IMX908 Lachlan Michael
2026-07-30 2:15 ` [PATCH 1/2] media: dt-bindings: imx908: Add Sony IMX908 sensor bindings Lachlan Michael
2026-07-30 3:24 ` Rob Herring (Arm)
2026-07-30 9:06 ` Krzysztof Kozlowski
2026-08-02 23:03 ` Laurent Pinchart
2026-08-03 6:16 ` Krzysztof Kozlowski
2026-08-03 19:18 ` Laurent Pinchart
2026-08-05 9:15 ` Lachlan.Michael
2026-07-30 2:15 ` [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver Lachlan Michael
2026-07-30 9:09 ` Krzysztof Kozlowski
2026-08-05 9:23 ` Lachlan.Michael
2026-08-03 0:53 ` Laurent Pinchart
2026-08-03 17:37 ` Dave Stevenson
2026-08-03 19:12 ` Laurent Pinchart
2026-08-04 1:00 ` Lachlan.Michael
2026-08-05 10:08 ` Lachlan.Michael
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox