* [PATCH v3 0/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
@ 2025-10-30 11:57 Svyatoslav Ryhel
2025-10-30 11:57 ` [PATCH v3 1/2] dt-bindings: media: i2c: document Sony IMX111 CMOS sensor Svyatoslav Ryhel
2025-10-30 11:57 ` [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver Svyatoslav Ryhel
0 siblings, 2 replies; 13+ messages in thread
From: Svyatoslav Ryhel @ 2025-10-30 11:57 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sakari Ailus, Svyatoslav Ryhel, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson,
Tarang Raval
Cc: linux-media, devicetree, linux-kernel
Add driver for Sony IMX111 CMOS sensor found in LG Optimus 4X and Vu
smartphones.
---
Changes in v2:
- added nvmem-consumer and dropped eeprom
- switched to unevaluatedProperties
- dropped flash-leds, lens-focus, orientation and rotation since they are
inherited from video-interface-devices by unevaluatedProperties
- dropped assigned-clock* props
- improved code style
- adjusted colorspace definitions
Changes in v3:
- switched to devm_regulator_bulk_get_const
- set V4L2_MBUS_CSI2_DPHY bus type unconditionaly
- removed pm_runtime_mark_last_busy
- removed properties from port endpoint and bus-type requirement
- removed unused headers
- adjusted register macros
- added vblank and hblank
- swapped register writed with known purpuse with macros
- container_of > container_of_const
- added return check to group write
- adjusted PM similar to imx219
- switched to enable/disable streams
- dropped enum_frame_interval, get_frame_interval and set_frame_interval ops
- devm_clk_get > devm_v4l2_sensor_clk_get
- adjusted default_link_freq calculation
- dropped imx111_id i2c_device_id
- improved code formatting
---
Svyatoslav Ryhel (2):
dt-bindings: media: i2c: document Sony IMX111 CMOS sensor
media: i2c: add Sony IMX111 CMOS camera sensor driver
.../bindings/media/i2c/sony,imx111.yaml | 105 ++
drivers/media/i2c/Kconfig | 10 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/imx111.c | 1548 +++++++++++++++++
4 files changed, 1664 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx111.yaml
create mode 100644 drivers/media/i2c/imx111.c
--
2.48.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] dt-bindings: media: i2c: document Sony IMX111 CMOS sensor
2025-10-30 11:57 [PATCH v3 0/2] media: i2c: add Sony IMX111 CMOS camera sensor driver Svyatoslav Ryhel
@ 2025-10-30 11:57 ` Svyatoslav Ryhel
2025-10-30 19:23 ` Conor Dooley
2025-10-30 11:57 ` [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver Svyatoslav Ryhel
1 sibling, 1 reply; 13+ messages in thread
From: Svyatoslav Ryhel @ 2025-10-30 11:57 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sakari Ailus, Svyatoslav Ryhel, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson,
Tarang Raval
Cc: linux-media, devicetree, linux-kernel
Add bindings for Sony IMX111 CMOS Digital Image Sensor found in LG
Optimus 4X (P880) and Optimus Vu (P895) smartphones.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
.../bindings/media/i2c/sony,imx111.yaml | 105 ++++++++++++++++++
1 file changed, 105 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx111.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx111.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx111.yaml
new file mode 100644
index 000000000000..20f48d5e9b2d
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx111.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx111.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX111 8MP CMOS Digital Image Sensor
+
+maintainers:
+ - Svyatoslav Ryhel <clamor95@gmail.com>
+
+description:
+ IMX111 sensor is a Sony CMOS active pixel digital image sensor with an active
+ array size of 2464H x 3280V. It is programmable through I2C interface. Image
+ data is sent through MIPI CSI-2, through 1 or 2 lanes.
+
+allOf:
+ - $ref: /schemas/media/video-interface-devices.yaml#
+ - $ref: /schemas/nvmem/nvmem-consumer.yaml#
+
+properties:
+ compatible:
+ const: sony,imx111
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ description: EXTCLK with possible frequency from 6 to 54 MHz
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ iovdd-supply:
+ description: Digital IO power supply (1.8V)
+
+ dvdd-supply:
+ description: Digital power supply (1.2V)
+
+ avdd-supply:
+ description: Analog power supply (2.7V)
+
+ port:
+ additionalProperties: false
+ $ref: /schemas/graph.yaml#/$defs/port-base
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ required:
+ - data-lanes
+ - link-frequencies
+
+ required:
+ - endpoint
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - port
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/media/video-interfaces.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ camera@10 {
+ compatible = "sony,imx111";
+ reg = <0x10>;
+
+ clocks = <&imx111_clk>;
+
+ iovdd-supply = <&camera_vddio_1v8>;
+ dvdd-supply = <&camera_vddd_1v2>;
+ avdd-supply = <&camera_vdda_2v7>;
+
+ orientation = <1>;
+ rotation = <90>;
+
+ nvmem = <&eeprom>;
+ flash-leds = <&led>;
+ lens-focus = <&vcm>;
+
+ reset-gpios = <&gpio 84 GPIO_ACTIVE_LOW>;
+
+ port {
+ imx111_output: endpoint {
+ data-lanes = <1 2>;
+ link-frequencies = /bits/ 64 <542400000>;
+ remote-endpoint = <&csi_input>;
+ };
+ };
+ };
+ };
+...
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 11:57 [PATCH v3 0/2] media: i2c: add Sony IMX111 CMOS camera sensor driver Svyatoslav Ryhel
2025-10-30 11:57 ` [PATCH v3 1/2] dt-bindings: media: i2c: document Sony IMX111 CMOS sensor Svyatoslav Ryhel
@ 2025-10-30 11:57 ` Svyatoslav Ryhel
2025-10-30 14:55 ` Tarang Raval
1 sibling, 1 reply; 13+ messages in thread
From: Svyatoslav Ryhel @ 2025-10-30 11:57 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sakari Ailus, Svyatoslav Ryhel, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson,
Tarang Raval
Cc: linux-media, devicetree, linux-kernel
Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
camera sensor using the i2c bus for control and the csi-2 bus for data.
The following features are supported:
- manual exposure, digital and analog gain control support
- pixel rate/link freq control support
- supported resolution up to 3280x2464 for single shot capture
- supported resolution up to 1920x1080 @ 30fps for video
- supported bayer order output SGBRG10 and SGBRG8
Camera module seems to be partially compatible with Nokia SMIA but it
lacks a few registers required for clock calculations and has different
vendor-specific per-mode configurations which makes it incompatible with
existing CCS driver.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/media/i2c/Kconfig | 10 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/imx111.c | 1548 ++++++++++++++++++++++++++++++++++++
3 files changed, 1559 insertions(+)
create mode 100644 drivers/media/i2c/imx111.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 6237fe804a5c..2283a8d5b54b 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -127,6 +127,16 @@ config VIDEO_HI847
To compile this driver as a module, choose M here: the
module will be called hi847.
+config VIDEO_IMX111
+ tristate "Sony IMX111 sensor support"
+ select V4L2_CCI_I2C
+ help
+ This is a V4L2 sensor driver for the Sony IMX111 camera
+ sensors.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx111.
+
config VIDEO_IMX208
tristate "Sony IMX208 sensor support"
help
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 5873d29433ee..67b810c91870 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_VIDEO_HI556) += hi556.o
obj-$(CONFIG_VIDEO_HI846) += hi846.o
obj-$(CONFIG_VIDEO_HI847) += hi847.o
obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
+obj-$(CONFIG_VIDEO_IMX111) += imx111.o
obj-$(CONFIG_VIDEO_IMX208) += imx208.o
obj-$(CONFIG_VIDEO_IMX214) += imx214.o
obj-$(CONFIG_VIDEO_IMX219) += imx219.o
diff --git a/drivers/media/i2c/imx111.c b/drivers/media/i2c/imx111.c
new file mode 100644
index 000000000000..3838dac2b230
--- /dev/null
+++ b/drivers/media/i2c/imx111.c
@@ -0,0 +1,1548 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/media.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+#include <linux/videodev2.h>
+#include <linux/units.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-mediabus.h>
+
+/* product information registers */
+#define IMX111_PRODUCT_ID CCI_REG16(0x0000)
+#define IMX111_CHIP_ID 0x111
+#define IMX111_REVISION CCI_REG8(0x0002)
+#define IMX111_MANUFACTURER_ID CCI_REG8(0x0003)
+#define IMX111_FRAME_COUNTER CCI_REG8(0x0005)
+#define IMX111_PIXEL_ORDER CCI_REG8(0x0006)
+
+/* general configuration registers */
+#define IMX111_STREAMING_MODE CCI_REG8(0x0100)
+#define IMX111_MODE_STANDBY 0
+#define IMX111_MODE_STREAMING 1
+#define IMX111_IMAGE_ORIENTATION CCI_REG8(0x0101)
+#define IMX111_IMAGE_HFLIP BIT(0)
+#define IMX111_IMAGE_VFLIP BIT(1)
+#define IMX111_SOFTWARE_RESET CCI_REG8(0x0103)
+#define IMX111_RESET_ON 1
+#define IMX111_GROUP_WRITE CCI_REG8(0x0104)
+#define IMX111_GROUP_WRITE_ON 1
+#define IMX111_FRAME_DROP CCI_REG8(0x0105)
+#define IMX111_FRAME_DROP_ON 1
+#define IMX111_CHANNEL_ID CCI_REG8(0x0110)
+#define IMX111_SIGNALLING_MODE CCI_REG8(0x0111)
+#define IMX111_DATA_DEPTH CCI_REG16(0x0112)
+#define IMX111_DATA_DEPTH_RAW8 0x08
+#define IMX111_DATA_DEPTH_RAW10 0x0a
+
+/* integration time registers */
+#define IMX111_INTEGRATION_TIME CCI_REG16(0x0202)
+#define IMX111_INTEGRATION_TIME_MIN 0x1
+#define IMX111_INTEGRATION_TIME_MAX 0xffff
+#define IMX111_INTEGRATION_TIME_STEP 1
+
+/* analog gain control */
+#define IMX111_REG_ANALOG_GAIN CCI_REG8(0x0205)
+#define IMX111_ANA_GAIN_MIN 0
+#define IMX111_ANA_GAIN_MAX 240
+#define IMX111_ANA_GAIN_STEP 1
+#define IMX111_ANA_GAIN_DEFAULT 0
+
+/* digital gain control */
+#define IMX111_REG_DIG_GAIN_GREENR CCI_REG16(0x020e)
+#define IMX111_REG_DIG_GAIN_RED CCI_REG16(0x0210)
+#define IMX111_REG_DIG_GAIN_BLUE CCI_REG16(0x0212)
+#define IMX111_REG_DIG_GAIN_GREENB CCI_REG16(0x0214)
+#define IMX111_DGTL_GAIN_MIN 0x0100
+#define IMX111_DGTL_GAIN_MAX 0x0fff
+#define IMX111_DGTL_GAIN_DEFAULT 0x0100
+#define IMX111_DGTL_GAIN_STEP 1
+
+/* clock configuration registers */
+#define IMX111_PIXEL_CLK_DIVIDER_PLL1 CCI_REG8(0x0301) /* fixed to 10 */
+#define IMX111_SYSTEM_CLK_DIVIDER_PLL1 CCI_REG8(0x0303) /* fixed to 1 */
+#define IMX111_PRE_PLL_CLK_DIVIDER_PLL1 CCI_REG8(0x0305)
+#define IMX111_PLL_MULTIPLIER_PLL1 CCI_REG8(0x0307)
+#define IMX111_PLL_SETTLING_TIME CCI_REG8(0x303c)
+#define IMX111_PLL_SETTLING_TIME_DEFAULT 200
+#define IMX111_POST_DIVIDER CCI_REG8(0x30a4)
+#define IMX111_POST_DIVIDER_DIV1 2
+#define IMX111_POST_DIVIDER_DIV2 0
+#define IMX111_POST_DIVIDER_DIV4 1
+
+/* frame timing registers */
+#define IMX111_VERTICAL_TOTAL_LENGTH CCI_REG16(0x0340)
+#define IMX111_VTL_MAX 0x09d8
+#define IMX111_VBLANK_MIN 16
+#define IMX111_HORIZONTAL_TOTAL_LENGTH CCI_REG16(0x0342)
+#define IMX111_HTL_MAX 0x0dd0
+#define IMX111_HBLANK_MIN 16
+
+/* image size registers */
+#define IMX111_HORIZONTAL_START CCI_REG16(0x0344)
+#define IMX111_VERTICAL_START CCI_REG16(0x0346)
+#define IMX111_HORIZONTAL_END CCI_REG16(0x0348)
+#define IMX111_VERTICAL_END CCI_REG16(0x034a)
+#define IMX111_IMAGE_WIDTH CCI_REG16(0x034c)
+#define IMX111_IMAGE_HEIGHT CCI_REG16(0x034e)
+#define IMX111_H_EVEN_INC CCI_REG8(0x0381)
+#define IMX111_H_ODD_INC CCI_REG8(0x0383)
+#define IMX111_W_EVEN_INC CCI_REG8(0x0385)
+#define IMX111_W_ODD_INC CCI_REG8(0x0387)
+
+/* test pattern registers */
+#define IMX111_TEST_PATTERN CCI_REG8(0x0601)
+#define IMX111_TEST_PATTERN_NONE 0
+#define IMX111_TEST_PATTERN_SOLID 1
+#define IMX111_TEST_PATTERN_BARS 2
+#define IMX111_TEST_PATTERN_FADE 3
+#define IMX111_TEST_PATTERN_PN9 4
+#define IMX111_SOLID_COLOR_RED CCI_REG16(0x0602)
+#define IMX111_SOLID_COLOR_GR CCI_REG16(0x0604)
+#define IMX111_SOLID_COLOR_BLUE CCI_REG16(0x0606)
+#define IMX111_SOLID_COLOR_GB CCI_REG16(0x0608)
+#define IMX111_TESTP_COLOUR_MIN 0
+#define IMX111_TESTP_COLOUR_MAX 0x03ff
+#define IMX111_TESTP_COLOUR_STEP 1
+
+#define IMX111_FRAME_RATE_STEP 5
+
+#define IMX111_PIXEL_ARRAY_WIDTH 3280U
+#define IMX111_PIXEL_ARRAY_HEIGHT 2464U
+
+enum {
+ IMX111_MODE_3280x2464,
+ IMX111_MODE_3280x1848,
+ IMX111_MODE_3280x1098,
+ IMX111_MODE_2100x1200,
+ IMX111_MODE_1952x1098,
+ IMX111_MODE_1920x1080,
+ IMX111_MODE_1640x1232,
+ IMX111_MODE_1440x1080,
+ IMX111_MODE_1640x924,
+ IMX111_MODE_1308x736,
+ IMX111_MODE_1280x720,
+ IMX111_MODE_820x614,
+ IMX111_MODE_640x480,
+};
+
+static const struct regulator_bulk_data imx111_supplies[] = {
+ { .supply = "iovdd" },
+ { .supply = "dvdd" },
+ { .supply = "avdd" },
+};
+
+struct imx111_mode {
+ u32 width;
+ u32 height;
+
+ /* Default vertical and horizontal total length */
+ u32 vtl_def;
+ u32 htl_def;
+
+ struct {
+ const struct cci_reg_sequence *regs;
+ u32 num_of_regs;
+ } reg_list;
+};
+
+struct imx111_pll {
+ u64 extclk_rate;
+ u8 pre_div;
+ u8 mult;
+};
+
+struct imx111 {
+ struct regmap *regmap;
+
+ struct clk *extclk;
+ struct gpio_desc *reset;
+ struct regulator_bulk_data *supplies;
+
+ struct v4l2_fwnode_endpoint bus_cfg;
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+
+ /* V4L2 Controls */
+ struct v4l2_ctrl_handler hdl;
+ struct v4l2_ctrl *pixel_rate;
+ struct v4l2_ctrl *link_freq;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *vblank;
+ struct v4l2_ctrl *hblank;
+ struct v4l2_ctrl *hflip;
+ struct v4l2_ctrl *vflip;
+
+ /* Current mode */
+ const struct imx111_mode *cur_mode;
+ const struct imx111_pll *pll;
+ u32 data_depth;
+
+ u64 pixel_clk_raw;
+ s64 default_link_freq;
+};
+
+static const struct imx111_pll imx111_pll[] = {
+ { .extclk_rate = 6000000, .pre_div = 1, .mult = 113, },
+ { .extclk_rate = 12000000, .pre_div = 2, .mult = 113, },
+ { .extclk_rate = 13500000, .pre_div = 1, .mult = 50, },
+ { .extclk_rate = 18000000, .pre_div = 2, .mult = 75, },
+ { .extclk_rate = 24000000, .pre_div = 4, .mult = 113, },
+ { .extclk_rate = 27000000, .pre_div = 2, .mult = 50, },
+ { .extclk_rate = 36000000, .pre_div = 4, .mult = 75, },
+ { .extclk_rate = 54000000, .pre_div = 4, .mult = 50, },
+};
+
+/*
+ * This table MUST contain 4 entries per format, to cover the various flip
+ * combinations in the order
+ * - no flip
+ * - h flip
+ * - v flip
+ * - h&v flips
+ */
+static const u32 imx111_mbus_formats[] = {
+ MEDIA_BUS_FMT_SGBRG10_1X10,
+ MEDIA_BUS_FMT_SBGGR10_1X10,
+ MEDIA_BUS_FMT_SRGGB10_1X10,
+ MEDIA_BUS_FMT_SGRBG10_1X10,
+
+ MEDIA_BUS_FMT_SGBRG8_1X8,
+ MEDIA_BUS_FMT_SBGGR8_1X8,
+ MEDIA_BUS_FMT_SRGGB8_1X8,
+ MEDIA_BUS_FMT_SGRBG8_1X8,
+};
+
+static const struct cci_reg_sequence imx111_global_init[] = {
+ { CCI_REG8(0x3080), 0x50 },
+ { CCI_REG8(0x3087), 0x53 },
+ { CCI_REG8(0x309d), 0x94 },
+ { CCI_REG8(0x30b1), 0x03 },
+ { CCI_REG8(0x30c6), 0x00 },
+ { CCI_REG8(0x30c7), 0x00 },
+ { CCI_REG8(0x3115), 0x0b },
+ { CCI_REG8(0x3118), 0x30 },
+ { CCI_REG8(0x311d), 0x25 },
+ { CCI_REG8(0x3121), 0x0a },
+ { CCI_REG8(0x3212), 0xf2 },
+ { CCI_REG8(0x3213), 0x0f },
+ { CCI_REG8(0x3215), 0x0f },
+ { CCI_REG8(0x3217), 0x0b },
+ { CCI_REG8(0x3219), 0x0b },
+ { CCI_REG8(0x321b), 0x0d },
+ { CCI_REG8(0x321d), 0x0d },
+ { CCI_REG8(0x32aa), 0x11 },
+ { CCI_REG8(0x3032), 0x40 },
+};
+
+static const struct cci_reg_sequence mode_820x614[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0008 }, { IMX111_VERTICAL_START, 0x0034 },
+ { IMX111_HORIZONTAL_END, 0x0cd7 }, { IMX111_VERTICAL_END, 0x09cb },
+ { IMX111_IMAGE_WIDTH, 0x0334 }, { IMX111_IMAGE_HEIGHT, 0x0266 },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x05 }, { IMX111_H_ODD_INC, 0x03 },
+ { IMX111_W_EVEN_INC, 0x05 }, { IMX111_W_ODD_INC, 0x03 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x10 },
+ { CCI_REG8(0x303e), 0x40 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x97 }, { CCI_REG8(0x3048), 0x01 },
+ { CCI_REG8(0x304c), 0x6f }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0x00 },
+ { CCI_REG8(0x3074), 0x11 }, { CCI_REG8(0x3075), 0x11 },
+ { CCI_REG8(0x3076), 0x11 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x00 }, { CCI_REG8(0x307a), 0x00 },
+ { CCI_REG8(0x309b), 0x28 }, { CCI_REG8(0x309c), 0x13 },
+ { CCI_REG8(0x309e), 0x00 }, { CCI_REG8(0x30a0), 0x14 },
+ { CCI_REG8(0x30a1), 0x09 }, { CCI_REG8(0x30aa), 0x03 },
+ { CCI_REG8(0x30b2), 0x03 }, { CCI_REG8(0x30d5), 0x09 },
+ { CCI_REG8(0x30d6), 0x00 }, { CCI_REG8(0x30d7), 0x00 },
+ { CCI_REG8(0x30d8), 0x00 }, { CCI_REG8(0x30d9), 0x00 },
+ { CCI_REG8(0x30de), 0x04 }, { CCI_REG8(0x30df), 0x20 },
+ { CCI_REG8(0x3102), 0x08 }, { CCI_REG8(0x3103), 0x22 },
+ { CCI_REG8(0x3104), 0x20 }, { CCI_REG8(0x3105), 0x00 },
+ { CCI_REG8(0x3106), 0x87 }, { CCI_REG8(0x3107), 0x00 },
+ { CCI_REG8(0x3108), 0x03 }, { CCI_REG8(0x3109), 0x02 },
+ { CCI_REG8(0x310a), 0x03 }, { CCI_REG8(0x315c), 0x9c },
+ { CCI_REG8(0x315d), 0x9b }, { CCI_REG8(0x316e), 0x9d },
+ { CCI_REG8(0x316f), 0x9c }, { CCI_REG8(0x3318), 0x7a },
+ { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct cci_reg_sequence mode_1308x736[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0154 }, { IMX111_VERTICAL_START, 0x0220 },
+ { IMX111_HORIZONTAL_END, 0x0b8b }, { IMX111_VERTICAL_END, 0x07df },
+ { IMX111_IMAGE_WIDTH, 0x051c }, { IMX111_IMAGE_HEIGHT, 0x02e0 },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x01 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x03 },
+ { CCI_REG8(0x3033), 0x84 }, { CCI_REG8(0x303d), 0x10 },
+ { CCI_REG8(0x303e), 0x40 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x97 }, { CCI_REG8(0x3048), 0x01 },
+ { CCI_REG8(0x304c), 0xd7 }, { CCI_REG8(0x304d), 0x01 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0x00 },
+ { CCI_REG8(0x3074), 0x11 }, { CCI_REG8(0x3075), 0x11 },
+ { CCI_REG8(0x3076), 0x11 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x00 }, { CCI_REG8(0x307a), 0x00 },
+ { CCI_REG8(0x309b), 0x48 }, { CCI_REG8(0x309c), 0x12 },
+ { CCI_REG8(0x309e), 0x04 }, { CCI_REG8(0x30a0), 0x14 },
+ { CCI_REG8(0x30a1), 0x0a }, { CCI_REG8(0x30aa), 0x01 },
+ { CCI_REG8(0x30b2), 0x05 }, { CCI_REG8(0x30d5), 0x04 },
+ { CCI_REG8(0x30d6), 0x85 }, { CCI_REG8(0x30d7), 0x2a },
+ { CCI_REG8(0x30d8), 0x64 }, { CCI_REG8(0x30d9), 0x89 },
+ { CCI_REG8(0x30de), 0x00 }, { CCI_REG8(0x30df), 0x20 },
+ { CCI_REG8(0x3102), 0x08 }, { CCI_REG8(0x3103), 0x22 },
+ { CCI_REG8(0x3104), 0x20 }, { CCI_REG8(0x3105), 0x00 },
+ { CCI_REG8(0x3106), 0x87 }, { CCI_REG8(0x3107), 0x00 },
+ { CCI_REG8(0x3108), 0x03 }, { CCI_REG8(0x3109), 0x02 },
+ { CCI_REG8(0x310a), 0x03 }, { CCI_REG8(0x315c), 0x42 },
+ { CCI_REG8(0x315d), 0x41 }, { CCI_REG8(0x316e), 0x43 },
+ { CCI_REG8(0x316f), 0x42 }, { CCI_REG8(0x3318), 0x62 },
+ { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct cci_reg_sequence mode_1640x924[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0008 }, { IMX111_VERTICAL_START, 0x0164 },
+ { IMX111_HORIZONTAL_END, 0x0cd7 }, { IMX111_VERTICAL_END, 0x089b },
+ { IMX111_IMAGE_WIDTH, 0x0668 }, { IMX111_IMAGE_HEIGHT, 0x039c },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x03 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x03 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x10 },
+ { CCI_REG8(0x303e), 0x40 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x97 }, { CCI_REG8(0x3048), 0x01 },
+ { CCI_REG8(0x304c), 0x6f }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0x00 },
+ { CCI_REG8(0x3074), 0x11 }, { CCI_REG8(0x3075), 0x11 },
+ { CCI_REG8(0x3076), 0x11 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x00 }, { CCI_REG8(0x307a), 0x00 },
+ { CCI_REG8(0x309b), 0x28 }, { CCI_REG8(0x309c), 0x13 },
+ { CCI_REG8(0x309e), 0x00 }, { CCI_REG8(0x30a0), 0x14 },
+ { CCI_REG8(0x30a1), 0x09 }, { CCI_REG8(0x30aa), 0x03 },
+ { CCI_REG8(0x30b2), 0x05 }, { CCI_REG8(0x30d5), 0x09 },
+ { CCI_REG8(0x30d6), 0x01 }, { CCI_REG8(0x30d7), 0x01 },
+ { CCI_REG8(0x30d8), 0x64 }, { CCI_REG8(0x30d9), 0x89 },
+ { CCI_REG8(0x30de), 0x02 }, { CCI_REG8(0x30df), 0x20 },
+ { CCI_REG8(0x3102), 0x08 }, { CCI_REG8(0x3103), 0x22 },
+ { CCI_REG8(0x3104), 0x20 }, { CCI_REG8(0x3105), 0x00 },
+ { CCI_REG8(0x3106), 0x87 }, { CCI_REG8(0x3107), 0x00 },
+ { CCI_REG8(0x3108), 0x03 }, { CCI_REG8(0x3109), 0x02 },
+ { CCI_REG8(0x310a), 0x03 }, { CCI_REG8(0x315c), 0x9c },
+ { CCI_REG8(0x315d), 0x9b }, { CCI_REG8(0x316e), 0x9d },
+ { CCI_REG8(0x316f), 0x9c }, { CCI_REG8(0x3318), 0x72 },
+ { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct cci_reg_sequence mode_1640x1232[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0008 }, { IMX111_VERTICAL_START, 0x0030 },
+ { IMX111_HORIZONTAL_END, 0x0cd7 }, { IMX111_VERTICAL_END, 0x09cf },
+ { IMX111_IMAGE_WIDTH, 0x0668 }, { IMX111_IMAGE_HEIGHT, 0x04d0 },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x03 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x03 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x10 },
+ { CCI_REG8(0x303e), 0x40 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x97 }, { CCI_REG8(0x3048), 0x01 },
+ { CCI_REG8(0x304c), 0x6f }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0x00 },
+ { CCI_REG8(0x3074), 0x11 }, { CCI_REG8(0x3075), 0x11 },
+ { CCI_REG8(0x3076), 0x11 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x00 }, { CCI_REG8(0x307a), 0x00 },
+ { CCI_REG8(0x309b), 0x28 }, { CCI_REG8(0x309c), 0x13 },
+ { CCI_REG8(0x309e), 0x00 }, { CCI_REG8(0x30a0), 0x14 },
+ { CCI_REG8(0x30a1), 0x09 }, { CCI_REG8(0x30aa), 0x03 },
+ { CCI_REG8(0x30b2), 0x05 }, { CCI_REG8(0x30d5), 0x09 },
+ { CCI_REG8(0x30d6), 0x01 }, { CCI_REG8(0x30d7), 0x01 },
+ { CCI_REG8(0x30d8), 0x64 }, { CCI_REG8(0x30d9), 0x89 },
+ { CCI_REG8(0x30de), 0x02 }, { CCI_REG8(0x30df), 0x20 },
+ { CCI_REG8(0x3102), 0x08 }, { CCI_REG8(0x3103), 0x22 },
+ { CCI_REG8(0x3104), 0x20 }, { CCI_REG8(0x3105), 0x00 },
+ { CCI_REG8(0x3106), 0x87 }, { CCI_REG8(0x3107), 0x00 },
+ { CCI_REG8(0x3108), 0x03 }, { CCI_REG8(0x3109), 0x02 },
+ { CCI_REG8(0x310a), 0x03 }, { CCI_REG8(0x315c), 0x9c },
+ { CCI_REG8(0x315d), 0x9b }, { CCI_REG8(0x316e), 0x9d },
+ { CCI_REG8(0x316f), 0x9c }, { CCI_REG8(0x3318), 0x72 },
+ { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct cci_reg_sequence mode_1952x1098[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0016 }, { IMX111_VERTICAL_START, 0x016e },
+ { IMX111_HORIZONTAL_END, 0x0ccb }, { IMX111_VERTICAL_END, 0x0893 },
+ { IMX111_IMAGE_WIDTH, 0x07a0 }, { IMX111_IMAGE_HEIGHT, 0x044a },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x01 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x01 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x10 },
+ { CCI_REG8(0x303e), 0x00 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x91 }, { CCI_REG8(0x3048), 0x00 },
+ { CCI_REG8(0x304c), 0x67 }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x10 }, { CCI_REG8(0x3073), 0xa0 },
+ { CCI_REG8(0x3074), 0x12 }, { CCI_REG8(0x3075), 0x12 },
+ { CCI_REG8(0x3076), 0x12 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x0a }, { CCI_REG8(0x307a), 0x0a },
+ { CCI_REG8(0x309b), 0x60 }, { CCI_REG8(0x309e), 0x04 },
+ { CCI_REG8(0x30a0), 0x15 }, { CCI_REG8(0x30a1), 0x08 },
+ { CCI_REG8(0x30aa), 0x03 }, { CCI_REG8(0x30b2), 0x05 },
+ { CCI_REG8(0x30d5), 0x20 }, { CCI_REG8(0x30d6), 0x85 },
+ { CCI_REG8(0x30d7), 0x2a }, { CCI_REG8(0x30d8), 0x64 },
+ { CCI_REG8(0x30d9), 0x89 }, { CCI_REG8(0x30de), 0x00 },
+ { CCI_REG8(0x30df), 0x21 }, { CCI_REG8(0x3102), 0x08 },
+ { CCI_REG8(0x3103), 0x1d }, { CCI_REG8(0x3104), 0x1e },
+ { CCI_REG8(0x3105), 0x00 }, { CCI_REG8(0x3106), 0x74 },
+ { CCI_REG8(0x3107), 0x00 }, { CCI_REG8(0x3108), 0x03 },
+ { CCI_REG8(0x3109), 0x02 }, { CCI_REG8(0x310a), 0x03 },
+ { CCI_REG8(0x315c), 0x37 }, { CCI_REG8(0x315d), 0x36 },
+ { CCI_REG8(0x316e), 0x38 }, { CCI_REG8(0x316f), 0x37 },
+ { CCI_REG8(0x3318), 0x63 }, { CCI_REG8(0x3348), 0xA0 },
+};
+
+static const struct cci_reg_sequence mode_2100x1200[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0256 }, { IMX111_VERTICAL_START, 0x02a8 },
+ { IMX111_HORIZONTAL_END, 0x0a89 }, { IMX111_VERTICAL_END, 0x0757 },
+ { IMX111_IMAGE_WIDTH, 0x0834 }, { IMX111_IMAGE_HEIGHT, 0x04b0 },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x01 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x01 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x10 },
+ { CCI_REG8(0x303e), 0x40 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x97 }, { CCI_REG8(0x3048), 0x00 },
+ { CCI_REG8(0x304c), 0x6f }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0x00 },
+ { CCI_REG8(0x3074), 0x11 }, { CCI_REG8(0x3075), 0x11 },
+ { CCI_REG8(0x3076), 0x11 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x00 }, { CCI_REG8(0x307a), 0x00 },
+ { CCI_REG8(0x309b), 0x20 }, { CCI_REG8(0x309c), 0x13 },
+ { CCI_REG8(0x309e), 0x00 }, { CCI_REG8(0x30a0), 0x14 },
+ { CCI_REG8(0x30a1), 0x08 }, { CCI_REG8(0x30aa), 0x03 },
+ { CCI_REG8(0x30b2), 0x07 }, { CCI_REG8(0x30d5), 0x00 },
+ { CCI_REG8(0x30d6), 0x85 }, { CCI_REG8(0x30d7), 0x2a },
+ { CCI_REG8(0x30d8), 0x64 }, { CCI_REG8(0x30d9), 0x89 },
+ { CCI_REG8(0x30de), 0x00 }, { CCI_REG8(0x30df), 0x20 },
+ { CCI_REG8(0x3102), 0x08 }, { CCI_REG8(0x3103), 0x22 },
+ { CCI_REG8(0x3104), 0x20 }, { CCI_REG8(0x3105), 0x00 },
+ { CCI_REG8(0x3106), 0x87 }, { CCI_REG8(0x3107), 0x00 },
+ { CCI_REG8(0x3108), 0x03 }, { CCI_REG8(0x3109), 0x02 },
+ { CCI_REG8(0x310a), 0x03 }, { CCI_REG8(0x315c), 0x9c },
+ { CCI_REG8(0x315d), 0x9b }, { CCI_REG8(0x316e), 0x9d },
+ { CCI_REG8(0x316f), 0x9c }, { CCI_REG8(0x3318), 0x62 },
+ { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct cci_reg_sequence mode_3280x1098[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0008 }, { IMX111_VERTICAL_START, 0x01f6 },
+ { IMX111_HORIZONTAL_END, 0x0cd7 }, { IMX111_VERTICAL_END, 0x080b },
+ { IMX111_IMAGE_WIDTH, 0x0cd0 }, { IMX111_IMAGE_HEIGHT, 0x044a },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x01 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x01 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x10 },
+ { CCI_REG8(0x303e), 0x40 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x93 }, { CCI_REG8(0x3048), 0x00 },
+ { CCI_REG8(0x304c), 0x67 }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0xe0 },
+ { CCI_REG8(0x3074), 0x12 }, { CCI_REG8(0x3075), 0x12 },
+ { CCI_REG8(0x3076), 0x12 }, { CCI_REG8(0x3077), 0x12 },
+ { CCI_REG8(0x3079), 0x2a }, { CCI_REG8(0x307a), 0x0a },
+ { CCI_REG8(0x309b), 0x60 }, { CCI_REG8(0x309e), 0x04 },
+ { CCI_REG8(0x30a0), 0x15 }, { CCI_REG8(0x30a1), 0x08 },
+ { CCI_REG8(0x30aa), 0x03 }, { CCI_REG8(0x30b2), 0x05 },
+ { CCI_REG8(0x30d5), 0x00 }, { CCI_REG8(0x30d6), 0x85 },
+ { CCI_REG8(0x30d7), 0x2a }, { CCI_REG8(0x30d8), 0x64 },
+ { CCI_REG8(0x30d9), 0x89 }, { CCI_REG8(0x30de), 0x00 },
+ { CCI_REG8(0x30df), 0x20 }, { CCI_REG8(0x3102), 0x08 },
+ { CCI_REG8(0x3103), 0x1d }, { CCI_REG8(0x3104), 0x1e },
+ { CCI_REG8(0x3105), 0x00 }, { CCI_REG8(0x3106), 0x74 },
+ { CCI_REG8(0x3107), 0x00 }, { CCI_REG8(0x3108), 0x03 },
+ { CCI_REG8(0x3109), 0x02 }, { CCI_REG8(0x310a), 0x03 },
+ { CCI_REG8(0x315c), 0x37 }, { CCI_REG8(0x315d), 0x36 },
+ { CCI_REG8(0x316e), 0x38 }, { CCI_REG8(0x316f), 0x37 },
+ { CCI_REG8(0x3318), 0x63 }, { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct cci_reg_sequence mode_3280x1848[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0008 }, { IMX111_VERTICAL_START, 0x0164 },
+ { IMX111_HORIZONTAL_END, 0x0cd7 }, { IMX111_VERTICAL_END, 0x089b },
+ { IMX111_IMAGE_WIDTH, 0x0cd0 }, { IMX111_IMAGE_HEIGHT, 0x0738 },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x01 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x01 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x00 },
+ { CCI_REG8(0x303e), 0x41 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x97 }, { CCI_REG8(0x3048), 0x00 },
+ { CCI_REG8(0x304c), 0x6f }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0x00 },
+ { CCI_REG8(0x3074), 0x11 }, { CCI_REG8(0x3075), 0x11 },
+ { CCI_REG8(0x3076), 0x11 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x00 }, { CCI_REG8(0x307a), 0x00 },
+ { CCI_REG8(0x309b), 0x20 }, { CCI_REG8(0x309c), 0x13 },
+ { CCI_REG8(0x309e), 0x00 }, { CCI_REG8(0x30a0), 0x14 },
+ { CCI_REG8(0x30a1), 0x08 }, { CCI_REG8(0x30aa), 0x03 },
+ { CCI_REG8(0x30b2), 0x07 }, { CCI_REG8(0x30d5), 0x00 },
+ { CCI_REG8(0x30d6), 0x85 }, { CCI_REG8(0x30d7), 0x2a },
+ { CCI_REG8(0x30d8), 0x64 }, { CCI_REG8(0x30d9), 0x89 },
+ { CCI_REG8(0x30de), 0x00 }, { CCI_REG8(0x30df), 0x20 },
+ { CCI_REG8(0x3102), 0x10 }, { CCI_REG8(0x3103), 0x44 },
+ { CCI_REG8(0x3104), 0x40 }, { CCI_REG8(0x3105), 0x00 },
+ { CCI_REG8(0x3106), 0x0d }, { CCI_REG8(0x3107), 0x01 },
+ { CCI_REG8(0x3108), 0x09 }, { CCI_REG8(0x3109), 0x08 },
+ { CCI_REG8(0x310a), 0x0f }, { CCI_REG8(0x315c), 0x5d },
+ { CCI_REG8(0x315d), 0x5c }, { CCI_REG8(0x316e), 0x5e },
+ { CCI_REG8(0x316f), 0x5d }, { CCI_REG8(0x3318), 0x60 },
+ { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct cci_reg_sequence mode_3280x2464[] = {
+ { IMX111_GROUP_WRITE, 1 },
+ { IMX111_HORIZONTAL_START, 0x0008 }, { IMX111_VERTICAL_START, 0x0030 },
+ { IMX111_HORIZONTAL_END, 0x0cd7 }, { IMX111_VERTICAL_END, 0x09cf },
+ { IMX111_IMAGE_WIDTH, 0x0cd0 }, { IMX111_IMAGE_HEIGHT, 0x09a0 },
+ { IMX111_GROUP_WRITE, 0 },
+ { IMX111_H_EVEN_INC, 0x01 }, { IMX111_H_ODD_INC, 0x01 },
+ { IMX111_W_EVEN_INC, 0x01 }, { IMX111_W_ODD_INC, 0x01 },
+ { CCI_REG8(0x3033), 0x00 }, { CCI_REG8(0x303d), 0x00 },
+ { CCI_REG8(0x303e), 0x41 }, { CCI_REG8(0x3040), 0x08 },
+ { CCI_REG8(0x3041), 0x97 }, { CCI_REG8(0x3048), 0x00 },
+ { CCI_REG8(0x304c), 0x6f }, { CCI_REG8(0x304d), 0x03 },
+ { CCI_REG8(0x3064), 0x12 }, { CCI_REG8(0x3073), 0x00 },
+ { CCI_REG8(0x3074), 0x11 }, { CCI_REG8(0x3075), 0x11 },
+ { CCI_REG8(0x3076), 0x11 }, { CCI_REG8(0x3077), 0x11 },
+ { CCI_REG8(0x3079), 0x00 }, { CCI_REG8(0x307a), 0x00 },
+ { CCI_REG8(0x309b), 0x20 }, { CCI_REG8(0x309c), 0x13 },
+ { CCI_REG8(0x309e), 0x00 }, { CCI_REG8(0x30a0), 0x14 },
+ { CCI_REG8(0x30a1), 0x08 }, { CCI_REG8(0x30aa), 0x03 },
+ { CCI_REG8(0x30b2), 0x07 }, { CCI_REG8(0x30d5), 0x00 },
+ { CCI_REG8(0x30d6), 0x85 }, { CCI_REG8(0x30d7), 0x2a },
+ { CCI_REG8(0x30d8), 0x64 }, { CCI_REG8(0x30d9), 0x89 },
+ { CCI_REG8(0x30de), 0x00 }, { CCI_REG8(0x30df), 0x20 },
+ { CCI_REG8(0x3102), 0x10 }, { CCI_REG8(0x3103), 0x44 },
+ { CCI_REG8(0x3104), 0x40 }, { CCI_REG8(0x3105), 0x00 },
+ { CCI_REG8(0x3106), 0x0d }, { CCI_REG8(0x3107), 0x01 },
+ { CCI_REG8(0x3108), 0x09 }, { CCI_REG8(0x3109), 0x08 },
+ { CCI_REG8(0x310a), 0x0f }, { CCI_REG8(0x315c), 0x5d },
+ { CCI_REG8(0x315d), 0x5c }, { CCI_REG8(0x316e), 0x5e },
+ { CCI_REG8(0x316f), 0x5d }, { CCI_REG8(0x3318), 0x60 },
+ { CCI_REG8(0x3348), 0xe0 },
+};
+
+static const struct imx111_mode imx111_modes[] = {
+ [IMX111_MODE_3280x2464] = {
+ .width = 3280,
+ .height = 2464,
+ .vtl_def = 2490,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_3280x2464,
+ .num_of_regs = ARRAY_SIZE(mode_3280x2464),
+ },
+ },
+ [IMX111_MODE_3280x1848] = {
+ .width = 3280,
+ .height = 1848,
+ .vtl_def = 1874,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_3280x1848,
+ .num_of_regs = ARRAY_SIZE(mode_3280x1848),
+ },
+ },
+ [IMX111_MODE_3280x1098] = {
+ .width = 3280,
+ .height = 1098,
+ .vtl_def = 1130,
+ .htl_def = 3500,
+ .reg_list = {
+ .regs = mode_3280x1098,
+ .num_of_regs = ARRAY_SIZE(mode_3280x1098),
+ },
+ },
+ [IMX111_MODE_2100x1200] = {
+ .width = 2100,
+ .height = 1200,
+ .vtl_def = 1260,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_2100x1200,
+ .num_of_regs = ARRAY_SIZE(mode_2100x1200),
+ },
+ },
+ [IMX111_MODE_1952x1098] = {
+ .width = 1952,
+ .height = 1098,
+ .vtl_def = 1884,
+ .htl_def = 3500,
+ .reg_list = {
+ .regs = mode_1952x1098,
+ .num_of_regs = ARRAY_SIZE(mode_1952x1098),
+ },
+ },
+ [IMX111_MODE_1920x1080] = {
+ .width = 1920,
+ .height = 1080,
+ .vtl_def = 1884,
+ .htl_def = 3500,
+ .reg_list = {
+ .regs = mode_1952x1098,
+ .num_of_regs = ARRAY_SIZE(mode_1952x1098),
+ },
+ },
+ [IMX111_MODE_1640x1232] = {
+ .width = 1640,
+ .height = 1232,
+ .vtl_def = 1254,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_1640x1232,
+ .num_of_regs = ARRAY_SIZE(mode_1640x1232),
+ },
+ },
+ [IMX111_MODE_1440x1080] = {
+ .width = 1440,
+ .height = 1080,
+ .vtl_def = 1254,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_1640x1232,
+ .num_of_regs = ARRAY_SIZE(mode_1640x1232),
+ },
+ },
+ [IMX111_MODE_1640x924] = {
+ .width = 1640,
+ .height = 924,
+ .vtl_def = 946,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_1640x924,
+ .num_of_regs = ARRAY_SIZE(mode_1640x924),
+ },
+ },
+ [IMX111_MODE_1308x736] = {
+ .width = 1308,
+ .height = 736,
+ .vtl_def = 2369,
+ .htl_def = 1896,
+ .reg_list = {
+ .regs = mode_1308x736,
+ .num_of_regs = ARRAY_SIZE(mode_1308x736),
+ },
+ },
+ [IMX111_MODE_1280x720] = {
+ .width = 1280,
+ .height = 720,
+ .vtl_def = 2369,
+ .htl_def = 1896,
+ .reg_list = {
+ .regs = mode_1308x736,
+ .num_of_regs = ARRAY_SIZE(mode_1308x736),
+ },
+ },
+ [IMX111_MODE_820x614] = {
+ .width = 820,
+ .height = 614,
+ .vtl_def = 1260,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_820x614,
+ .num_of_regs = ARRAY_SIZE(mode_820x614),
+ },
+ },
+ [IMX111_MODE_640x480] = {
+ .width = 640,
+ .height = 480,
+ .vtl_def = 1260,
+ .htl_def = 3536,
+ .reg_list = {
+ .regs = mode_820x614,
+ .num_of_regs = ARRAY_SIZE(mode_820x614),
+ },
+ },
+};
+
+static inline struct imx111 *sd_to_imx111(struct v4l2_subdev *sd)
+{
+ return container_of_const(sd, struct imx111, sd);
+}
+
+static inline struct imx111 *ctrl_to_imx111(struct v4l2_ctrl *ctrl)
+{
+ return container_of_const(ctrl->handler, struct imx111, hdl);
+}
+
+static u8 to_settle_delay(u64 extclk_rate)
+{
+ u64 extclk_mhz = div_u64(extclk_rate, MEGA);
+
+ return DIV_ROUND_UP(IMX111_PLL_SETTLING_TIME_DEFAULT * extclk_mhz - 63, 64);
+}
+
+static u32 imx111_get_format_code(struct imx111 *sensor, u32 code, bool test)
+{
+ u32 i;
+
+ for (i = 0; i < ARRAY_SIZE(imx111_mbus_formats); i++)
+ if (imx111_mbus_formats[i] == code)
+ break;
+
+ if (i >= ARRAY_SIZE(imx111_mbus_formats))
+ i = 0;
+
+ if (test)
+ return imx111_mbus_formats[i];
+
+ i = (i & ~3) | (sensor->vflip->val ? 2 : 0) |
+ (sensor->hflip->val ? 1 : 0);
+
+ return imx111_mbus_formats[i];
+}
+
+static u32 imx111_get_format_bpp(const struct v4l2_mbus_framefmt *format)
+{
+ switch (format->code) {
+ case MEDIA_BUS_FMT_SRGGB8_1X8:
+ case MEDIA_BUS_FMT_SGRBG8_1X8:
+ case MEDIA_BUS_FMT_SGBRG8_1X8:
+ case MEDIA_BUS_FMT_SBGGR8_1X8:
+ return 8;
+
+ case MEDIA_BUS_FMT_SRGGB10_1X10:
+ case MEDIA_BUS_FMT_SGRBG10_1X10:
+ case MEDIA_BUS_FMT_SGBRG10_1X10:
+ case MEDIA_BUS_FMT_SBGGR10_1X10:
+ default:
+ return 10;
+ }
+}
+
+static int imx111_update_digital_gain(struct imx111 *sensor, u32 val)
+{
+ int ret = 0;
+
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+
+ cci_write(sensor->regmap, IMX111_REG_DIG_GAIN_GREENR, val, &ret);
+ cci_write(sensor->regmap, IMX111_REG_DIG_GAIN_RED, val, &ret);
+ cci_write(sensor->regmap, IMX111_REG_DIG_GAIN_BLUE, val, &ret);
+ cci_write(sensor->regmap, IMX111_REG_DIG_GAIN_GREENB, val, &ret);
+
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+
+ return ret;
+}
+
+static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct imx111 *sensor = ctrl_to_imx111(ctrl);
+ struct device *dev = regmap_get_device(sensor->regmap);
+ s64 max;
+ int ret = 0;
+
+ /* Propagate change of current control to all related controls */
+ switch (ctrl->id) {
+ case V4L2_CID_VBLANK:
+ /* Update max exposure while meeting expected vblanking */
+ max = sensor->cur_mode->height + ctrl->val - 5;
+ __v4l2_ctrl_modify_range(sensor->exposure,
+ sensor->exposure->minimum,
+ max, sensor->exposure->step, max);
+ break;
+ }
+
+ /*
+ * Applying V4L2 control value only happens
+ * when power is up for streaming
+ */
+ if (!pm_runtime_get_if_in_use(dev))
+ return 0;
+
+ switch (ctrl->id) {
+ case V4L2_CID_ANALOGUE_GAIN:
+ cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
+ break;
+ case V4L2_CID_DIGITAL_GAIN:
+ ret = imx111_update_digital_gain(sensor, ctrl->val);
+ break;
+ case V4L2_CID_EXPOSURE:
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+ break;
+ case V4L2_CID_HBLANK:
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
+ cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
+ sensor->cur_mode->width + ctrl->val, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+ break;
+ case V4L2_CID_VBLANK:
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
+ cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
+ sensor->cur_mode->height + ctrl->val, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+ break;
+ case V4L2_CID_HFLIP:
+ case V4L2_CID_VFLIP:
+ cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
+ sensor->hflip->val | sensor->vflip->val << 1, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN:
+ cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_RED:
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_GREENR:
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_BLUE:
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN_GREENB:
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ pm_runtime_put(dev);
+
+ return ret;
+}
+
+static const struct v4l2_ctrl_ops imx111_ctrl_ops = {
+ .s_ctrl = imx111_set_ctrl,
+};
+
+static const char * const test_pattern_menu[] = {
+ "Disabled",
+ "Solid Color Fill",
+ "Standard Color Bars",
+ "Fade To Grey Color Bars",
+ "Pseudorandom data",
+};
+
+static int imx111_init_controls(struct imx111 *sensor)
+{
+ const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
+ struct device *dev = regmap_get_device(sensor->regmap);
+ const struct imx111_mode *mode = sensor->cur_mode;
+ struct v4l2_fwnode_device_properties props;
+ struct v4l2_subdev *sd = &sensor->sd;
+ struct v4l2_ctrl_handler *hdl = &sensor->hdl;
+ s64 pixel_rate_min, pixel_rate_max;
+ int i, ret;
+
+ ret = v4l2_fwnode_device_parse(dev, &props);
+ if (ret < 0)
+ return ret;
+
+ ret = v4l2_ctrl_handler_init(hdl, 13);
+ if (ret)
+ return ret;
+
+ pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
+ pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
+ sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
+ pixel_rate_min, pixel_rate_max,
+ 1, div_u64(sensor->pixel_clk_raw,
+ 2 * sensor->data_depth));
+
+ sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
+ 0, 0, &sensor->default_link_freq);
+ if (sensor->link_freq)
+ sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
+ IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
+ IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
+
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
+ IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
+ IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
+
+ sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
+ if (sensor->hflip)
+ sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+ sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
+ if (sensor->vflip)
+ sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+ sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
+ IMX111_VTL_MAX - mode->height, 1,
+ mode->vtl_def - mode->height);
+ sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
+ IMX111_HTL_MAX - mode->width, 1,
+ mode->htl_def - mode->width);
+
+ /*
+ * The maximum coarse integration time is the frame length in lines
+ * minus five.
+ */
+ sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
+ IMX111_INTEGRATION_TIME_MIN,
+ IMX111_PIXEL_ARRAY_HEIGHT - 5,
+ IMX111_INTEGRATION_TIME_STEP,
+ IMX111_PIXEL_ARRAY_HEIGHT - 5);
+
+ v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
+
+ v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
+ test_pattern_menu);
+ for (i = 0; i < 4; i++) {
+ /*
+ * The assumption is that
+ * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
+ * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
+ * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
+ */
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
+ IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
+ IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
+ /* The "Solid color" pattern is white by default */
+ }
+
+ if (hdl->error)
+ return hdl->error;
+
+ sd->ctrl_handler = hdl;
+
+ return 0;
+};
+
+static int imx111_enable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ struct imx111 *sensor = sd_to_imx111(sd);
+ struct device *dev = regmap_get_device(sensor->regmap);
+ const struct imx111_mode *mode = sensor->cur_mode;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ return ret;
+
+ /* Apply default values of current mode */
+ ret = cci_multi_reg_write(sensor->regmap, mode->reg_list.regs,
+ mode->reg_list.num_of_regs, NULL);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize the sensor\n");
+ goto err_rpm_put;
+ }
+
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ IMX111_GROUP_WRITE_ON, &ret);
+ cci_write(sensor->regmap, IMX111_DATA_DEPTH,
+ sensor->data_depth | sensor->data_depth << 8, &ret);
+ cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
+ 0, &ret);
+
+ if (ret)
+ goto err_rpm_put;
+
+ ret = __v4l2_ctrl_handler_setup(&sensor->hdl);
+ if (ret)
+ goto err_rpm_put;
+
+ ret = cci_write(sensor->regmap, IMX111_STREAMING_MODE, IMX111_MODE_STREAMING, NULL);
+ if (ret)
+ dev_err(dev, "failed to start stream");
+
+ /* vflip and hflip cannot change during streaming */
+ __v4l2_ctrl_grab(sensor->vflip, true);
+ __v4l2_ctrl_grab(sensor->hflip, true);
+
+ msleep(30);
+
+ return 0;
+
+err_rpm_put:
+ pm_runtime_put_autosuspend(dev);
+ return ret;
+}
+
+static int imx111_disable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ struct imx111 *sensor = sd_to_imx111(sd);
+ struct device *dev = regmap_get_device(sensor->regmap);
+ int ret;
+
+ ret = cci_write(sensor->regmap, IMX111_STREAMING_MODE,
+ IMX111_MODE_STANDBY, NULL);
+ if (ret)
+ dev_err(dev, "failed to stop stream\n");
+
+ __v4l2_ctrl_grab(sensor->vflip, false);
+ __v4l2_ctrl_grab(sensor->hflip, false);
+
+ pm_runtime_put_autosuspend(dev);
+
+ return ret;
+}
+
+static int imx111_initialize(struct imx111 *sensor)
+{
+ struct device *dev = regmap_get_device(sensor->regmap);
+ int ret;
+
+ /* Configure the PLL. */
+ cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
+ sensor->pll->pre_div, &ret);
+ cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
+ cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
+ cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
+ to_settle_delay(sensor->pll->extclk_rate), &ret);
+
+ ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
+ ARRAY_SIZE(imx111_global_init), NULL);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize the sensor\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * IMX111 Pad Subdev Init and Operations
+ */
+static int imx111_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct imx111 *sensor = sd_to_imx111(sd);
+
+ if (code->index >= ARRAY_SIZE(imx111_mbus_formats) / 4)
+ return -EINVAL;
+
+ code->code = imx111_get_format_code(sensor, imx111_mbus_formats[code->index * 4], false);
+
+ return 0;
+}
+
+static int imx111_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ struct imx111 *sensor = sd_to_imx111(sd);
+ u32 code;
+
+ if (fse->index >= ARRAY_SIZE(imx111_modes))
+ return -EINVAL;
+
+ code = imx111_get_format_code(sensor, fse->code, true);
+ if (fse->code != code)
+ return -EINVAL;
+
+ fse->min_width = imx111_modes[fse->index].width;
+ fse->max_width = fse->min_width;
+ fse->min_height = imx111_modes[fse->index].height;
+ fse->max_height = fse->min_height;
+
+ return 0;
+}
+
+static int imx111_set_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_format *format)
+{
+ struct imx111 *sensor = sd_to_imx111(sd);
+ struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
+ struct v4l2_mbus_framefmt *fmt;
+ const struct imx111_mode *mode;
+
+ mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
+ width, height,
+ mbus_fmt->width, mbus_fmt->height);
+
+ fmt = v4l2_subdev_state_get_format(state, format->pad);
+
+ fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
+ fmt->width = mode->width;
+ fmt->height = mode->height;
+ fmt->colorspace = V4L2_COLORSPACE_RAW;
+
+ *mbus_fmt = *fmt;
+
+ if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ sensor->cur_mode = mode;
+ sensor->data_depth = imx111_get_format_bpp(fmt);
+ __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
+ div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
+
+ __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
+ IMX111_VTL_MAX - mode->height, 1,
+ mode->vtl_def - mode->height);
+ __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
+
+ __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
+ IMX111_HTL_MAX - mode->width, 1,
+ mode->htl_def - mode->width);
+ __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
+ }
+
+ return 0;
+}
+
+static int imx111_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state)
+{
+ struct imx111 *sensor = sd_to_imx111(sd);
+ const struct imx111_mode *mode = sensor->cur_mode;
+ struct v4l2_mbus_framefmt *fmt;
+
+ fmt = v4l2_subdev_state_get_format(sd_state, 0);
+
+ fmt->code = MEDIA_BUS_FMT_SGBRG10_1X10;
+ fmt->width = mode->width;
+ fmt->height = mode->height;
+ fmt->field = V4L2_FIELD_NONE;
+ fmt->colorspace = V4L2_COLORSPACE_RAW;
+ fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+ fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+ fmt->xfer_func = V4L2_XFER_FUNC_NONE;
+
+ return 0;
+}
+
+static const struct v4l2_subdev_video_ops imx111_video_ops = {
+ .s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops imx111_pad_ops = {
+ .enum_mbus_code = imx111_enum_mbus_code,
+ .enum_frame_size = imx111_enum_frame_size,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = imx111_set_format,
+ .enable_streams = imx111_enable_streams,
+ .disable_streams = imx111_disable_streams,
+};
+
+static const struct v4l2_subdev_ops imx111_subdev_ops = {
+ .video = &imx111_video_ops,
+ .pad = &imx111_pad_ops,
+};
+
+static const struct media_entity_operations imx111_subdev_entity_ops = {
+ .link_validate = v4l2_subdev_link_validate,
+};
+
+static const struct v4l2_subdev_internal_ops imx111_internal_ops = {
+ .init_state = imx111_init_state,
+};
+
+static int imx111_init_subdev(struct imx111 *sensor, struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct v4l2_subdev *sd = &sensor->sd;
+ struct media_pad *pad = &sensor->pad;
+ struct v4l2_ctrl_handler *hdl = &sensor->hdl;
+ int ret;
+
+ /* Initialize the subdev. */
+ v4l2_i2c_subdev_init(sd, client, &imx111_subdev_ops);
+
+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ sd->internal_ops = &imx111_internal_ops;
+
+ /* Initialize the media entity. */
+ sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
+ sd->entity.ops = &imx111_subdev_entity_ops;
+ pad->flags = MEDIA_PAD_FL_SOURCE;
+
+ ret = media_entity_pads_init(&sd->entity, 1, pad);
+ if (ret < 0) {
+ dev_err(dev, "failed to init entity pads: %d", ret);
+ return ret;
+ }
+
+ /* Initialize the control handler. */
+ ret = imx111_init_controls(sensor);
+ if (ret)
+ goto error;
+
+ return 0;
+error:
+ v4l2_ctrl_handler_free(hdl);
+ media_entity_cleanup(&sd->entity);
+ return ret;
+};
+
+/* -----------------------------------------------------------------------------
+ * Power Management
+ */
+
+static int imx111_power_on(struct imx111 *sensor)
+{
+ int ret;
+
+ if (sensor->reset)
+ gpiod_set_value(sensor->reset, 1);
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(imx111_supplies),
+ sensor->supplies);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(500, 600);
+
+ if (sensor->reset)
+ gpiod_set_value(sensor->reset, 0);
+
+ usleep_range(200, 250);
+
+ ret = clk_prepare_enable(sensor->extclk);
+ if (ret < 0)
+ goto error_regulator;
+
+ usleep_range(200, 250);
+
+ return 0;
+
+error_regulator:
+ regulator_bulk_disable(ARRAY_SIZE(imx111_supplies), sensor->supplies);
+ return ret;
+}
+
+static void imx111_power_off(struct imx111 *sensor)
+{
+ if (sensor->reset)
+ gpiod_set_value(sensor->reset, 1);
+ usleep_range(1000, 2000);
+
+ clk_disable_unprepare(sensor->extclk);
+ regulator_bulk_disable(ARRAY_SIZE(imx111_supplies), sensor->supplies);
+}
+
+static int __maybe_unused imx111_pm_runtime_resume(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct imx111 *sensor = sd_to_imx111(sd);
+ int ret;
+
+ ret = imx111_power_on(sensor);
+ if (ret)
+ return ret;
+
+ ret = imx111_initialize(sensor);
+ if (ret) {
+ imx111_power_off(sensor);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int __maybe_unused imx111_pm_runtime_suspend(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct imx111 *sensor = sd_to_imx111(sd);
+
+ imx111_power_off(sensor);
+
+ return 0;
+}
+
+static const struct dev_pm_ops imx111_pm_ops = {
+ SET_RUNTIME_PM_OPS(imx111_pm_runtime_suspend,
+ imx111_pm_runtime_resume, NULL)
+};
+
+/* -----------------------------------------------------------------------------
+ * Probe & Remove
+ */
+
+static int imx111_identify_module(struct imx111 *sensor)
+{
+ struct device *dev = regmap_get_device(sensor->regmap);
+ u64 value, revision, manufacturer;
+ int ret;
+
+ ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
+ if (ret)
+ return ret;
+
+ if (value != IMX111_CHIP_ID) {
+ dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
+ return -ENXIO;
+ }
+
+ cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
+ cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
+
+ dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
+ value, revision, manufacturer);
+
+ return 0;
+}
+
+static int imx111_clk_init(struct imx111 *sensor)
+{
+ struct device *dev = regmap_get_device(sensor->regmap);
+ u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
+ u64 extclk_rate, system_clk;
+ unsigned int i;
+
+ extclk_rate = clk_get_rate(sensor->extclk);
+ if (!extclk_rate)
+ return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
+
+ for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
+ if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
+ sensor->pll = &imx111_pll[i];
+ break;
+ }
+ }
+ if (!sensor->pll)
+ return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
+
+ system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
+
+ /*
+ * Pixel clock or Logic clock is used for internal image processing is
+ * generated by dividing into 1/10 or 1/8 frequency according to the
+ * word length of the CSI2 interface. This clock is designating the pixel
+ * rate and used as the base of integration time, frame rate etc.
+ */
+ sensor->pixel_clk_raw = system_clk * ndata_lanes;
+
+ /*
+ * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
+ * for RAW10 default format.
+ */
+ sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
+ 2 * IMX111_DATA_DEPTH_RAW10);
+
+ if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
+ sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
+ return dev_err_probe(dev, -EINVAL,
+ "Unsupported DT link-frequencies, expected %llu\n",
+ sensor->default_link_freq);
+
+ return 0;
+}
+
+static int imx111_parse_dt(struct imx111 *sensor)
+{
+ struct device *dev = regmap_get_device(sensor->regmap);
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
+ struct fwnode_handle *ep;
+ int ret;
+
+ ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+ if (!ep) {
+ dev_err(dev, "No endpoint found\n");
+ return -EINVAL;
+ }
+
+ ret = v4l2_fwnode_endpoint_alloc_parse(ep, &sensor->bus_cfg);
+ fwnode_handle_put(ep);
+ if (ret < 0) {
+ dev_err(dev, "Failed to parse endpoint\n");
+ goto error;
+ }
+
+ sensor->bus_cfg.bus_type = V4L2_MBUS_CSI2_DPHY;
+
+ /* Check the number of MIPI CSI2 data lanes */
+ if (sensor->bus_cfg.bus.mipi_csi2.num_data_lanes > 2) {
+ dev_err(dev, "number of lanes is more than 2\n");
+ ret = -EINVAL;
+ goto error;
+ }
+
+ return 0;
+
+error:
+ v4l2_fwnode_endpoint_free(&sensor->bus_cfg);
+ return ret;
+}
+
+static int imx111_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct imx111 *sensor;
+ int ret;
+
+ sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+ if (!sensor)
+ return -ENOMEM;
+
+ sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
+ if (IS_ERR(sensor->regmap))
+ return dev_err_probe(dev, PTR_ERR(sensor->regmap),
+ "Failed to allocate register map\n");
+
+ sensor->extclk = devm_v4l2_sensor_clk_get(dev, NULL);
+ if (IS_ERR(sensor->extclk))
+ return dev_err_probe(dev, PTR_ERR(sensor->extclk), "Failed to get clock\n");
+
+ sensor->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(sensor->reset))
+ return dev_err_probe(dev, PTR_ERR(sensor->reset), "Failed to get reset GPIO\n");
+
+ ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(imx111_supplies),
+ imx111_supplies, &sensor->supplies);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+ ret = imx111_parse_dt(sensor);
+ if (ret < 0)
+ return ret;
+
+ ret = imx111_clk_init(sensor);
+ if (ret < 0)
+ goto error_ep_free;
+
+ ret = imx111_power_on(sensor);
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "Could not power on the device\n");
+ goto error_ep_free;
+ }
+
+ ret = imx111_identify_module(sensor);
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "Could not identify module\n");
+ goto error_power_off;
+ }
+
+ sensor->cur_mode = &imx111_modes[IMX111_MODE_3280x2464];
+ sensor->data_depth = IMX111_DATA_DEPTH_RAW10;
+
+ ret = imx111_initialize(sensor);
+ if (ret < 0)
+ goto error_power_off;
+
+ ret = imx111_init_subdev(sensor, client);
+ if (ret < 0) {
+ dev_err(dev, "failed to init controls: %d", ret);
+ goto error_v4l2_ctrl_handler_free;
+ }
+
+ ret = v4l2_subdev_init_finalize(&sensor->sd);
+ if (ret)
+ goto error_v4l2_ctrl_handler_free;
+
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+
+ ret = v4l2_async_register_subdev_sensor(&sensor->sd);
+ if (ret < 0) {
+ dev_err(dev, "failed to register V4L2 subdev: %d", ret);
+ goto error_pm;
+ }
+
+ pm_runtime_idle(dev);
+ pm_runtime_set_autosuspend_delay(dev, 1000);
+ pm_runtime_use_autosuspend(dev);
+
+ return 0;
+
+error_pm:
+ v4l2_subdev_cleanup(&sensor->sd);
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+
+error_v4l2_ctrl_handler_free:
+ v4l2_ctrl_handler_free(&sensor->hdl);
+ media_entity_cleanup(&sensor->sd.entity);
+
+error_power_off:
+ imx111_power_off(sensor);
+
+error_ep_free:
+ v4l2_fwnode_endpoint_free(&sensor->bus_cfg);
+
+ return ret;
+}
+
+static void imx111_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx111 *sensor = sd_to_imx111(sd);
+
+ v4l2_async_unregister_subdev(&sensor->sd);
+ v4l2_subdev_cleanup(sd);
+ media_entity_cleanup(&sensor->sd.entity);
+ v4l2_ctrl_handler_free(&sensor->hdl);
+ v4l2_fwnode_endpoint_free(&sensor->bus_cfg);
+
+ /*
+ * Disable runtime PM. In case runtime PM is disabled in the kernel,
+ * make sure to turn power off manually.
+ */
+ pm_runtime_disable(&client->dev);
+ if (!pm_runtime_status_suspended(&client->dev)) {
+ imx111_power_off(sensor);
+ pm_runtime_set_suspended(&client->dev);
+ }
+}
+
+static const struct of_device_id imx111_of_match[] = {
+ { .compatible = "sony,imx111" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx111_of_match);
+
+static struct i2c_driver imx111_i2c_driver = {
+ .driver = {
+ .name = "imx111",
+ .of_match_table = imx111_of_match,
+ .pm = &imx111_pm_ops,
+ },
+ .probe = imx111_probe,
+ .remove = imx111_remove,
+};
+module_i2c_driver(imx111_i2c_driver);
+
+MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
+MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
+MODULE_LICENSE("GPL");
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 11:57 ` [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver Svyatoslav Ryhel
@ 2025-10-30 14:55 ` Tarang Raval
2025-10-30 15:03 ` Svyatoslav Ryhel
2025-10-30 15:13 ` Svyatoslav Ryhel
0 siblings, 2 replies; 13+ messages in thread
From: Tarang Raval @ 2025-10-30 14:55 UTC (permalink / raw)
To: Svyatoslav Ryhel, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Svyatoslav,
> Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> camera sensor using the i2c bus for control and the csi-2 bus for data.
>
> The following features are supported:
> - manual exposure, digital and analog gain control support
> - pixel rate/link freq control support
> - supported resolution up to 3280x2464 for single shot capture
> - supported resolution up to 1920x1080 @ 30fps for video
> - supported bayer order output SGBRG10 and SGBRG8
>
> Camera module seems to be partially compatible with Nokia SMIA but it
> lacks a few registers required for clock calculations and has different
> vendor-specific per-mode configurations which makes it incompatible with
> existing CCS driver.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
> +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> + struct device *dev = regmap_get_device(sensor->regmap);
> + s64 max;
> + int ret = 0;
> +
> + /* Propagate change of current control to all related controls */
> + switch (ctrl->id) {
Do we need the switch statement, since only one case is present?
You can use an 'if' instead.
> + case V4L2_CID_VBLANK:
> + /* Update max exposure while meeting expected vblanking */
> + max = sensor->cur_mode->height + ctrl->val - 5;
You can define a macro for the value 5 to improve readability.
Also, make this change in the init_control function.
> + __v4l2_ctrl_modify_range(sensor->exposure,
> + sensor->exposure->minimum,
> + max, sensor->exposure->step, max);
This may fail; consider adding an error check.
> + break;
> + }
> +
> + /*
> + * Applying V4L2 control value only happens
> + * when power is up for streaming
> + */
> + if (!pm_runtime_get_if_in_use(dev))
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_ANALOGUE_GAIN:
> + cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> + break;
> + case V4L2_CID_DIGITAL_GAIN:
> + ret = imx111_update_digital_gain(sensor, ctrl->val);
> + break;
> + case V4L2_CID_EXPOSURE:
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + IMX111_GROUP_WRITE_ON, &ret);
> + cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + 0, &ret);
> + break;
> + case V4L2_CID_HBLANK:
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + IMX111_GROUP_WRITE_ON, &ret);
> + dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> + cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> + sensor->cur_mode->width + ctrl->val, &ret);
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + 0, &ret);
> + break;
> + case V4L2_CID_VBLANK:
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + IMX111_GROUP_WRITE_ON, &ret);
> + dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> + cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> + sensor->cur_mode->height + ctrl->val, &ret);
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + 0, &ret);
> + break;
> + case V4L2_CID_HFLIP:
> + case V4L2_CID_VFLIP:
> + cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> + sensor->hflip->val | sensor->vflip->val << 1, &ret);
> + break;
> + case V4L2_CID_TEST_PATTERN:
> + cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> + break;
> + case V4L2_CID_TEST_PATTERN_RED:
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + IMX111_GROUP_WRITE_ON, &ret);
> + cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + 0, &ret);
> + break;
> + case V4L2_CID_TEST_PATTERN_GREENR:
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + IMX111_GROUP_WRITE_ON, &ret);
> + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + 0, &ret);
> + break;
> + case V4L2_CID_TEST_PATTERN_BLUE:
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + IMX111_GROUP_WRITE_ON, &ret);
> + cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + 0, &ret);
> + break;
> + case V4L2_CID_TEST_PATTERN_GREENB:
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + IMX111_GROUP_WRITE_ON, &ret);
> + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> + 0, &ret);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + pm_runtime_put(dev);
> +
> + return ret;
> +}
---
> +static int imx111_init_controls(struct imx111 *sensor)
> +{
> + const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> + struct device *dev = regmap_get_device(sensor->regmap);
> + const struct imx111_mode *mode = sensor->cur_mode;
> + struct v4l2_fwnode_device_properties props;
> + struct v4l2_subdev *sd = &sensor->sd;
No need for a new variable; there is only one user in the function.
> + struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> + s64 pixel_rate_min, pixel_rate_max;
> + int i, ret;
> +
> + ret = v4l2_fwnode_device_parse(dev, &props);
> + if (ret < 0)
> + return ret;
> +
> + ret = v4l2_ctrl_handler_init(hdl, 13);
Now there are 15 controls.
No need for explicit error checking; you can omit the error check if you'd like.
> + if (ret)
> + return ret;
> +
> + pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> + pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> + sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> + pixel_rate_min, pixel_rate_max,
> + 1, div_u64(sensor->pixel_clk_raw,
> + 2 * sensor->data_depth));
> +
> + sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> + 0, 0, &sensor->default_link_freq);
> + if (sensor->link_freq)
> + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> + IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> + IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> +
> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> + IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> + IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> +
> + sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> + if (sensor->hflip)
> + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> + sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> + if (sensor->vflip)
> + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> + sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> + IMX111_VTL_MAX - mode->height, 1,
> + mode->vtl_def - mode->height);
> + sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> + IMX111_HTL_MAX - mode->width, 1,
> + mode->htl_def - mode->width);
> +
> + /*
> + * The maximum coarse integration time is the frame length in lines
> + * minus five.
> + */
> + sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> + IMX111_INTEGRATION_TIME_MIN,
> + IMX111_PIXEL_ARRAY_HEIGHT - 5,
> + IMX111_INTEGRATION_TIME_STEP,
> + IMX111_PIXEL_ARRAY_HEIGHT - 5);
> +
> + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> +
> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> + test_pattern_menu);
> + for (i = 0; i < 4; i++) {
> + /*
> + * The assumption is that
> + * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> + * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
> + * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> + */
> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> + IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> + IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> + /* The "Solid color" pattern is white by default */
> + }
> +
> + if (hdl->error)
> + return hdl->error;
> +
> + sd->ctrl_handler = hdl;
> +
> + return 0;
> +};
---
> +static int imx111_initialize(struct imx111 *sensor)
> +{
> + struct device *dev = regmap_get_device(sensor->regmap);
> + int ret;
ret = 0;
> +
> + /* Configure the PLL. */
> + cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> + sensor->pll->pre_div, &ret);
> + cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> + cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> + cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> + to_settle_delay(sensor->pll->extclk_rate), &ret);
> +
> + ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> + ARRAY_SIZE(imx111_global_init), NULL);
You are overwriting the previous errors.
please use ret |=
> + if (ret < 0) {
> + dev_err(dev, "Failed to initialize the sensor\n");
> + return ret;
> + }
> +
> + return 0;
> +}
---
> +static int imx111_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_format *format)
> +{
> + struct imx111 *sensor = sd_to_imx111(sd);
> + struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> + struct v4l2_mbus_framefmt *fmt;
> + const struct imx111_mode *mode;
> +
> + mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> + width, height,
> + mbus_fmt->width, mbus_fmt->height);
> +
> + fmt = v4l2_subdev_state_get_format(state, format->pad);
> +
> + fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> + fmt->width = mode->width;
> + fmt->height = mode->height;
> + fmt->colorspace = V4L2_COLORSPACE_RAW;
> +
> + *mbus_fmt = *fmt;
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + sensor->cur_mode = mode;
> + sensor->data_depth = imx111_get_format_bpp(fmt);
> + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> + div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> +
> + __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> + IMX111_VTL_MAX - mode->height, 1,
> + mode->vtl_def - mode->height);
> + __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> +
> + __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> + IMX111_HTL_MAX - mode->width, 1,
> + mode->htl_def - mode->width);
> + __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
All the above V4L2 operations need to check for errors.
> + }
> +
> + return 0;
> +}
---
> +static int imx111_identify_module(struct imx111 *sensor)
> +{
> + struct device *dev = regmap_get_device(sensor->regmap);
> + u64 value, revision, manufacturer;
> + int ret;
> +
> + ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> + if (ret)
> + return ret;
> +
> + if (value != IMX111_CHIP_ID) {
> + dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> + return -ENXIO;
> + }
> +
> + cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> + cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
Instead of NULL, pass ret for the error code, and return ret at the end.
> +
> + dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> + value, revision, manufacturer);
> +
> + return 0;
> +}
> +
> +static int imx111_clk_init(struct imx111 *sensor)
> +{
> + struct device *dev = regmap_get_device(sensor->regmap);
> + u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> + u64 extclk_rate, system_clk;
> + unsigned int i;
> +
> + extclk_rate = clk_get_rate(sensor->extclk);
> + if (!extclk_rate)
> + return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> +
> + for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> + if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> + sensor->pll = &imx111_pll[i];
> + break;
> + }
> + }
> + if (!sensor->pll)
> + return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
Max line length should be 80 columns. This applies everywhere the line
length exceeds 80 characters.
> +
> + system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> +
> + /*
> + * Pixel clock or Logic clock is used for internal image processing is
> + * generated by dividing into 1/10 or 1/8 frequency according to the
> + * word length of the CSI2 interface. This clock is designating the pixel
> + * rate and used as the base of integration time, frame rate etc.
> + */
> + sensor->pixel_clk_raw = system_clk * ndata_lanes;
> +
> + /*
> + * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> + * for RAW10 default format.
> + */
> + sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> + 2 * IMX111_DATA_DEPTH_RAW10);
> +
> + if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> + sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> + return dev_err_probe(dev, -EINVAL,
> + "Unsupported DT link-frequencies, expected %llu\n",
> + sensor->default_link_freq);
> +
> + return 0;
> +}
---
> +static const struct of_device_id imx111_of_match[] = {
> + { .compatible = "sony,imx111" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx111_of_match);
> +
> +static struct i2c_driver imx111_i2c_driver = {
> + .driver = {
> + .name = "imx111",
> + .of_match_table = imx111_of_match,
> + .pm = &imx111_pm_ops,
> + },
> + .probe = imx111_probe,
> + .remove = imx111_remove,
> +};
> +module_i2c_driver(imx111_i2c_driver);
> +
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> +MODULE_LICENSE("GPL");
Best Regards,
Tarang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 14:55 ` Tarang Raval
@ 2025-10-30 15:03 ` Svyatoslav Ryhel
2025-10-30 15:13 ` Svyatoslav Ryhel
1 sibling, 0 replies; 13+ messages in thread
From: Svyatoslav Ryhel @ 2025-10-30 15:03 UTC (permalink / raw)
To: Tarang Raval
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sakari Ailus, Hans Verkuil, Hans de Goede,
André Apitzsch, Sylvain Petinot, Benjamin Mugnier,
Dongcheng Yan, Heimir Thor Sverrisson,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
>
> Hi Svyatoslav,
>
> > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > camera sensor using the i2c bus for control and the csi-2 bus for data.
> >
> > The following features are supported:
> > - manual exposure, digital and analog gain control support
> > - pixel rate/link freq control support
> > - supported resolution up to 3280x2464 for single shot capture
> > - supported resolution up to 1920x1080 @ 30fps for video
> > - supported bayer order output SGBRG10 and SGBRG8
> >
> > Camera module seems to be partially compatible with Nokia SMIA but it
> > lacks a few registers required for clock calculations and has different
> > vendor-specific per-mode configurations which makes it incompatible with
> > existing CCS driver.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>
> ---
>
> > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + s64 max;
> > + int ret = 0;
> > +
> > + /* Propagate change of current control to all related controls */
> > + switch (ctrl->id) {
>
> Do we need the switch statement, since only one case is present?
> You can use an 'if' instead.
>
> > + case V4L2_CID_VBLANK:
> > + /* Update max exposure while meeting expected vblanking */
> > + max = sensor->cur_mode->height + ctrl->val - 5;
>
> You can define a macro for the value 5 to improve readability.
> Also, make this change in the init_control function.
>
> > + __v4l2_ctrl_modify_range(sensor->exposure,
> > + sensor->exposure->minimum,
> > + max, sensor->exposure->step, max);
>
> This may fail; consider adding an error check.
>
> > + break;
> > + }
> > +
> > + /*
> > + * Applying V4L2 control value only happens
> > + * when power is up for streaming
> > + */
> > + if (!pm_runtime_get_if_in_use(dev))
> > + return 0;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_ANALOGUE_GAIN:
> > + cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> > + break;
> > + case V4L2_CID_DIGITAL_GAIN:
> > + ret = imx111_update_digital_gain(sensor, ctrl->val);
> > + break;
> > + case V4L2_CID_EXPOSURE:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_HBLANK:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> > + cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> > + sensor->cur_mode->width + ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_VBLANK:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> > + cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> > + sensor->cur_mode->height + ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_HFLIP:
> > + case V4L2_CID_VFLIP:
> > + cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> > + sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN:
> > + cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_RED:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_GREENR:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_BLUE:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_GREENB:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > +
> > + pm_runtime_put(dev);
> > +
> > + return ret;
> > +}
>
> ---
>
> > +static int imx111_init_controls(struct imx111 *sensor)
> > +{
> > + const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + const struct imx111_mode *mode = sensor->cur_mode;
> > + struct v4l2_fwnode_device_properties props;
> > + struct v4l2_subdev *sd = &sensor->sd;
>
> No need for a new variable; there is only one user in the function.
>
> > + struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> > + s64 pixel_rate_min, pixel_rate_max;
> > + int i, ret;
> > +
> > + ret = v4l2_fwnode_device_parse(dev, &props);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = v4l2_ctrl_handler_init(hdl, 13);
>
> Now there are 15 controls.
>
> No need for explicit error checking; you can omit the error check if you'd like.
>
> > + if (ret)
> > + return ret;
> > +
> > + pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> > + pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> > + sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> > + pixel_rate_min, pixel_rate_max,
> > + 1, div_u64(sensor->pixel_clk_raw,
> > + 2 * sensor->data_depth));
> > +
> > + sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> > + 0, 0, &sensor->default_link_freq);
> > + if (sensor->link_freq)
> > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > + IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> > + IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> > +
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> > + IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> > + IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> > +
> > + sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + if (sensor->hflip)
> > + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +
> > + sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > + if (sensor->vflip)
> > + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +
> > + sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> > + IMX111_VTL_MAX - mode->height, 1,
> > + mode->vtl_def - mode->height);
> > + sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> > + IMX111_HTL_MAX - mode->width, 1,
> > + mode->htl_def - mode->width);
> > +
> > + /*
> > + * The maximum coarse integration time is the frame length in lines
> > + * minus five.
> > + */
> > + sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > + IMX111_INTEGRATION_TIME_MIN,
> > + IMX111_PIXEL_ARRAY_HEIGHT - 5,
> > + IMX111_INTEGRATION_TIME_STEP,
> > + IMX111_PIXEL_ARRAY_HEIGHT - 5);
> > +
> > + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > +
> > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> > + ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> > + test_pattern_menu);
> > + for (i = 0; i < 4; i++) {
> > + /*
> > + * The assumption is that
> > + * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> > + * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
> > + * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> > + */
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> > + IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> > + IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> > + /* The "Solid color" pattern is white by default */
> > + }
> > +
> > + if (hdl->error)
> > + return hdl->error;
> > +
> > + sd->ctrl_handler = hdl;
> > +
> > + return 0;
> > +};
>
> ---
>
> > +static int imx111_initialize(struct imx111 *sensor)
> > +{
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + int ret;
>
> ret = 0;
>
> > +
> > + /* Configure the PLL. */
> > + cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> > + sensor->pll->pre_div, &ret);
> > + cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> > + cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> > + cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> > + to_settle_delay(sensor->pll->extclk_rate), &ret);
> > +
> > + ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> > + ARRAY_SIZE(imx111_global_init), NULL);
>
> You are overwriting the previous errors.
>
> please use ret |=
>
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to initialize the sensor\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> ---
>
> > +static int imx111_set_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct imx111 *sensor = sd_to_imx111(sd);
> > + struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > + struct v4l2_mbus_framefmt *fmt;
> > + const struct imx111_mode *mode;
> > +
> > + mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> > + width, height,
> > + mbus_fmt->width, mbus_fmt->height);
> > +
> > + fmt = v4l2_subdev_state_get_format(state, format->pad);
> > +
> > + fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> > + fmt->width = mode->width;
> > + fmt->height = mode->height;
> > + fmt->colorspace = V4L2_COLORSPACE_RAW;
> > +
> > + *mbus_fmt = *fmt;
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + sensor->cur_mode = mode;
> > + sensor->data_depth = imx111_get_format_bpp(fmt);
> > + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> > + div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> > +
> > + __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> > + IMX111_VTL_MAX - mode->height, 1,
> > + mode->vtl_def - mode->height);
> > + __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> > +
> > + __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> > + IMX111_HTL_MAX - mode->width, 1,
> > + mode->htl_def - mode->width);
> > + __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
>
> All the above V4L2 operations need to check for errors.
>
> > + }
> > +
> > + return 0;
> > +}
>
> ---
>
> > +static int imx111_identify_module(struct imx111 *sensor)
> > +{
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + u64 value, revision, manufacturer;
> > + int ret;
> > +
> > + ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + if (value != IMX111_CHIP_ID) {
> > + dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> > + return -ENXIO;
> > + }
> > +
> > + cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> > + cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
>
> Instead of NULL, pass ret for the error code, and return ret at the end.
>
> > +
> > + dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> > + value, revision, manufacturer);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx111_clk_init(struct imx111 *sensor)
> > +{
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> > + u64 extclk_rate, system_clk;
> > + unsigned int i;
> > +
> > + extclk_rate = clk_get_rate(sensor->extclk);
> > + if (!extclk_rate)
> > + return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> > +
> > + for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> > + if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> > + sensor->pll = &imx111_pll[i];
> > + break;
> > + }
> > + }
> > + if (!sensor->pll)
> > + return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
>
> Max line length should be 80 columns. This applies everywhere the line
> length exceeds 80 characters.
>
80 character line is not enforced, checkpatch.pl does not complain.
Consider sending patch to checkpatch.pl if you don't like this.
> > +
> > + system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> > +
> > + /*
> > + * Pixel clock or Logic clock is used for internal image processing is
> > + * generated by dividing into 1/10 or 1/8 frequency according to the
> > + * word length of the CSI2 interface. This clock is designating the pixel
> > + * rate and used as the base of integration time, frame rate etc.
> > + */
> > + sensor->pixel_clk_raw = system_clk * ndata_lanes;
> > +
> > + /*
> > + * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> > + * for RAW10 default format.
> > + */
> > + sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> > + 2 * IMX111_DATA_DEPTH_RAW10);
> > +
> > + if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> > + sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> > + return dev_err_probe(dev, -EINVAL,
> > + "Unsupported DT link-frequencies, expected %llu\n",
> > + sensor->default_link_freq);
> > +
> > + return 0;
> > +}
>
> ---
>
> > +static const struct of_device_id imx111_of_match[] = {
> > + { .compatible = "sony,imx111" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx111_of_match);
> > +
> > +static struct i2c_driver imx111_i2c_driver = {
> > + .driver = {
> > + .name = "imx111",
> > + .of_match_table = imx111_of_match,
> > + .pm = &imx111_pm_ops,
> > + },
> > + .probe = imx111_probe,
> > + .remove = imx111_remove,
> > +};
> > +module_i2c_driver(imx111_i2c_driver);
> > +
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> > +MODULE_LICENSE("GPL");
>
> Best Regards,
> Tarang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 14:55 ` Tarang Raval
2025-10-30 15:03 ` Svyatoslav Ryhel
@ 2025-10-30 15:13 ` Svyatoslav Ryhel
2025-10-30 15:50 ` Jacopo Mondi
` (2 more replies)
1 sibling, 3 replies; 13+ messages in thread
From: Svyatoslav Ryhel @ 2025-10-30 15:13 UTC (permalink / raw)
To: Tarang Raval
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sakari Ailus, Hans Verkuil, Hans de Goede,
André Apitzsch, Sylvain Petinot, Benjamin Mugnier,
Dongcheng Yan, Heimir Thor Sverrisson,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
>
> Hi Svyatoslav,
>
> > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > camera sensor using the i2c bus for control and the csi-2 bus for data.
> >
> > The following features are supported:
> > - manual exposure, digital and analog gain control support
> > - pixel rate/link freq control support
> > - supported resolution up to 3280x2464 for single shot capture
> > - supported resolution up to 1920x1080 @ 30fps for video
> > - supported bayer order output SGBRG10 and SGBRG8
> >
> > Camera module seems to be partially compatible with Nokia SMIA but it
> > lacks a few registers required for clock calculations and has different
> > vendor-specific per-mode configurations which makes it incompatible with
> > existing CCS driver.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>
> ---
>
> > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + s64 max;
> > + int ret = 0;
> > +
> > + /* Propagate change of current control to all related controls */
> > + switch (ctrl->id) {
>
> Do we need the switch statement, since only one case is present?
> You can use an 'if' instead.
>
imx219 and imx319 which are recommended references use switch, and it
seems that media maintainters are particularly picky to code style, I
have copied it from there.
> > + case V4L2_CID_VBLANK:
> > + /* Update max exposure while meeting expected vblanking */
> > + max = sensor->cur_mode->height + ctrl->val - 5;
>
> You can define a macro for the value 5 to improve readability.
> Also, make this change in the init_control function.
>
imx219 does not specifies this as a define
> > + __v4l2_ctrl_modify_range(sensor->exposure,
> > + sensor->exposure->minimum,
> > + max, sensor->exposure->step, max);
>
> This may fail; consider adding an error check.
>
imx219 does not return error here too
> > + break;
> > + }
> > +
> > + /*
> > + * Applying V4L2 control value only happens
> > + * when power is up for streaming
> > + */
> > + if (!pm_runtime_get_if_in_use(dev))
> > + return 0;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_ANALOGUE_GAIN:
> > + cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> > + break;
> > + case V4L2_CID_DIGITAL_GAIN:
> > + ret = imx111_update_digital_gain(sensor, ctrl->val);
> > + break;
> > + case V4L2_CID_EXPOSURE:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_HBLANK:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> > + cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> > + sensor->cur_mode->width + ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_VBLANK:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> > + cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> > + sensor->cur_mode->height + ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_HFLIP:
> > + case V4L2_CID_VFLIP:
> > + cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> > + sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN:
> > + cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_RED:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_GREENR:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_BLUE:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + case V4L2_CID_TEST_PATTERN_GREENB:
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + IMX111_GROUP_WRITE_ON, &ret);
> > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > + 0, &ret);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > +
> > + pm_runtime_put(dev);
> > +
> > + return ret;
> > +}
>
> ---
>
> > +static int imx111_init_controls(struct imx111 *sensor)
> > +{
> > + const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + const struct imx111_mode *mode = sensor->cur_mode;
> > + struct v4l2_fwnode_device_properties props;
> > + struct v4l2_subdev *sd = &sensor->sd;
>
> No need for a new variable; there is only one user in the function.
>
This make code reading cleaner, no?
> > + struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> > + s64 pixel_rate_min, pixel_rate_max;
> > + int i, ret;
> > +
> > + ret = v4l2_fwnode_device_parse(dev, &props);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = v4l2_ctrl_handler_init(hdl, 13);
>
> Now there are 15 controls.
>
> No need for explicit error checking; you can omit the error check if you'd like.
>
> > + if (ret)
> > + return ret;
> > +
> > + pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> > + pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> > + sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> > + pixel_rate_min, pixel_rate_max,
> > + 1, div_u64(sensor->pixel_clk_raw,
> > + 2 * sensor->data_depth));
> > +
> > + sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> > + 0, 0, &sensor->default_link_freq);
> > + if (sensor->link_freq)
> > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > + IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> > + IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> > +
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> > + IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> > + IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> > +
> > + sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + if (sensor->hflip)
> > + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +
> > + sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > + if (sensor->vflip)
> > + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +
> > + sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> > + IMX111_VTL_MAX - mode->height, 1,
> > + mode->vtl_def - mode->height);
> > + sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> > + IMX111_HTL_MAX - mode->width, 1,
> > + mode->htl_def - mode->width);
> > +
> > + /*
> > + * The maximum coarse integration time is the frame length in lines
> > + * minus five.
> > + */
> > + sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > + IMX111_INTEGRATION_TIME_MIN,
> > + IMX111_PIXEL_ARRAY_HEIGHT - 5,
> > + IMX111_INTEGRATION_TIME_STEP,
> > + IMX111_PIXEL_ARRAY_HEIGHT - 5);
> > +
> > + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > +
> > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> > + ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> > + test_pattern_menu);
> > + for (i = 0; i < 4; i++) {
> > + /*
> > + * The assumption is that
> > + * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> > + * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
> > + * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> > + */
> > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> > + IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> > + IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> > + /* The "Solid color" pattern is white by default */
> > + }
> > +
> > + if (hdl->error)
> > + return hdl->error;
> > +
> > + sd->ctrl_handler = hdl;
> > +
> > + return 0;
> > +};
>
> ---
>
> > +static int imx111_initialize(struct imx111 *sensor)
> > +{
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + int ret;
>
> ret = 0;
>
cci_write does not state that ret must be initiated.
> > +
> > + /* Configure the PLL. */
> > + cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> > + sensor->pll->pre_div, &ret);
> > + cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> > + cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> > + cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> > + to_settle_delay(sensor->pll->extclk_rate), &ret);
> > +
> > + ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> > + ARRAY_SIZE(imx111_global_init), NULL);
>
> You are overwriting the previous errors.
>
> please use ret |=
>
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to initialize the sensor\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> ---
>
> > +static int imx111_set_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct imx111 *sensor = sd_to_imx111(sd);
> > + struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > + struct v4l2_mbus_framefmt *fmt;
> > + const struct imx111_mode *mode;
> > +
> > + mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> > + width, height,
> > + mbus_fmt->width, mbus_fmt->height);
> > +
> > + fmt = v4l2_subdev_state_get_format(state, format->pad);
> > +
> > + fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> > + fmt->width = mode->width;
> > + fmt->height = mode->height;
> > + fmt->colorspace = V4L2_COLORSPACE_RAW;
> > +
> > + *mbus_fmt = *fmt;
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + sensor->cur_mode = mode;
> > + sensor->data_depth = imx111_get_format_bpp(fmt);
> > + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> > + div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> > +
> > + __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> > + IMX111_VTL_MAX - mode->height, 1,
> > + mode->vtl_def - mode->height);
> > + __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> > +
> > + __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> > + IMX111_HTL_MAX - mode->width, 1,
> > + mode->htl_def - mode->width);
> > + __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
>
> All the above V4L2 operations need to check for errors.
>
yet again imx219 and imx319 do not check any of those
> > + }
> > +
> > + return 0;
> > +}
>
> ---
>
> > +static int imx111_identify_module(struct imx111 *sensor)
> > +{
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + u64 value, revision, manufacturer;
> > + int ret;
> > +
> > + ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + if (value != IMX111_CHIP_ID) {
> > + dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> > + return -ENXIO;
> > + }
> > +
> > + cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> > + cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
>
> Instead of NULL, pass ret for the error code, and return ret at the end.
>
> > +
> > + dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> > + value, revision, manufacturer);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx111_clk_init(struct imx111 *sensor)
> > +{
> > + struct device *dev = regmap_get_device(sensor->regmap);
> > + u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> > + u64 extclk_rate, system_clk;
> > + unsigned int i;
> > +
> > + extclk_rate = clk_get_rate(sensor->extclk);
> > + if (!extclk_rate)
> > + return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> > +
> > + for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> > + if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> > + sensor->pll = &imx111_pll[i];
> > + break;
> > + }
> > + }
> > + if (!sensor->pll)
> > + return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
>
> Max line length should be 80 columns. This applies everywhere the line
> length exceeds 80 characters.
>
> > +
> > + system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> > +
> > + /*
> > + * Pixel clock or Logic clock is used for internal image processing is
> > + * generated by dividing into 1/10 or 1/8 frequency according to the
> > + * word length of the CSI2 interface. This clock is designating the pixel
> > + * rate and used as the base of integration time, frame rate etc.
> > + */
> > + sensor->pixel_clk_raw = system_clk * ndata_lanes;
> > +
> > + /*
> > + * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> > + * for RAW10 default format.
> > + */
> > + sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> > + 2 * IMX111_DATA_DEPTH_RAW10);
> > +
> > + if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> > + sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> > + return dev_err_probe(dev, -EINVAL,
> > + "Unsupported DT link-frequencies, expected %llu\n",
> > + sensor->default_link_freq);
> > +
> > + return 0;
> > +}
>
> ---
>
> > +static const struct of_device_id imx111_of_match[] = {
> > + { .compatible = "sony,imx111" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx111_of_match);
> > +
> > +static struct i2c_driver imx111_i2c_driver = {
> > + .driver = {
> > + .name = "imx111",
> > + .of_match_table = imx111_of_match,
> > + .pm = &imx111_pm_ops,
> > + },
> > + .probe = imx111_probe,
> > + .remove = imx111_remove,
> > +};
> > +module_i2c_driver(imx111_i2c_driver);
> > +
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> > +MODULE_LICENSE("GPL");
>
> Best Regards,
> Tarang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 15:13 ` Svyatoslav Ryhel
@ 2025-10-30 15:50 ` Jacopo Mondi
2025-10-30 16:11 ` Svyatoslav Ryhel
2025-10-30 15:57 ` Kieran Bingham
2025-11-03 19:24 ` Sakari Ailus
2 siblings, 1 reply; 13+ messages in thread
From: Jacopo Mondi @ 2025-10-30 15:50 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Tarang Raval, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Hello,
sorry for entering the conversation
On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
> >
> > Hi Svyatoslav,
> >
> > > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > > camera sensor using the i2c bus for control and the csi-2 bus for data.
> > >
> > > The following features are supported:
> > > - manual exposure, digital and analog gain control support
> > > - pixel rate/link freq control support
> > > - supported resolution up to 3280x2464 for single shot capture
> > > - supported resolution up to 1920x1080 @ 30fps for video
> > > - supported bayer order output SGBRG10 and SGBRG8
> > >
> > > Camera module seems to be partially compatible with Nokia SMIA but it
> > > lacks a few registers required for clock calculations and has different
> > > vendor-specific per-mode configurations which makes it incompatible with
> > > existing CCS driver.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >
> > ---
> >
> > > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + s64 max;
> > > + int ret = 0;
> > > +
> > > + /* Propagate change of current control to all related controls */
> > > + switch (ctrl->id) {
> >
> > Do we need the switch statement, since only one case is present?
> > You can use an 'if' instead.
> >
>
> imx219 and imx319 which are recommended references use switch, and it
> seems that media maintainters are particularly picky to code style, I
> have copied it from there.
>
Personally, whenever doing reviews, receiving a reply that ignores the
merit of the comment and simply refers to the existing code base as an
excuse for not caring, it's what put me off the most.
Please respect the time reviewers have invested in looking at your
code by at least considering their comment instead of dismissing them.
In this specific case you could have easily said "I like it more this
way and it's consistent with what other drivers do". The same cannot
be said for other comments that you have decided to ignore.
> > > + case V4L2_CID_VBLANK:
> > > + /* Update max exposure while meeting expected vblanking */
> > > + max = sensor->cur_mode->height + ctrl->val - 5;
> >
> > You can define a macro for the value 5 to improve readability.
> > Also, make this change in the init_control function.
> >
>
> imx219 does not specifies this as a define
>
It doesn't but it should, like 90% of other drivers in mainline do
> > > + __v4l2_ctrl_modify_range(sensor->exposure,
> > > + sensor->exposure->minimum,
> > > + max, sensor->exposure->step, max);
> >
> > This may fail; consider adding an error check.
> >
>
> imx219 does not return error here too
>
so ?
> > > + break;
> > > + }
> > > +
> > > + /*
> > > + * Applying V4L2 control value only happens
> > > + * when power is up for streaming
> > > + */
> > > + if (!pm_runtime_get_if_in_use(dev))
> > > + return 0;
> > > +
> > > + switch (ctrl->id) {
> > > + case V4L2_CID_ANALOGUE_GAIN:
> > > + cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> > > + break;
> > > + case V4L2_CID_DIGITAL_GAIN:
> > > + ret = imx111_update_digital_gain(sensor, ctrl->val);
> > > + break;
> > > + case V4L2_CID_EXPOSURE:
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + IMX111_GROUP_WRITE_ON, &ret);
> > > + cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + 0, &ret);
> > > + break;
> > > + case V4L2_CID_HBLANK:
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + IMX111_GROUP_WRITE_ON, &ret);
> > > + dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> > > + cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> > > + sensor->cur_mode->width + ctrl->val, &ret);
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + 0, &ret);
> > > + break;
> > > + case V4L2_CID_VBLANK:
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + IMX111_GROUP_WRITE_ON, &ret);
> > > + dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> > > + cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> > > + sensor->cur_mode->height + ctrl->val, &ret);
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + 0, &ret);
> > > + break;
> > > + case V4L2_CID_HFLIP:
> > > + case V4L2_CID_VFLIP:
> > > + cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> > > + sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > > + break;
> > > + case V4L2_CID_TEST_PATTERN:
> > > + cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> > > + break;
> > > + case V4L2_CID_TEST_PATTERN_RED:
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + IMX111_GROUP_WRITE_ON, &ret);
> > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + 0, &ret);
> > > + break;
> > > + case V4L2_CID_TEST_PATTERN_GREENR:
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + IMX111_GROUP_WRITE_ON, &ret);
> > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + 0, &ret);
> > > + break;
> > > + case V4L2_CID_TEST_PATTERN_BLUE:
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + IMX111_GROUP_WRITE_ON, &ret);
> > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + 0, &ret);
> > > + break;
> > > + case V4L2_CID_TEST_PATTERN_GREENB:
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + IMX111_GROUP_WRITE_ON, &ret);
> > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > + 0, &ret);
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + }
> > > +
> > > + pm_runtime_put(dev);
> > > +
> > > + return ret;
> > > +}
> >
> > ---
> >
> > > +static int imx111_init_controls(struct imx111 *sensor)
> > > +{
> > > + const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + const struct imx111_mode *mode = sensor->cur_mode;
> > > + struct v4l2_fwnode_device_properties props;
> > > + struct v4l2_subdev *sd = &sensor->sd;
> >
> > No need for a new variable; there is only one user in the function.
> >
>
> This make code reading cleaner, no?
>
> > > + struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> > > + s64 pixel_rate_min, pixel_rate_max;
> > > + int i, ret;
> > > +
> > > + ret = v4l2_fwnode_device_parse(dev, &props);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = v4l2_ctrl_handler_init(hdl, 13);
> >
> > Now there are 15 controls.
> >
> > No need for explicit error checking; you can omit the error check if you'd like.
> >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> > > + pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> > > + sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> > > + pixel_rate_min, pixel_rate_max,
> > > + 1, div_u64(sensor->pixel_clk_raw,
> > > + 2 * sensor->data_depth));
> > > +
> > > + sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> > > + 0, 0, &sensor->default_link_freq);
> > > + if (sensor->link_freq)
> > > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > > + IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> > > + IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> > > +
> > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> > > + IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> > > + IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> > > +
> > > + sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > + if (sensor->hflip)
> > > + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > +
> > > + sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > + if (sensor->vflip)
> > > + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > +
> > > + sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> > > + IMX111_VTL_MAX - mode->height, 1,
> > > + mode->vtl_def - mode->height);
> > > + sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> > > + IMX111_HTL_MAX - mode->width, 1,
> > > + mode->htl_def - mode->width);
> > > +
> > > + /*
> > > + * The maximum coarse integration time is the frame length in lines
> > > + * minus five.
> > > + */
> > > + sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > > + IMX111_INTEGRATION_TIME_MIN,
> > > + IMX111_PIXEL_ARRAY_HEIGHT - 5,
> > > + IMX111_INTEGRATION_TIME_STEP,
> > > + IMX111_PIXEL_ARRAY_HEIGHT - 5);
> > > +
> > > + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > > +
> > > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> > > + ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> > > + test_pattern_menu);
> > > + for (i = 0; i < 4; i++) {
> > > + /*
> > > + * The assumption is that
> > > + * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> > > + * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
> > > + * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> > > + */
> > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> > > + IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> > > + IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> > > + /* The "Solid color" pattern is white by default */
> > > + }
> > > +
> > > + if (hdl->error)
> > > + return hdl->error;
> > > +
> > > + sd->ctrl_handler = hdl;
> > > +
> > > + return 0;
> > > +};
> >
> > ---
> >
> > > +static int imx111_initialize(struct imx111 *sensor)
> > > +{
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + int ret;
> >
> > ret = 0;
> >
>
> cci_write does not state that ret must be initiated.
Could you at least take the time to read the code ?
int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
{
bool little_endian;
unsigned int len;
u8 buf[8];
int ret;
if (err && *err)
return *err;
And by the way, the documentation says:
/**
* cci_write() - Write a value to a single CCI register
*
* @map: Register map to write to
* @reg: Register address to write, use CCI_REG#() macros to encode reg width
* @val: Value to be written
* @err: Optional pointer to store errors, if a previous error is set
* then the write will be skipped
*
* Return: %0 on success or a negative error code on failure.
*/
int cci_write(struct regmap *map, u32 reg, u64 val, int *err);
>
> > > +
> > > + /* Configure the PLL. */
> > > + cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> > > + sensor->pll->pre_div, &ret);
I'm very surprised this doesn't sometimes fail as ret is not
initialized
> > > + cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> > > + cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> > > + cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> > > + to_settle_delay(sensor->pll->extclk_rate), &ret);
> > > +
> > > + ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> > > + ARRAY_SIZE(imx111_global_init), NULL);
> >
> > You are overwriting the previous errors.
> >
> > please use ret |=
or you can pass ret to cci_multi_reg_write() as well
Maybe that's why you don't see errors causes by uninitialized ret ?
> >
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to initialize the sensor\n");
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > ---
> >
> > > +static int imx111_set_format(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_state *state,
> > > + struct v4l2_subdev_format *format)
> > > +{
> > > + struct imx111 *sensor = sd_to_imx111(sd);
> > > + struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > > + struct v4l2_mbus_framefmt *fmt;
> > > + const struct imx111_mode *mode;
> > > +
> > > + mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> > > + width, height,
> > > + mbus_fmt->width, mbus_fmt->height);
> > > +
> > > + fmt = v4l2_subdev_state_get_format(state, format->pad);
> > > +
> > > + fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> > > + fmt->width = mode->width;
> > > + fmt->height = mode->height;
> > > + fmt->colorspace = V4L2_COLORSPACE_RAW;
> > > +
> > > + *mbus_fmt = *fmt;
> > > +
> > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > + sensor->cur_mode = mode;
> > > + sensor->data_depth = imx111_get_format_bpp(fmt);
> > > + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> > > + div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> > > +
> > > + __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> > > + IMX111_VTL_MAX - mode->height, 1,
> > > + mode->vtl_def - mode->height);
> > > + __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> > > +
> > > + __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> > > + IMX111_HTL_MAX - mode->width, 1,
> > > + mode->htl_def - mode->width);
> > > + __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
> >
> > All the above V4L2 operations need to check for errors.
> >
>
> yet again imx219 and imx319 do not check any of those
>
And we recently got an error on imx219 controls update that went
unnoticed because of this and I'm now fixing 40+ drivers because this
has been copied over and over. Want to make them 41 ?
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > ---
> >
> > > +static int imx111_identify_module(struct imx111 *sensor)
> > > +{
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + u64 value, revision, manufacturer;
> > > + int ret;
> > > +
> > > + ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (value != IMX111_CHIP_ID) {
> > > + dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> > > + return -ENXIO;
> > > + }
> > > +
> > > + cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> > > + cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
> >
> > Instead of NULL, pass ret for the error code, and return ret at the end.
> >
> > > +
> > > + dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> > > + value, revision, manufacturer);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int imx111_clk_init(struct imx111 *sensor)
> > > +{
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> > > + u64 extclk_rate, system_clk;
> > > + unsigned int i;
> > > +
> > > + extclk_rate = clk_get_rate(sensor->extclk);
> > > + if (!extclk_rate)
> > > + return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> > > + if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> > > + sensor->pll = &imx111_pll[i];
> > > + break;
> > > + }
> > > + }
> > > + if (!sensor->pll)
> > > + return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
> >
> > Max line length should be 80 columns. This applies everywhere the line
> > length exceeds 80 characters.
In response to your reply in a separate email:
https://www.kernel.org/doc/html/latest/driver-api/media/maintainer-entry-profile.html#coding-style-addendum
Media development uses checkpatch.pl on strict mode to verify the code style, e.g.:
$ ./scripts/checkpatch.pl --strict --max-line-length=80
I don't like being that strict too. Feel free to send a patch, you'll
have my ack
> >
> > > +
> > > + system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> > > +
> > > + /*
> > > + * Pixel clock or Logic clock is used for internal image processing is
> > > + * generated by dividing into 1/10 or 1/8 frequency according to the
> > > + * word length of the CSI2 interface. This clock is designating the pixel
> > > + * rate and used as the base of integration time, frame rate etc.
> > > + */
> > > + sensor->pixel_clk_raw = system_clk * ndata_lanes;
> > > +
> > > + /*
> > > + * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> > > + * for RAW10 default format.
> > > + */
> > > + sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> > > + 2 * IMX111_DATA_DEPTH_RAW10);
> > > +
> > > + if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> > > + sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> > > + return dev_err_probe(dev, -EINVAL,
> > > + "Unsupported DT link-frequencies, expected %llu\n",
> > > + sensor->default_link_freq);
> > > +
> > > + return 0;
> > > +}
> >
> > ---
> >
> > > +static const struct of_device_id imx111_of_match[] = {
> > > + { .compatible = "sony,imx111" },
> > > + { /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, imx111_of_match);
> > > +
> > > +static struct i2c_driver imx111_i2c_driver = {
> > > + .driver = {
> > > + .name = "imx111",
> > > + .of_match_table = imx111_of_match,
> > > + .pm = &imx111_pm_ops,
> > > + },
> > > + .probe = imx111_probe,
> > > + .remove = imx111_remove,
> > > +};
> > > +module_i2c_driver(imx111_i2c_driver);
> > > +
> > > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > > +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> > > +MODULE_LICENSE("GPL");
> >
> > Best Regards,
> > Tarang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 15:13 ` Svyatoslav Ryhel
2025-10-30 15:50 ` Jacopo Mondi
@ 2025-10-30 15:57 ` Kieran Bingham
2025-11-03 19:24 ` Sakari Ailus
2 siblings, 0 replies; 13+ messages in thread
From: Kieran Bingham @ 2025-10-30 15:57 UTC (permalink / raw)
To: Svyatoslav Ryhel, Tarang Raval
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sakari Ailus, Hans Verkuil, Hans de Goede,
André Apitzsch, Sylvain Petinot, Benjamin Mugnier,
Dongcheng Yan, Heimir Thor Sverrisson, linux-media
Quoting Svyatoslav Ryhel (2025-10-30 15:13:31)
...
> >
> > > +static int imx111_initialize(struct imx111 *sensor)
> > > +{
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + int ret;
> >
> > ret = 0;
> >
>
> cci_write does not state that ret must be initiated.
>
> > > +
> > > + /* Configure the PLL. */
> > > + cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> > > + sensor->pll->pre_div, &ret);
You definitely need to initialise ret = 0 or this line above will
probably fail because cci_write uses 'ret' to decide to do anything or
not.
- https://docs.kernel.org/driver-api/media/v4l2-cci.html
===============================================================================
int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
Write a value to a single CCI register
Parameters
struct regmap *map
Register map to write to
u32 reg
Register address to write, use CCI_REG#() macros to encode reg width
u64 val
Value to be written
int *err
Optional pointer to store errors, if a previous error is set then
the write will be skipped
Return
0 on success or a negative error code on failure.
===============================================================================
So it should be initialised in a good state *or* you use NULL on
this first call and assign it - but init to ret = 0 is cleaner here I
think.
> > > + cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> > > + cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> > > + cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> > > + to_settle_delay(sensor->pll->extclk_rate), &ret);
> > > +
> > > + ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> > > + ARRAY_SIZE(imx111_global_init), NULL);
> >
> > You are overwriting the previous errors.
> >
> > please use ret |=
No - just continue with the same pattern:
cci_multi_reg_write(sensor->regmap, imx111_global_init,
ARRAY_SIZE(imx111_global_init), &ret);
> >
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to initialize the sensor\n");
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 15:50 ` Jacopo Mondi
@ 2025-10-30 16:11 ` Svyatoslav Ryhel
2025-10-30 16:37 ` Jacopo Mondi
0 siblings, 1 reply; 13+ messages in thread
From: Svyatoslav Ryhel @ 2025-10-30 16:11 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Tarang Raval, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
чт, 30 жовт. 2025 р. о 17:50 Jacopo Mondi <jacopo.mondi@ideasonboard.com> пише:
>
> Hello,
> sorry for entering the conversation
>
> On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> > чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
> > >
> > > Hi Svyatoslav,
> > >
> > > > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > > > camera sensor using the i2c bus for control and the csi-2 bus for data.
> > > >
> > > > The following features are supported:
> > > > - manual exposure, digital and analog gain control support
> > > > - pixel rate/link freq control support
> > > > - supported resolution up to 3280x2464 for single shot capture
> > > > - supported resolution up to 1920x1080 @ 30fps for video
> > > > - supported bayer order output SGBRG10 and SGBRG8
> > > >
> > > > Camera module seems to be partially compatible with Nokia SMIA but it
> > > > lacks a few registers required for clock calculations and has different
> > > > vendor-specific per-mode configurations which makes it incompatible with
> > > > existing CCS driver.
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > >
> > > ---
> > >
> > > > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > +{
> > > > + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > + s64 max;
> > > > + int ret = 0;
> > > > +
> > > > + /* Propagate change of current control to all related controls */
> > > > + switch (ctrl->id) {
> > >
> > > Do we need the switch statement, since only one case is present?
> > > You can use an 'if' instead.
> > >
> >
> > imx219 and imx319 which are recommended references use switch, and it
> > seems that media maintainters are particularly picky to code style, I
> > have copied it from there.
> >
>
> Personally, whenever doing reviews, receiving a reply that ignores the
> merit of the comment and simply refers to the existing code base as an
> excuse for not caring, it's what put me off the most.
>
> Please respect the time reviewers have invested in looking at your
> code by at least considering their comment instead of dismissing them.
> In this specific case you could have easily said "I like it more this
> way and it's consistent with what other drivers do". The same cannot
> be said for other comments that you have decided to ignore.
>
I did not state that I will ignore all the comments. Contrary, I will
be applying most/all of them. My point is that Linux kernel
documentation, which Sakari Ailus (not an offence or blaming) pointed
in v2 clearly pointed to imx219 and imx319 as an valid reference
drivers, while they seem to be outdated. So by using those as
reference I wasted both my time and maintainers time. You say "Please
respect the time reviewers" but no one respects my time.
>
> > > > + case V4L2_CID_VBLANK:
> > > > + /* Update max exposure while meeting expected vblanking */
> > > > + max = sensor->cur_mode->height + ctrl->val - 5;
> > >
> > > You can define a macro for the value 5 to improve readability.
> > > Also, make this change in the init_control function.
> > >
> >
> > imx219 does not specifies this as a define
> >
>
> It doesn't but it should, like 90% of other drivers in mainline do
>
>
> > > > + __v4l2_ctrl_modify_range(sensor->exposure,
> > > > + sensor->exposure->minimum,
> > > > + max, sensor->exposure->step, max);
> > >
> > > This may fail; consider adding an error check.
> > >
> >
> > imx219 does not return error here too
> >
>
> so ?
>
> > > > + break;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Applying V4L2 control value only happens
> > > > + * when power is up for streaming
> > > > + */
> > > > + if (!pm_runtime_get_if_in_use(dev))
> > > > + return 0;
> > > > +
> > > > + switch (ctrl->id) {
> > > > + case V4L2_CID_ANALOGUE_GAIN:
> > > > + cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> > > > + break;
> > > > + case V4L2_CID_DIGITAL_GAIN:
> > > > + ret = imx111_update_digital_gain(sensor, ctrl->val);
> > > > + break;
> > > > + case V4L2_CID_EXPOSURE:
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > + cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + 0, &ret);
> > > > + break;
> > > > + case V4L2_CID_HBLANK:
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > + dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> > > > + cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> > > > + sensor->cur_mode->width + ctrl->val, &ret);
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + 0, &ret);
> > > > + break;
> > > > + case V4L2_CID_VBLANK:
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > + dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> > > > + cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> > > > + sensor->cur_mode->height + ctrl->val, &ret);
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + 0, &ret);
> > > > + break;
> > > > + case V4L2_CID_HFLIP:
> > > > + case V4L2_CID_VFLIP:
> > > > + cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> > > > + sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > > > + break;
> > > > + case V4L2_CID_TEST_PATTERN:
> > > > + cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> > > > + break;
> > > > + case V4L2_CID_TEST_PATTERN_RED:
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + 0, &ret);
> > > > + break;
> > > > + case V4L2_CID_TEST_PATTERN_GREENR:
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + 0, &ret);
> > > > + break;
> > > > + case V4L2_CID_TEST_PATTERN_BLUE:
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + 0, &ret);
> > > > + break;
> > > > + case V4L2_CID_TEST_PATTERN_GREENB:
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > + 0, &ret);
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + }
> > > > +
> > > > + pm_runtime_put(dev);
> > > > +
> > > > + return ret;
> > > > +}
> > >
> > > ---
> > >
> > > > +static int imx111_init_controls(struct imx111 *sensor)
> > > > +{
> > > > + const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > + const struct imx111_mode *mode = sensor->cur_mode;
> > > > + struct v4l2_fwnode_device_properties props;
> > > > + struct v4l2_subdev *sd = &sensor->sd;
> > >
> > > No need for a new variable; there is only one user in the function.
> > >
> >
> > This make code reading cleaner, no?
> >
> > > > + struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> > > > + s64 pixel_rate_min, pixel_rate_max;
> > > > + int i, ret;
> > > > +
> > > > + ret = v4l2_fwnode_device_parse(dev, &props);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = v4l2_ctrl_handler_init(hdl, 13);
> > >
> > > Now there are 15 controls.
> > >
> > > No need for explicit error checking; you can omit the error check if you'd like.
> > >
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> > > > + pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> > > > + sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> > > > + pixel_rate_min, pixel_rate_max,
> > > > + 1, div_u64(sensor->pixel_clk_raw,
> > > > + 2 * sensor->data_depth));
> > > > +
> > > > + sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> > > > + 0, 0, &sensor->default_link_freq);
> > > > + if (sensor->link_freq)
> > > > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > +
> > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > > > + IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> > > > + IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> > > > +
> > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> > > > + IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> > > > + IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> > > > +
> > > > + sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > > + if (sensor->hflip)
> > > > + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > +
> > > > + sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > + if (sensor->vflip)
> > > > + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > +
> > > > + sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> > > > + IMX111_VTL_MAX - mode->height, 1,
> > > > + mode->vtl_def - mode->height);
> > > > + sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> > > > + IMX111_HTL_MAX - mode->width, 1,
> > > > + mode->htl_def - mode->width);
> > > > +
> > > > + /*
> > > > + * The maximum coarse integration time is the frame length in lines
> > > > + * minus five.
> > > > + */
> > > > + sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > > > + IMX111_INTEGRATION_TIME_MIN,
> > > > + IMX111_PIXEL_ARRAY_HEIGHT - 5,
> > > > + IMX111_INTEGRATION_TIME_STEP,
> > > > + IMX111_PIXEL_ARRAY_HEIGHT - 5);
> > > > +
> > > > + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > > > +
> > > > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> > > > + ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> > > > + test_pattern_menu);
> > > > + for (i = 0; i < 4; i++) {
> > > > + /*
> > > > + * The assumption is that
> > > > + * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> > > > + * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
> > > > + * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> > > > + */
> > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> > > > + IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> > > > + IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> > > > + /* The "Solid color" pattern is white by default */
> > > > + }
> > > > +
> > > > + if (hdl->error)
> > > > + return hdl->error;
> > > > +
> > > > + sd->ctrl_handler = hdl;
> > > > +
> > > > + return 0;
> > > > +};
> > >
> > > ---
> > >
> > > > +static int imx111_initialize(struct imx111 *sensor)
> > > > +{
> > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > + int ret;
> > >
> > > ret = 0;
> > >
> >
> > cci_write does not state that ret must be initiated.
>
> Could you at least take the time to read the code ?
>
> int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
> {
> bool little_endian;
> unsigned int len;
> u8 buf[8];
> int ret;
>
> if (err && *err)
> return *err;
>
> And by the way, the documentation says:
>
> /**
> * cci_write() - Write a value to a single CCI register
> *
> * @map: Register map to write to
> * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> * @val: Value to be written
> * @err: Optional pointer to store errors, if a previous error is set
> * then the write will be skipped
> *
> * Return: %0 on success or a negative error code on failure.
> */
> int cci_write(struct regmap *map, u32 reg, u64 val, int *err);
>
>
> >
> > > > +
> > > > + /* Configure the PLL. */
> > > > + cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> > > > + sensor->pll->pre_div, &ret);
>
> I'm very surprised this doesn't sometimes fail as ret is not
> initialized
>
> > > > + cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> > > > + cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> > > > + cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> > > > + to_settle_delay(sensor->pll->extclk_rate), &ret);
> > > > +
> > > > + ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> > > > + ARRAY_SIZE(imx111_global_init), NULL);
> > >
> > > You are overwriting the previous errors.
> > >
> > > please use ret |=
>
> or you can pass ret to cci_multi_reg_write() as well
>
> Maybe that's why you don't see errors causes by uninitialized ret ?
>
> > >
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "Failed to initialize the sensor\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > ---
> > >
> > > > +static int imx111_set_format(struct v4l2_subdev *sd,
> > > > + struct v4l2_subdev_state *state,
> > > > + struct v4l2_subdev_format *format)
> > > > +{
> > > > + struct imx111 *sensor = sd_to_imx111(sd);
> > > > + struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > > > + struct v4l2_mbus_framefmt *fmt;
> > > > + const struct imx111_mode *mode;
> > > > +
> > > > + mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> > > > + width, height,
> > > > + mbus_fmt->width, mbus_fmt->height);
> > > > +
> > > > + fmt = v4l2_subdev_state_get_format(state, format->pad);
> > > > +
> > > > + fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> > > > + fmt->width = mode->width;
> > > > + fmt->height = mode->height;
> > > > + fmt->colorspace = V4L2_COLORSPACE_RAW;
> > > > +
> > > > + *mbus_fmt = *fmt;
> > > > +
> > > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > + sensor->cur_mode = mode;
> > > > + sensor->data_depth = imx111_get_format_bpp(fmt);
> > > > + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> > > > + div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> > > > +
> > > > + __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> > > > + IMX111_VTL_MAX - mode->height, 1,
> > > > + mode->vtl_def - mode->height);
> > > > + __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> > > > +
> > > > + __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> > > > + IMX111_HTL_MAX - mode->width, 1,
> > > > + mode->htl_def - mode->width);
> > > > + __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
> > >
> > > All the above V4L2 operations need to check for errors.
> > >
> >
> > yet again imx219 and imx319 do not check any of those
> >
>
> And we recently got an error on imx219 controls update that went
> unnoticed because of this and I'm now fixing 40+ drivers because this
> has been copied over and over. Want to make them 41 ?
>
There will pop 41 - 42 - 43 if documentation continues pointing to
outdated driver as a reference.
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > ---
> > >
> > > > +static int imx111_identify_module(struct imx111 *sensor)
> > > > +{
> > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > + u64 value, revision, manufacturer;
> > > > + int ret;
> > > > +
> > > > + ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + if (value != IMX111_CHIP_ID) {
> > > > + dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> > > > + return -ENXIO;
> > > > + }
> > > > +
> > > > + cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> > > > + cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
> > >
> > > Instead of NULL, pass ret for the error code, and return ret at the end.
> > >
> > > > +
> > > > + dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> > > > + value, revision, manufacturer);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int imx111_clk_init(struct imx111 *sensor)
> > > > +{
> > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > + u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> > > > + u64 extclk_rate, system_clk;
> > > > + unsigned int i;
> > > > +
> > > > + extclk_rate = clk_get_rate(sensor->extclk);
> > > > + if (!extclk_rate)
> > > > + return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> > > > + if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> > > > + sensor->pll = &imx111_pll[i];
> > > > + break;
> > > > + }
> > > > + }
> > > > + if (!sensor->pll)
> > > > + return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
> > >
> > > Max line length should be 80 columns. This applies everywhere the line
> > > length exceeds 80 characters.
>
> In response to your reply in a separate email:
>
> https://www.kernel.org/doc/html/latest/driver-api/media/maintainer-entry-profile.html#coding-style-addendum
> Media development uses checkpatch.pl on strict mode to verify the code style, e.g.:
> $ ./scripts/checkpatch.pl --strict --max-line-length=80
>
> I don't like being that strict too. Feel free to send a patch, you'll
> have my ack
>
Oh God, I would like to but I don't want to touch this more than I
need to. I have not faced such restrictiveness in any other kernel
subsystem so far.
"People with restrictive hardware shouldn't make it more inconvenient
for people who have better resources. Yes, we'll accommodate things to
within reasonable limits. But no, 80-column terminals in 2020 isn't
"reasonable" any more as far as I'm concerned. People commonly used
132-column terminals even back in the 80's, for chrissake, don't try
to make 80 columns some immovable standard."
> > >
> > > > +
> > > > + system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> > > > +
> > > > + /*
> > > > + * Pixel clock or Logic clock is used for internal image processing is
> > > > + * generated by dividing into 1/10 or 1/8 frequency according to the
> > > > + * word length of the CSI2 interface. This clock is designating the pixel
> > > > + * rate and used as the base of integration time, frame rate etc.
> > > > + */
> > > > + sensor->pixel_clk_raw = system_clk * ndata_lanes;
> > > > +
> > > > + /*
> > > > + * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> > > > + * for RAW10 default format.
> > > > + */
> > > > + sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> > > > + 2 * IMX111_DATA_DEPTH_RAW10);
> > > > +
> > > > + if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> > > > + sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> > > > + return dev_err_probe(dev, -EINVAL,
> > > > + "Unsupported DT link-frequencies, expected %llu\n",
> > > > + sensor->default_link_freq);
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > ---
> > >
> > > > +static const struct of_device_id imx111_of_match[] = {
> > > > + { .compatible = "sony,imx111" },
> > > > + { /* sentinel */ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx111_of_match);
> > > > +
> > > > +static struct i2c_driver imx111_i2c_driver = {
> > > > + .driver = {
> > > > + .name = "imx111",
> > > > + .of_match_table = imx111_of_match,
> > > > + .pm = &imx111_pm_ops,
> > > > + },
> > > > + .probe = imx111_probe,
> > > > + .remove = imx111_remove,
> > > > +};
> > > > +module_i2c_driver(imx111_i2c_driver);
> > > > +
> > > > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > > > +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> > > > +MODULE_LICENSE("GPL");
> > >
> > > Best Regards,
> > > Tarang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 16:11 ` Svyatoslav Ryhel
@ 2025-10-30 16:37 ` Jacopo Mondi
2025-10-31 10:36 ` Tarang Raval
0 siblings, 1 reply; 13+ messages in thread
From: Jacopo Mondi @ 2025-10-30 16:37 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Jacopo Mondi, Tarang Raval, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Svyatoslav
On Thu, Oct 30, 2025 at 06:11:42PM +0200, Svyatoslav Ryhel wrote:
> чт, 30 жовт. 2025 р. о 17:50 Jacopo Mondi <jacopo.mondi@ideasonboard.com> пише:
> >
> > Hello,
> > sorry for entering the conversation
> >
> > On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> > > чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
> > > >
> > > > Hi Svyatoslav,
> > > >
> > > > > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > > > > camera sensor using the i2c bus for control and the csi-2 bus for data.
> > > > >
> > > > > The following features are supported:
> > > > > - manual exposure, digital and analog gain control support
> > > > > - pixel rate/link freq control support
> > > > > - supported resolution up to 3280x2464 for single shot capture
> > > > > - supported resolution up to 1920x1080 @ 30fps for video
> > > > > - supported bayer order output SGBRG10 and SGBRG8
> > > > >
> > > > > Camera module seems to be partially compatible with Nokia SMIA but it
> > > > > lacks a few registers required for clock calculations and has different
> > > > > vendor-specific per-mode configurations which makes it incompatible with
> > > > > existing CCS driver.
> > > > >
> > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > +{
> > > > > + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > + s64 max;
> > > > > + int ret = 0;
> > > > > +
> > > > > + /* Propagate change of current control to all related controls */
> > > > > + switch (ctrl->id) {
> > > >
> > > > Do we need the switch statement, since only one case is present?
> > > > You can use an 'if' instead.
> > > >
> > >
> > > imx219 and imx319 which are recommended references use switch, and it
> > > seems that media maintainters are particularly picky to code style, I
> > > have copied it from there.
> > >
> >
> > Personally, whenever doing reviews, receiving a reply that ignores the
> > merit of the comment and simply refers to the existing code base as an
> > excuse for not caring, it's what put me off the most.
> >
> > Please respect the time reviewers have invested in looking at your
> > code by at least considering their comment instead of dismissing them.
> > In this specific case you could have easily said "I like it more this
> > way and it's consistent with what other drivers do". The same cannot
> > be said for other comments that you have decided to ignore.
> >
>
> I did not state that I will ignore all the comments. Contrary, I will
> be applying most/all of them. My point is that Linux kernel
> documentation, which Sakari Ailus (not an offence or blaming) pointed
> in v2 clearly pointed to imx219 and imx319 as an valid reference
I don't think imx219 is outdated, it's actually one of the most
developed one and is a very good reference. Simply, it's not perfect,
and we keep finding bugs that you could avoid from the very beginning
if you take the time to look into comments you receive.
> drivers, while they seem to be outdated. So by using those as
> reference I wasted both my time and maintainers time. You say "Please
> respect the time reviewers" but no one respects my time.
>
I'm sorry if you feel disrispected but I think you have been pointed
to the best reference we have that simply happens not to be perfect ?
And you might ask "if they got a pass for broken code, why didn't I
get one" ? This is not what's happening though, nobody got a pass,
simply changing code that is already mainline takes more effort and
motivation and if we can avoid bugs from the very beginning we should
do so (otherwise someone will have to fix it later on).
> >
> > > > > + case V4L2_CID_VBLANK:
> > > > > + /* Update max exposure while meeting expected vblanking */
> > > > > + max = sensor->cur_mode->height + ctrl->val - 5;
> > > >
> > > > You can define a macro for the value 5 to improve readability.
> > > > Also, make this change in the init_control function.
> > > >
> > >
> > > imx219 does not specifies this as a define
> > >
> >
> > It doesn't but it should, like 90% of other drivers in mainline do
> >
> >
> > > > > + __v4l2_ctrl_modify_range(sensor->exposure,
> > > > > + sensor->exposure->minimum,
> > > > > + max, sensor->exposure->step, max);
> > > >
> > > > This may fail; consider adding an error check.
> > > >
> > >
> > > imx219 does not return error here too
> > >
> >
> > so ?
> >
> > > > > + break;
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > + * Applying V4L2 control value only happens
> > > > > + * when power is up for streaming
> > > > > + */
> > > > > + if (!pm_runtime_get_if_in_use(dev))
> > > > > + return 0;
> > > > > +
> > > > > + switch (ctrl->id) {
> > > > > + case V4L2_CID_ANALOGUE_GAIN:
> > > > > + cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_DIGITAL_GAIN:
> > > > > + ret = imx111_update_digital_gain(sensor, ctrl->val);
> > > > > + break;
> > > > > + case V4L2_CID_EXPOSURE:
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > + cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + 0, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_HBLANK:
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > + dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> > > > > + cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> > > > > + sensor->cur_mode->width + ctrl->val, &ret);
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + 0, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_VBLANK:
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > + dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> > > > > + cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> > > > > + sensor->cur_mode->height + ctrl->val, &ret);
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + 0, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_HFLIP:
> > > > > + case V4L2_CID_VFLIP:
> > > > > + cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> > > > > + sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_TEST_PATTERN:
> > > > > + cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_TEST_PATTERN_RED:
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + 0, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_TEST_PATTERN_GREENR:
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + 0, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_TEST_PATTERN_BLUE:
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + 0, &ret);
> > > > > + break;
> > > > > + case V4L2_CID_TEST_PATTERN_GREENB:
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > + 0, &ret);
> > > > > + break;
> > > > > + default:
> > > > > + ret = -EINVAL;
> > > > > + }
> > > > > +
> > > > > + pm_runtime_put(dev);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_init_controls(struct imx111 *sensor)
> > > > > +{
> > > > > + const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > + const struct imx111_mode *mode = sensor->cur_mode;
> > > > > + struct v4l2_fwnode_device_properties props;
> > > > > + struct v4l2_subdev *sd = &sensor->sd;
> > > >
> > > > No need for a new variable; there is only one user in the function.
> > > >
> > >
> > > This make code reading cleaner, no?
> > >
> > > > > + struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> > > > > + s64 pixel_rate_min, pixel_rate_max;
> > > > > + int i, ret;
> > > > > +
> > > > > + ret = v4l2_fwnode_device_parse(dev, &props);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + ret = v4l2_ctrl_handler_init(hdl, 13);
> > > >
> > > > Now there are 15 controls.
> > > >
> > > > No need for explicit error checking; you can omit the error check if you'd like.
> > > >
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> > > > > + pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> > > > > + sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> > > > > + pixel_rate_min, pixel_rate_max,
> > > > > + 1, div_u64(sensor->pixel_clk_raw,
> > > > > + 2 * sensor->data_depth));
> > > > > +
> > > > > + sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> > > > > + 0, 0, &sensor->default_link_freq);
> > > > > + if (sensor->link_freq)
> > > > > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > +
> > > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > > > > + IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> > > > > + IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> > > > > +
> > > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> > > > > + IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> > > > > + IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> > > > > +
> > > > > + sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > > > + if (sensor->hflip)
> > > > > + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > > +
> > > > > + sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > > + if (sensor->vflip)
> > > > > + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > > +
> > > > > + sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> > > > > + IMX111_VTL_MAX - mode->height, 1,
> > > > > + mode->vtl_def - mode->height);
> > > > > + sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> > > > > + IMX111_HTL_MAX - mode->width, 1,
> > > > > + mode->htl_def - mode->width);
> > > > > +
> > > > > + /*
> > > > > + * The maximum coarse integration time is the frame length in lines
> > > > > + * minus five.
> > > > > + */
> > > > > + sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > > > > + IMX111_INTEGRATION_TIME_MIN,
> > > > > + IMX111_PIXEL_ARRAY_HEIGHT - 5,
> > > > > + IMX111_INTEGRATION_TIME_STEP,
> > > > > + IMX111_PIXEL_ARRAY_HEIGHT - 5);
> > > > > +
> > > > > + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > > > > +
> > > > > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> > > > > + ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> > > > > + test_pattern_menu);
> > > > > + for (i = 0; i < 4; i++) {
> > > > > + /*
> > > > > + * The assumption is that
> > > > > + * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> > > > > + * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
> > > > > + * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> > > > > + */
> > > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> > > > > + IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> > > > > + IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> > > > > + /* The "Solid color" pattern is white by default */
> > > > > + }
> > > > > +
> > > > > + if (hdl->error)
> > > > > + return hdl->error;
> > > > > +
> > > > > + sd->ctrl_handler = hdl;
> > > > > +
> > > > > + return 0;
> > > > > +};
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_initialize(struct imx111 *sensor)
> > > > > +{
> > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > + int ret;
> > > >
> > > > ret = 0;
> > > >
> > >
> > > cci_write does not state that ret must be initiated.
> >
> > Could you at least take the time to read the code ?
> >
To be honest, this is what had me jump into this conversation as an
indication you had not really put attention to the comments you
received.
> > int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
> > {
> > bool little_endian;
> > unsigned int len;
> > u8 buf[8];
> > int ret;
> >
> > if (err && *err)
> > return *err;
> >
> > And by the way, the documentation says:
> >
> > /**
> > * cci_write() - Write a value to a single CCI register
> > *
> > * @map: Register map to write to
> > * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> > * @val: Value to be written
> > * @err: Optional pointer to store errors, if a previous error is set
> > * then the write will be skipped
> > *
> > * Return: %0 on success or a negative error code on failure.
> > */
> > int cci_write(struct regmap *map, u32 reg, u64 val, int *err);
> >
> >
> > >
> > > > > +
> > > > > + /* Configure the PLL. */
> > > > > + cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> > > > > + sensor->pll->pre_div, &ret);
> >
> > I'm very surprised this doesn't sometimes fail as ret is not
> > initialized
> >
> > > > > + cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> > > > > + cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> > > > > + cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> > > > > + to_settle_delay(sensor->pll->extclk_rate), &ret);
> > > > > +
> > > > > + ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> > > > > + ARRAY_SIZE(imx111_global_init), NULL);
> > > >
> > > > You are overwriting the previous errors.
> > > >
> > > > please use ret |=
> >
> > or you can pass ret to cci_multi_reg_write() as well
> >
> > Maybe that's why you don't see errors causes by uninitialized ret ?
> >
> > > >
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "Failed to initialize the sensor\n");
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_set_format(struct v4l2_subdev *sd,
> > > > > + struct v4l2_subdev_state *state,
> > > > > + struct v4l2_subdev_format *format)
> > > > > +{
> > > > > + struct imx111 *sensor = sd_to_imx111(sd);
> > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > > > > + struct v4l2_mbus_framefmt *fmt;
> > > > > + const struct imx111_mode *mode;
> > > > > +
> > > > > + mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> > > > > + width, height,
> > > > > + mbus_fmt->width, mbus_fmt->height);
> > > > > +
> > > > > + fmt = v4l2_subdev_state_get_format(state, format->pad);
> > > > > +
> > > > > + fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> > > > > + fmt->width = mode->width;
> > > > > + fmt->height = mode->height;
> > > > > + fmt->colorspace = V4L2_COLORSPACE_RAW;
> > > > > +
> > > > > + *mbus_fmt = *fmt;
> > > > > +
> > > > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > > + sensor->cur_mode = mode;
> > > > > + sensor->data_depth = imx111_get_format_bpp(fmt);
> > > > > + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> > > > > + div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> > > > > +
> > > > > + __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> > > > > + IMX111_VTL_MAX - mode->height, 1,
> > > > > + mode->vtl_def - mode->height);
> > > > > + __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> > > > > +
> > > > > + __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> > > > > + IMX111_HTL_MAX - mode->width, 1,
> > > > > + mode->htl_def - mode->width);
> > > > > + __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
> > > >
> > > > All the above V4L2 operations need to check for errors.
> > > >
> > >
> > > yet again imx219 and imx319 do not check any of those
> > >
> >
> > And we recently got an error on imx219 controls update that went
> > unnoticed because of this and I'm now fixing 40+ drivers because this
> > has been copied over and over. Want to make them 41 ?
> >
>
> There will pop 41 - 42 - 43 if documentation continues pointing to
> outdated driver as a reference.
>
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_identify_module(struct imx111 *sensor)
> > > > > +{
> > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > + u64 value, revision, manufacturer;
> > > > > + int ret;
> > > > > +
> > > > > + ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + if (value != IMX111_CHIP_ID) {
> > > > > + dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> > > > > + return -ENXIO;
> > > > > + }
> > > > > +
> > > > > + cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> > > > > + cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
> > > >
> > > > Instead of NULL, pass ret for the error code, and return ret at the end.
> > > >
> > > > > +
> > > > > + dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> > > > > + value, revision, manufacturer);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx111_clk_init(struct imx111 *sensor)
> > > > > +{
> > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > + u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> > > > > + u64 extclk_rate, system_clk;
> > > > > + unsigned int i;
> > > > > +
> > > > > + extclk_rate = clk_get_rate(sensor->extclk);
> > > > > + if (!extclk_rate)
> > > > > + return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> > > > > +
> > > > > + for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> > > > > + if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> > > > > + sensor->pll = &imx111_pll[i];
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > + if (!sensor->pll)
> > > > > + return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
> > > >
> > > > Max line length should be 80 columns. This applies everywhere the line
> > > > length exceeds 80 characters.
> >
> > In response to your reply in a separate email:
> >
> > https://www.kernel.org/doc/html/latest/driver-api/media/maintainer-entry-profile.html#coding-style-addendum
> > Media development uses checkpatch.pl on strict mode to verify the code style, e.g.:
> > $ ./scripts/checkpatch.pl --strict --max-line-length=80
> >
> > I don't like being that strict too. Feel free to send a patch, you'll
> > have my ack
> >
>
> Oh God, I would like to but I don't want to touch this more than I
> need to. I have not faced such restrictiveness in any other kernel
> subsystem so far.
>
I agree we should move over 80 cols, it's a discussion we already had
many times but if rules have to be changed they have to be changed at
the subsystem level, not because you and me dislike them.
I'll ignore the following as it doesn't add anything to the
conversation.
> "People with restrictive hardware shouldn't make it more inconvenient
> for people who have better resources. Yes, we'll accommodate things to
> within reasonable limits. But no, 80-column terminals in 2020 isn't
> "reasonable" any more as far as I'm concerned. People commonly used
> 132-column terminals even back in the 80's, for chrissake, don't try
> to make 80 columns some immovable standard."
>
> > > >
> > > > > +
> > > > > + system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> > > > > +
> > > > > + /*
> > > > > + * Pixel clock or Logic clock is used for internal image processing is
> > > > > + * generated by dividing into 1/10 or 1/8 frequency according to the
> > > > > + * word length of the CSI2 interface. This clock is designating the pixel
> > > > > + * rate and used as the base of integration time, frame rate etc.
> > > > > + */
> > > > > + sensor->pixel_clk_raw = system_clk * ndata_lanes;
> > > > > +
> > > > > + /*
> > > > > + * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> > > > > + * for RAW10 default format.
> > > > > + */
> > > > > + sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> > > > > + 2 * IMX111_DATA_DEPTH_RAW10);
> > > > > +
> > > > > + if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> > > > > + sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> > > > > + return dev_err_probe(dev, -EINVAL,
> > > > > + "Unsupported DT link-frequencies, expected %llu\n",
> > > > > + sensor->default_link_freq);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static const struct of_device_id imx111_of_match[] = {
> > > > > + { .compatible = "sony,imx111" },
> > > > > + { /* sentinel */ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, imx111_of_match);
> > > > > +
> > > > > +static struct i2c_driver imx111_i2c_driver = {
> > > > > + .driver = {
> > > > > + .name = "imx111",
> > > > > + .of_match_table = imx111_of_match,
> > > > > + .pm = &imx111_pm_ops,
> > > > > + },
> > > > > + .probe = imx111_probe,
> > > > > + .remove = imx111_remove,
> > > > > +};
> > > > > +module_i2c_driver(imx111_i2c_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > > > > +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> > > > > +MODULE_LICENSE("GPL");
> > > >
> > > > Best Regards,
> > > > Tarang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: media: i2c: document Sony IMX111 CMOS sensor
2025-10-30 11:57 ` [PATCH v3 1/2] dt-bindings: media: i2c: document Sony IMX111 CMOS sensor Svyatoslav Ryhel
@ 2025-10-30 19:23 ` Conor Dooley
0 siblings, 0 replies; 13+ messages in thread
From: Conor Dooley @ 2025-10-30 19:23 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sakari Ailus, Hans Verkuil, Hans de Goede,
André Apitzsch, Sylvain Petinot, Benjamin Mugnier,
Dongcheng Yan, Heimir Thor Sverrisson, Tarang Raval, linux-media,
devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 335 bytes --]
On Thu, Oct 30, 2025 at 01:57:56PM +0200, Svyatoslav Ryhel wrote:
> Add bindings for Sony IMX111 CMOS Digital Image Sensor found in LG
> Optimus 4X (P880) and Optimus Vu (P895) smartphones.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 16:37 ` Jacopo Mondi
@ 2025-10-31 10:36 ` Tarang Raval
0 siblings, 0 replies; 13+ messages in thread
From: Tarang Raval @ 2025-10-31 10:36 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Jacopo Mondi, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil,
Hans de Goede, André Apitzsch, Sylvain Petinot,
Benjamin Mugnier, Dongcheng Yan, Heimir Thor Sverrisson,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Svyatoslav,
> On Thu, Oct 30, 2025 at 06:11:42PM +0200, Svyatoslav Ryhel wrote:
> > чт, 30 жовт. 2025 р. о 17:50 Jacopo Mondi <jacopo.mondi@ideasonboard.com> пише:
> > >
> > > Hello,
> > > sorry for entering the conversation
> > >
> > > On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> > > > чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
> > > > >
> > > > > Hi Svyatoslav,
> > > > >
> > > > > > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > > > > > camera sensor using the i2c bus for control and the csi-2 bus for data.
> > > > > >
> > > > > > The following features are supported:
> > > > > > - manual exposure, digital and analog gain control support
> > > > > > - pixel rate/link freq control support
> > > > > > - supported resolution up to 3280x2464 for single shot capture
> > > > > > - supported resolution up to 1920x1080 @ 30fps for video
> > > > > > - supported bayer order output SGBRG10 and SGBRG8
> > > > > >
> > > > > > Camera module seems to be partially compatible with Nokia SMIA but it
> > > > > > lacks a few registers required for clock calculations and has different
> > > > > > vendor-specific per-mode configurations which makes it incompatible with
> > > > > > existing CCS driver.
> > > > > >
> > > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > >
> > > > > ---
> > > > >
> > > > > > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > +{
> > > > > > + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > > + s64 max;
> > > > > > + int ret = 0;
> > > > > > +
> > > > > > + /* Propagate change of current control to all related controls */
> > > > > > + switch (ctrl->id) {
> > > > >
> > > > > Do we need the switch statement, since only one case is present?
> > > > > You can use an 'if' instead.
> > > > >
> > > >
> > > > imx219 and imx319 which are recommended references use switch, and it
> > > > seems that media maintainters are particularly picky to code style, I
> > > > have copied it from there.
> > > >
> > >
> > > Personally, whenever doing reviews, receiving a reply that ignores the
> > > merit of the comment and simply refers to the existing code base as an
> > > excuse for not caring, it's what put me off the most.
> > >
> > > Please respect the time reviewers have invested in looking at your
> > > code by at least considering their comment instead of dismissing them.
> > > In this specific case you could have easily said "I like it more this
> > > way and it's consistent with what other drivers do". The same cannot
> > > be said for other comments that you have decided to ignore.
> > >
> >
> > I did not state that I will ignore all the comments. Contrary, I will
> > be applying most/all of them. My point is that Linux kernel
> > documentation, which Sakari Ailus (not an offence or blaming) pointed
> > in v2 clearly pointed to imx219 and imx319 as an valid reference
I believe you can refer to any recently merged camera sensor driver, as it
will likely cover the necessary points.
> I don't think imx219 is outdated, it's actually one of the most
> developed one and is a very good reference. Simply, it's not perfect,
> and we keep finding bugs that you could avoid from the very beginning
> if you take the time to look into comments you receive.
>
> > drivers, while they seem to be outdated. So by using those as
> > reference I wasted both my time and maintainers time. You say "Please
> > respect the time reviewers" but no one respects my time.
>
>
> I'm sorry if you feel disrispected but I think you have been pointed
> to the best reference we have that simply happens not to be perfect ?
>
> And you might ask "if they got a pass for broken code, why didn't I
> get one" ? This is not what's happening though, nobody got a pass,
> simply changing code that is already mainline takes more effort and
> motivation and if we can avoid bugs from the very beginning we should
> do so (otherwise someone will have to fix it later on).
>
> > >
> > > > > > + case V4L2_CID_VBLANK:
> > > > > > + /* Update max exposure while meeting expected vblanking */
> > > > > > + max = sensor->cur_mode->height + ctrl->val - 5;
> > > > >
> > > > > You can define a macro for the value 5 to improve readability.
> > > > > Also, make this change in the init_control function.
> > > > >
> > > >
> > > > imx219 does not specifies this as a define
> > > >
> > >
> > > It doesn't but it should, like 90% of other drivers in mainline do
> > >
> > >
> > > > > > + __v4l2_ctrl_modify_range(sensor->exposure,
> > > > > > + sensor->exposure->minimum,
> > > > > > + max, sensor->exposure->step, max);
> > > > >
> > > > > This may fail; consider adding an error check.
> > > > >
> > > >
> > > > imx219 does not return error here too
> > > >
> > >
> > > so ?
> > >
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + /*
> > > > > > + * Applying V4L2 control value only happens
> > > > > > + * when power is up for streaming
> > > > > > + */
> > > > > > + if (!pm_runtime_get_if_in_use(dev))
> > > > > > + return 0;
> > > > > > +
> > > > > > + switch (ctrl->id) {
> > > > > > + case V4L2_CID_ANALOGUE_GAIN:
> > > > > > + cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_DIGITAL_GAIN:
> > > > > > + ret = imx111_update_digital_gain(sensor, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_EXPOSURE:
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > > + cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + 0, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_HBLANK:
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > > + dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> > > > > > + cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> > > > > > + sensor->cur_mode->width + ctrl->val, &ret);
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + 0, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_VBLANK:
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > > + dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> > > > > > + cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> > > > > > + sensor->cur_mode->height + ctrl->val, &ret);
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + 0, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_HFLIP:
> > > > > > + case V4L2_CID_VFLIP:
> > > > > > + cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> > > > > > + sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_TEST_PATTERN:
> > > > > > + cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_TEST_PATTERN_RED:
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + 0, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_TEST_PATTERN_GREENR:
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + 0, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_TEST_PATTERN_BLUE:
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + 0, &ret);
> > > > > > + break;
> > > > > > + case V4L2_CID_TEST_PATTERN_GREENB:
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + IMX111_GROUP_WRITE_ON, &ret);
> > > > > > + cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> > > > > > + cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > > + 0, &ret);
> > > > > > + break;
> > > > > > + default:
> > > > > > + ret = -EINVAL;
> > > > > > + }
> > > > > > +
> > > > > > + pm_runtime_put(dev);
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > >
> > > > > ---
> > > > >
> > > > > > +static int imx111_init_controls(struct imx111 *sensor)
> > > > > > +{
> > > > > > + const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> > > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > > + const struct imx111_mode *mode = sensor->cur_mode;
> > > > > > + struct v4l2_fwnode_device_properties props;
> > > > > > + struct v4l2_subdev *sd = &sensor->sd;
> > > > >
> > > > > No need for a new variable; there is only one user in the function.
> > > > >
> > > >
> > > > This make code reading cleaner, no?
> > > >
> > > > > > + struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> > > > > > + s64 pixel_rate_min, pixel_rate_max;
> > > > > > + int i, ret;
> > > > > > +
> > > > > > + ret = v4l2_fwnode_device_parse(dev, &props);
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + ret = v4l2_ctrl_handler_init(hdl, 13);
> > > > >
> > > > > Now there are 15 controls.
> > > > >
> > > > > No need for explicit error checking; you can omit the error check if you'd like.
> > > > >
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> > > > > > + pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> > > > > > + sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> > > > > > + pixel_rate_min, pixel_rate_max,
> > > > > > + 1, div_u64(sensor->pixel_clk_raw,
> > > > > > + 2 * sensor->data_depth));
> > > > > > +
> > > > > > + sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> > > > > > + 0, 0, &sensor->default_link_freq);
> > > > > > + if (sensor->link_freq)
> > > > > > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > +
> > > > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > > > > > + IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> > > > > > + IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> > > > > > +
> > > > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> > > > > > + IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> > > > > > + IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> > > > > > +
> > > > > > + sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > > > > + if (sensor->hflip)
> > > > > > + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > > > +
> > > > > > + sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > > > + if (sensor->vflip)
> > > > > > + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > > > +
> > > > > > + sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> > > > > > + IMX111_VTL_MAX - mode->height, 1,
> > > > > > + mode->vtl_def - mode->height);
> > > > > > + sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> > > > > > + IMX111_HTL_MAX - mode->width, 1,
> > > > > > + mode->htl_def - mode->width);
> > > > > > +
> > > > > > + /*
> > > > > > + * The maximum coarse integration time is the frame length in lines
> > > > > > + * minus five.
> > > > > > + */
> > > > > > + sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > > > > > + IMX111_INTEGRATION_TIME_MIN,
> > > > > > + IMX111_PIXEL_ARRAY_HEIGHT - 5,
> > > > > > + IMX111_INTEGRATION_TIME_STEP,
> > > > > > + IMX111_PIXEL_ARRAY_HEIGHT - 5);
> > > > > > +
> > > > > > + v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > > > > > +
> > > > > > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> > > > > > + ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> > > > > > + test_pattern_menu);
> > > > > > + for (i = 0; i < 4; i++) {
> > > > > > + /*
> > > > > > + * The assumption is that
> > > > > > + * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> > > > > > + * V4L2_CID_TEST_PATTERN_BLUE == V4L2_CID_TEST_PATTERN_RED + 2
> > > > > > + * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> > > > > > + */
> > > > > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> > > > > > + IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> > > > > > + IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> > > > > > + /* The "Solid color" pattern is white by default */
> > > > > > + }
> > > > > > +
> > > > > > + if (hdl->error)
> > > > > > + return hdl->error;
> > > > > > +
> > > > > > + sd->ctrl_handler = hdl;
> > > > > > +
> > > > > > + return 0;
> > > > > > +};
> > > > >
> > > > > ---
> > > > >
> > > > > > +static int imx111_initialize(struct imx111 *sensor)
> > > > > > +{
> > > > > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > > > > + int ret;
> > > > >
> > > > > ret = 0;
> > > > >
> > > >
> > > > cci_write does not state that ret must be initiated.
> > >
I think I should have explained the reasoning behind the change
instead of providing a direct modification. I'll make sure to provide more
context next time.
Best Regards,
Tarang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver
2025-10-30 15:13 ` Svyatoslav Ryhel
2025-10-30 15:50 ` Jacopo Mondi
2025-10-30 15:57 ` Kieran Bingham
@ 2025-11-03 19:24 ` Sakari Ailus
2 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2025-11-03 19:24 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Tarang Raval, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, Hans de Goede,
André Apitzsch, Sylvain Petinot, Benjamin Mugnier,
Dongcheng Yan, Heimir Thor Sverrisson,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Svyatoslav,
On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
> >
> > Hi Svyatoslav,
> >
> > > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > > camera sensor using the i2c bus for control and the csi-2 bus for data.
> > >
> > > The following features are supported:
> > > - manual exposure, digital and analog gain control support
> > > - pixel rate/link freq control support
> > > - supported resolution up to 3280x2464 for single shot capture
> > > - supported resolution up to 1920x1080 @ 30fps for video
> > > - supported bayer order output SGBRG10 and SGBRG8
> > >
> > > Camera module seems to be partially compatible with Nokia SMIA but it
> > > lacks a few registers required for clock calculations and has different
> > > vendor-specific per-mode configurations which makes it incompatible with
> > > existing CCS driver.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >
> > ---
> >
> > > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > + struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + s64 max;
> > > + int ret = 0;
> > > +
> > > + /* Propagate change of current control to all related controls */
> > > + switch (ctrl->id) {
> >
> > Do we need the switch statement, since only one case is present?
> > You can use an 'if' instead.
> >
>
> imx219 and imx319 which are recommended references use switch, and it
> seems that media maintainters are particularly picky to code style, I
> have copied it from there.
The documentation lists areas where the mentioned drivers serve as good
examples, it does not say everything in those drivers is done the way it is
best. Common sense indeed should prevail.
The reason why we mention these these drivers as examples is that there are
things that have been notoriously hard to get right (such as runtime PM
usage in sensor drivers).
...
> > > +static int imx111_initialize(struct imx111 *sensor)
> > > +{
> > > + struct device *dev = regmap_get_device(sensor->regmap);
> > > + int ret;
> >
> > ret = 0;
> >
>
> cci_write does not state that ret must be initiated.
It does not explicitly use that wording but the documentation is clear
enough IMO.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-11-03 19:24 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-30 11:57 [PATCH v3 0/2] media: i2c: add Sony IMX111 CMOS camera sensor driver Svyatoslav Ryhel
2025-10-30 11:57 ` [PATCH v3 1/2] dt-bindings: media: i2c: document Sony IMX111 CMOS sensor Svyatoslav Ryhel
2025-10-30 19:23 ` Conor Dooley
2025-10-30 11:57 ` [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver Svyatoslav Ryhel
2025-10-30 14:55 ` Tarang Raval
2025-10-30 15:03 ` Svyatoslav Ryhel
2025-10-30 15:13 ` Svyatoslav Ryhel
2025-10-30 15:50 ` Jacopo Mondi
2025-10-30 16:11 ` Svyatoslav Ryhel
2025-10-30 16:37 ` Jacopo Mondi
2025-10-31 10:36 ` Tarang Raval
2025-10-30 15:57 ` Kieran Bingham
2025-11-03 19:24 ` Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox