[PATCH v5 0/2] media: add Himax HM1246 image sensor

[PATCH v5 0/2] media: add Himax HM1246 image sensor

[Matthias Fend]

Hello,

this series adds support for the Himax HM1246 image sensor.
The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
array size of 1296 x 976.
Currently, only the native RAW mode is supported. Other modes and the
internal image signal processing pipeline are not currently supported.
The data sheet is available on the manufacturer's website [1].
Tested on i.MX8MP hardware. A Toshiba TC358746 bridge was used to convert
the sensor's parallel video output into MIPI signals for the i.MX8MP.

Best regards
 ~Matthias
 
[1] https://www.himax.com.tw/wp-content/uploads/2024/03/HM1246-AWD_DS_v01.pdf

v4l2-compliance 1.28.1, 64 bits, 64-bit time_t

Compliance test for device /dev/v4l-subdev4:

Driver Info:
        Driver version   : 6.12.0
        Capabilities     : 0x00000000
        Client Capabilities: 0x0000000000000003
streams interval-uses-which
Required ioctls:
        test VIDIOC_SUDBEV_QUERYCAP: OK
        test invalid ioctls: OK

Allow for multiple opens:
        test second /dev/v4l-subdev4 open: OK
        test VIDIOC_SUBDEV_QUERYCAP: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 15 Private Controls: 0

Format ioctls:
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK (Not Supported)
        test VIDIOC_TRY_FMT: OK (Not Supported)
        test VIDIOC_S_FMT: OK (Not Supported)
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls:
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
        test CREATE_BUFS maximum buffers: OK
        test VIDIOC_REMOVE_BUFS: OK
        test VIDIOC_EXPBUF: OK (Not Supported)
        test Requests: OK (Not Supported)

Total for device /dev/v4l-subdev4: 45, Succeeded: 45, Failed: 0, Warnings: 0

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
Changes in v5:
- Converted to lower case hexadecimals
- Use consistent returns in switch of hm1246_get_selection()
- Adjust some variable types/attributes
- Removed redundant parentheses
- Rework PLL calc to use goto
- Simplified some function returns
- Use array definition for test patterns
- Source format adjustments
- Properly init minimum of pixel_rate control
- dropped hm1246_update_controls()
- require and check DT link frequencies
- Link to v4: https://lore.kernel.org/r/20251017-hm1246-v4-0-e3388ea2f08c@emfend.at

Changes in v4:
- Split changes to MAINTAINERS into commits
- Fix comma after statement (use semicolon)
- Replace abs() with abs_diff() in PLL calculation
- Inverse needs_cmu_update logic
- Drop mode from hm1246_set_ctrl()
- Return if xclk frequency is out of range
- Fix reset_gpio dev_err_probe()
- Rebased on media-committers/next
- Link to v3: https://lore.kernel.org/r/20250912-hm1246-v3-0-3b89f47dfa43@emfend.at

Changes in v3:
- Bindings: Remove bus-type and add default polarity values
- Select V4L2_CCI_I2C
- Convert additional macros to use HZ_PER_*
- Replace cur_mode with v4l2_find_nearest_size()
- Remove duplicates in the register init sequence
- Use container_of_const
- Check return of hm1246_update_controls()
- Correct multi-line comments
- Replace hm1246_cci_write_cmu()
- Consistently use hm1246->dev
- Use pm_runtime_put_autosuspend()
- Remove v4l2 event handling
- Convert to devm_v4l2_sensor_clk_get()
- Configure PM before registering subdev
- Link to v2: https://lore.kernel.org/r/20250526-hm1246-v2-0-6b882827a3a5@emfend.at
- Depends-on: https://lore.kernel.org/all/20250707143253.167910-1-mehdi.djait@linux.intel.com/

Changes in v2:
- Use macros for 64-bit division
- Avoid compiler warnings about potentially uninitialized variables
- Fix two uses of dev_err_probe
- Link to v1: https://lore.kernel.org/r/20250403-hm1246-v1-0-30990d71bc42@emfend.at

---
Matthias Fend (2):
      media: dt-bindings: i2c: add Himax HM1246 image sensor
      media: i2c: add Himax HM1246 image sensor driver

 .../bindings/media/i2c/himax,hm1246.yaml           |  117 ++
 MAINTAINERS                                        |    8 +
 drivers/media/i2c/Kconfig                          |   10 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/hm1246.c                         | 1347 ++++++++++++++++++++
 5 files changed, 1483 insertions(+)
---
base-commit: ea299a2164262ff787c9d33f46049acccd120672
change-id: 20250403-hm1246-96b0cdab773c

Best regards,
-- 
Matthias Fend <matthias.fend@emfend.at>

[PATCH v5 1/2] media: dt-bindings: i2c: add Himax HM1246 image sensor

[Matthias Fend]

Add YAML device tree binding for Himax HM1246 image sensor.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 .../bindings/media/i2c/himax,hm1246.yaml           | 117 +++++++++++++++++++++
 MAINTAINERS                                        |   7 ++
 2 files changed, 124 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml b/Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..5ab748259294fc46d7e71e783cf36a64639154f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2025 Matthias Fend <matthias.fend@emfend.at>
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/himax,hm1246.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax HM1246-AWD 1/3.7-Inch megapixel SoC image sensor
+
+maintainers:
+  - Matthias Fend <matthias.fend@emfend.at>
+
+description:
+  The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
+  array size of 1296 x 976. It is programmable through an I2C interface and
+  connected via parallel bus.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: himax,hm1246
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: Input reference clock (6 - 27 MHz)
+    maxItems: 1
+
+  reset-gpios:
+    description: Active low XSHUTDOWN pin
+    maxItems: 1
+
+  avdd-supply:
+    description: Power for analog circuit (3.0 - 3.6 V)
+
+  iovdd-supply:
+    description: Power for I/O circuit (1.7 - 3.6 V)
+
+  dvdd-supply:
+    description: Power for digital circuit (1.5 / 1.8 V)
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+    description: Parallel video output port
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          hsync-active:
+            default: 1
+
+          vsync-active:
+            default: 1
+
+          pclk-sample:
+            default: 0
+
+        required:
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - avdd-supply
+  - iovdd-supply
+  - dvdd-supply
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/media/video-interfaces.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@24 {
+            compatible =  "himax,hm1246";
+            reg = <0x24>;
+
+            clocks = <&hm1246_clk>;
+
+            reset-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
+
+            avdd-supply = <&hm1246_avdd>;
+            iovdd-supply = <&hm1246_iovdd>;
+            dvdd-supply = <&hm1246_dvdd>;
+
+            orientation = <2>;
+            rotation = <0>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&isp_par_in>;
+                    bus-width = <10>;
+                    hsync-active = <1>; /* active high */
+                    vsync-active = <1>; /* active high */
+                    pclk-sample = <1>; /* sample on rising edge */
+                    link-frequencies = /bits/ 64 <42174000>;
+                };
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index f7351fced572eff0a18038095ec1724047890b55..e415ebb3204472ccc41cfdaa52af000b7cb15771 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11162,6 +11162,13 @@ L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	drivers/misc/hisi_hikey_usb.c
 
+HIMAX HM1246 SENSOR DRIVER
+M:	Matthias Fend <matthias.fend@emfend.at>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml
+
 HIMAX HX83112B TOUCHSCREEN SUPPORT
 M:	Job Noorman <job@noorman.info>
 L:	linux-input@vger.kernel.org

-- 
2.34.1

[PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Matthias Fend]

Add a V4L2 sub-device driver for Himax HM1246 image sensor.

The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
array size of 1296 x 976. It is programmable through an I2C interface and
connected via parallel bus.

The sensor has an internal ISP with a complete image processing pipeline
including control loops. However, this driver uses the sensor in raw mode
and the entire ISP is bypassed.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 MAINTAINERS                |    1 +
 drivers/media/i2c/Kconfig  |   10 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/hm1246.c | 1347 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1359 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e415ebb3204472ccc41cfdaa52af000b7cb15771..0d227aa6053237dabe5d1051a0e5fc289a20157a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11168,6 +11168,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml
+F:	drivers/media/i2c/hm1246.c
 
 HIMAX HX83112B TOUCHSCREEN SUPPORT
 M:	Job Noorman <job@noorman.info>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index cdd7ba5da0d5016ffc062919e3ec1f3b0a93ec1a..807e1d258d10567a698fd41deee042fda900a59f 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -137,6 +137,16 @@ config VIDEO_HI847
           To compile this driver as a module, choose M here: the
           module will be called hi847.
 
+config VIDEO_HM1246
+	tristate "Himax HM1246 sensor support"
+	select V4L2_CCI_I2C
+	help
+	  This is a Video4Linux2 sensor driver for the Himax
+	  HM1246 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called hm1246.
+
 config VIDEO_IMX208
 	tristate "Sony IMX208 sensor support"
 	help
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 57cdd8dc96f63794f8f1f32e32c13d9c6dc93fa0..2244c7a774a430ca2bea3e3a6556a08958f0a8de 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
 obj-$(CONFIG_VIDEO_HI556) += hi556.o
 obj-$(CONFIG_VIDEO_HI846) += hi846.o
 obj-$(CONFIG_VIDEO_HI847) += hi847.o
+obj-$(CONFIG_VIDEO_HM1246) += hm1246.o
 obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
 obj-$(CONFIG_VIDEO_IMX208) += imx208.o
 obj-$(CONFIG_VIDEO_IMX214) += imx214.o
diff --git a/drivers/media/i2c/hm1246.c b/drivers/media/i2c/hm1246.c
new file mode 100644
index 0000000000000000000000000000000000000000..a460d858734dbce2efee100abf8cd2dc75379d90
--- /dev/null
+++ b/drivers/media/i2c/hm1246.c
@@ -0,0 +1,1347 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Himax HM1246 image sensor
+ *
+ * Copyright 2025 Matthias Fend <matthias.fend@emfend.at>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/units.h>
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+
+/* Status registers */
+#define HM1246_MODEL_ID_REG		 CCI_REG16(0x0000)
+
+/* General setup registers */
+#define HM1246_MODE_SELECT_REG		 CCI_REG8(0x0100)
+#define HM1246_MODE_SELECT_STANDBY	 0x00
+#define HM1246_MODE_SELECT_STREAM	 0x01
+#define HM1246_MODE_SELECT_STOP		 0x02
+#define HM1246_IMAGE_ORIENTATION_REG	 CCI_REG8(0x0101)
+#define HM1246_IMAGE_ORIENTATION_VFLIP	 BIT(1)
+#define HM1246_IMAGE_ORIENTATION_HFLIP	 BIT(0)
+#define HM1246_CMU_UPDATE_REG		 CCI_REG8(0x0104)
+
+/* Output setup registers */
+#define HM1246_COARSE_INTG_REG		 CCI_REG16(0x0202)
+#define HM1246_ANALOG_GLOBAL_GAIN_REG	 CCI_REG8(0x0205)
+
+/* Clock setup registers */
+#define HM1246_PLL1CFG_REG		 CCI_REG8(0x0303)
+#define HM1246_PLL1CFG_MULTIPLIER(x)	 (((x) & 0xff) << 0)
+#define HM1246_PLL2CFG_REG		 CCI_REG8(0x0305)
+#define HM1246_PLL2CFG_PRE_DIV(x)	 (((x) & 0x1f) << 1)
+#define HM1246_PLL2CFG_MULTIPLIER(x)	 (((x) & 0x01) << 0)
+#define HM1246_PLL3CFG_REG		 CCI_REG8(0x0307)
+#define HM1246_PLL3CFG_POST_DIV(x)	 (((x) & 0x3) << 6)
+#define HM1246_PLL3CFG_SYSCLK_DIV(x)	 (((x) & 0x3) << 4)
+#define HM1246_PLL3CFG_PCLK_DIV(x)	 (((x) & 0x7) << 0)
+
+/* Frame timing registers */
+#define HM1246_FRAME_LENGTH_LINES_REG	 CCI_REG16(0x0340)
+#define HM1246_LINE_LENGTH_PCK_REG	 CCI_REG16(0x0342)
+
+/* Image size registers */
+#define HM1246_X_ADDR_START_REG		 CCI_REG16(0x0344)
+#define HM1246_Y_ADDR_START_REG		 CCI_REG16(0x0346)
+#define HM1246_X_ADDR_END_REG		 CCI_REG16(0x0348)
+#define HM1246_Y_ADDR_END_REG		 CCI_REG16(0x034a)
+#define HM1246_X_LA_START_REG		 CCI_REG16(0x0351)
+#define HM1246_X_LA_END_REG		 CCI_REG16(0x0353)
+#define HM1246_Y_LA_START_REG		 CCI_REG16(0x0355)
+#define HM1246_Y_LA_END_REG		 CCI_REG16(0x0357)
+
+/* Test pattern registers */
+#define HM1246_TEST_PATTERN_MODE_REG	 CCI_REG8(0x0601)
+#define HM1246_TEST_PATTERN_MODE_MODE(x) (((x) & 0xf) << 4)
+#define HM1246_TEST_PATTERN_MODE_ENABLE	 BIT(0)
+#define HM1246_TEST_DATA_BLUE_REG	 CCI_REG16(0x0602)
+#define HM1246_TEST_DATA_GB_REG		 CCI_REG16(0x0604)
+#define HM1246_TEST_DATA_RED_REG	 CCI_REG16(0x0606)
+#define HM1246_TEST_DATA_GR_REG		 CCI_REG16(0x0608)
+
+/* SBC registers */
+#define HM1246_SBC_BOOT_REF2_REG	 CCI_REG8(0x2001)
+#define HM1246_SBC_BOOT_REF2_PLL_LOCK	 BIT(4)
+#define HM1246_SBC_CTRL_REG		 CCI_REG8(0x2003)
+#define HM1246_SBC_CTRL_PLL_EN		 BIT(0)
+
+/* System registers */
+#define HM1246_OUTPUT_PRT_CTRL_REG	 CCI_REG8(0x2f02)
+#define HM1246_POLARITY_CTRL_REG	 CCI_REG8(0x2f20)
+#define HM1246_POLARITY_CTRL_HSYNC	 BIT(7)
+#define HM1246_POLARITY_CTRL_VSYNC	 BIT(6)
+#define HM1246_PCLK_CTRL_REG		 CCI_REG8(0x2f24)
+#define HM1246_PCLK_CTRL_POL		 BIT(3)
+
+/* Digital window control & parameter registers */
+#define HM1246_DWIN_XOFFSET_REG		 CCI_REG16(0xd5e4)
+#define HM1246_DWIN_XSIZE_REG		 CCI_REG16(0xd5e6)
+#define HM1246_DWIN_YOFFSET_REG		 CCI_REG16(0xd5e8)
+#define HM1246_DWIN_YSIZE_REG		 CCI_REG16(0xd5ea)
+
+#define HM1246_MODEL_ID			 0x1245
+
+#define HM1246_NATIVE_WIDTH		 1296
+#define HM1246_NATIVE_HEIGHT		 976
+
+#define HM1246_VTS_MAX			 65535
+
+#define HM1246_COARSE_INTG_MARGIN	 2
+#define HM1246_COARSE_INTG_MIN		 4
+#define HM1246_COARSE_INTG_STEP		 1
+
+#define HM1246_ANALOG_GLOBAL_GAIN_MIN	 0x00
+#define HM1246_ANALOG_GLOBAL_GAIN_MAX	 0xe8
+#define HM1246_ANALOG_GLOBAL_GAIN_STEP	 1
+
+#define HM1246_XCLK_MIN			 (6 * HZ_PER_MHZ)
+#define HM1246_XCLK_MAX			 (27 * HZ_PER_MHZ)
+
+#define HM1246_PCLK_MIN			 (8 * HZ_PER_MHZ)
+#define HM1246_PCLK_MAX			 (96 * HZ_PER_MHZ)
+
+#define HM1246_PLL_VCO_MIN		 (360 * HZ_PER_MHZ)
+#define HM1246_PLL_VCO_MAX		 (680 * HZ_PER_MHZ)
+
+#define HM1246_PLL_INCLK_MIN		 (1000 * HZ_PER_KHZ)
+#define HM1246_PLL_INCLK_MAX		 (2500 * HZ_PER_KHZ)
+
+#define HM1246_PLL_MULTI_L_MIN		 1
+#define HM1246_PLL_MULTI_L_MAX		 256
+
+#define HM1246_PLL_MULTI_H_MIN		 2
+#define HM1246_PLL_MULTI_H_MAX		 3
+
+#define HM1246_PLL_MULTI_MIN \
+	(HM1246_PLL_MULTI_H_MIN * HM1246_PLL_MULTI_L_MIN)
+#define HM1246_PLL_MULTI_MAX \
+	(HM1246_PLL_MULTI_H_MAX * HM1246_PLL_MULTI_L_MAX)
+
+static const char *const hm1246_test_pattern_menu[] = {
+	"Disabled",
+	"Checkboard",
+	"Ramp",
+	"Moving ones",
+	"Blending color bars",
+	"Color bars",
+	"Solid white",
+	"Solid black",
+	"Solid red",
+	"Solid green",
+	"Solid blue",
+};
+
+static const s64 hm1246_link_freqs[] = {
+	42174000, /* 1420x990 @ 30Hz (RAW) */
+};
+
+static const char *const hm1246_supply_names[] = {
+	"avdd",
+	"iovdd",
+	"dvdd",
+};
+
+struct hm1246 {
+	struct device *dev;
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+
+	struct regulator_bulk_data supplies[ARRAY_SIZE(hm1246_supply_names)];
+	struct clk *xclk;
+	unsigned long xclk_freq;
+	struct gpio_desc *reset_gpio;
+	unsigned int mbus_flags;
+
+	struct v4l2_ctrl_handler ctrls;
+	struct v4l2_ctrl *pixel_rate_ctrl;
+	struct v4l2_ctrl *link_freq_ctrl;
+	struct v4l2_ctrl *exposure_ctrl;
+	struct v4l2_ctrl *vblank_ctrl;
+	struct v4l2_ctrl *hblank_ctrl;
+	struct v4l2_ctrl *hflip_ctrl;
+	struct v4l2_ctrl *vflip_ctrl;
+
+	struct regmap *regmap;
+
+	bool identified;
+};
+
+static const struct cci_reg_sequence mode_1296x976_raw[] = {
+	{ HM1246_X_LA_START_REG, 60 },
+	{ HM1246_X_LA_END_REG, 1355 },
+	{ HM1246_Y_LA_START_REG, 0 },
+	{ HM1246_Y_LA_END_REG, 975 },
+	{ HM1246_OUTPUT_PRT_CTRL_REG, 0x20 },
+	{ CCI_REG8(0x300a), 0x01 },
+	{ CCI_REG8(0x300b), 0x00 },
+	{ CCI_REG8(0x50f5), 0x01 },
+	{ CCI_REG8(0x50dd), 0x00 },
+	{ CCI_REG8(0x50a1), 0x02 },
+	{ CCI_REG8(0x50aa), 0x1c },
+	{ CCI_REG8(0x50ac), 0xdd },
+	{ CCI_REG8(0x50ad), 0x08 },
+	{ CCI_REG8(0x50ab), 0x04 },
+	{ CCI_REG8(0x50a0), 0x40 },
+	{ CCI_REG8(0x50a2), 0x12 },
+	{ CCI_REG8(0x50ae), 0x30 },
+	{ CCI_REG8(0x50b3), 0x04 },
+	{ CCI_REG8(0x5204), 0x40 },
+	{ CCI_REG8(0x5208), 0x55 },
+	{ CCI_REG8(0x520b), 0x05 },
+	{ CCI_REG8(0x520d), 0x40 },
+	{ CCI_REG8(0x5214), 0x18 },
+	{ CCI_REG8(0x5215), 0x0f },
+	{ CCI_REG8(0x5217), 0x01 },
+	{ CCI_REG8(0x5218), 0x07 },
+	{ CCI_REG8(0x5219), 0x01 },
+	{ CCI_REG8(0x521a), 0x50 },
+	{ CCI_REG8(0x521b), 0x24 },
+	{ CCI_REG8(0x5232), 0x01 },
+	{ CCI_REG8(0x5220), 0x11 },
+	{ CCI_REG8(0x5227), 0x01 },
+	{ CCI_REG8(0x5106), 0xc1 },
+	{ CCI_REG8(0x5115), 0xc0 },
+	{ CCI_REG8(0x5116), 0xc1 },
+	{ CCI_REG8(0x5138), 0x40 },
+	{ CCI_REG8(0x5139), 0x60 },
+	{ CCI_REG8(0x513a), 0x80 },
+	{ CCI_REG8(0x513b), 0xa0 },
+	{ CCI_REG8(0x513c), 0xa1 },
+	{ CCI_REG8(0x513d), 0xa2 },
+	{ CCI_REG8(0x513e), 0xa3 },
+	{ CCI_REG8(0x5140), 0x40 },
+	{ CCI_REG8(0x5141), 0x60 },
+	{ CCI_REG8(0x5142), 0x80 },
+	{ CCI_REG8(0x5143), 0x81 },
+	{ CCI_REG8(0x5144), 0x82 },
+	{ CCI_REG8(0x5145), 0x83 },
+	{ CCI_REG8(0x5146), 0x93 },
+	{ CCI_REG8(0x51c1), 0xc3 },
+	{ CCI_REG8(0x51c5), 0xc3 },
+	{ CCI_REG8(0x51c9), 0xc3 },
+	{ CCI_REG8(0x51cd), 0xc2 },
+	{ CCI_REG8(0x51d1), 0xc1 },
+	{ CCI_REG8(0x51d5), 0xc1 },
+	{ CCI_REG8(0x51d9), 0x81 },
+	{ CCI_REG8(0x51dd), 0x81 },
+	{ CCI_REG8(0x51c2), 0x49 },
+	{ CCI_REG8(0x51c6), 0x49 },
+	{ CCI_REG8(0x51ca), 0x49 },
+	{ CCI_REG8(0x51ce), 0x49 },
+	{ CCI_REG8(0x51d2), 0x49 },
+	{ CCI_REG8(0x51d6), 0x59 },
+	{ CCI_REG8(0x51da), 0x59 },
+	{ CCI_REG8(0x51de), 0x59 },
+	{ CCI_REG8(0x51c3), 0x20 },
+	{ CCI_REG8(0x51c7), 0x38 },
+	{ CCI_REG8(0x51cb), 0x21 },
+	{ CCI_REG8(0x51cf), 0x11 },
+	{ CCI_REG8(0x51d3), 0x11 },
+	{ CCI_REG8(0x51d7), 0x13 },
+	{ CCI_REG8(0x51db), 0x13 },
+	{ CCI_REG8(0x51df), 0x13 },
+	{ CCI_REG8(0x51e0), 0x03 },
+	{ CCI_REG8(0x51e2), 0x03 },
+	{ CCI_REG8(0x51f0), 0x42 },
+	{ CCI_REG8(0x51f1), 0x40 },
+	{ CCI_REG8(0x51f2), 0x4a },
+	{ CCI_REG8(0x51f3), 0x48 },
+	{ CCI_REG8(0x5015), 0x73 },
+	{ CCI_REG8(0x504a), 0x04 },
+	{ CCI_REG8(0x5044), 0x07 },
+	{ CCI_REG8(0x5040), 0x03 },
+	{ CCI_REG8(0x5135), 0xc4 },
+	{ CCI_REG8(0x5136), 0xc5 },
+	{ CCI_REG8(0x5166), 0xc4 },
+	{ CCI_REG8(0x5196), 0xc4 },
+	{ CCI_REG8(0x51c0), 0x10 },
+	{ CCI_REG8(0x51c4), 0x10 },
+	{ CCI_REG8(0x51c8), 0xa0 },
+	{ CCI_REG8(0x51cc), 0xa0 },
+	{ CCI_REG8(0x51d0), 0xa1 },
+	{ CCI_REG8(0x51d4), 0xa5 },
+	{ CCI_REG8(0x51d8), 0xa5 },
+	{ CCI_REG8(0x51dc), 0xa5 },
+	{ CCI_REG8(0x5200), 0xe4 },
+	{ CCI_REG8(0x5209), 0x04 },
+	{ CCI_REG8(0x301b), 0x01 },
+	{ CCI_REG8(0x3130), 0x01 },
+	{ CCI_REG8(0x5013), 0x07 },
+	{ CCI_REG8(0x5016), 0x01 },
+	{ CCI_REG8(0x501d), 0x50 },
+	{ CCI_REG8(0x0350), 0xfe },
+	{ CCI_REG8(0x2f03), 0x15 },
+	{ CCI_REG8(0xd380), 0x00 },
+	{ CCI_REG8(0x3047), 0x7f },
+	{ CCI_REG8(0x304d), 0x34 },
+	{ CCI_REG8(0x3041), 0x4b },
+	{ CCI_REG8(0x3042), 0x2d },
+	{ CCI_REG8(0x3056), 0x64 },
+	{ CCI_REG8(0x3059), 0x1e },
+	{ CCI_REG8(0x305e), 0x10 },
+	{ CCI_REG8(0x305f), 0x10 },
+	{ CCI_REG8(0x306d), 0x10 },
+	{ CCI_REG8(0x306e), 0x0c },
+	{ CCI_REG8(0x3064), 0x50 },
+	{ CCI_REG8(0x3067), 0x78 },
+	{ CCI_REG8(0x3068), 0x4b },
+	{ CCI_REG8(0x306a), 0x78 },
+	{ CCI_REG8(0x306b), 0x4b },
+	{ CCI_REG8(0xd442), 0x3d },
+	{ CCI_REG8(0xd443), 0x06 },
+	{ CCI_REG8(0xd440), 0x63 },
+	{ CCI_REG8(0xd446), 0xb0 },
+	{ CCI_REG8(0xd447), 0x60 },
+	{ CCI_REG8(0xd448), 0x48 },
+	{ CCI_REG8(0xd449), 0x30 },
+	{ CCI_REG8(0xd44a), 0x18 },
+	{ CCI_REG8(0xd360), 0x03 },
+	{ CCI_REG8(0x30ac), 0x10 },
+	{ CCI_REG8(0x30ad), 0x10 },
+	{ CCI_REG8(0x30ae), 0x10 },
+	{ CCI_REG8(0x3040), 0x0b },
+	{ CCI_REG8(0x2002), 0x00 },
+	{ CCI_REG8(0x2000), 0x08 },
+};
+
+struct hm1246_reg_list {
+	u32 num_of_regs;
+	const struct cci_reg_sequence *regs;
+};
+
+struct hm1246_mode {
+	u32 codes[4];
+	u32 link_freq_index;
+	u32 clocks_per_pixel;
+	u32 top;
+	u32 left;
+	u32 width;
+	u32 height;
+	u32 hts;
+	u32 vts_min;
+	const struct hm1246_reg_list reg_list;
+};
+
+#define FLIP_FORMAT_INDEX(v, h) ((v ? 2 : 0) | (h ? 1 : 0))
+
+/* Get the format code of the mode considering current flip setting. */
+static u32 hm1246_get_format_code(struct hm1246 *hm1246,
+				  const struct hm1246_mode *hm1246_mode)
+{
+	return hm1246_mode->codes[FLIP_FORMAT_INDEX(hm1246->vflip_ctrl->val,
+						    hm1246->hflip_ctrl->val)];
+}
+
+static const struct hm1246_mode hm1246_modes[] = {
+	{
+		.codes = {
+			[FLIP_FORMAT_INDEX(0, 0)] = MEDIA_BUS_FMT_SBGGR10_1X10,
+			[FLIP_FORMAT_INDEX(0, 1)] = MEDIA_BUS_FMT_SGBRG10_1X10,
+			[FLIP_FORMAT_INDEX(1, 0)] = MEDIA_BUS_FMT_SGRBG10_1X10,
+			[FLIP_FORMAT_INDEX(1, 1)] = MEDIA_BUS_FMT_SRGGB10_1X10,
+		},
+		.link_freq_index = 0,
+		.clocks_per_pixel = 1,
+		.top = 0,
+		.left = 0,
+		.width = 1296,
+		.height = 976,
+		.hts = 1420,
+		.vts_min = 990,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1296x976_raw),
+			.regs = mode_1296x976_raw,
+		},
+	},
+};
+
+static inline struct hm1246 *to_hm1246(struct v4l2_subdev *sd)
+{
+	return container_of_const(sd, struct hm1246, sd);
+}
+
+static const struct hm1246_mode *
+hm1246_find_mode_by_mbus_code(struct hm1246 *hm1246, u32 code)
+{
+	for (unsigned int i = 0; i < ARRAY_SIZE(hm1246_modes); i++) {
+		if (code == hm1246_get_format_code(hm1246, &hm1246_modes[i]))
+			return &hm1246_modes[i];
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
+static int hm1246_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct hm1246 *hm1246 = to_hm1246(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(hm1246_supply_names),
+				    hm1246->supplies);
+	if (ret) {
+		dev_err(hm1246->dev, "failed to enable regulators\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(hm1246->xclk);
+	if (ret) {
+		regulator_bulk_disable(ARRAY_SIZE(hm1246_supply_names),
+				       hm1246->supplies);
+		dev_err(hm1246->dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(hm1246->reset_gpio, 0);
+
+	/*
+	 * XSHUTDOWN to crystal clock oscillation:  tcrystal typ.  650us
+	 * Sample bootstrap pin:                    tsample  max. 2000us
+	 * Built in self test:                      tbist    max. 3000us
+	 */
+	fsleep(6000);
+
+	return 0;
+}
+
+static int hm1246_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct hm1246 *hm1246 = to_hm1246(sd);
+
+	gpiod_set_value_cansleep(hm1246->reset_gpio, 1);
+
+	clk_disable_unprepare(hm1246->xclk);
+
+	regulator_bulk_disable(ARRAY_SIZE(hm1246_supply_names),
+			       hm1246->supplies);
+
+	return 0;
+}
+
+static int hm1246_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct hm1246 *hm1246 = to_hm1246(sd);
+
+	if (code->index >= ARRAY_SIZE(hm1246_modes))
+		return -EINVAL;
+
+	code->code = hm1246_get_format_code(hm1246, &hm1246_modes[code->index]);
+
+	return 0;
+}
+
+static int hm1246_enum_frame_size(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_state *sd_state,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct hm1246 *hm1246 = to_hm1246(subdev);
+	const struct hm1246_mode *mode;
+
+	if (fse->index > 0)
+		return -EINVAL;
+
+	mode = hm1246_find_mode_by_mbus_code(hm1246, fse->code);
+	if (IS_ERR(mode))
+		return PTR_ERR(mode);
+
+	fse->min_width = mode->width;
+	fse->max_width = mode->width;
+	fse->min_height = mode->height;
+	fse->max_height = mode->height;
+
+	return 0;
+}
+
+static void hm1246_update_pad_format(struct hm1246 *hm1246,
+				     const struct hm1246_mode *hm1246_mode,
+				     struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->width = hm1246_mode->width;
+	fmt->height = hm1246_mode->height;
+	fmt->code = hm1246_get_format_code(hm1246, hm1246_mode);
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
+}
+
+static int hm1246_set_format(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state,
+			     struct v4l2_subdev_format *fmt)
+{
+	struct hm1246 *hm1246 = to_hm1246(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt;
+	struct v4l2_rect *crop;
+	const struct hm1246_mode *mode;
+
+	mode = hm1246_find_mode_by_mbus_code(hm1246, fmt->format.code);
+	if (IS_ERR(mode))
+		mode = &hm1246_modes[0];
+
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	crop->top = mode->top;
+	crop->left = mode->left;
+	crop->width = mode->width;
+	crop->height = mode->height;
+
+	hm1246_update_pad_format(hm1246, mode, &fmt->format);
+	mbus_fmt = v4l2_subdev_state_get_format(state, 0);
+	*mbus_fmt = fmt->format;
+
+	return 0;
+}
+
+static int hm1246_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	const struct v4l2_mbus_framefmt *format;
+	const struct hm1246_mode *mode;
+
+	format = v4l2_subdev_state_get_format(state, 0);
+	mode = v4l2_find_nearest_size(hm1246_modes, ARRAY_SIZE(hm1246_modes),
+				      width, height, format->width,
+				      format->height);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		sel->r = *v4l2_subdev_state_get_crop(state, 0);
+		return 0;
+
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = HM1246_NATIVE_WIDTH;
+		sel->r.height = HM1246_NATIVE_HEIGHT;
+		return 0;
+
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = mode->top;
+		sel->r.left = mode->left;
+		sel->r.width = mode->width;
+		sel->r.height = mode->height;
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int hm1246_init_state(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state)
+{
+	struct hm1246 *hm1246 = to_hm1246(sd);
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = 0,
+		.format = {
+			.code = hm1246_get_format_code(hm1246,
+						       &hm1246_modes[0]),
+			.width = hm1246_modes[0].width,
+			.height = hm1246_modes[0].height,
+		},
+	};
+
+	hm1246_set_format(sd, state, &fmt);
+
+	return 0;
+}
+
+static int hm1246_calc_pll(struct hm1246 *hm1246, u32 xclk, u32 link_freq,
+			   u32 clocks_per_pixel, u8 *pll1, u8 *pll2, u8 *pll3)
+{
+	const u8 pclk_div_table[] = { 4, 5, 6, 7, 8, 12, 14, 16 };
+	const u8 sysclk_div_table[] = { 1, 2, 3, 4 };
+	const u8 post_div_table[] = { 1, 2, 4, 8 };
+	const int sysclk_pclk_ratio = 3; /* Recommended value */
+	u32 pclk, vco_out, best_vco_diff;
+	int pclk_div_index, sysclk_div_index, post_div_index;
+	u8 pre_div = 0, multiplier_h = 0, multiplier_l = 0;
+
+	if (link_freq < HM1246_PCLK_MIN || link_freq > HM1246_PCLK_MAX)
+		return -EINVAL;
+
+	/*
+	 * In raw mode (1 pixel per clock) the pixel clock is internally
+	 * divided by two.
+	 */
+	pclk = 2 * link_freq / clocks_per_pixel;
+
+	/* Find suitable PCLK and SYSCLK dividers. */
+	for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table);
+	     pclk_div_index++) {
+		for (sysclk_div_index = 0;
+		     sysclk_div_index < ARRAY_SIZE(sysclk_div_table);
+		     sysclk_div_index++) {
+			if (sysclk_div_table[sysclk_div_index] *
+				    sysclk_pclk_ratio ==
+			    pclk_div_table[pclk_div_index])
+				goto sysclk_pclk_ratio_found;
+		}
+	}
+
+	return -EINVAL;
+
+sysclk_pclk_ratio_found:
+
+	/* Determine an appropriate post divider. */
+	for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
+	     post_div_index++) {
+		vco_out = pclk * pclk_div_table[pclk_div_index] *
+			  post_div_table[post_div_index];
+
+		if (vco_out >= HM1246_PLL_VCO_MIN &&
+		    vco_out <= HM1246_PLL_VCO_MAX)
+			break;
+	}
+	if (post_div_index >= ARRAY_SIZE(post_div_table))
+		return -EINVAL;
+
+	/* Find best pre-divider and multiplier values. */
+	best_vco_diff = U32_MAX;
+	for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX);
+	     div <= xclk / HM1246_PLL_INCLK_MIN; div++) {
+		u32 multi, multi_h, multi_l, vco, diff;
+
+		multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
+		if (multi < HM1246_PLL_MULTI_MIN ||
+		    multi > HM1246_PLL_MULTI_MAX)
+			continue;
+
+		multi_h = multi / (HM1246_PLL_MULTI_H_MIN *
+				   HM1246_PLL_MULTI_L_MAX) +
+			  2;
+		multi_l = multi / multi_h;
+		vco = div_u64((u64)xclk * multi_h * multi_l, div);
+
+		diff = abs_diff(vco_out, vco);
+
+		if (diff < best_vco_diff) {
+			best_vco_diff = diff;
+			pre_div = div;
+			multiplier_h = multi_h;
+			multiplier_l = multi_l;
+		}
+
+		if (!diff)
+			break;
+	}
+
+	if (best_vco_diff == U32_MAX)
+		return -EINVAL;
+
+	*pll1 = HM1246_PLL1CFG_MULTIPLIER(multiplier_l - 1);
+	*pll2 = HM1246_PLL2CFG_PRE_DIV(pre_div - 1) |
+		HM1246_PLL2CFG_MULTIPLIER(multiplier_h - 2);
+	*pll3 = HM1246_PLL3CFG_POST_DIV(post_div_index) |
+		HM1246_PLL3CFG_SYSCLK_DIV(sysclk_div_index) |
+		HM1246_PLL3CFG_PCLK_DIV(pclk_div_index);
+
+	return 0;
+}
+
+static int hm1246_cci_write_pll(struct hm1246 *hm1246, u8 pll1, u8 pll2,
+				u8 pll3)
+{
+	const struct cci_reg_sequence pll_regs[] = {
+		{ HM1246_PLL1CFG_REG, pll1 },
+		{ HM1246_PLL2CFG_REG, pll2 },
+		{ HM1246_PLL3CFG_REG, pll3 },
+		{ HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
+	};
+
+	return cci_multi_reg_write(hm1246->regmap, pll_regs,
+				   ARRAY_SIZE(pll_regs), NULL);
+}
+
+static int hm1246_pll_check_locked(struct hm1246 *hm1246)
+{
+	u64 boot_ref2;
+	int ret;
+
+	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
+		       NULL);
+	if (ret)
+		return ret;
+
+	return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;
+}
+
+static int hm1246_setup_pll(struct hm1246 *hm1246,
+			    const struct hm1246_mode *mode)
+{
+	u8 pll1, pll2, pll3;
+	int ret;
+
+	ret = hm1246_calc_pll(hm1246, hm1246->xclk_freq,
+			      hm1246_link_freqs[mode->link_freq_index],
+			      mode->clocks_per_pixel, &pll1, &pll2, &pll3);
+	if (ret)
+		return ret;
+
+	ret = hm1246_cci_write_pll(hm1246, pll1, pll2, pll3);
+	if (ret)
+		return ret;
+
+	/* PLL lock time: tpll typ. 100us */
+	fsleep(200);
+
+	return hm1246_pll_check_locked(hm1246);
+}
+
+static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode, u16 r,
+					 u16 g, u16 b)
+{
+	const struct cci_reg_sequence tpg_enable_regs[] = {
+		{ HM1246_TEST_DATA_RED_REG, r },
+		{ HM1246_TEST_DATA_GR_REG, g },
+		{ HM1246_TEST_DATA_GB_REG, g },
+		{ HM1246_TEST_DATA_BLUE_REG, b },
+		{ HM1246_TEST_PATTERN_MODE_REG, mode },
+	};
+
+	return cci_multi_reg_write(hm1246->regmap, tpg_enable_regs,
+				   ARRAY_SIZE(tpg_enable_regs), NULL);
+}
+
+static int hm1246_test_pattern(struct hm1246 *hm1246, u32 index)
+{
+	const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;
+	const struct tp {
+		int pattern;
+		u16 r, g, b;
+	} tps[] = {
+		/* 0 - Disabled */
+		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
+		/* 1 - Checkboard pattern */
+		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
+		/* 2 - Ramp */
+		{ .pattern = 1, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
+		/* 3 - Moving ones */
+		{ .pattern = 2, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
+		/* 4 - Blending color bars */
+		{ .pattern = 3, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
+		/* 5 - Color bars */
+		{ .pattern = 4, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
+		/* 6 - Solid white */
+		{ .pattern = 15, .r = RGBMAX, .g = RGBMAX, .b = RGBMAX },
+		/* 7 - Solid black */
+		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
+		/* 8 - Solid red */
+		{ .pattern = 15, .r = RGBMAX, .g = RGBMIN, .b = RGBMIN },
+		/* 9 - Solid green */
+		{ .pattern = 15, .r = RGBMIN, .g = RGBMAX, .b = RGBMIN },
+		/* 10- Solid blue */
+		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMAX },
+	};
+	u8 mode;
+
+	if (index >= ARRAY_SIZE(tps))
+		return -EINVAL;
+
+	mode = HM1246_TEST_PATTERN_MODE_MODE(tps[index].pattern);
+	if (index)
+		mode |= HM1246_TEST_PATTERN_MODE_ENABLE;
+
+	return hm1246_cci_write_test_pattern(hm1246, mode, tps[index].r,
+					     tps[index].g, tps[index].b);
+}
+
+static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct hm1246 *hm1246 = container_of_const(ctrl->handler, struct hm1246,
+						   ctrls);
+	struct v4l2_subdev_state *state;
+	const struct v4l2_mbus_framefmt *format;
+	u32 val;
+	bool needs_cmu_update = true;
+	int ret = 0;
+
+	state = v4l2_subdev_get_locked_active_state(&hm1246->sd);
+	format = v4l2_subdev_state_get_format(state, 0);
+
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		s64 exposure_max;
+
+		exposure_max =
+			format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN;
+		ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl,
+					       hm1246->exposure_ctrl->minimum,
+					       exposure_max,
+					       hm1246->exposure_ctrl->step,
+					       exposure_max);
+
+		if (ret) {
+			dev_err(hm1246->dev, "exposure ctrl range update failed\n");
+			return ret;
+		}
+	}
+
+	if (!pm_runtime_get_if_active(hm1246->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE:
+		cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
+			  &ret);
+		break;
+
+	case V4L2_CID_ANALOGUE_GAIN:
+		cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
+			  ctrl->val, &ret);
+		break;
+
+	case V4L2_CID_VBLANK:
+		val = format->height + ctrl->val;
+		cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
+			  &ret);
+		break;
+
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+		val = 0;
+		if (hm1246->hflip_ctrl->val)
+			val |= HM1246_IMAGE_ORIENTATION_HFLIP;
+		if (hm1246->vflip_ctrl->val)
+			val |= HM1246_IMAGE_ORIENTATION_VFLIP;
+
+		cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val,
+			  &ret);
+		break;
+
+	case V4L2_CID_TEST_PATTERN:
+		ret = hm1246_test_pattern(hm1246, ctrl->val);
+		needs_cmu_update = false;
+		break;
+
+	default:
+		ret = -EINVAL;
+		needs_cmu_update = false;
+		break;
+	}
+
+	if (needs_cmu_update)
+		cci_write(hm1246->regmap, HM1246_CMU_UPDATE_REG, 0, &ret);
+
+	pm_runtime_put(hm1246->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops hm1246_ctrl_ops = {
+	.s_ctrl = hm1246_set_ctrl,
+};
+
+static int hm1246_identify_module(struct hm1246 *hm1246)
+{
+	u64 model_id;
+	int ret;
+
+	if (hm1246->identified)
+		return 0;
+
+	ret = cci_read(hm1246->regmap, HM1246_MODEL_ID_REG, &model_id, NULL);
+	if (ret)
+		return ret;
+
+	if (model_id != HM1246_MODEL_ID) {
+		dev_err(hm1246->dev, "model id mismatch: 0x%llx!=0x%x\n",
+			model_id, HM1246_MODEL_ID);
+		return -ENXIO;
+	}
+
+	hm1246->identified = true;
+
+	return 0;
+}
+
+static int hm1246_setup_moderegs(struct hm1246 *hm1246,
+				 const struct hm1246_mode *mode)
+{
+	const struct hm1246_reg_list *reg_list = &mode->reg_list;
+	const struct cci_reg_sequence modeaw[] = {
+		{ HM1246_X_ADDR_START_REG, mode->left },
+		{ HM1246_Y_ADDR_START_REG, mode->top },
+		{ HM1246_X_ADDR_END_REG, mode->width - 1 },
+		{ HM1246_Y_ADDR_END_REG, mode->height - 1 },
+		{ HM1246_DWIN_XOFFSET_REG, mode->left },
+		{ HM1246_DWIN_YOFFSET_REG, mode->top },
+		{ HM1246_DWIN_XSIZE_REG, mode->width },
+		{ HM1246_DWIN_YSIZE_REG, mode->height },
+		{ HM1246_LINE_LENGTH_PCK_REG, mode->hts },
+	};
+	int ret = 0;
+
+	cci_multi_reg_write(hm1246->regmap, modeaw, ARRAY_SIZE(modeaw), &ret);
+	cci_multi_reg_write(hm1246->regmap, reg_list->regs,
+			    reg_list->num_of_regs, &ret);
+
+	return ret;
+}
+
+static int hm1246_setup_bus(struct hm1246 *hm1246)
+{
+	u64 polarity_ctrl = 0, pclk_ctrl = 0;
+	int ret = 0;
+
+	if (hm1246->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
+		polarity_ctrl |= HM1246_POLARITY_CTRL_HSYNC;
+
+	if (hm1246->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+		polarity_ctrl |= HM1246_POLARITY_CTRL_VSYNC;
+
+	cci_write(hm1246->regmap, HM1246_POLARITY_CTRL_REG, polarity_ctrl,
+		  &ret);
+
+	/*
+	 * If the clock output polarity flag PCLK_CTRL[3] is set (high), the
+	 * data lines change state on the falling edge of PCLK and should
+	 * therefore be sampled on the rising edge.
+	 * This is different than described in the data sheet.
+	 */
+	if (hm1246->mbus_flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+		pclk_ctrl |= HM1246_PCLK_CTRL_POL;
+
+	cci_write(hm1246->regmap, HM1246_PCLK_CTRL_REG, pclk_ctrl, &ret);
+
+	return ret;
+}
+
+static int hm1246_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state, u32 pad,
+				 u64 streams_mask)
+{
+	struct hm1246 *hm1246 = to_hm1246(sd);
+	const struct v4l2_mbus_framefmt *format;
+	const struct hm1246_mode *mode;
+	int ret;
+
+	format = v4l2_subdev_state_get_format(state, 0);
+	mode = v4l2_find_nearest_size(hm1246_modes, ARRAY_SIZE(hm1246_modes),
+				      width, height, format->width,
+				      format->height);
+
+	ret = pm_runtime_resume_and_get(hm1246->dev);
+	if (ret)
+		return ret;
+
+	ret = hm1246_identify_module(hm1246);
+	if (ret)
+		goto err_rpm_put;
+
+	ret = hm1246_setup_pll(hm1246, mode);
+	if (ret) {
+		dev_err(hm1246->dev, "failed to setup PLL\n");
+		goto err_rpm_put;
+	}
+
+	ret = hm1246_setup_moderegs(hm1246, mode);
+	if (ret)
+		goto err_rpm_put;
+
+	ret = hm1246_setup_bus(hm1246);
+	if (ret)
+		goto err_rpm_put;
+
+	ret = __v4l2_ctrl_handler_setup(&hm1246->ctrls);
+	if (ret) {
+		dev_err(hm1246->dev, "failed to setup v4l2 controls\n");
+		goto err_rpm_put;
+	}
+
+	ret = cci_write(hm1246->regmap, HM1246_MODE_SELECT_REG,
+			HM1246_MODE_SELECT_STREAM, NULL);
+	if (ret)
+		goto err_rpm_put;
+
+	/*
+	 * Since mirroring may change the actual pixel format, it must not be
+	 * changed during streaming.
+	 */
+	__v4l2_ctrl_grab(hm1246->vflip_ctrl, true);
+	__v4l2_ctrl_grab(hm1246->hflip_ctrl, true);
+
+	return 0;
+
+err_rpm_put:
+	pm_runtime_put_autosuspend(hm1246->dev);
+
+	return ret;
+}
+
+static int hm1246_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state, u32 pad,
+				  u64 streams_mask)
+{
+	struct hm1246 *hm1246 = to_hm1246(sd);
+	int ret;
+
+	ret = cci_write(hm1246->regmap, HM1246_MODE_SELECT_REG,
+			HM1246_MODE_SELECT_STANDBY, NULL);
+
+	__v4l2_ctrl_grab(hm1246->vflip_ctrl, false);
+	__v4l2_ctrl_grab(hm1246->hflip_ctrl, false);
+
+	pm_runtime_put_autosuspend(hm1246->dev);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_video_ops hm1246_video_ops = {
+	.s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops hm1246_subdev_pad_ops = {
+	.enum_mbus_code = hm1246_enum_mbus_code,
+	.enum_frame_size = hm1246_enum_frame_size,
+	.get_fmt = v4l2_subdev_get_fmt,
+	.set_fmt = hm1246_set_format,
+	.get_selection = hm1246_get_selection,
+	.enable_streams = hm1246_enable_streams,
+	.disable_streams = hm1246_disable_streams,
+};
+
+static int __maybe_unused hm1246_g_register(struct v4l2_subdev *sd,
+					    struct v4l2_dbg_register *reg)
+{
+	struct hm1246 *hm1246 = to_hm1246(sd);
+	u64 val;
+	int ret;
+
+	if (!pm_runtime_get_if_in_use(hm1246->dev))
+		return 0;
+
+	ret = cci_read(hm1246->regmap, CCI_REG8(reg->reg), &val, NULL);
+	reg->val = val;
+
+	pm_runtime_put(hm1246->dev);
+
+	return ret;
+}
+
+static int __maybe_unused hm1246_s_register(struct v4l2_subdev *sd,
+					    const struct v4l2_dbg_register *reg)
+{
+	struct hm1246 *hm1246 = to_hm1246(sd);
+	int ret;
+
+	if (!pm_runtime_get_if_in_use(hm1246->dev))
+		return 0;
+
+	ret = cci_write(hm1246->regmap, CCI_REG8(reg->reg), (u64)reg->val,
+			NULL);
+
+	pm_runtime_put(hm1246->dev);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_core_ops hm1246_core_ops = {
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register = hm1246_g_register,
+	.s_register = hm1246_s_register,
+#endif
+};
+
+static const struct v4l2_subdev_ops hm1246_subdev_ops = {
+	.core = &hm1246_core_ops,
+	.video = &hm1246_video_ops,
+	.pad = &hm1246_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops hm1246_internal_ops = {
+	.init_state = hm1246_init_state,
+};
+
+static int hm1246_get_regulators(struct device *dev, struct hm1246 *hm1246)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(hm1246_supply_names); i++)
+		hm1246->supplies[i].supply = hm1246_supply_names[i];
+
+	return devm_regulator_bulk_get(dev, ARRAY_SIZE(hm1246_supply_names),
+				       hm1246->supplies);
+}
+
+static int hm1246_parse_fwnode(struct hm1246 *hm1246)
+{
+	struct fwnode_handle *endpoint;
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_PARALLEL,
+	};
+	unsigned long link_freq_bitmap;
+	int ret;
+
+	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(hm1246->dev), 0,
+						   0,
+						   FWNODE_GRAPH_ENDPOINT_NEXT);
+	if (!endpoint)
+		return dev_err_probe(hm1246->dev, -EINVAL,
+				     "missing endpoint node\n");
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
+	fwnode_handle_put(endpoint);
+	if (ret)
+		return dev_err_probe(hm1246->dev, ret,
+				     "parsing endpoint node failed\n");
+
+	hm1246->mbus_flags = bus_cfg.bus.parallel.flags;
+
+	ret = v4l2_link_freq_to_bitmap(hm1246->dev, bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       hm1246_link_freqs,
+				       ARRAY_SIZE(hm1246_link_freqs),
+				       &link_freq_bitmap);
+
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+
+	return ret;
+}
+
+static int hm1246_init_controls(struct hm1246 *hm1246)
+{
+	const struct hm1246_mode *mode = &hm1246_modes[0];
+	struct v4l2_fwnode_device_properties props;
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	s64 pixel_rate, exposure_max, vblank_min, hblank;
+	int ret;
+
+	ctrl_hdlr = &hm1246->ctrls;
+	v4l2_ctrl_handler_init(ctrl_hdlr, 11);
+
+	hm1246->hflip_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
+					       V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (hm1246->hflip_ctrl)
+		hm1246->hflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	hm1246->vflip_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
+					       V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (hm1246->vflip_ctrl)
+		hm1246->vflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	v4l2_ctrl_cluster(2, &hm1246->hflip_ctrl);
+
+	hm1246->link_freq_ctrl =
+		v4l2_ctrl_new_int_menu(ctrl_hdlr, &hm1246_ctrl_ops,
+				       V4L2_CID_LINK_FREQ,
+				       ARRAY_SIZE(hm1246_link_freqs) - 1, 0,
+				       hm1246_link_freqs);
+	if (hm1246->link_freq_ctrl)
+		hm1246->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	pixel_rate = div_u64(hm1246_link_freqs[mode->link_freq_index],
+			     mode->clocks_per_pixel);
+	hm1246->pixel_rate_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
+						    V4L2_CID_PIXEL_RATE,
+						    pixel_rate, pixel_rate, 1,
+						    pixel_rate);
+
+	vblank_min = mode->vts_min - mode->height;
+	hm1246->vblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
+						V4L2_CID_VBLANK, vblank_min,
+						HM1246_VTS_MAX - mode->height,
+						1, vblank_min);
+
+	hblank = mode->hts - mode->width;
+	hm1246->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
+						V4L2_CID_HBLANK, hblank, hblank,
+						1, hblank);
+	if (hm1246->hblank_ctrl)
+		hm1246->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+			  HM1246_ANALOG_GLOBAL_GAIN_MIN,
+			  HM1246_ANALOG_GLOBAL_GAIN_MAX,
+			  HM1246_ANALOG_GLOBAL_GAIN_STEP,
+			  HM1246_ANALOG_GLOBAL_GAIN_MIN);
+
+	exposure_max = mode->vts_min - HM1246_COARSE_INTG_MARGIN;
+	hm1246->exposure_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
+						  V4L2_CID_EXPOSURE,
+						  HM1246_COARSE_INTG_MIN,
+						  exposure_max,
+						  HM1246_COARSE_INTG_STEP,
+						  exposure_max);
+
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &hm1246_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(hm1246_test_pattern_menu) - 1,
+				     0, 0, hm1246_test_pattern_menu);
+
+	ret = v4l2_fwnode_device_parse(hm1246->dev, &props);
+	if (ret)
+		goto err_v4l2_ctrl_handler_free;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &hm1246_ctrl_ops,
+					      &props);
+	if (ret)
+		goto err_v4l2_ctrl_handler_free;
+
+	if (ctrl_hdlr->error) {
+		ret = ctrl_hdlr->error;
+		goto err_v4l2_ctrl_handler_free;
+	}
+
+	hm1246->sd.ctrl_handler = ctrl_hdlr;
+
+	return 0;
+
+err_v4l2_ctrl_handler_free:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+
+	return ret;
+}
+
+static int hm1246_probe(struct i2c_client *client)
+{
+	struct hm1246 *hm1246;
+	int ret;
+
+	hm1246 = devm_kzalloc(&client->dev, sizeof(*hm1246), GFP_KERNEL);
+	if (!hm1246)
+		return -ENOMEM;
+
+	hm1246->dev = &client->dev;
+
+	ret = hm1246_parse_fwnode(hm1246);
+	if (ret)
+		return ret;
+
+	hm1246->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(hm1246->regmap))
+		return dev_err_probe(hm1246->dev, PTR_ERR(hm1246->regmap),
+				     "failed to init CCI\n");
+
+	hm1246->xclk = devm_v4l2_sensor_clk_get(hm1246->dev, NULL);
+	if (IS_ERR(hm1246->xclk))
+		return dev_err_probe(hm1246->dev, PTR_ERR(hm1246->xclk),
+				     "failed to get xclk\n");
+
+	hm1246->xclk_freq = clk_get_rate(hm1246->xclk);
+	if (hm1246->xclk_freq < HM1246_XCLK_MIN ||
+	    hm1246->xclk_freq > HM1246_XCLK_MAX)
+		return dev_err_probe(hm1246->dev, -EINVAL,
+				     "xclk frequency out of range: %luHz\n",
+				     hm1246->xclk_freq);
+
+	ret = hm1246_get_regulators(hm1246->dev, hm1246);
+	if (ret)
+		return dev_err_probe(hm1246->dev, ret,
+				     "failed to get regulators\n");
+
+	hm1246->reset_gpio =
+		devm_gpiod_get_optional(hm1246->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(hm1246->reset_gpio))
+		return dev_err_probe(hm1246->dev, PTR_ERR(hm1246->reset_gpio),
+				     "failed to get reset GPIO\n");
+
+	v4l2_i2c_subdev_init(&hm1246->sd, client, &hm1246_subdev_ops);
+	hm1246->sd.internal_ops = &hm1246_internal_ops;
+
+	ret = hm1246_init_controls(hm1246);
+	if (ret)
+		return dev_err_probe(hm1246->dev, ret,
+				     "failed to init controls\n");
+
+	hm1246->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	hm1246->pad.flags = MEDIA_PAD_FL_SOURCE;
+	hm1246->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	ret = media_entity_pads_init(&hm1246->sd.entity, 1, &hm1246->pad);
+	if (ret) {
+		dev_err_probe(hm1246->dev, ret, "failed to init media pads\n");
+		goto err_v4l2_ctrl_handler_free;
+	}
+
+	hm1246->sd.state_lock = hm1246->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&hm1246->sd);
+	if (ret) {
+		dev_err_probe(hm1246->dev, ret, "failed to init v4l2 subdev\n");
+		goto err_media_entity_cleanup;
+	}
+
+	pm_runtime_enable(hm1246->dev);
+	pm_runtime_set_autosuspend_delay(hm1246->dev, 1000);
+	pm_runtime_use_autosuspend(hm1246->dev);
+	pm_runtime_idle(hm1246->dev);
+
+	ret = v4l2_async_register_subdev_sensor(&hm1246->sd);
+	if (ret) {
+		dev_err_probe(hm1246->dev, ret,
+			      "failed to register v4l2 subdev\n");
+		goto err_subdev_cleanup;
+	}
+
+	return 0;
+
+err_subdev_cleanup:
+	v4l2_subdev_cleanup(&hm1246->sd);
+	pm_runtime_disable(hm1246->dev);
+	pm_runtime_set_suspended(hm1246->dev);
+
+err_media_entity_cleanup:
+	media_entity_cleanup(&hm1246->sd.entity);
+
+err_v4l2_ctrl_handler_free:
+	v4l2_ctrl_handler_free(&hm1246->ctrls);
+
+	return ret;
+}
+
+static void hm1246_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct hm1246 *hm1246 = to_hm1246(sd);
+
+	v4l2_async_unregister_subdev(&hm1246->sd);
+	v4l2_subdev_cleanup(sd);
+	media_entity_cleanup(&hm1246->sd.entity);
+	v4l2_ctrl_handler_free(&hm1246->ctrls);
+
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev)) {
+		hm1246_power_off(hm1246->dev);
+		pm_runtime_set_suspended(&client->dev);
+	}
+}
+
+static const struct of_device_id hm1246_of_match[] = {
+	{ .compatible = "himax,hm1246" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, hm1246_of_match);
+
+static DEFINE_RUNTIME_DEV_PM_OPS(hm1246_pm_ops, hm1246_power_off,
+				 hm1246_power_on, NULL);
+
+static struct i2c_driver hm1246_i2c_driver = {
+	.driver = {
+		.of_match_table = hm1246_of_match,
+		.pm = pm_ptr(&hm1246_pm_ops),
+		.name = "hm1246",
+	},
+	.probe = hm1246_probe,
+	.remove = hm1246_remove,
+};
+module_i2c_driver(hm1246_i2c_driver);
+
+MODULE_DESCRIPTION("Himax HM1246 camera driver");
+MODULE_AUTHOR("Matthias Fend <matthias.fend@emfend.at>");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Andy Shevchenko]

On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:
> Add a V4L2 sub-device driver for Himax HM1246 image sensor.
> 
> The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
> array size of 1296 x 976. It is programmable through an I2C interface and
> connected via parallel bus.
> 
> The sensor has an internal ISP with a complete image processing pipeline
> including control loops. However, this driver uses the sensor in raw mode
> and the entire ISP is bypassed.

...

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/units.h>

This block is semi-random.
First of all, no new code must use gpio.h, use the proper one.
Second, many are missing, e.g., bits.h, regmap.h, types.h.
Please, follow IWYU principle (Include What You Use).

...

> +static inline struct hm1246 *to_hm1246(struct v4l2_subdev *sd)
> +{
> +	return container_of_const(sd, struct hm1246, sd);

It's unclear and confusing that _const() variant is used here.
Either const qualifier is missed somewhere, or _const is redundant.

> +}

...

> +	/*
> +	 * XSHUTDOWN to crystal clock oscillation:  tcrystal typ.  650us
> +	 * Sample bootstrap pin:                    tsample  max. 2000us
> +	 * Built in self test:                      tbist    max. 3000us
> +	 */
> +	fsleep(6000);

Also possible to write as 6 * USEC_PER_MSEC

...

> +	format = v4l2_subdev_state_get_format(state, 0);
> +	mode = v4l2_find_nearest_size(hm1246_modes, ARRAY_SIZE(hm1246_modes),
> +				      width, height, format->width,
> +				      format->height);
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> +		return 0;
> +
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = HM1246_NATIVE_WIDTH;
> +		sel->r.height = HM1246_NATIVE_HEIGHT;
> +		return 0;
> +
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = mode->top;
> +		sel->r.left = mode->left;
> +		sel->r.width = mode->width;
> +		sel->r.height = mode->height;
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}

> +	return 0;

Do we need this line?

...

> +static int hm1246_calc_pll(struct hm1246 *hm1246, u32 xclk, u32 link_freq,
> +			   u32 clocks_per_pixel, u8 *pll1, u8 *pll2, u8 *pll3)
> +{
> +	const u8 pclk_div_table[] = { 4, 5, 6, 7, 8, 12, 14, 16 };
> +	const u8 sysclk_div_table[] = { 1, 2, 3, 4 };
> +	const u8 post_div_table[] = { 1, 2, 4, 8 };
> +	const int sysclk_pclk_ratio = 3; /* Recommended value */
> +	u32 pclk, vco_out, best_vco_diff;
> +	int pclk_div_index, sysclk_div_index, post_div_index;
> +	u8 pre_div = 0, multiplier_h = 0, multiplier_l = 0;
> +
> +	if (link_freq < HM1246_PCLK_MIN || link_freq > HM1246_PCLK_MAX)
> +		return -EINVAL;
> +
> +	/*
> +	 * In raw mode (1 pixel per clock) the pixel clock is internally
> +	 * divided by two.
> +	 */
> +	pclk = 2 * link_freq / clocks_per_pixel;
> +
> +	/* Find suitable PCLK and SYSCLK dividers. */
> +	for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table);
> +	     pclk_div_index++) {
> +		for (sysclk_div_index = 0;
> +		     sysclk_div_index < ARRAY_SIZE(sysclk_div_table);
> +		     sysclk_div_index++) {
> +			if (sysclk_div_table[sysclk_div_index] *
> +				    sysclk_pclk_ratio ==
> +			    pclk_div_table[pclk_div_index])

> +				goto sysclk_pclk_ratio_found;

> +		}
> +	}

And instead of this goto mess, factor out to a helper.

> +	return -EINVAL;

> +sysclk_pclk_ratio_found:
> +
> +	/* Determine an appropriate post divider. */
> +	for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
> +	     post_div_index++) {
> +		vco_out = pclk * pclk_div_table[pclk_div_index] *
> +			  post_div_table[post_div_index];
> +
> +		if (vco_out >= HM1246_PLL_VCO_MIN &&
> +		    vco_out <= HM1246_PLL_VCO_MAX)
> +			break;
> +	}
> +	if (post_div_index >= ARRAY_SIZE(post_div_table))
> +		return -EINVAL;
> +
> +	/* Find best pre-divider and multiplier values. */
> +	best_vco_diff = U32_MAX;
> +	for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX);
> +	     div <= xclk / HM1246_PLL_INCLK_MIN; div++) {
> +		u32 multi, multi_h, multi_l, vco, diff;
> +
> +		multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
> +		if (multi < HM1246_PLL_MULTI_MIN ||
> +		    multi > HM1246_PLL_MULTI_MAX)
> +			continue;
> +
> +		multi_h = multi / (HM1246_PLL_MULTI_H_MIN *
> +				   HM1246_PLL_MULTI_L_MAX) +
> +			  2;
> +		multi_l = multi / multi_h;
> +		vco = div_u64((u64)xclk * multi_h * multi_l, div);
> +
> +		diff = abs_diff(vco_out, vco);
> +
> +		if (diff < best_vco_diff) {
> +			best_vco_diff = diff;
> +			pre_div = div;
> +			multiplier_h = multi_h;
> +			multiplier_l = multi_l;
> +		}
> +
> +		if (!diff)
> +			break;
> +	}
> +
> +	if (best_vco_diff == U32_MAX)
> +		return -EINVAL;
> +
> +	*pll1 = HM1246_PLL1CFG_MULTIPLIER(multiplier_l - 1);
> +	*pll2 = HM1246_PLL2CFG_PRE_DIV(pre_div - 1) |
> +		HM1246_PLL2CFG_MULTIPLIER(multiplier_h - 2);
> +	*pll3 = HM1246_PLL3CFG_POST_DIV(post_div_index) |
> +		HM1246_PLL3CFG_SYSCLK_DIV(sysclk_div_index) |
> +		HM1246_PLL3CFG_PCLK_DIV(pclk_div_index);
> +
> +	return 0;
> +}

...

> +static int hm1246_cci_write_pll(struct hm1246 *hm1246, u8 pll1, u8 pll2,
> +				u8 pll3)
> +{
> +	const struct cci_reg_sequence pll_regs[] = {

static ?

> +		{ HM1246_PLL1CFG_REG, pll1 },
> +		{ HM1246_PLL2CFG_REG, pll2 },
> +		{ HM1246_PLL3CFG_REG, pll3 },
> +		{ HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
> +	};

I would even move it outside the function. Note, static const maybe located in
ro memory while w/o static it's just a guarantee that compiler doesn't change
the values. Hence there is no guarantee it will be in ro memory.

> +	return cci_multi_reg_write(hm1246->regmap, pll_regs,
> +				   ARRAY_SIZE(pll_regs), NULL);
> +}
> +
> +static int hm1246_pll_check_locked(struct hm1246 *hm1246)
> +{
> +	u64 boot_ref2;
> +	int ret;
> +
> +	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
> +		       NULL);

Despite being longer 80 I still would put it on one line. It will increase readability.

	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);

Another option is to define local regmap:

	struct regmap *map = hm1246->regmap;
	...
	ret = cci_read(map, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);

which will be most readable and satisfy 80 limit.


> +	if (ret)
> +		return ret;
> +
> +	return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;

Think about similar improvements elsewhere in this driver.

> +}

...

> +	/* PLL lock time: tpll typ. 100us */

It's not a variable name, use proper English.

> +	fsleep(200);

...

> +static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode, u16 r,
> +					 u16 g, u16 b)

Use logical split.

static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode,
					 u16 r, u16 g, u16 b)

This applies to other similar places in the code.

...

> +static int hm1246_test_pattern(struct hm1246 *hm1246, u32 index)
> +{
> +	const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;

0 is enough (no need 0x).


So, the MAX is 10-bit, Can we use rather (BIT(10) - 1) to show this?

> +	const struct tp {
> +		int pattern;
> +		u16 r, g, b;
> +	} tps[] = {
> +		/* 0 - Disabled */

Instead of indices in the comment, make the code robust

> +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },

		[0] = { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },

It even fit 80 characters.

> +		/* 1 - Checkboard pattern */
> +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> +		/* 2 - Ramp */
> +		{ .pattern = 1, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> +		/* 3 - Moving ones */
> +		{ .pattern = 2, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> +		/* 4 - Blending color bars */
> +		{ .pattern = 3, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> +		/* 5 - Color bars */
> +		{ .pattern = 4, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> +		/* 6 - Solid white */
> +		{ .pattern = 15, .r = RGBMAX, .g = RGBMAX, .b = RGBMAX },
> +		/* 7 - Solid black */
> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> +		/* 8 - Solid red */
> +		{ .pattern = 15, .r = RGBMAX, .g = RGBMIN, .b = RGBMIN },
> +		/* 9 - Solid green */
> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMAX, .b = RGBMIN },
> +		/* 10- Solid blue */
> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMAX },
> +	};
> +	u8 mode;
> +
> +	if (index >= ARRAY_SIZE(tps))
> +		return -EINVAL;
> +
> +	mode = HM1246_TEST_PATTERN_MODE_MODE(tps[index].pattern);
> +	if (index)
> +		mode |= HM1246_TEST_PATTERN_MODE_ENABLE;
> +
> +	return hm1246_cci_write_test_pattern(hm1246, mode, tps[index].r,
> +					     tps[index].g, tps[index].b);
> +}

...

> +static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct hm1246 *hm1246 = container_of_const(ctrl->handler, struct hm1246,
> +						   ctrls);

Why _const()?
Why not split it between lines like:

	struct hm1246 *hm1246 =
		container_of_const(ctrl->handler, struct hm1246, ctrls);

> +	struct v4l2_subdev_state *state;
> +	const struct v4l2_mbus_framefmt *format;
> +	u32 val;
> +	bool needs_cmu_update = true;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&hm1246->sd);
> +	format = v4l2_subdev_state_get_format(state, 0);
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		s64 exposure_max;
> +
> +		exposure_max =
> +			format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN;
> +		ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl,
> +					       hm1246->exposure_ctrl->minimum,
> +					       exposure_max,
> +					       hm1246->exposure_ctrl->step,
> +					       exposure_max);
> +
> +		if (ret) {
> +			dev_err(hm1246->dev, "exposure ctrl range update failed\n");
> +			return ret;
> +		}
> +	}

> +	if (!pm_runtime_get_if_active(hm1246->dev))
> +		return 0;

Use ACQUIRE() and return directly where it makes sense.

> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
> +			  &ret);
> +		break;
> +
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
> +			  ctrl->val, &ret);
> +		break;
> +
> +	case V4L2_CID_VBLANK:
> +		val = format->height + ctrl->val;
> +		cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
> +			  &ret);
> +		break;
> +
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		val = 0;
> +		if (hm1246->hflip_ctrl->val)
> +			val |= HM1246_IMAGE_ORIENTATION_HFLIP;
> +		if (hm1246->vflip_ctrl->val)
> +			val |= HM1246_IMAGE_ORIENTATION_VFLIP;
> +
> +		cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val,
> +			  &ret);
> +		break;
> +
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = hm1246_test_pattern(hm1246, ctrl->val);
> +		needs_cmu_update = false;

Like here, and you won't need needs_cmu_update anymore.

> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		needs_cmu_update = false;
> +		break;
> +	}
> +
> +	if (needs_cmu_update)
> +		cci_write(hm1246->regmap, HM1246_CMU_UPDATE_REG, 0, &ret);
> +
> +	pm_runtime_put(hm1246->dev);
> +
> +	return ret;
> +}

...

> +static int hm1246_identify_module(struct hm1246 *hm1246)

This is a singleton function, right?

Check what once.h (or even once_lite.h) provides for you for such a case,
and drop unneeded "identified" variable.

> +{
> +	u64 model_id;
> +	int ret;
> +
> +	if (hm1246->identified)
> +		return 0;
> +
> +	ret = cci_read(hm1246->regmap, HM1246_MODEL_ID_REG, &model_id, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (model_id != HM1246_MODEL_ID) {
> +		dev_err(hm1246->dev, "model id mismatch: 0x%llx!=0x%x\n",
> +			model_id, HM1246_MODEL_ID);
> +		return -ENXIO;
> +	}
> +
> +	hm1246->identified = true;
> +
> +	return 0;
> +}

...

> +static int __maybe_unused hm1246_g_register(struct v4l2_subdev *sd,
> +					    struct v4l2_dbg_register *reg)

If v4l2.h doesn't provide a ptr_*() macro for these cases, I recommend to add and drop these __maybe_unused.

> +{
> +	struct hm1246 *hm1246 = to_hm1246(sd);
> +	u64 val;
> +	int ret;
> +
> +	if (!pm_runtime_get_if_in_use(hm1246->dev))
> +		return 0;
> +
> +	ret = cci_read(hm1246->regmap, CCI_REG8(reg->reg), &val, NULL);
> +	reg->val = val;
> +
> +	pm_runtime_put(hm1246->dev);
> +
> +	return ret;
> +}

...

> +	ret = cci_write(hm1246->regmap, CCI_REG8(reg->reg), (u64)reg->val,
> +			NULL);

Do you need casting?

...

> +	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(hm1246->dev), 0,
> +						   0,
> +						   FWNODE_GRAPH_ENDPOINT_NEXT);

Better split is

	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(hm1246->dev),
						   0, 0,
						   FWNODE_GRAPH_ENDPOINT_NEXT);

> +	if (!endpoint)
> +		return dev_err_probe(hm1246->dev, -EINVAL,
> +				     "missing endpoint node\n");

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(hm1246_pm_ops, hm1246_power_off,
> +				 hm1246_power_on, NULL);

Use logical split:

static DEFINE_RUNTIME_DEV_PM_OPS(hm1246_pm_ops,
				 hm1246_power_off, hm1246_power_on, NULL);
-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Tarang Raval]

Hi Andy,

> > +static inline struct hm1246 *to_hm1246(struct v4l2_subdev *sd)
> > +{
> > +     return container_of_const(sd, struct hm1246, sd);
>
> It's unclear and confusing that _const() variant is used here.
> Either const qualifier is missed somewhere, or _const is redundant.

The use of container_of_const() here is intentional and follows the direction 
taken across multiple recent sensor drivers suggested by Sakari. 
(e.g. ov2735, vd56g3, vd55g1, ov64a40, imx283).

AFAIK, using container_of_const() is a no-op for non-const 
arguments, but keeps the helper type-safe and future-proof against 
possible changes in the V4L2 API. This also maintains consistency with 
other upstream drivers and avoids subtle warnings if any of the subdev 
Callbacks later become const-qualified.

But yes, I guess Sakari can give you a better answer.

Best Regards,
Tarang

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Andy Shevchenko]

On Wed, Nov 05, 2025 at 05:53:30AM +0000, Tarang Raval wrote:

...

> > > +static inline struct hm1246 *to_hm1246(struct v4l2_subdev *sd)

> > > +     return container_of_const(sd, struct hm1246, sd);
> >
> > It's unclear and confusing that _const() variant is used here.
> > Either const qualifier is missed somewhere, or _const is redundant.
> 
> The use of container_of_const() here is intentional and follows the direction 
> taken across multiple recent sensor drivers suggested by Sakari. 
> (e.g. ov2735, vd56g3, vd55g1, ov64a40, imx283).
> 
> AFAIK, using container_of_const() is a no-op for non-const 
> arguments,

I believe you want to say that it has no additional effect on the result or so.
Because it may not be a no-op, otherwise code won't work as expected.

> but keeps the helper type-safe and future-proof against 
> possible changes in the V4L2 API. This also maintains consistency with 
> other upstream drivers and avoids subtle warnings if any of the subdev 
> Callbacks later become const-qualified.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Tarang Raval]

Hi Andy,

> > > > +static inline struct hm1246 *to_hm1246(struct v4l2_subdev *sd)
>
> > > > +     return container_of_const(sd, struct hm1246, sd);
> > >
> > > It's unclear and confusing that _const() variant is used here.
> > > Either const qualifier is missed somewhere, or _const is redundant.
> >
> > The use of container_of_const() here is intentional and follows the direction
> > taken across multiple recent sensor drivers suggested by Sakari.
> > (e.g. ov2735, vd56g3, vd55g1, ov64a40, imx283).
> >
> > AFAIK, using container_of_const() is a no-op for non-const
> > arguments,
>
> I believe you want to say that it has no additional effect on the result or so.
> Because it may not be a no-op, otherwise code won't work as expected.

Yes, that's what I meant.

Best Regards,
Tarang

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Matthias Fend]

Hi Andy,

thank you for your feedback and suggestions for improvement.
I have some questions about a few points.

Am 04.11.2025 um 14:28 schrieb Andy Shevchenko:
> On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:
>> Add a V4L2 sub-device driver for Himax HM1246 image sensor.
>>
>> The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
>> array size of 1296 x 976. It is programmable through an I2C interface and
>> connected via parallel bus.
>>
>> The sensor has an internal ISP with a complete image processing pipeline
>> including control loops. However, this driver uses the sensor in raw mode
>> and the entire ISP is bypassed.
> 
> ...
> 
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/units.h>
> 
> This block is semi-random.
> First of all, no new code must use gpio.h, use the proper one.
> Second, many are missing, e.g., bits.h, regmap.h, types.h.
> Please, follow IWYU principle (Include What You Use).

I've noticed that you've already modified the include statements in many 
source files. I assume you've automated this somehow. May I ask how you 
did that, or is there a reliable process for verifying the include 
statements?

...

> 
>> +static int hm1246_cci_write_pll(struct hm1246 *hm1246, u8 pll1, u8 pll2,
>> +				u8 pll3)
>> +{
>> +	const struct cci_reg_sequence pll_regs[] = {
> 
> static ?
> 
>> +		{ HM1246_PLL1CFG_REG, pll1 },
>> +		{ HM1246_PLL2CFG_REG, pll2 },
>> +		{ HM1246_PLL3CFG_REG, pll3 },
>> +		{ HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
>> +	};
> 
> I would even move it outside the function. Note, static const maybe located in
> ro memory while w/o static it's just a guarantee that compiler doesn't change
> the values. Hence there is no guarantee it will be in ro memory.

The sequence is initialized with values ​​from the arguments, which are 
not constant. Therefore, the sequence cannot be put into a `ro` section.

> 
>> +	return cci_multi_reg_write(hm1246->regmap, pll_regs,
>> +				   ARRAY_SIZE(pll_regs), NULL);
>> +}
>> +
>> +static int hm1246_pll_check_locked(struct hm1246 *hm1246)
>> +{
>> +	u64 boot_ref2;
>> +	int ret;
>> +
>> +	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
>> +		       NULL);
> 
> Despite being longer 80 I still would put it on one line. It will increase readability.
> 
> 	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> 
> Another option is to define local regmap:
> 
> 	struct regmap *map = hm1246->regmap;
> 	...
> 	ret = cci_read(map, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> 
> which will be most readable and satisfy 80 limit.

Doing things differently is kind of a dilemma.
Compliance with the 80-line limit is required, and local variables that 
are only used once are also undesirable.
The unsightly line break seems to be the most acceptable option, right?
Or rename the local variable to 'bref2'.

> 
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;
> 
> Think about similar improvements elsewhere in this driver.
> 
>> +}
> 
> ...
> 
>> +	/* PLL lock time: tpll typ. 100us */
> 
> It's not a variable name, use proper English.

I think you refer to "tpll typ."? I intentionally wanted to use the same 
symbols as in the datasheet. I also used them for the other delays:

/*
  * XSHUTDOWN to crystal clock oscillation:  tcrystal typ.  650us
  * Sample bootstrap pin:                    tsample  max. 2000us
  * Built in self test:                      tbist    max. 3000us
  */

Is this acceptable from this perspective?
> 
>> +	fsleep(200);
> 
> ...
> 
>> +static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode, u16 r,
>> +					 u16 g, u16 b)
> 
> Use logical split.
> 
> static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode,
> 					 u16 r, u16 g, u16 b)
> 
> This applies to other similar places in the code.
> 
> ...
> 
>> +static int hm1246_test_pattern(struct hm1246 *hm1246, u32 index)
>> +{
>> +	const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;
> 
> 0 is enough (no need 0x).
> 
> 
> So, the MAX is 10-bit, Can we use rather (BIT(10) - 1) to show this?

Sure, I can write it that way too, even though the hexadecimal number 
seems easier for me to read.

> 
>> +	const struct tp {
>> +		int pattern;
>> +		u16 r, g, b;
>> +	} tps[] = {
>> +		/* 0 - Disabled */
> 
> Instead of indices in the comment, make the code robust
> 
>> +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> 
> 		[0] = { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> 
> It even fit 80 characters.
> 
>> +		/* 1 - Checkboard pattern */
>> +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>> +		/* 2 - Ramp */
>> +		{ .pattern = 1, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>> +		/* 3 - Moving ones */
>> +		{ .pattern = 2, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>> +		/* 4 - Blending color bars */
>> +		{ .pattern = 3, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>> +		/* 5 - Color bars */
>> +		{ .pattern = 4, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>> +		/* 6 - Solid white */
>> +		{ .pattern = 15, .r = RGBMAX, .g = RGBMAX, .b = RGBMAX },
>> +		/* 7 - Solid black */
>> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>> +		/* 8 - Solid red */
>> +		{ .pattern = 15, .r = RGBMAX, .g = RGBMIN, .b = RGBMIN },
>> +		/* 9 - Solid green */
>> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMAX, .b = RGBMIN },
>> +		/* 10- Solid blue */
>> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMAX },
>> +	};
>> +	u8 mode;
>> +
>> +	if (index >= ARRAY_SIZE(tps))
>> +		return -EINVAL;
>> +
>> +	mode = HM1246_TEST_PATTERN_MODE_MODE(tps[index].pattern);
>> +	if (index)
>> +		mode |= HM1246_TEST_PATTERN_MODE_ENABLE;
>> +
>> +	return hm1246_cci_write_test_pattern(hm1246, mode, tps[index].r,
>> +					     tps[index].g, tps[index].b);
>> +}
> 
> ...
> 
>> +static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct hm1246 *hm1246 = container_of_const(ctrl->handler, struct hm1246,
>> +						   ctrls);
> 
> Why _const()?
> Why not split it between lines like:
> 
> 	struct hm1246 *hm1246 =
> 		container_of_const(ctrl->handler, struct hm1246, ctrls);
> 
>> +	struct v4l2_subdev_state *state;
>> +	const struct v4l2_mbus_framefmt *format;
>> +	u32 val;
>> +	bool needs_cmu_update = true;
>> +	int ret = 0;
>> +
>> +	state = v4l2_subdev_get_locked_active_state(&hm1246->sd);
>> +	format = v4l2_subdev_state_get_format(state, 0);
>> +
>> +	if (ctrl->id == V4L2_CID_VBLANK) {
>> +		s64 exposure_max;
>> +
>> +		exposure_max =
>> +			format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN;
>> +		ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl,
>> +					       hm1246->exposure_ctrl->minimum,
>> +					       exposure_max,
>> +					       hm1246->exposure_ctrl->step,
>> +					       exposure_max);
>> +
>> +		if (ret) {
>> +			dev_err(hm1246->dev, "exposure ctrl range update failed\n");
>> +			return ret;
>> +		}
>> +	}
> 
>> +	if (!pm_runtime_get_if_active(hm1246->dev))
>> +		return 0;
> 
> Use ACQUIRE() and return directly where it makes sense.

That would indeed be a great solution. However, I haven't found any 
guard definitions for pm_runtime_get_if_active and therefore don't know 
how to adapt this functionality (increase usage only if already enabled) 
to use ACQUIRE.
Any clue on this?

> 
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_EXPOSURE:
>> +		cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
>> +			  &ret);
>> +		break;
>> +
>> +	case V4L2_CID_ANALOGUE_GAIN:
>> +		cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
>> +			  ctrl->val, &ret);
>> +		break;
>> +
>> +	case V4L2_CID_VBLANK:
>> +		val = format->height + ctrl->val;
>> +		cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
>> +			  &ret);
>> +		break;
>> +
>> +	case V4L2_CID_HFLIP:
>> +	case V4L2_CID_VFLIP:
>> +		val = 0;
>> +		if (hm1246->hflip_ctrl->val)
>> +			val |= HM1246_IMAGE_ORIENTATION_HFLIP;
>> +		if (hm1246->vflip_ctrl->val)
>> +			val |= HM1246_IMAGE_ORIENTATION_VFLIP;
>> +
>> +		cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val,
>> +			  &ret);
>> +		break;
>> +
>> +	case V4L2_CID_TEST_PATTERN:
>> +		ret = hm1246_test_pattern(hm1246, ctrl->val);
>> +		needs_cmu_update = false;
> 
> Like here, and you won't need needs_cmu_update anymore.
> 
>> +		break;
>> +
>> +	default:
>> +		ret = -EINVAL;
>> +		needs_cmu_update = false;
>> +		break;
>> +	}
>> +
>> +	if (needs_cmu_update)
>> +		cci_write(hm1246->regmap, HM1246_CMU_UPDATE_REG, 0, &ret);
>> +
>> +	pm_runtime_put(hm1246->dev);
>> +
>> +	return ret;
>> +}
> 
> ...
> 
>> +static int hm1246_identify_module(struct hm1246 *hm1246)
> 
> This is a singleton function, right?
> 
> Check what once.h (or even once_lite.h) provides for you for such a case,
> and drop unneeded "identified" variable.

As I understand it, the ONCE macros create a static variable in the 
driver module. This means the function would only be called once in 
total, but the function should be called once per device.
Therefore, I don't think that's an option here.

> 
>> +{
>> +	u64 model_id;
>> +	int ret;
>> +
>> +	if (hm1246->identified)
>> +		return 0;
>> +
>> +	ret = cci_read(hm1246->regmap, HM1246_MODEL_ID_REG, &model_id, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (model_id != HM1246_MODEL_ID) {
>> +		dev_err(hm1246->dev, "model id mismatch: 0x%llx!=0x%x\n",
>> +			model_id, HM1246_MODEL_ID);
>> +		return -ENXIO;
>> +	}
>> +
>> +	hm1246->identified = true;
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int __maybe_unused hm1246_g_register(struct v4l2_subdev *sd,
>> +					    struct v4l2_dbg_register *reg)
> 
> If v4l2.h doesn't provide a ptr_*() macro for these cases, I recommend to add and drop these __maybe_unused.

I'm not aware of anything like that.
Currently, it's common practice to use '#ifdef CONFIG_VIDEO_ADV_DEBUG', 
and a macro that would simplify that would probably be worthwhile.
But I think that's for something outside the scope of this patch.

Thanks
  ~Matthias

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Andy Shevchenko]

On Wed, Nov 05, 2025 at 05:59:20PM +0100, Matthias Fend wrote:
> Am 04.11.2025 um 14:28 schrieb Andy Shevchenko:
> > On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:

...

> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/units.h>
> > 
> > This block is semi-random.
> > First of all, no new code must use gpio.h, use the proper one.
> > Second, many are missing, e.g., bits.h, regmap.h, types.h.
> > Please, follow IWYU principle (Include What You Use).
> 
> I've noticed that you've already modified the include statements in many
> source files. I assume you've automated this somehow. May I ask how you did
> that, or is there a reliable process for verifying the include statements?

No automation is available so far (the iwyu tool needs a lot of tuning for
Linux kernel). I did it by simply reading the code. Since you are the author
you should know how to fill the inclusions much better than me.

...

> > > +	const struct cci_reg_sequence pll_regs[] = {
> > 
> > static ?
> > 
> > > +		{ HM1246_PLL1CFG_REG, pll1 },
> > > +		{ HM1246_PLL2CFG_REG, pll2 },
> > > +		{ HM1246_PLL3CFG_REG, pll3 },
> > > +		{ HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
> > > +	};
> > 
> > I would even move it outside the function. Note, static const maybe located in
> > ro memory while w/o static it's just a guarantee that compiler doesn't change
> > the values. Hence there is no guarantee it will be in ro memory.
> 
> The sequence is initialized with values ​​from the arguments, which are not
> constant. Therefore, the sequence cannot be put into a `ro` section.

Ah, indeed, you are right. It can, but it will be an intermediate (unnecessary)
step.

...

> > > +static int hm1246_pll_check_locked(struct hm1246 *hm1246)
> > > +{
> > > +	u64 boot_ref2;
> > > +	int ret;
> > > +
> > > +	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
> > > +		       NULL);
> > 
> > Despite being longer 80 I still would put it on one line. It will increase readability.
> > 
> > 	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> > 
> > Another option is to define local regmap:
> > 
> > 	struct regmap *map = hm1246->regmap;
> > 	...
> > 	ret = cci_read(map, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> > 
> > which will be most readable and satisfy 80 limit.
> 
> Doing things differently is kind of a dilemma.
> Compliance with the 80-line limit is required, and local variables that are
> only used once are also undesirable.

Why? If it increases readability than it _is_ desirable change.

> The unsightly line break seems to be the most acceptable option, right?
> Or rename the local variable to 'bref2'.

Making variable cryptic wouldn't help readability.

> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;
> > 
> > Think about similar improvements elsewhere in this driver.
> > 
> > > +}

...

> > > +	/* PLL lock time: tpll typ. 100us */
> > 
> > It's not a variable name, use proper English.
> 
> I think you refer to "tpll typ."? I intentionally wanted to use the same
> symbols as in the datasheet. I also used them for the other delays:
> 
> /*
>  * XSHUTDOWN to crystal clock oscillation:  tcrystal typ.  650us
>  * Sample bootstrap pin:                    tsample  max. 2000us
>  * Built in self test:                      tbist    max. 3000us
>  */
> 
> Is this acceptable from this perspective?

This is unfortunate. When it goes with max/min is much more understandable than
standalone. Try to find a compromise. Datasheet is not a golden standard, it's
just good to have something close enough to it, but it doesn't mean we have to
follow it _literally_.

> > > +	fsleep(200);

...

> > > +	const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;
> > 
> > 0 is enough (no need 0x).
> > 
> > So, the MAX is 10-bit, Can we use rather (BIT(10) - 1) to show this?
> 
> Sure, I can write it that way too, even though the hexadecimal number seems
> easier for me to read.

The hexadecimal sometimes too abstract and writing the same in a slightly
different form may give a useful additional information.

...

> > > +	if (!pm_runtime_get_if_active(hm1246->dev))
> > > +		return 0;
> > 
> > Use ACQUIRE() and return directly where it makes sense.
> 
> That would indeed be a great solution. However, I haven't found any guard
> definitions for pm_runtime_get_if_active and therefore don't know how to
> adapt this functionality (increase usage only if already enabled) to use
> ACQUIRE.
> Any clue on this?

Add a patch to add this conditional guard to pm_runtime.h.

...

> > > +static int hm1246_identify_module(struct hm1246 *hm1246)
> > 
> > This is a singleton function, right?
> > 
> > Check what once.h (or even once_lite.h) provides for you for such a case,
> > and drop unneeded "identified" variable.
> 
> As I understand it, the ONCE macros create a static variable in the driver
> module. This means the function would only be called once in total, but the
> function should be called once per device.
> Therefore, I don't think that's an option here.

Good point.

...

> > > +static int __maybe_unused hm1246_g_register(struct v4l2_subdev *sd,
> > > +					    struct v4l2_dbg_register *reg)
> > 
> > If v4l2.h doesn't provide a ptr_*() macro for these cases, I recommend to add and drop these __maybe_unused.
> 
> I'm not aware of anything like that.
> Currently, it's common practice to use '#ifdef CONFIG_VIDEO_ADV_DEBUG', and
> a macro that would simplify that would probably be worthwhile.
> But I think that's for something outside the scope of this patch.

This one may be out of scope, so up to you. But just note that using
__maybe_unused is discouraged.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Sakari Ailus]

Hi Matthias,

On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:
> +static int hm1246_init_controls(struct hm1246 *hm1246)
> +{
> +	const struct hm1246_mode *mode = &hm1246_modes[0];
> +	struct v4l2_fwnode_device_properties props;
> +	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	s64 pixel_rate, exposure_max, vblank_min, hblank;
> +	int ret;
> +
> +	ctrl_hdlr = &hm1246->ctrls;
> +	v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +
> +	hm1246->hflip_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
> +					       V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	if (hm1246->hflip_ctrl)
> +		hm1246->hflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	hm1246->vflip_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
> +					       V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	if (hm1246->vflip_ctrl)
> +		hm1246->vflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	v4l2_ctrl_cluster(2, &hm1246->hflip_ctrl);
> +
> +	hm1246->link_freq_ctrl =
> +		v4l2_ctrl_new_int_menu(ctrl_hdlr, &hm1246_ctrl_ops,
> +				       V4L2_CID_LINK_FREQ,
> +				       ARRAY_SIZE(hm1246_link_freqs) - 1, 0,
> +				       hm1246_link_freqs);
> +	if (hm1246->link_freq_ctrl)
> +		hm1246->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	pixel_rate = div_u64(hm1246_link_freqs[mode->link_freq_index],
> +			     mode->clocks_per_pixel);
> +	hm1246->pixel_rate_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
> +						    V4L2_CID_PIXEL_RATE,
> +						    pixel_rate, pixel_rate, 1,
> +						    pixel_rate);
> +
> +	vblank_min = mode->vts_min - mode->height;
> +	hm1246->vblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
> +						V4L2_CID_VBLANK, vblank_min,
> +						HM1246_VTS_MAX - mode->height,
> +						1, vblank_min);
> +
> +	hblank = mode->hts - mode->width;
> +	hm1246->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
> +						V4L2_CID_HBLANK, hblank, hblank,
> +						1, hblank);
> +	if (hm1246->hblank_ctrl)
> +		hm1246->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +			  HM1246_ANALOG_GLOBAL_GAIN_MIN,
> +			  HM1246_ANALOG_GLOBAL_GAIN_MAX,
> +			  HM1246_ANALOG_GLOBAL_GAIN_STEP,
> +			  HM1246_ANALOG_GLOBAL_GAIN_MIN);
> +
> +	exposure_max = mode->vts_min - HM1246_COARSE_INTG_MARGIN;
> +	hm1246->exposure_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &hm1246_ctrl_ops,
> +						  V4L2_CID_EXPOSURE,
> +						  HM1246_COARSE_INTG_MIN,
> +						  exposure_max,
> +						  HM1246_COARSE_INTG_STEP,
> +						  exposure_max);
> +
> +	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &hm1246_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(hm1246_test_pattern_menu) - 1,
> +				     0, 0, hm1246_test_pattern_menu);
> +
> +	ret = v4l2_fwnode_device_parse(hm1246->dev, &props);
> +	if (ret)
> +		goto err_v4l2_ctrl_handler_free;

If you move this to the beginning of the function, you can return if this
fails.

> +
> +	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &hm1246_ctrl_ops,
> +					      &props);
> +	if (ret)
> +		goto err_v4l2_ctrl_handler_free;

I'd omit the error check for this altogether. See also
<20251111112057.95655-1-sakari.ailus@linux.intel.com> on LMML.

> +
> +	if (ctrl_hdlr->error) {
> +		ret = ctrl_hdlr->error;
> +		goto err_v4l2_ctrl_handler_free;
> +	}

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Sakari Ailus]

Hi Andy, Matthias,

On Tue, Nov 04, 2025 at 03:28:54PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:
> > Add a V4L2 sub-device driver for Himax HM1246 image sensor.
> > 
> > The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
> > array size of 1296 x 976. It is programmable through an I2C interface and
> > connected via parallel bus.
> > 
> > The sensor has an internal ISP with a complete image processing pipeline
> > including control loops. However, this driver uses the sensor in raw mode
> > and the entire ISP is bypassed.
> 
> ...
> 
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/units.h>
> 
> This block is semi-random.
> First of all, no new code must use gpio.h, use the proper one.
> Second, many are missing, e.g., bits.h, regmap.h, types.h.
> Please, follow IWYU principle (Include What You Use).
> 
> ...
> 
> > +static inline struct hm1246 *to_hm1246(struct v4l2_subdev *sd)
> > +{
> > +	return container_of_const(sd, struct hm1246, sd);
> 
> It's unclear and confusing that _const() variant is used here.
> Either const qualifier is missed somewhere, or _const is redundant.
> 
> > +}
> 
> ...
> 
> > +	/*
> > +	 * XSHUTDOWN to crystal clock oscillation:  tcrystal typ.  650us
> > +	 * Sample bootstrap pin:                    tsample  max. 2000us
> > +	 * Built in self test:                      tbist    max. 3000us
> > +	 */
> > +	fsleep(6000);
> 
> Also possible to write as 6 * USEC_PER_MSEC
> 
> ...
> 
> > +	format = v4l2_subdev_state_get_format(state, 0);
> > +	mode = v4l2_find_nearest_size(hm1246_modes, ARRAY_SIZE(hm1246_modes),
> > +				      width, height, format->width,
> > +				      format->height);
> > +
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> > +		return 0;
> > +
> > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > +		sel->r.top = 0;
> > +		sel->r.left = 0;
> > +		sel->r.width = HM1246_NATIVE_WIDTH;
> > +		sel->r.height = HM1246_NATIVE_HEIGHT;
> > +		return 0;
> > +
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +		sel->r.top = mode->top;
> > +		sel->r.left = mode->left;
> > +		sel->r.width = mode->width;
> > +		sel->r.height = mode->height;
> > +		return 0;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> > +	return 0;
> 
> Do we need this line?
> 
> ...
> 
> > +static int hm1246_calc_pll(struct hm1246 *hm1246, u32 xclk, u32 link_freq,
> > +			   u32 clocks_per_pixel, u8 *pll1, u8 *pll2, u8 *pll3)
> > +{
> > +	const u8 pclk_div_table[] = { 4, 5, 6, 7, 8, 12, 14, 16 };
> > +	const u8 sysclk_div_table[] = { 1, 2, 3, 4 };
> > +	const u8 post_div_table[] = { 1, 2, 4, 8 };
> > +	const int sysclk_pclk_ratio = 3; /* Recommended value */
> > +	u32 pclk, vco_out, best_vco_diff;
> > +	int pclk_div_index, sysclk_div_index, post_div_index;
> > +	u8 pre_div = 0, multiplier_h = 0, multiplier_l = 0;
> > +
> > +	if (link_freq < HM1246_PCLK_MIN || link_freq > HM1246_PCLK_MAX)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * In raw mode (1 pixel per clock) the pixel clock is internally
> > +	 * divided by two.
> > +	 */
> > +	pclk = 2 * link_freq / clocks_per_pixel;
> > +
> > +	/* Find suitable PCLK and SYSCLK dividers. */
> > +	for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table);
> > +	     pclk_div_index++) {
> > +		for (sysclk_div_index = 0;
> > +		     sysclk_div_index < ARRAY_SIZE(sysclk_div_table);
> > +		     sysclk_div_index++) {
> > +			if (sysclk_div_table[sysclk_div_index] *
> > +				    sysclk_pclk_ratio ==
> > +			    pclk_div_table[pclk_div_index])
> 
> > +				goto sysclk_pclk_ratio_found;
> 
> > +		}
> > +	}
> 
> And instead of this goto mess, factor out to a helper.

I think the above looks fine as-is: it's easier to understand what it does
when it's all in a single location. This isn't overly complicated, which
would be another reason to do that.

> 
> > +	return -EINVAL;

I'd do:

	if (sysclk_div_index == ARRAY_SIZE(sysclk_div_table))
		return -EINVAL;

instead of using a goto.

> 
> > +sysclk_pclk_ratio_found:
> > +
> > +	/* Determine an appropriate post divider. */
> > +	for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
> > +	     post_div_index++) {
> > +		vco_out = pclk * pclk_div_table[pclk_div_index] *
> > +			  post_div_table[post_div_index];
> > +
> > +		if (vco_out >= HM1246_PLL_VCO_MIN &&
> > +		    vco_out <= HM1246_PLL_VCO_MAX)
> > +			break;
> > +	}
> > +	if (post_div_index >= ARRAY_SIZE(post_div_table))
> > +		return -EINVAL;
> > +
> > +	/* Find best pre-divider and multiplier values. */
> > +	best_vco_diff = U32_MAX;
> > +	for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX);
> > +	     div <= xclk / HM1246_PLL_INCLK_MIN; div++) {
> > +		u32 multi, multi_h, multi_l, vco, diff;
> > +
> > +		multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
> > +		if (multi < HM1246_PLL_MULTI_MIN ||
> > +		    multi > HM1246_PLL_MULTI_MAX)
> > +			continue;
> > +
> > +		multi_h = multi / (HM1246_PLL_MULTI_H_MIN *
> > +				   HM1246_PLL_MULTI_L_MAX) +
> > +			  2;
> > +		multi_l = multi / multi_h;
> > +		vco = div_u64((u64)xclk * multi_h * multi_l, div);
> > +
> > +		diff = abs_diff(vco_out, vco);
> > +
> > +		if (diff < best_vco_diff) {
> > +			best_vco_diff = diff;
> > +			pre_div = div;
> > +			multiplier_h = multi_h;
> > +			multiplier_l = multi_l;
> > +		}
> > +
> > +		if (!diff)
> > +			break;
> > +	}
> > +
> > +	if (best_vco_diff == U32_MAX)
> > +		return -EINVAL;
> > +
> > +	*pll1 = HM1246_PLL1CFG_MULTIPLIER(multiplier_l - 1);
> > +	*pll2 = HM1246_PLL2CFG_PRE_DIV(pre_div - 1) |
> > +		HM1246_PLL2CFG_MULTIPLIER(multiplier_h - 2);
> > +	*pll3 = HM1246_PLL3CFG_POST_DIV(post_div_index) |
> > +		HM1246_PLL3CFG_SYSCLK_DIV(sysclk_div_index) |
> > +		HM1246_PLL3CFG_PCLK_DIV(pclk_div_index);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int hm1246_cci_write_pll(struct hm1246 *hm1246, u8 pll1, u8 pll2,
> > +				u8 pll3)
> > +{
> > +	const struct cci_reg_sequence pll_regs[] = {
> 
> static ?
> 
> > +		{ HM1246_PLL1CFG_REG, pll1 },
> > +		{ HM1246_PLL2CFG_REG, pll2 },
> > +		{ HM1246_PLL3CFG_REG, pll3 },
> > +		{ HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
> > +	};
> 
> I would even move it outside the function. Note, static const maybe located in
> ro memory while w/o static it's just a guarantee that compiler doesn't change
> the values. Hence there is no guarantee it will be in ro memory.
> 
> > +	return cci_multi_reg_write(hm1246->regmap, pll_regs,
> > +				   ARRAY_SIZE(pll_regs), NULL);
> > +}
> > +
> > +static int hm1246_pll_check_locked(struct hm1246 *hm1246)
> > +{
> > +	u64 boot_ref2;
> > +	int ret;
> > +
> > +	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
> > +		       NULL);
> 
> Despite being longer 80 I still would put it on one line. It will increase readability.

I prefer it as-is.

> 
> 	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> 
> Another option is to define local regmap:
> 
> 	struct regmap *map = hm1246->regmap;
> 	...
> 	ret = cci_read(map, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> 
> which will be most readable and satisfy 80 limit.
> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;
> 
> Think about similar improvements elsewhere in this driver.
> 
> > +}
> 
> ...
> 
> > +	/* PLL lock time: tpll typ. 100us */
> 
> It's not a variable name, use proper English.
> 
> > +	fsleep(200);
> 
> ...
> 
> > +static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode, u16 r,
> > +					 u16 g, u16 b)
> 
> Use logical split.
> 
> static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode,
> 					 u16 r, u16 g, u16 b)
> 
> This applies to other similar places in the code.
> 
> ...
> 
> > +static int hm1246_test_pattern(struct hm1246 *hm1246, u32 index)
> > +{
> > +	const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;
> 
> 0 is enough (no need 0x).
> 
> 
> So, the MAX is 10-bit, Can we use rather (BIT(10) - 1) to show this?

The above value likely comes from a sensor datasheet; I think 0x3ff is fine
as-is. If this was a bitmask with a row of bits set, I'd use GENMASK(), but
not BIT(10) - 1.

> 
> > +	const struct tp {
> > +		int pattern;
> > +		u16 r, g, b;
> > +	} tps[] = {
> > +		/* 0 - Disabled */
> 
> Instead of indices in the comment, make the code robust
> 
> > +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> 
> 		[0] = { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> 
> It even fit 80 characters.
> 
> > +		/* 1 - Checkboard pattern */
> > +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> > +		/* 2 - Ramp */
> > +		{ .pattern = 1, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> > +		/* 3 - Moving ones */
> > +		{ .pattern = 2, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> > +		/* 4 - Blending color bars */
> > +		{ .pattern = 3, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> > +		/* 5 - Color bars */
> > +		{ .pattern = 4, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> > +		/* 6 - Solid white */
> > +		{ .pattern = 15, .r = RGBMAX, .g = RGBMAX, .b = RGBMAX },
> > +		/* 7 - Solid black */
> > +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> > +		/* 8 - Solid red */
> > +		{ .pattern = 15, .r = RGBMAX, .g = RGBMIN, .b = RGBMIN },
> > +		/* 9 - Solid green */
> > +		{ .pattern = 15, .r = RGBMIN, .g = RGBMAX, .b = RGBMIN },
> > +		/* 10- Solid blue */
> > +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMAX },
> > +	};
> > +	u8 mode;
> > +
> > +	if (index >= ARRAY_SIZE(tps))
> > +		return -EINVAL;
> > +
> > +	mode = HM1246_TEST_PATTERN_MODE_MODE(tps[index].pattern);
> > +	if (index)
> > +		mode |= HM1246_TEST_PATTERN_MODE_ENABLE;
> > +
> > +	return hm1246_cci_write_test_pattern(hm1246, mode, tps[index].r,
> > +					     tps[index].g, tps[index].b);
> > +}
> 
> ...
> 
> > +static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct hm1246 *hm1246 = container_of_const(ctrl->handler, struct hm1246,
> > +						   ctrls);
> 
> Why _const()?
> Why not split it between lines like:
> 
> 	struct hm1246 *hm1246 =
> 		container_of_const(ctrl->handler, struct hm1246, ctrls);
> 
> > +	struct v4l2_subdev_state *state;
> > +	const struct v4l2_mbus_framefmt *format;
> > +	u32 val;
> > +	bool needs_cmu_update = true;
> > +	int ret = 0;
> > +
> > +	state = v4l2_subdev_get_locked_active_state(&hm1246->sd);
> > +	format = v4l2_subdev_state_get_format(state, 0);
> > +
> > +	if (ctrl->id == V4L2_CID_VBLANK) {
> > +		s64 exposure_max;
> > +
> > +		exposure_max =
> > +			format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN;
> > +		ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl,
> > +					       hm1246->exposure_ctrl->minimum,
> > +					       exposure_max,
> > +					       hm1246->exposure_ctrl->step,
> > +					       exposure_max);
> > +
> > +		if (ret) {
> > +			dev_err(hm1246->dev, "exposure ctrl range update failed\n");
> > +			return ret;
> > +		}
> > +	}
> 
> > +	if (!pm_runtime_get_if_active(hm1246->dev))
> > +		return 0;
> 
> Use ACQUIRE() and return directly where it makes sense.

That's interesting.

> 
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_EXPOSURE:
> > +		cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
> > +			  &ret);
> > +		break;
> > +
> > +	case V4L2_CID_ANALOGUE_GAIN:
> > +		cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
> > +			  ctrl->val, &ret);
> > +		break;
> > +
> > +	case V4L2_CID_VBLANK:
> > +		val = format->height + ctrl->val;
> > +		cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
> > +			  &ret);
> > +		break;
> > +
> > +	case V4L2_CID_HFLIP:
> > +	case V4L2_CID_VFLIP:
> > +		val = 0;
> > +		if (hm1246->hflip_ctrl->val)
> > +			val |= HM1246_IMAGE_ORIENTATION_HFLIP;
> > +		if (hm1246->vflip_ctrl->val)
> > +			val |= HM1246_IMAGE_ORIENTATION_VFLIP;
> > +
> > +		cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val,
> > +			  &ret);
> > +		break;
> > +
> > +	case V4L2_CID_TEST_PATTERN:
> > +		ret = hm1246_test_pattern(hm1246, ctrl->val);
> > +		needs_cmu_update = false;
> 
> Like here, and you won't need needs_cmu_update anymore.
> 
> > +		break;
> > +
> > +	default:
> > +		ret = -EINVAL;
> > +		needs_cmu_update = false;
> > +		break;
> > +	}
> > +
> > +	if (needs_cmu_update)
> > +		cci_write(hm1246->regmap, HM1246_CMU_UPDATE_REG, 0, &ret);
> > +
> > +	pm_runtime_put(hm1246->dev);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +static int hm1246_identify_module(struct hm1246 *hm1246)
> 
> This is a singleton function, right?
> 
> Check what once.h (or even once_lite.h) provides for you for such a case,
> and drop unneeded "identified" variable.

Once for every device, so I don't think this applies.

> 
> > +{
> > +	u64 model_id;
> > +	int ret;
> > +
> > +	if (hm1246->identified)
> > +		return 0;
> > +
> > +	ret = cci_read(hm1246->regmap, HM1246_MODEL_ID_REG, &model_id, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (model_id != HM1246_MODEL_ID) {
> > +		dev_err(hm1246->dev, "model id mismatch: 0x%llx!=0x%x\n",
> > +			model_id, HM1246_MODEL_ID);
> > +		return -ENXIO;
> > +	}
> > +
> > +	hm1246->identified = true;
> > +
> > +	return 0;
> > +}

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Matthias Fend]

Hi Sakari,

Am 11.11.2025 um 13:23 schrieb Sakari Ailus:
> Hi Andy, Matthias,
> 
> On Tue, Nov 04, 2025 at 03:28:54PM +0200, Andy Shevchenko wrote:
>> On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:
>>> Add a V4L2 sub-device driver for Himax HM1246 image sensor.
>>>
>>> The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
>>> array size of 1296 x 976. It is programmable through an I2C interface and
>>> connected via parallel bus.
>>>
>>> The sensor has an internal ISP with a complete image processing pipeline
>>> including control loops. However, this driver uses the sensor in raw mode
>>> and the entire ISP is bypassed.
>>

...

>>
>>> +static int hm1246_calc_pll(struct hm1246 *hm1246, u32 xclk, u32 link_freq,
>>> +			   u32 clocks_per_pixel, u8 *pll1, u8 *pll2, u8 *pll3)
>>> +{
>>> +	const u8 pclk_div_table[] = { 4, 5, 6, 7, 8, 12, 14, 16 };
>>> +	const u8 sysclk_div_table[] = { 1, 2, 3, 4 };
>>> +	const u8 post_div_table[] = { 1, 2, 4, 8 };
>>> +	const int sysclk_pclk_ratio = 3; /* Recommended value */
>>> +	u32 pclk, vco_out, best_vco_diff;
>>> +	int pclk_div_index, sysclk_div_index, post_div_index;
>>> +	u8 pre_div = 0, multiplier_h = 0, multiplier_l = 0;
>>> +
>>> +	if (link_freq < HM1246_PCLK_MIN || link_freq > HM1246_PCLK_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	/*
>>> +	 * In raw mode (1 pixel per clock) the pixel clock is internally
>>> +	 * divided by two.
>>> +	 */
>>> +	pclk = 2 * link_freq / clocks_per_pixel;
>>> +
>>> +	/* Find suitable PCLK and SYSCLK dividers. */
>>> +	for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table);
>>> +	     pclk_div_index++) {
>>> +		for (sysclk_div_index = 0;
>>> +		     sysclk_div_index < ARRAY_SIZE(sysclk_div_table);
>>> +		     sysclk_div_index++) {
>>> +			if (sysclk_div_table[sysclk_div_index] *
>>> +				    sysclk_pclk_ratio ==
>>> +			    pclk_div_table[pclk_div_index])
>>
>>> +				goto sysclk_pclk_ratio_found;
>>
>>> +		}
>>> +	}
>>
>> And instead of this goto mess, factor out to a helper.
> 
> I think the above looks fine as-is: it's easier to understand what it does
> when it's all in a single location. This isn't overly complicated, which
> would be another reason to do that.
> 
>>
>>> +	return -EINVAL;
> 
> I'd do:
> 
> 	if (sysclk_div_index == ARRAY_SIZE(sysclk_div_table))
> 		return -EINVAL;
> 
> instead of using a goto.

I don't think that will work. Once the matching divisors are found, the 
inner and outer loop must break immediately. Otherwise, the 
corresponding indices will be overwritten.

In the initial version [1], I used a variable to terminate the outer 
loop and check for success. I changed that to a goto statement, as you 
suggested.
Perhaps I should revert to the original version, which I don't consider 
overly complicated either?

Thanks
  ~Matthias

[1] https://lore.kernel.org/all/20250403-hm1246-v1-2-30990d71bc42@emfend.at/

> 
>>
>>> +sysclk_pclk_ratio_found:
>>> +
>>> +	/* Determine an appropriate post divider. */
>>> +	for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
>>> +	     post_div_index++) {
>>> +		vco_out = pclk * pclk_div_table[pclk_div_index] *
>>> +			  post_div_table[post_div_index];
>>> +
>>> +		if (vco_out >= HM1246_PLL_VCO_MIN &&
>>> +		    vco_out <= HM1246_PLL_VCO_MAX)
>>> +			break;
>>> +	}
>>> +	if (post_div_index >= ARRAY_SIZE(post_div_table))
>>> +		return -EINVAL;
>>> +
>>> +	/* Find best pre-divider and multiplier values. */
>>> +	best_vco_diff = U32_MAX;
>>> +	for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX);
>>> +	     div <= xclk / HM1246_PLL_INCLK_MIN; div++) {
>>> +		u32 multi, multi_h, multi_l, vco, diff;
>>> +
>>> +		multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
>>> +		if (multi < HM1246_PLL_MULTI_MIN ||
>>> +		    multi > HM1246_PLL_MULTI_MAX)
>>> +			continue;
>>> +
>>> +		multi_h = multi / (HM1246_PLL_MULTI_H_MIN *
>>> +				   HM1246_PLL_MULTI_L_MAX) +
>>> +			  2;
>>> +		multi_l = multi / multi_h;
>>> +		vco = div_u64((u64)xclk * multi_h * multi_l, div);
>>> +
>>> +		diff = abs_diff(vco_out, vco);
>>> +
>>> +		if (diff < best_vco_diff) {
>>> +			best_vco_diff = diff;
>>> +			pre_div = div;
>>> +			multiplier_h = multi_h;
>>> +			multiplier_l = multi_l;
>>> +		}
>>> +
>>> +		if (!diff)
>>> +			break;
>>> +	}
>>> +
>>> +	if (best_vco_diff == U32_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	*pll1 = HM1246_PLL1CFG_MULTIPLIER(multiplier_l - 1);
>>> +	*pll2 = HM1246_PLL2CFG_PRE_DIV(pre_div - 1) |
>>> +		HM1246_PLL2CFG_MULTIPLIER(multiplier_h - 2);
>>> +	*pll3 = HM1246_PLL3CFG_POST_DIV(post_div_index) |
>>> +		HM1246_PLL3CFG_SYSCLK_DIV(sysclk_div_index) |
>>> +		HM1246_PLL3CFG_PCLK_DIV(pclk_div_index);
>>> +
>>> +	return 0;
>>> +}
>>
>> ...
>>
>>> +static int hm1246_cci_write_pll(struct hm1246 *hm1246, u8 pll1, u8 pll2,
>>> +				u8 pll3)
>>> +{
>>> +	const struct cci_reg_sequence pll_regs[] = {
>>
>> static ?
>>
>>> +		{ HM1246_PLL1CFG_REG, pll1 },
>>> +		{ HM1246_PLL2CFG_REG, pll2 },
>>> +		{ HM1246_PLL3CFG_REG, pll3 },
>>> +		{ HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
>>> +	};
>>
>> I would even move it outside the function. Note, static const maybe located in
>> ro memory while w/o static it's just a guarantee that compiler doesn't change
>> the values. Hence there is no guarantee it will be in ro memory.
>>
>>> +	return cci_multi_reg_write(hm1246->regmap, pll_regs,
>>> +				   ARRAY_SIZE(pll_regs), NULL);
>>> +}
>>> +
>>> +static int hm1246_pll_check_locked(struct hm1246 *hm1246)
>>> +{
>>> +	u64 boot_ref2;
>>> +	int ret;
>>> +
>>> +	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
>>> +		       NULL);
>>
>> Despite being longer 80 I still would put it on one line. It will increase readability.
> 
> I prefer it as-is.
> 
>>
>> 	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
>>
>> Another option is to define local regmap:
>>
>> 	struct regmap *map = hm1246->regmap;
>> 	...
>> 	ret = cci_read(map, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
>>
>> which will be most readable and satisfy 80 limit.
>>
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;
>>
>> Think about similar improvements elsewhere in this driver.
>>
>>> +}
>>
>> ...
>>
>>> +	/* PLL lock time: tpll typ. 100us */
>>
>> It's not a variable name, use proper English.
>>
>>> +	fsleep(200);
>>
>> ...
>>
>>> +static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode, u16 r,
>>> +					 u16 g, u16 b)
>>
>> Use logical split.
>>
>> static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode,
>> 					 u16 r, u16 g, u16 b)
>>
>> This applies to other similar places in the code.
>>
>> ...
>>
>>> +static int hm1246_test_pattern(struct hm1246 *hm1246, u32 index)
>>> +{
>>> +	const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;
>>
>> 0 is enough (no need 0x).
>>
>>
>> So, the MAX is 10-bit, Can we use rather (BIT(10) - 1) to show this?
> 
> The above value likely comes from a sensor datasheet; I think 0x3ff is fine
> as-is. If this was a bitmask with a row of bits set, I'd use GENMASK(), but
> not BIT(10) - 1.
> 
>>
>>> +	const struct tp {
>>> +		int pattern;
>>> +		u16 r, g, b;
>>> +	} tps[] = {
>>> +		/* 0 - Disabled */
>>
>> Instead of indices in the comment, make the code robust
>>
>>> +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>
>> 		[0] = { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>
>> It even fit 80 characters.
>>
>>> +		/* 1 - Checkboard pattern */
>>> +		{ .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>> +		/* 2 - Ramp */
>>> +		{ .pattern = 1, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>> +		/* 3 - Moving ones */
>>> +		{ .pattern = 2, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>> +		/* 4 - Blending color bars */
>>> +		{ .pattern = 3, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>> +		/* 5 - Color bars */
>>> +		{ .pattern = 4, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>> +		/* 6 - Solid white */
>>> +		{ .pattern = 15, .r = RGBMAX, .g = RGBMAX, .b = RGBMAX },
>>> +		/* 7 - Solid black */
>>> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
>>> +		/* 8 - Solid red */
>>> +		{ .pattern = 15, .r = RGBMAX, .g = RGBMIN, .b = RGBMIN },
>>> +		/* 9 - Solid green */
>>> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMAX, .b = RGBMIN },
>>> +		/* 10- Solid blue */
>>> +		{ .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMAX },
>>> +	};
>>> +	u8 mode;
>>> +
>>> +	if (index >= ARRAY_SIZE(tps))
>>> +		return -EINVAL;
>>> +
>>> +	mode = HM1246_TEST_PATTERN_MODE_MODE(tps[index].pattern);
>>> +	if (index)
>>> +		mode |= HM1246_TEST_PATTERN_MODE_ENABLE;
>>> +
>>> +	return hm1246_cci_write_test_pattern(hm1246, mode, tps[index].r,
>>> +					     tps[index].g, tps[index].b);
>>> +}
>>
>> ...
>>
>>> +static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl)
>>> +{
>>> +	struct hm1246 *hm1246 = container_of_const(ctrl->handler, struct hm1246,
>>> +						   ctrls);
>>
>> Why _const()?
>> Why not split it between lines like:
>>
>> 	struct hm1246 *hm1246 =
>> 		container_of_const(ctrl->handler, struct hm1246, ctrls);
>>
>>> +	struct v4l2_subdev_state *state;
>>> +	const struct v4l2_mbus_framefmt *format;
>>> +	u32 val;
>>> +	bool needs_cmu_update = true;
>>> +	int ret = 0;
>>> +
>>> +	state = v4l2_subdev_get_locked_active_state(&hm1246->sd);
>>> +	format = v4l2_subdev_state_get_format(state, 0);
>>> +
>>> +	if (ctrl->id == V4L2_CID_VBLANK) {
>>> +		s64 exposure_max;
>>> +
>>> +		exposure_max =
>>> +			format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN;
>>> +		ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl,
>>> +					       hm1246->exposure_ctrl->minimum,
>>> +					       exposure_max,
>>> +					       hm1246->exposure_ctrl->step,
>>> +					       exposure_max);
>>> +
>>> +		if (ret) {
>>> +			dev_err(hm1246->dev, "exposure ctrl range update failed\n");
>>> +			return ret;
>>> +		}
>>> +	}
>>
>>> +	if (!pm_runtime_get_if_active(hm1246->dev))
>>> +		return 0;
>>
>> Use ACQUIRE() and return directly where it makes sense.
> 
> That's interesting.
> 
>>
>>> +	switch (ctrl->id) {
>>> +	case V4L2_CID_EXPOSURE:
>>> +		cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
>>> +			  &ret);
>>> +		break;
>>> +
>>> +	case V4L2_CID_ANALOGUE_GAIN:
>>> +		cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
>>> +			  ctrl->val, &ret);
>>> +		break;
>>> +
>>> +	case V4L2_CID_VBLANK:
>>> +		val = format->height + ctrl->val;
>>> +		cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
>>> +			  &ret);
>>> +		break;
>>> +
>>> +	case V4L2_CID_HFLIP:
>>> +	case V4L2_CID_VFLIP:
>>> +		val = 0;
>>> +		if (hm1246->hflip_ctrl->val)
>>> +			val |= HM1246_IMAGE_ORIENTATION_HFLIP;
>>> +		if (hm1246->vflip_ctrl->val)
>>> +			val |= HM1246_IMAGE_ORIENTATION_VFLIP;
>>> +
>>> +		cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val,
>>> +			  &ret);
>>> +		break;
>>> +
>>> +	case V4L2_CID_TEST_PATTERN:
>>> +		ret = hm1246_test_pattern(hm1246, ctrl->val);
>>> +		needs_cmu_update = false;
>>
>> Like here, and you won't need needs_cmu_update anymore.
>>
>>> +		break;
>>> +
>>> +	default:
>>> +		ret = -EINVAL;
>>> +		needs_cmu_update = false;
>>> +		break;
>>> +	}
>>> +
>>> +	if (needs_cmu_update)
>>> +		cci_write(hm1246->regmap, HM1246_CMU_UPDATE_REG, 0, &ret);
>>> +
>>> +	pm_runtime_put(hm1246->dev);
>>> +
>>> +	return ret;
>>> +}
>>
>> ...
>>
>>> +static int hm1246_identify_module(struct hm1246 *hm1246)
>>
>> This is a singleton function, right?
>>
>> Check what once.h (or even once_lite.h) provides for you for such a case,
>> and drop unneeded "identified" variable.
> 
> Once for every device, so I don't think this applies.
> 
>>
>>> +{
>>> +	u64 model_id;
>>> +	int ret;
>>> +
>>> +	if (hm1246->identified)
>>> +		return 0;
>>> +
>>> +	ret = cci_read(hm1246->regmap, HM1246_MODEL_ID_REG, &model_id, NULL);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (model_id != HM1246_MODEL_ID) {
>>> +		dev_err(hm1246->dev, "model id mismatch: 0x%llx!=0x%x\n",
>>> +			model_id, HM1246_MODEL_ID);
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	hm1246->identified = true;
>>> +
>>> +	return 0;
>>> +}
>