[PATCH v6 0/2] media: i2c: Add support for GC08A3 sensor

devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v6 0/2] media: i2c: Add support for GC08A3 sensor
@ 2024-02-27  1:32 Zhi Mao
  2024-02-27  1:32 ` [PATCH v6 1/2] media: dt-bindings: i2c: add GalaxyCore GC08A3 image sensor Zhi Mao
  2024-02-27  1:32 ` [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver Zhi Mao
  0 siblings, 2 replies; 12+ messages in thread
From: Zhi Mao @ 2024-02-27  1:32 UTC (permalink / raw)
  To: mchehab, robh+dt, krzysztof.kozlowski+dt, sakari.ailus
  Cc: laurent.pinchart, shengnan.wang, yaya.chang, 10572168,
	Project_Global_Chrome_Upstream_Group, yunkec, conor+dt,
	matthias.bgg, angelogioacchino.delregno, jacopo.mondi, zhi.mao,
	hverkuil-cisco, heiko, jernej.skrabec, macromorgan, linus.walleij,
	hdegoede, tomi.valkeinen, gerald.loacker, andy.shevchenko,
	bingbu.cao, dan.scally, linux-media, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

This series adds YAML DT binding and V4L2 sub-device driver for Galaxycore's
GC08A3 8-megapixel 10-bit RAW CMOS 1/4" sensor, with an MIPI CSI-2 image data
interface and the I2C control bus.

The driver is implemented with V4L2 framework.
 - Async registered as a V4L2 sub-device.
 - As the first component of camera system including Seninf, ISP pipeline.
 - A media entity that provides one source pad in common.
 - Used in camera features on ChromeOS application.

Also this driver supports following features:
 - manual exposure and analog gain control support
 - vertical blanking control support
 - test pattern support
 - media controller support
 - runtime PM support
 - support resolution: 3264x2448@30fps, 1920x1080@60fps

Previous versions of this patch-set can be found here:
v5: https://lore.kernel.org/linux-media/20240220012540.10607-1-zhi.mao@mediatek.com/
v4: https://lore.kernel.org/linux-media/20240204061538.2105-1-zhi.mao@mediatek.com/
v3: https://lore.kernel.org/linux-media/20240109022715.30278-1-zhi.mao@mediatek.com/
v2: https://lore.kernel.org/linux-media/20231207052016.25954-1-zhi.mao@mediatek.com/
v1: https://lore.kernel.org/linux-media/20231123115104.32094-1-zhi.mao@mediatek.com/

This series is based on linux-next, tag: next-20240226
Changes in v6:
- gc08a3 sensor driver：
-- refine header files in driver code
-- use macro HZ_PER_MHZ/USEC_PER_MSEC for some variable define
-- define reset-pin as active-low in DT,and use logical states: gpiod_set_value_cansleep(reset_gpio, 0) in driver
-- use new PM macros: pm_ptr()

Thanks

Zhi Mao (2):
  media: dt-bindings: i2c: add GalaxyCore GC08A3 image sensor
  media: i2c: Add GC08A3 image sensor driver

 .../bindings/media/i2c/galaxycore,gc08a3.yaml |  112 ++
 drivers/media/i2c/Kconfig                     |   10 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/gc08a3.c                    | 1333 +++++++++++++++++
 4 files changed, 1456 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/galaxycore,gc08a3.yaml
 create mode 100644 drivers/media/i2c/gc08a3.c

-- 
2.25.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v6 1/2] media: dt-bindings: i2c: add GalaxyCore GC08A3 image sensor
  2024-02-27  1:32 [PATCH v6 0/2] media: i2c: Add support for GC08A3 sensor Zhi Mao
@ 2024-02-27  1:32 ` Zhi Mao
  2024-02-27  1:32 ` [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver Zhi Mao
  1 sibling, 0 replies; 12+ messages in thread
From: Zhi Mao @ 2024-02-27  1:32 UTC (permalink / raw)
  To: mchehab, robh+dt, krzysztof.kozlowski+dt, sakari.ailus
  Cc: laurent.pinchart, shengnan.wang, yaya.chang, 10572168,
	Project_Global_Chrome_Upstream_Group, yunkec, conor+dt,
	matthias.bgg, angelogioacchino.delregno, jacopo.mondi, zhi.mao,
	hverkuil-cisco, heiko, jernej.skrabec, macromorgan, linus.walleij,
	hdegoede, tomi.valkeinen, gerald.loacker, andy.shevchenko,
	bingbu.cao, dan.scally, linux-media, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, Krzysztof Kozlowski

Add YAML device tree binding for GC08A3 CMOS image sensor,
and the relevant MAINTAINERS entries.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 .../bindings/media/i2c/galaxycore,gc08a3.yaml | 112 ++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/galaxycore,gc08a3.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc08a3.yaml b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc08a3.yaml
new file mode 100644
index 000000000000..51b8ece09c72
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc08a3.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2023 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/galaxycore,gc08a3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GalaxyCore gc08a3 1/4" 8M Pixel MIPI CSI-2 sensor
+
+maintainers:
+  - Zhi Mao <zhi.mao@mediatek.com>
+
+description:
+  The gc08a3 is a raw image sensor with an MIPI CSI-2 image data
+  interface and CCI (I2C compatible) control bus. The output format
+  is raw Bayer.
+
+properties:
+  compatible:
+    const: galaxycore,gc08a3
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  dovdd-supply: true
+
+  avdd-supply: true
+
+  dvdd-supply: true
+
+  reset-gpios:
+    description: Reference to the GPIO connected to the RESETB pin.
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+    description:
+      Output port node, single endpoint describing the CSI-2 transmitter.
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            oneOf:
+              - items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+              - items:
+                  - const: 1
+                  - const: 2
+
+          link-frequencies: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@31 {
+            compatible = "galaxycore,gc08a3";
+            reg = <0x31>;
+
+            clocks = <&gc08a3_clk>;
+
+            reset-gpios = <&pio 19 GPIO_ACTIVE_LOW>;
+
+            avdd-supply = <&gc08a3_avdd>;
+            dovdd-supply = <&gc08a3_dovdd>;
+            dvdd-supply = <&gc08a3_dvdd>;
+
+            port {
+                sensor_out: endpoint {
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <336000000 207000000>;
+                    remote-endpoint = <&seninf_csi_port_0_in>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-27  1:32 [PATCH v6 0/2] media: i2c: Add support for GC08A3 sensor Zhi Mao
  2024-02-27  1:32 ` [PATCH v6 1/2] media: dt-bindings: i2c: add GalaxyCore GC08A3 image sensor Zhi Mao
@ 2024-02-27  1:32 ` Zhi Mao
  2024-02-27 12:46   ` Andy Shevchenko
  1 sibling, 1 reply; 12+ messages in thread
From: Zhi Mao @ 2024-02-27  1:32 UTC (permalink / raw)
  To: mchehab, robh+dt, krzysztof.kozlowski+dt, sakari.ailus
  Cc: laurent.pinchart, shengnan.wang, yaya.chang, 10572168,
	Project_Global_Chrome_Upstream_Group, yunkec, conor+dt,
	matthias.bgg, angelogioacchino.delregno, jacopo.mondi, zhi.mao,
	hverkuil-cisco, heiko, jernej.skrabec, macromorgan, linus.walleij,
	hdegoede, tomi.valkeinen, gerald.loacker, andy.shevchenko,
	bingbu.cao, dan.scally, linux-media, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek

Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/media/i2c/Kconfig  |   10 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/gc08a3.c | 1333 ++++++++++++++++++++++++++++++++++++
 3 files changed, 1344 insertions(+)
 create mode 100644 drivers/media/i2c/gc08a3.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 56f276b920ab..e4da68835683 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -70,6 +70,16 @@ config VIDEO_GC0308
 	  To compile this driver as a module, choose M here: the
 	  module will be called gc0308.
 
+config VIDEO_GC08A3
+	tristate "GalaxyCore gc08a3 sensor support"
+	select V4L2_CCI_I2C
+	help
+	  This is a Video4Linux2 sensor driver for the GalaxyCore gc08a3
+	  camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gc08a3.
+
 config VIDEO_GC2145
 	select V4L2_CCI_I2C
 	tristate "GalaxyCore GC2145 sensor support"
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index dfbe6448b549..b82e99ca7578 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
 obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
 obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
 obj-$(CONFIG_VIDEO_GC0308) += gc0308.o
+obj-$(CONFIG_VIDEO_GC08A3) += gc08a3.o
 obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
 obj-$(CONFIG_VIDEO_HI556) += hi556.o
 obj-$(CONFIG_VIDEO_HI846) += hi846.o
diff --git a/drivers/media/i2c/gc08a3.c b/drivers/media/i2c/gc08a3.c
new file mode 100644
index 000000000000..ed20cf457520
--- /dev/null
+++ b/drivers/media/i2c/gc08a3.c
@@ -0,0 +1,1333 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * gc08a3.c - gc08a3 sensor driver
+ *
+ * Copyright 2023 MediaTek
+ *
+ * Zhi Mao <zhi.mao@mediatek.com>
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define GC08A3_REG_TEST_PATTERN_EN CCI_REG8(0x008c)
+#define GC08A3_REG_TEST_PATTERN_IDX CCI_REG8(0x008d)
+#define GC08A3_TEST_PATTERN_EN 0x01
+
+#define GC08A3_STREAMING_REG CCI_REG8(0x0100)
+
+#define GC08A3_FLIP_REG CCI_REG8(0x0101)
+#define GC08A3_FLIP_H_MASK BIT(0)
+#define GC08A3_FLIP_V_MASK BIT(1)
+
+#define GC08A3_EXP_REG CCI_REG16(0x0202)
+#define GC08A3_EXP_MARGIN 16
+#define GC08A3_EXP_MIN 4
+#define GC08A3_EXP_STEP 1
+
+#define GC08A3_AGAIN_REG CCI_REG16(0x0204)
+#define GC08A3_AGAIN_MIN 1024
+#define GC08A3_AGAIN_MAX (1024 * 16)
+#define GC08A3_AGAIN_STEP 1
+
+#define GC08A3_FRAME_LENGTH_REG CCI_REG16(0x0340)
+#define GC08A3_VTS_MAX 0xfff0
+
+#define GC08A3_REG_CHIP_ID CCI_REG16(0x03f0)
+#define GC08A3_CHIP_ID 0x08a3
+
+#define GC08A3_NATIVE_WIDTH 3264
+#define GC08A3_NATIVE_HEIGHT 2448
+
+#define GC08A3_DEFAULT_CLK_FREQ (24 * HZ_PER_MHZ)
+#define GC08A3_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
+#define GC08A3_DATA_LANES 4
+
+#define GC08A3_RGB_DEPTH 10
+
+#define GC08A3_SLEEP_US  (2 * USEC_PER_MSEC)
+
+static const char *const gc08a3_test_pattern_menu[] = {
+	"No Pattern", "Solid Black", "Colour Bar", "Solid White",
+	"Solid Red", "Solid Green", "Solid Blue", "Solid Yellow",
+};
+
+static const s64 gc08a3_link_freq_menu_items[] = {
+	(336 * HZ_PER_MHZ),
+	(207 * HZ_PER_MHZ),
+};
+
+static const char *const gc08a3_supply_name[] = {
+	"avdd",
+	"dvdd",
+	"dovdd",
+};
+
+struct gc08a3 {
+	struct device *dev;
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+
+	struct clk *xclk;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(gc08a3_supply_name)];
+	struct gpio_desc *reset_gpio;
+
+	struct v4l2_ctrl_handler ctrls;
+	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;
+
+	struct regmap *regmap;
+	unsigned long link_freq_bitmap;
+	const struct gc08a3_mode *cur_mode;
+};
+
+struct gc08a3_reg_list {
+	u32 num_of_regs;
+	const struct cci_reg_sequence *regs;
+};
+
+static const struct cci_reg_sequence mode_3264x2448[] = {
+	/* system */
+	{ CCI_REG8(0x0336), 0x70 },
+	{ CCI_REG8(0x0383), 0xbb },
+	{ CCI_REG8(0x0344), 0x00 },
+	{ CCI_REG8(0x0345), 0x06 },
+	{ CCI_REG8(0x0346), 0x00 },
+	{ CCI_REG8(0x0347), 0x04 },
+	{ CCI_REG8(0x0348), 0x0c },
+	{ CCI_REG8(0x0349), 0xd0 },
+	{ CCI_REG8(0x034a), 0x09 },
+	{ CCI_REG8(0x034b), 0x9c },
+	{ CCI_REG8(0x0202), 0x09 },
+	{ CCI_REG8(0x0203), 0x04 },
+	{ CCI_REG8(0x0340), 0x09 },
+	{ CCI_REG8(0x0341), 0xf4 },
+	{ CCI_REG8(0x0342), 0x07 },
+	{ CCI_REG8(0x0343), 0x1c },
+
+	{ CCI_REG8(0x0226), 0x00 },
+	{ CCI_REG8(0x0227), 0x28 },
+	{ CCI_REG8(0x0e38), 0x49 },
+	{ CCI_REG8(0x0210), 0x13 },
+	{ CCI_REG8(0x0218), 0x00 },
+	{ CCI_REG8(0x0241), 0x88 },
+	{ CCI_REG8(0x0392), 0x60 },
+
+	/* ISP */
+	{ CCI_REG8(0x00a2), 0x00 },
+	{ CCI_REG8(0x00a3), 0x00 },
+	{ CCI_REG8(0x00ab), 0x00 },
+	{ CCI_REG8(0x00ac), 0x00 },
+
+	/* GAIN */
+	{ CCI_REG8(0x0204), 0x04 },
+	{ CCI_REG8(0x0205), 0x00 },
+	{ CCI_REG8(0x0050), 0x5c },
+	{ CCI_REG8(0x0051), 0x44 },
+
+	/* out window */
+	{ CCI_REG8(0x009a), 0x66 },
+	{ CCI_REG8(0x0351), 0x00 },
+	{ CCI_REG8(0x0352), 0x06 },
+	{ CCI_REG8(0x0353), 0x00 },
+	{ CCI_REG8(0x0354), 0x08 },
+	{ CCI_REG8(0x034c), 0x0c },
+	{ CCI_REG8(0x034d), 0xc0 },
+	{ CCI_REG8(0x034e), 0x09 },
+	{ CCI_REG8(0x034f), 0x90 },
+
+	/* MIPI */
+	{ CCI_REG8(0x0114), 0x03 },
+	{ CCI_REG8(0x0180), 0x65 },
+	{ CCI_REG8(0x0181), 0xf0 },
+	{ CCI_REG8(0x0185), 0x01 },
+	{ CCI_REG8(0x0115), 0x30 },
+	{ CCI_REG8(0x011b), 0x12 },
+	{ CCI_REG8(0x011c), 0x12 },
+	{ CCI_REG8(0x0121), 0x06 },
+	{ CCI_REG8(0x0122), 0x06 },
+	{ CCI_REG8(0x0123), 0x15 },
+	{ CCI_REG8(0x0124), 0x01 },
+	{ CCI_REG8(0x0125), 0x0b },
+	{ CCI_REG8(0x0126), 0x08 },
+	{ CCI_REG8(0x0129), 0x06 },
+	{ CCI_REG8(0x012a), 0x08 },
+	{ CCI_REG8(0x012b), 0x08 },
+
+	{ CCI_REG8(0x0a73), 0x60 },
+	{ CCI_REG8(0x0a70), 0x11 },
+	{ CCI_REG8(0x0313), 0x80 },
+	{ CCI_REG8(0x0aff), 0x00 },
+	{ CCI_REG8(0x0a70), 0x00 },
+	{ CCI_REG8(0x00a4), 0x80 },
+	{ CCI_REG8(0x0316), 0x01 },
+	{ CCI_REG8(0x0a67), 0x00 },
+	{ CCI_REG8(0x0084), 0x10 },
+	{ CCI_REG8(0x0102), 0x09 },
+};
+
+static const struct cci_reg_sequence mode_1920x1080[] = {
+	/* system */
+	{ CCI_REG8(0x0336), 0x45 },
+	{ CCI_REG8(0x0383), 0x8b },
+	{ CCI_REG8(0x0344), 0x02 },
+	{ CCI_REG8(0x0345), 0xa6 },
+	{ CCI_REG8(0x0346), 0x02 },
+	{ CCI_REG8(0x0347), 0xb0 },
+	{ CCI_REG8(0x0348), 0x07 },
+	{ CCI_REG8(0x0349), 0x90 },
+	{ CCI_REG8(0x034a), 0x04 },
+	{ CCI_REG8(0x034b), 0x44 },
+	{ CCI_REG8(0x0202), 0x03 },
+	{ CCI_REG8(0x0203), 0x00 },
+	{ CCI_REG8(0x0340), 0x04 },
+	{ CCI_REG8(0x0341), 0xfc },
+	{ CCI_REG8(0x0342), 0x07 },
+	{ CCI_REG8(0x0343), 0x1c },
+	{ CCI_REG8(0x0226), 0x00 },
+	{ CCI_REG8(0x0227), 0x88 },
+	{ CCI_REG8(0x0e38), 0x49 },
+	{ CCI_REG8(0x0210), 0x13 },
+	{ CCI_REG8(0x0218), 0x00 },
+	{ CCI_REG8(0x0241), 0x88 },
+	{ CCI_REG8(0x0392), 0x60 },
+
+	/* ISP */
+	{ CCI_REG8(0x00a2), 0xac },
+	{ CCI_REG8(0x00a3), 0x02 },
+	{ CCI_REG8(0x00ab), 0xa0 },
+	{ CCI_REG8(0x00ac), 0x02 },
+
+	/* GAIN */
+	{ CCI_REG8(0x0204), 0x04 },
+	{ CCI_REG8(0x0205), 0x00 },
+	{ CCI_REG8(0x0050), 0x38 },
+	{ CCI_REG8(0x0051), 0x20 },
+
+	/* out window */
+	{ CCI_REG8(0x009a), 0x66 },
+	{ CCI_REG8(0x0351), 0x00 },
+	{ CCI_REG8(0x0352), 0x06 },
+	{ CCI_REG8(0x0353), 0x00 },
+	{ CCI_REG8(0x0354), 0x08 },
+	{ CCI_REG8(0x034c), 0x07 },
+	{ CCI_REG8(0x034d), 0x80 },
+	{ CCI_REG8(0x034e), 0x04 },
+	{ CCI_REG8(0x034f), 0x38 },
+
+	/* MIPI */
+	{ CCI_REG8(0x0114), 0x03 },
+	{ CCI_REG8(0x0180), 0x65 },
+	{ CCI_REG8(0x0181), 0xf0 },
+	{ CCI_REG8(0x0185), 0x01 },
+	{ CCI_REG8(0x0115), 0x30 },
+	{ CCI_REG8(0x011b), 0x12 },
+	{ CCI_REG8(0x011c), 0x12 },
+	{ CCI_REG8(0x0121), 0x02 },
+	{ CCI_REG8(0x0122), 0x03 },
+	{ CCI_REG8(0x0123), 0x0c },
+	{ CCI_REG8(0x0124), 0x00 },
+	{ CCI_REG8(0x0125), 0x09 },
+	{ CCI_REG8(0x0126), 0x06 },
+	{ CCI_REG8(0x0129), 0x04 },
+	{ CCI_REG8(0x012a), 0x03 },
+	{ CCI_REG8(0x012b), 0x06 },
+
+	{ CCI_REG8(0x0a73), 0x60 },
+	{ CCI_REG8(0x0a70), 0x11 },
+	{ CCI_REG8(0x0313), 0x80 },
+	{ CCI_REG8(0x0aff), 0x00 },
+	{ CCI_REG8(0x0a70), 0x00 },
+	{ CCI_REG8(0x00a4), 0x80 },
+	{ CCI_REG8(0x0316), 0x01 },
+	{ CCI_REG8(0x0a67), 0x00 },
+	{ CCI_REG8(0x0084), 0x10 },
+	{ CCI_REG8(0x0102), 0x09 },
+};
+
+static const struct cci_reg_sequence mode_table_common[] = {
+	{ GC08A3_STREAMING_REG, 0x00 },
+	/* system */
+	{ CCI_REG8(0x031c), 0x60 },
+	{ CCI_REG8(0x0337), 0x04 },
+	{ CCI_REG8(0x0335), 0x51 },
+	{ CCI_REG8(0x0336), 0x70 },
+	{ CCI_REG8(0x0383), 0xbb },
+	{ CCI_REG8(0x031a), 0x00 },
+	{ CCI_REG8(0x0321), 0x10 },
+	{ CCI_REG8(0x0327), 0x03 },
+	{ CCI_REG8(0x0325), 0x40 },
+	{ CCI_REG8(0x0326), 0x23 },
+	{ CCI_REG8(0x0314), 0x11 },
+	{ CCI_REG8(0x0315), 0xd6 },
+	{ CCI_REG8(0x0316), 0x01 },
+	{ CCI_REG8(0x0334), 0x40 },
+	{ CCI_REG8(0x0324), 0x42 },
+	{ CCI_REG8(0x031c), 0x00 },
+	{ CCI_REG8(0x031c), 0x9f },
+	{ CCI_REG8(0x039a), 0x13 },
+	{ CCI_REG8(0x0084), 0x30 },
+	{ CCI_REG8(0x02b3), 0x08 },
+	{ CCI_REG8(0x0057), 0x0c },
+	{ CCI_REG8(0x05c3), 0x50 },
+	{ CCI_REG8(0x0311), 0x90 },
+	{ CCI_REG8(0x05a0), 0x02 },
+	{ CCI_REG8(0x0074), 0x0a },
+	{ CCI_REG8(0x0059), 0x11 },
+	{ CCI_REG8(0x0070), 0x05 },
+	{ CCI_REG8(0x0101), 0x00 },
+
+	/* analog */
+	{ CCI_REG8(0x0344), 0x00 },
+	{ CCI_REG8(0x0345), 0x06 },
+	{ CCI_REG8(0x0346), 0x00 },
+	{ CCI_REG8(0x0347), 0x04 },
+	{ CCI_REG8(0x0348), 0x0c },
+	{ CCI_REG8(0x0349), 0xd0 },
+	{ CCI_REG8(0x034a), 0x09 },
+	{ CCI_REG8(0x034b), 0x9c },
+	{ CCI_REG8(0x0202), 0x09 },
+	{ CCI_REG8(0x0203), 0x04 },
+
+	{ CCI_REG8(0x0219), 0x05 },
+	{ CCI_REG8(0x0226), 0x00 },
+	{ CCI_REG8(0x0227), 0x28 },
+	{ CCI_REG8(0x0e0a), 0x00 },
+	{ CCI_REG8(0x0e0b), 0x00 },
+	{ CCI_REG8(0x0e24), 0x04 },
+	{ CCI_REG8(0x0e25), 0x04 },
+	{ CCI_REG8(0x0e26), 0x00 },
+	{ CCI_REG8(0x0e27), 0x10 },
+	{ CCI_REG8(0x0e01), 0x74 },
+	{ CCI_REG8(0x0e03), 0x47 },
+	{ CCI_REG8(0x0e04), 0x33 },
+	{ CCI_REG8(0x0e05), 0x44 },
+	{ CCI_REG8(0x0e06), 0x44 },
+	{ CCI_REG8(0x0e0c), 0x1e },
+	{ CCI_REG8(0x0e17), 0x3a },
+	{ CCI_REG8(0x0e18), 0x3c },
+	{ CCI_REG8(0x0e19), 0x40 },
+	{ CCI_REG8(0x0e1a), 0x42 },
+	{ CCI_REG8(0x0e28), 0x21 },
+	{ CCI_REG8(0x0e2b), 0x68 },
+	{ CCI_REG8(0x0e2c), 0x0d },
+	{ CCI_REG8(0x0e2d), 0x08 },
+	{ CCI_REG8(0x0e34), 0xf4 },
+	{ CCI_REG8(0x0e35), 0x44 },
+	{ CCI_REG8(0x0e36), 0x07 },
+	{ CCI_REG8(0x0e38), 0x49 },
+	{ CCI_REG8(0x0210), 0x13 },
+	{ CCI_REG8(0x0218), 0x00 },
+	{ CCI_REG8(0x0241), 0x88 },
+	{ CCI_REG8(0x0e32), 0x00 },
+	{ CCI_REG8(0x0e33), 0x18 },
+	{ CCI_REG8(0x0e42), 0x03 },
+	{ CCI_REG8(0x0e43), 0x80 },
+	{ CCI_REG8(0x0e44), 0x04 },
+	{ CCI_REG8(0x0e45), 0x00 },
+	{ CCI_REG8(0x0e4f), 0x04 },
+	{ CCI_REG8(0x057a), 0x20 },
+	{ CCI_REG8(0x0381), 0x7c },
+	{ CCI_REG8(0x0382), 0x9b },
+	{ CCI_REG8(0x0384), 0xfb },
+	{ CCI_REG8(0x0389), 0x38 },
+	{ CCI_REG8(0x038a), 0x03 },
+	{ CCI_REG8(0x0390), 0x6a },
+	{ CCI_REG8(0x0391), 0x0b },
+	{ CCI_REG8(0x0392), 0x60 },
+	{ CCI_REG8(0x0393), 0xc1 },
+	{ CCI_REG8(0x0396), 0xff },
+	{ CCI_REG8(0x0398), 0x62 },
+
+	/* cisctl reset */
+	{ CCI_REG8(0x031c), 0x80 },
+	{ CCI_REG8(0x03fe), 0x10 },
+	{ CCI_REG8(0x03fe), 0x00 },
+	{ CCI_REG8(0x031c), 0x9f },
+	{ CCI_REG8(0x03fe), 0x00 },
+	{ CCI_REG8(0x03fe), 0x00 },
+	{ CCI_REG8(0x03fe), 0x00 },
+	{ CCI_REG8(0x03fe), 0x00 },
+	{ CCI_REG8(0x031c), 0x80 },
+	{ CCI_REG8(0x03fe), 0x10 },
+	{ CCI_REG8(0x03fe), 0x00 },
+	{ CCI_REG8(0x031c), 0x9f },
+	{ CCI_REG8(0x0360), 0x01 },
+	{ CCI_REG8(0x0360), 0x00 },
+	{ CCI_REG8(0x0316), 0x09 },
+	{ CCI_REG8(0x0a67), 0x80 },
+	{ CCI_REG8(0x0313), 0x00 },
+	{ CCI_REG8(0x0a53), 0x0e },
+	{ CCI_REG8(0x0a65), 0x17 },
+	{ CCI_REG8(0x0a68), 0xa1 },
+	{ CCI_REG8(0x0a58), 0x00 },
+	{ CCI_REG8(0x0ace), 0x0c },
+	{ CCI_REG8(0x00a4), 0x00 },
+	{ CCI_REG8(0x00a5), 0x01 },
+	{ CCI_REG8(0x00a7), 0x09 },
+	{ CCI_REG8(0x00a8), 0x9c },
+	{ CCI_REG8(0x00a9), 0x0c },
+	{ CCI_REG8(0x00aa), 0xd0 },
+	{ CCI_REG8(0x0a8a), 0x00 },
+	{ CCI_REG8(0x0a8b), 0xe0 },
+	{ CCI_REG8(0x0a8c), 0x13 },
+	{ CCI_REG8(0x0a8d), 0xe8 },
+	{ CCI_REG8(0x0a90), 0x0a },
+	{ CCI_REG8(0x0a91), 0x10 },
+	{ CCI_REG8(0x0a92), 0xf8 },
+	{ CCI_REG8(0x0a71), 0xf2 },
+	{ CCI_REG8(0x0a72), 0x12 },
+	{ CCI_REG8(0x0a73), 0x64 },
+	{ CCI_REG8(0x0a75), 0x41 },
+	{ CCI_REG8(0x0a70), 0x07 },
+	{ CCI_REG8(0x0313), 0x80 },
+
+	/* ISP */
+	{ CCI_REG8(0x00a0), 0x01 },
+	{ CCI_REG8(0x0080), 0xd2 },
+	{ CCI_REG8(0x0081), 0x3f },
+	{ CCI_REG8(0x0087), 0x51 },
+	{ CCI_REG8(0x0089), 0x03 },
+	{ CCI_REG8(0x009b), 0x40 },
+	{ CCI_REG8(0x05a0), 0x82 },
+	{ CCI_REG8(0x05ac), 0x00 },
+	{ CCI_REG8(0x05ad), 0x01 },
+	{ CCI_REG8(0x05ae), 0x00 },
+	{ CCI_REG8(0x0800), 0x0a },
+	{ CCI_REG8(0x0801), 0x14 },
+	{ CCI_REG8(0x0802), 0x28 },
+	{ CCI_REG8(0x0803), 0x34 },
+	{ CCI_REG8(0x0804), 0x0e },
+	{ CCI_REG8(0x0805), 0x33 },
+	{ CCI_REG8(0x0806), 0x03 },
+	{ CCI_REG8(0x0807), 0x8a },
+	{ CCI_REG8(0x0808), 0x50 },
+	{ CCI_REG8(0x0809), 0x00 },
+	{ CCI_REG8(0x080a), 0x34 },
+	{ CCI_REG8(0x080b), 0x03 },
+	{ CCI_REG8(0x080c), 0x26 },
+	{ CCI_REG8(0x080d), 0x03 },
+	{ CCI_REG8(0x080e), 0x18 },
+	{ CCI_REG8(0x080f), 0x03 },
+	{ CCI_REG8(0x0810), 0x10 },
+	{ CCI_REG8(0x0811), 0x03 },
+	{ CCI_REG8(0x0812), 0x00 },
+	{ CCI_REG8(0x0813), 0x00 },
+	{ CCI_REG8(0x0814), 0x01 },
+	{ CCI_REG8(0x0815), 0x00 },
+	{ CCI_REG8(0x0816), 0x01 },
+	{ CCI_REG8(0x0817), 0x00 },
+	{ CCI_REG8(0x0818), 0x00 },
+	{ CCI_REG8(0x0819), 0x0a },
+	{ CCI_REG8(0x081a), 0x01 },
+	{ CCI_REG8(0x081b), 0x6c },
+	{ CCI_REG8(0x081c), 0x00 },
+	{ CCI_REG8(0x081d), 0x0b },
+	{ CCI_REG8(0x081e), 0x02 },
+	{ CCI_REG8(0x081f), 0x00 },
+	{ CCI_REG8(0x0820), 0x00 },
+	{ CCI_REG8(0x0821), 0x0c },
+	{ CCI_REG8(0x0822), 0x02 },
+	{ CCI_REG8(0x0823), 0xd9 },
+	{ CCI_REG8(0x0824), 0x00 },
+	{ CCI_REG8(0x0825), 0x0d },
+	{ CCI_REG8(0x0826), 0x03 },
+	{ CCI_REG8(0x0827), 0xf0 },
+	{ CCI_REG8(0x0828), 0x00 },
+	{ CCI_REG8(0x0829), 0x0e },
+	{ CCI_REG8(0x082a), 0x05 },
+	{ CCI_REG8(0x082b), 0x94 },
+	{ CCI_REG8(0x082c), 0x09 },
+	{ CCI_REG8(0x082d), 0x6e },
+	{ CCI_REG8(0x082e), 0x07 },
+	{ CCI_REG8(0x082f), 0xe6 },
+	{ CCI_REG8(0x0830), 0x10 },
+	{ CCI_REG8(0x0831), 0x0e },
+	{ CCI_REG8(0x0832), 0x0b },
+	{ CCI_REG8(0x0833), 0x2c },
+	{ CCI_REG8(0x0834), 0x14 },
+	{ CCI_REG8(0x0835), 0xae },
+	{ CCI_REG8(0x0836), 0x0f },
+	{ CCI_REG8(0x0837), 0xc4 },
+	{ CCI_REG8(0x0838), 0x18 },
+	{ CCI_REG8(0x0839), 0x0e },
+	{ CCI_REG8(0x05ac), 0x01 },
+	{ CCI_REG8(0x059a), 0x00 },
+	{ CCI_REG8(0x059b), 0x00 },
+	{ CCI_REG8(0x059c), 0x01 },
+	{ CCI_REG8(0x0598), 0x00 },
+	{ CCI_REG8(0x0597), 0x14 },
+	{ CCI_REG8(0x05ab), 0x09 },
+	{ CCI_REG8(0x05a4), 0x02 },
+	{ CCI_REG8(0x05a3), 0x05 },
+	{ CCI_REG8(0x05a0), 0xc2 },
+	{ CCI_REG8(0x0207), 0xc4 },
+
+	/* GAIN */
+	{ CCI_REG8(0x0208), 0x01 },
+	{ CCI_REG8(0x0209), 0x72 },
+	{ CCI_REG8(0x0204), 0x04 },
+	{ CCI_REG8(0x0205), 0x00 },
+
+	{ CCI_REG8(0x0040), 0x22 },
+	{ CCI_REG8(0x0041), 0x20 },
+	{ CCI_REG8(0x0043), 0x10 },
+	{ CCI_REG8(0x0044), 0x00 },
+	{ CCI_REG8(0x0046), 0x08 },
+	{ CCI_REG8(0x0047), 0xf0 },
+	{ CCI_REG8(0x0048), 0x0f },
+	{ CCI_REG8(0x004b), 0x0f },
+	{ CCI_REG8(0x004c), 0x00 },
+	{ CCI_REG8(0x0050), 0x5c },
+	{ CCI_REG8(0x0051), 0x44 },
+	{ CCI_REG8(0x005b), 0x03 },
+	{ CCI_REG8(0x00c0), 0x00 },
+	{ CCI_REG8(0x00c1), 0x80 },
+	{ CCI_REG8(0x00c2), 0x31 },
+	{ CCI_REG8(0x00c3), 0x00 },
+	{ CCI_REG8(0x0460), 0x04 },
+	{ CCI_REG8(0x0462), 0x08 },
+	{ CCI_REG8(0x0464), 0x0e },
+	{ CCI_REG8(0x0466), 0x0a },
+	{ CCI_REG8(0x0468), 0x12 },
+	{ CCI_REG8(0x046a), 0x12 },
+	{ CCI_REG8(0x046c), 0x10 },
+	{ CCI_REG8(0x046e), 0x0c },
+	{ CCI_REG8(0x0461), 0x03 },
+	{ CCI_REG8(0x0463), 0x03 },
+	{ CCI_REG8(0x0465), 0x03 },
+	{ CCI_REG8(0x0467), 0x03 },
+	{ CCI_REG8(0x0469), 0x04 },
+	{ CCI_REG8(0x046b), 0x04 },
+	{ CCI_REG8(0x046d), 0x04 },
+	{ CCI_REG8(0x046f), 0x04 },
+	{ CCI_REG8(0x0470), 0x04 },
+	{ CCI_REG8(0x0472), 0x10 },
+	{ CCI_REG8(0x0474), 0x26 },
+	{ CCI_REG8(0x0476), 0x38 },
+	{ CCI_REG8(0x0478), 0x20 },
+	{ CCI_REG8(0x047a), 0x30 },
+	{ CCI_REG8(0x047c), 0x38 },
+	{ CCI_REG8(0x047e), 0x60 },
+	{ CCI_REG8(0x0471), 0x05 },
+	{ CCI_REG8(0x0473), 0x05 },
+	{ CCI_REG8(0x0475), 0x05 },
+	{ CCI_REG8(0x0477), 0x05 },
+	{ CCI_REG8(0x0479), 0x04 },
+	{ CCI_REG8(0x047b), 0x04 },
+	{ CCI_REG8(0x047d), 0x04 },
+	{ CCI_REG8(0x047f), 0x04 },
+};
+
+struct gc08a3_mode {
+	u32 width;
+	u32 height;
+	const struct gc08a3_reg_list reg_list;
+
+	u32 hts; /* Horizontal timining size */
+	u32 vts_def; /* Default vertical timining size */
+	u32 vts_min; /* Min vertical timining size */
+};
+
+/* Declare modes in order, from biggest to smallest height. */
+static const struct gc08a3_mode gc08a3_modes[] = {
+	{
+		/* 3264*2448@30fps */
+		.width = GC08A3_NATIVE_WIDTH,
+		.height = GC08A3_NATIVE_HEIGHT,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_3264x2448),
+			.regs = mode_3264x2448,
+		},
+		.hts = 3640,
+		.vts_def = 2548,
+		.vts_min = 2548,
+	},
+	{
+		/* 1920*1080@60fps */
+		.width = 1920,
+		.height = 1080,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920x1080),
+			.regs = mode_1920x1080,
+		},
+		.hts = 3640,
+		.vts_def = 1276,
+		.vts_min = 1276,
+	},
+};
+
+static inline struct gc08a3 *to_gc08a3(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct gc08a3, sd);
+}
+
+static int gc08a3_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gc08a3 *gc08a3 = to_gc08a3(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name),
+				    gc08a3->supplies);
+	if (ret < 0) {
+		dev_err(gc08a3->dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(gc08a3->xclk);
+	if (ret < 0) {
+		regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name),
+				       gc08a3->supplies);
+		dev_err(gc08a3->dev, "clk prepare enable failed\n");
+		return ret;
+	}
+
+	fsleep(GC08A3_SLEEP_US);
+
+	gpiod_set_value_cansleep(gc08a3->reset_gpio, 0);
+	fsleep(GC08A3_SLEEP_US);
+
+	return 0;
+}
+
+static int gc08a3_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gc08a3 *gc08a3 = to_gc08a3(sd);
+
+	clk_disable_unprepare(gc08a3->xclk);
+	gpiod_set_value_cansleep(gc08a3->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name),
+			       gc08a3->supplies);
+
+	return 0;
+}
+
+static int gc08a3_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index > 0)
+		return -EINVAL;
+
+	code->code = GC08A3_MBUS_CODE;
+
+	return 0;
+}
+
+static int gc08a3_enum_frame_size(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_state *sd_state,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->code != GC08A3_MBUS_CODE)
+		return -EINVAL;
+
+	if (fse->index >= ARRAY_SIZE(gc08a3_modes))
+		return -EINVAL;
+
+	fse->min_width = gc08a3_modes[fse->index].width;
+	fse->max_width = gc08a3_modes[fse->index].width;
+	fse->min_height = gc08a3_modes[fse->index].height;
+	fse->max_height = gc08a3_modes[fse->index].height;
+
+	return 0;
+}
+
+static int gc08a3_update_cur_mode_controls(struct gc08a3 *gc08a3,
+					   const struct gc08a3_mode *mode)
+{
+	s64 exposure_max, h_blank;
+	int ret;
+
+	ret = __v4l2_ctrl_modify_range(gc08a3->vblank,
+				       mode->vts_min - mode->height,
+				       GC08A3_VTS_MAX - mode->height, 1,
+				       mode->vts_def - mode->height);
+	if (ret) {
+		dev_err(gc08a3->dev, "VB ctrl range update failed\n");
+		return ret;
+	}
+
+	h_blank = mode->hts - mode->width;
+	ret = __v4l2_ctrl_modify_range(gc08a3->hblank, h_blank, h_blank, 1,
+				       h_blank);
+	if (ret) {
+		dev_err(gc08a3->dev, "HB ctrl range update failed\n");
+		return ret;
+	}
+
+	exposure_max = mode->vts_def - GC08A3_EXP_MARGIN;
+	ret = __v4l2_ctrl_modify_range(gc08a3->exposure, GC08A3_EXP_MIN,
+				       exposure_max, GC08A3_EXP_STEP,
+				       exposure_max);
+	if (ret) {
+		dev_err(gc08a3->dev, "exposure ctrl range update failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void gc08a3_update_pad_format(struct gc08a3 *gc08a3,
+				     const struct gc08a3_mode *mode,
+				     struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->width = mode->width;
+	fmt->height = mode->height;
+	fmt->code = GC08A3_MBUS_CODE;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
+}
+
+static int gc08a3_set_format(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state,
+			     struct v4l2_subdev_format *fmt)
+{
+	struct gc08a3 *gc08a3 = to_gc08a3(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt;
+	struct v4l2_rect *crop;
+	const struct gc08a3_mode *mode;
+
+	mode = v4l2_find_nearest_size(gc08a3_modes, ARRAY_SIZE(gc08a3_modes),
+				      width, height, fmt->format.width,
+				      fmt->format.height);
+
+	/*update crop info to subdev state*/
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	crop->width = mode->width;
+	crop->height = mode->height;
+
+	/*update fmt info to subdev state*/
+	gc08a3_update_pad_format(gc08a3, mode, &fmt->format);
+	mbus_fmt = v4l2_subdev_state_get_format(state, 0);
+	*mbus_fmt = fmt->format;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		return 0;
+
+	gc08a3->cur_mode = mode;
+	gc08a3_update_cur_mode_controls(gc08a3, mode);
+
+	return 0;
+}
+
+static int gc08a3_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP:
+		sel->r = *v4l2_subdev_state_get_crop(state, 0);
+		break;
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = GC08A3_NATIVE_WIDTH;
+		sel->r.height = GC08A3_NATIVE_HEIGHT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int gc08a3_init_state(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state)
+{
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = 0,
+		.format = {
+			.code = GC08A3_MBUS_CODE,
+			.width = gc08a3_modes[0].width,
+			.height = gc08a3_modes[0].height,
+		},
+	};
+
+	gc08a3_set_format(sd, state, &fmt);
+
+	return 0;
+}
+
+static int gc08a3_set_ctrl_hflip(struct gc08a3 *gc08a3, u32 ctrl_val)
+{
+	int ret;
+	u64 val;
+
+	ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL);
+	if (ret) {
+		dev_err(gc08a3->dev, "read hflip register failed: %d\n", ret);
+		return ret;
+	}
+
+	return cci_update_bits(gc08a3->regmap, GC08A3_FLIP_REG,
+			       GC08A3_FLIP_H_MASK,
+			       ctrl_val ? GC08A3_FLIP_H_MASK : 0, NULL);
+}
+
+static int gc08a3_set_ctrl_vflip(struct gc08a3 *gc08a3, u32 ctrl_val)
+{
+	int ret;
+	u64 val;
+
+	ret = cci_read(gc08a3->regmap, GC08A3_FLIP_REG, &val, NULL);
+	if (ret) {
+		dev_err(gc08a3->dev, "read vflip register failed: %d\n", ret);
+		return ret;
+	}
+
+	return cci_update_bits(gc08a3->regmap, GC08A3_FLIP_REG,
+			       GC08A3_FLIP_V_MASK,
+			       ctrl_val ? GC08A3_FLIP_V_MASK : 0, NULL);
+}
+
+static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 pattern_menu)
+{
+	u32 pattern = 0;
+	int ret;
+
+	if (pattern_menu) {
+		switch (pattern_menu) {
+		case 1:
+			pattern = 0x00;
+			break;
+		case 2:
+			pattern = 0x10;
+			break;
+		case 3:
+		case 4:
+		case 5:
+		case 6:
+		case 7:
+			pattern = pattern_menu + 1;
+			break;
+		}
+
+		ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX,
+				pattern, NULL);
+		if (ret)
+			return ret;
+
+		ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN,
+				GC08A3_TEST_PATTERN_EN, NULL);
+		if (ret)
+			return ret;
+	} else {
+		ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN,
+				0x00, NULL);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int gc08a3_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct gc08a3 *gc08a3 =
+		container_of(ctrl->handler, struct gc08a3, ctrls);
+	int ret = 0;
+	s64 exposure_max;
+	struct v4l2_subdev_state *state;
+	const struct v4l2_mbus_framefmt *format;
+
+	state = v4l2_subdev_get_locked_active_state(&gc08a3->sd);
+	format = v4l2_subdev_state_get_format(state, 0);
+
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		/* Update max exposure while meeting expected vblanking */
+		exposure_max = format->height + ctrl->val - GC08A3_EXP_MARGIN;
+		__v4l2_ctrl_modify_range(gc08a3->exposure,
+					 gc08a3->exposure->minimum,
+					 exposure_max, gc08a3->exposure->step,
+					 exposure_max);
+	}
+
+	/*
+	 * Applying V4L2 control value only happens
+	 * when power is on for streaming
+	 */
+	if (!pm_runtime_get_if_in_use(gc08a3->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE:
+		ret = cci_write(gc08a3->regmap, GC08A3_EXP_REG,
+				ctrl->val, NULL);
+		break;
+
+	case V4L2_CID_ANALOGUE_GAIN:
+		ret = cci_write(gc08a3->regmap, GC08A3_AGAIN_REG,
+				ctrl->val, NULL);
+		break;
+
+	case V4L2_CID_VBLANK:
+		ret = cci_write(gc08a3->regmap, GC08A3_FRAME_LENGTH_REG,
+				gc08a3->cur_mode->height + ctrl->val, NULL);
+		break;
+
+	case V4L2_CID_HFLIP:
+		ret = gc08a3_set_ctrl_hflip(gc08a3, ctrl->val);
+		break;
+
+	case V4L2_CID_VFLIP:
+		ret = gc08a3_set_ctrl_vflip(gc08a3, ctrl->val);
+		break;
+
+	case V4L2_CID_TEST_PATTERN:
+		ret = gc08a3_test_pattern(gc08a3, ctrl->val);
+		break;
+
+	default:
+		break;
+	}
+
+	pm_runtime_put(gc08a3->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops gc08a3_ctrl_ops = {
+	.s_ctrl = gc08a3_set_ctrl,
+};
+
+static int gc08a3_start_streaming(struct gc08a3 *gc08a3)
+{
+	const struct gc08a3_mode *mode;
+	const struct gc08a3_reg_list *reg_list;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(gc08a3->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = cci_multi_reg_write(gc08a3->regmap,
+				  mode_table_common,
+				  ARRAY_SIZE(mode_table_common), NULL);
+	if (ret)
+		goto err_rpm_put;
+
+	mode = gc08a3->cur_mode;
+	reg_list = &mode->reg_list;
+	ret = cci_multi_reg_write(gc08a3->regmap,
+				  reg_list->regs, reg_list->num_of_regs, NULL);
+	if (ret < 0)
+		goto err_rpm_put;
+
+	ret = __v4l2_ctrl_handler_setup(&gc08a3->ctrls);
+	if (ret < 0) {
+		dev_err(gc08a3->dev, "could not sync v4l2 controls\n");
+		goto err_rpm_put;
+	}
+
+	ret = cci_write(gc08a3->regmap, GC08A3_STREAMING_REG, 1, NULL);
+	if (ret < 0) {
+		dev_err(gc08a3->dev, "write STRAEMING_REG failed: %d\n", ret);
+		goto err_rpm_put;
+	}
+
+	return 0;
+
+err_rpm_put:
+	pm_runtime_put(gc08a3->dev);
+	return ret;
+}
+
+static int gc08a3_stop_streaming(struct gc08a3 *gc08a3)
+{
+	int ret;
+
+	ret = cci_write(gc08a3->regmap, GC08A3_STREAMING_REG, 0, NULL);
+	if (ret < 0)
+		dev_err(gc08a3->dev, "could not sent stop streaming %d\n", ret);
+
+	pm_runtime_put(gc08a3->dev);
+	return ret;
+}
+
+static int gc08a3_s_stream(struct v4l2_subdev *subdev, int enable)
+{
+	struct gc08a3 *gc08a3 = to_gc08a3(subdev);
+	struct v4l2_subdev_state *state;
+	int ret;
+
+	state = v4l2_subdev_lock_and_get_active_state(subdev);
+
+	if (enable)
+		ret = gc08a3_start_streaming(gc08a3);
+	else
+		ret = gc08a3_stop_streaming(gc08a3);
+
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_video_ops gc08a3_video_ops = {
+	.s_stream = gc08a3_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops gc08a3_subdev_pad_ops = {
+	.enum_mbus_code = gc08a3_enum_mbus_code,
+	.enum_frame_size = gc08a3_enum_frame_size,
+	.get_fmt = v4l2_subdev_get_fmt,
+	.set_fmt = gc08a3_set_format,
+	.get_selection = gc08a3_get_selection,
+};
+
+static const struct v4l2_subdev_core_ops gc08a3_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_ops gc08a3_subdev_ops = {
+	.core = &gc08a3_core_ops,
+	.video = &gc08a3_video_ops,
+	.pad = &gc08a3_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops gc08a3_internal_ops = {
+	.init_state = gc08a3_init_state,
+};
+
+static int gc08a3_get_regulators(struct device *dev, struct gc08a3 *gc08a3)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(gc08a3_supply_name); i++)
+		gc08a3->supplies[i].supply = gc08a3_supply_name[i];
+
+	return devm_regulator_bulk_get(dev, ARRAY_SIZE(gc08a3_supply_name),
+				       gc08a3->supplies);
+}
+
+static int gc08a3_parse_fwnode(struct gc08a3 *gc08a3)
+{
+	struct fwnode_handle *endpoint;
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	int ret;
+	struct device *dev = gc08a3->dev;
+
+	endpoint =
+		fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
+						FWNODE_GRAPH_ENDPOINT_NEXT);
+	if (!endpoint) {
+		dev_err(dev, "endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
+	if (ret) {
+		dev_err(dev, "parsing endpoint node failed\n");
+		goto done;
+	}
+
+	ret = v4l2_link_freq_to_bitmap(dev, bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       gc08a3_link_freq_menu_items,
+				       ARRAY_SIZE(gc08a3_link_freq_menu_items),
+				       &gc08a3->link_freq_bitmap);
+	if (ret)
+		goto done;
+
+done:
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+	fwnode_handle_put(endpoint);
+	return ret;
+}
+
+static u64 gc08a3_to_pixel_rate(u32 f_index)
+{
+	u64 pixel_rate =
+		gc08a3_link_freq_menu_items[f_index] * 2 * GC08A3_DATA_LANES;
+
+	return div_u64(pixel_rate, GC08A3_RGB_DEPTH);
+}
+
+static int gc08a3_init_controls(struct gc08a3 *gc08a3)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&gc08a3->sd);
+	const struct gc08a3_mode *mode = &gc08a3_modes[0];
+	const struct v4l2_ctrl_ops *ops = &gc08a3_ctrl_ops;
+	struct v4l2_fwnode_device_properties props;
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	s64 exposure_max, h_blank;
+	int ret;
+
+	ctrl_hdlr = &gc08a3->ctrls;
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
+	if (ret)
+		return ret;
+
+	gc08a3->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	gc08a3->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_cluster(2, &gc08a3->hflip);
+
+	gc08a3->link_freq =
+	v4l2_ctrl_new_int_menu(ctrl_hdlr,
+			       &gc08a3_ctrl_ops,
+			       V4L2_CID_LINK_FREQ,
+			       ARRAY_SIZE(gc08a3_link_freq_menu_items) - 1,
+			       0,
+			       gc08a3_link_freq_menu_items);
+	if (gc08a3->link_freq)
+		gc08a3->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	gc08a3->pixel_rate =
+		v4l2_ctrl_new_std(ctrl_hdlr,
+				  &gc08a3_ctrl_ops,
+				  V4L2_CID_PIXEL_RATE, 0,
+				  gc08a3_to_pixel_rate(0),
+				  1,
+				  gc08a3_to_pixel_rate(0));
+
+	gc08a3->vblank =
+		v4l2_ctrl_new_std(ctrl_hdlr,
+				  &gc08a3_ctrl_ops, V4L2_CID_VBLANK,
+				  mode->vts_min - mode->height,
+				  GC08A3_VTS_MAX - mode->height, 1,
+				  mode->vts_def - mode->height);
+
+	h_blank = mode->hts - mode->width;
+	gc08a3->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops,
+					   V4L2_CID_HBLANK, h_blank, h_blank, 1,
+					   h_blank);
+	if (gc08a3->hblank)
+		gc08a3->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops,
+			  V4L2_CID_ANALOGUE_GAIN, GC08A3_AGAIN_MIN,
+			  GC08A3_AGAIN_MAX, GC08A3_AGAIN_STEP,
+			  GC08A3_AGAIN_MIN);
+
+	exposure_max = mode->vts_def - GC08A3_EXP_MARGIN;
+	gc08a3->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &gc08a3_ctrl_ops,
+					     V4L2_CID_EXPOSURE, GC08A3_EXP_MIN,
+					     exposure_max, GC08A3_EXP_STEP,
+					     exposure_max);
+
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &gc08a3_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(gc08a3_test_pattern_menu) - 1,
+				     0, 0, gc08a3_test_pattern_menu);
+
+	/* register properties to fwnode (e.g. rotation, orientation) */
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		goto error_ctrls;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, ops, &props);
+	if (ret)
+		goto error_ctrls;
+
+	if (ctrl_hdlr->error) {
+		ret = ctrl_hdlr->error;
+		goto error_ctrls;
+	}
+
+	gc08a3->sd.ctrl_handler = ctrl_hdlr;
+
+	return 0;
+
+error_ctrls:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+
+	return ret;
+}
+
+static int gc08a3_identify_module(struct gc08a3 *gc08a3)
+{
+	u64 val;
+	int ret;
+
+	ret = cci_read(gc08a3->regmap, GC08A3_REG_CHIP_ID, &val, NULL);
+	if (ret) {
+		dev_err(gc08a3->dev, "failed to read chip id");
+		return ret;
+	}
+
+	if (val != GC08A3_CHIP_ID) {
+		dev_err(gc08a3->dev, "chip id mismatch: 0x%x!=0x%llx",
+			GC08A3_CHIP_ID, val);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static int gc08a3_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct gc08a3 *gc08a3;
+	int ret;
+
+	gc08a3 = devm_kzalloc(dev, sizeof(*gc08a3), GFP_KERNEL);
+	if (!gc08a3)
+		return -ENOMEM;
+
+	gc08a3->dev = dev;
+
+	ret = gc08a3_parse_fwnode(gc08a3);
+	if (ret)
+		return ret;
+
+	gc08a3->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(gc08a3->regmap))
+		return dev_err_probe(dev, PTR_ERR(gc08a3->regmap),
+				     "failed to init CCI\n");
+
+	gc08a3->xclk = devm_clk_get(dev, NULL);
+	if (IS_ERR(gc08a3->xclk))
+		return dev_err_probe(dev, PTR_ERR(gc08a3->xclk),
+				     "failed to get xclk\n");
+
+	ret = clk_set_rate(gc08a3->xclk, GC08A3_DEFAULT_CLK_FREQ);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to set xclk frequency\n");
+
+	ret = gc08a3_get_regulators(dev, gc08a3);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "failed to get regulators\n");
+
+	gc08a3->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(gc08a3->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(gc08a3->reset_gpio),
+				     "failed to get gpio\n");
+
+	v4l2_i2c_subdev_init(&gc08a3->sd, client, &gc08a3_subdev_ops);
+	gc08a3->sd.internal_ops = &gc08a3_internal_ops;
+	gc08a3->cur_mode = &gc08a3_modes[0];
+
+	ret = gc08a3_power_on(gc08a3->dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to sensor power on\n");
+
+	ret = gc08a3_identify_module(gc08a3);
+	if (ret) {
+		dev_err(&client->dev, "failed to find sensor: %d\n", ret);
+		goto err_power_off;
+	}
+
+	ret = gc08a3_init_controls(gc08a3);
+	if (ret) {
+		dev_err(&client->dev, "failed to init controls: %d", ret);
+		goto err_power_off;
+	}
+
+	gc08a3->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
+	gc08a3->pad.flags = MEDIA_PAD_FL_SOURCE;
+	gc08a3->sd.dev = &client->dev;
+	gc08a3->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	ret = media_entity_pads_init(&gc08a3->sd.entity, 1, &gc08a3->pad);
+	if (ret < 0) {
+		dev_err(dev, "could not register media entity\n");
+		goto err_v4l2_ctrl_handler_free;
+	}
+
+	gc08a3->sd.state_lock = gc08a3->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&gc08a3->sd);
+	if (ret < 0) {
+		dev_err(dev, "v4l2 subdev init error: %d\n", ret);
+		goto err_media_entity_cleanup;
+	}
+
+	pm_runtime_set_active(gc08a3->dev);
+	pm_runtime_enable(gc08a3->dev);
+	pm_runtime_set_autosuspend_delay(gc08a3->dev, 1000);
+	pm_runtime_use_autosuspend(gc08a3->dev);
+	pm_runtime_idle(gc08a3->dev);
+
+	ret = v4l2_async_register_subdev_sensor(&gc08a3->sd);
+	if (ret < 0) {
+		dev_err(dev, "could not register v4l2 device\n");
+		goto err_rpm;
+	}
+
+	return 0;
+
+err_rpm:
+	pm_runtime_disable(gc08a3->dev);
+	v4l2_subdev_cleanup(&gc08a3->sd);
+
+err_media_entity_cleanup:
+	media_entity_cleanup(&gc08a3->sd.entity);
+
+err_v4l2_ctrl_handler_free:
+	v4l2_ctrl_handler_free(gc08a3->sd.ctrl_handler);
+
+err_power_off:
+	gc08a3_power_off(gc08a3->dev);
+
+	return ret;
+}
+
+static void gc08a3_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct gc08a3 *gc08a3 = to_gc08a3(sd);
+
+	v4l2_async_unregister_subdev(&gc08a3->sd);
+	v4l2_subdev_cleanup(sd);
+	media_entity_cleanup(&gc08a3->sd.entity);
+	v4l2_ctrl_handler_free(&gc08a3->ctrls);
+
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		gc08a3_power_off(gc08a3->dev);
+	pm_runtime_set_suspended(&client->dev);
+}
+
+static const struct of_device_id gc08a3_of_match[] = {
+	{ .compatible = "galaxycore,gc08a3" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gc08a3_of_match);
+
+static const struct dev_pm_ops gc08a3_pm_ops = {
+	RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL)
+};
+
+static struct i2c_driver gc08a3_i2c_driver = {
+	.driver = {
+		.of_match_table = gc08a3_of_match,
+		.pm = pm_ptr(&gc08a3_pm_ops),
+		.name  = "gc08a3",
+	},
+	.probe  = gc08a3_probe,
+	.remove = gc08a3_remove,
+};
+
+module_i2c_driver(gc08a3_i2c_driver);
+
+MODULE_DESCRIPTION("GalaxyCore gc08a3 Camera driver");
+MODULE_AUTHOR("Zhi Mao <zhi.mao@mediatek.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-27  1:32 ` [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver Zhi Mao
@ 2024-02-27 12:46   ` Andy Shevchenko
  2024-02-27 13:26     ` Sakari Ailus
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Andy Shevchenko @ 2024-02-27 12:46 UTC (permalink / raw)
  To: Zhi Mao
  Cc: mchehab, robh+dt, krzysztof.kozlowski+dt, sakari.ailus,
	laurent.pinchart, shengnan.wang, yaya.chang, 10572168,
	Project_Global_Chrome_Upstream_Group, yunkec, conor+dt,
	matthias.bgg, angelogioacchino.delregno, jacopo.mondi,
	hverkuil-cisco, heiko, jernej.skrabec, macromorgan, linus.walleij,
	hdegoede, tomi.valkeinen, gerald.loacker, bingbu.cao, dan.scally,
	linux-media, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

On Tue, Feb 27, 2024 at 3:33 AM Zhi Mao <zhi.mao@mediatek.com> wrote:
>
> Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor.

...

> +/*
> + * gc08a3.c - gc08a3 sensor driver

Drop the filename from the file, it's impractical (esp. if the file
will be renamed for some reason in the future).

> + *
> + * Copyright 2023 MediaTek
> + *
> + * Zhi Mao <zhi.mao@mediatek.com>
> + */

...

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>

This is a semi-random list of inclusions. Please, follow the IWYU
(Include What You Use) principle. *At least* this list misses the
following:  array_size.h, bits.h, container_of.h, device.h, err.h,
i2c.h, math64.h, module.h, mod_devicetable.h, property.h, types.h.

...

> +       /*update crop info to subdev state*/

Missing spaces.

...

> +       /*update fmt info to subdev state*/

Ditto.

...

> +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 pattern_menu)
> +{
> +       u32 pattern = 0;

No, please use the default case for this assignment.

> +       int ret;
> +
> +       if (pattern_menu) {
> +               switch (pattern_menu) {
> +               case 1:
> +                       pattern = 0x00;
> +                       break;
> +               case 2:
> +                       pattern = 0x10;
> +                       break;
> +               case 3:
> +               case 4:
> +               case 5:
> +               case 6:
> +               case 7:
> +                       pattern = pattern_menu + 1;
> +                       break;
> +               }
> +
> +               ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX,
> +                               pattern, NULL);
> +               if (ret)
> +                       return ret;
> +

> +               ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN,
> +                               GC08A3_TEST_PATTERN_EN, NULL);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN,
> +                               0x00, NULL);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;

'else' is redundant, but you can deduplicate code above with

  return cci_write(...);
} else {
  return cci_write(...);
}

Of course you can go even further, but I think with the above it will
be balanced to the way that it's easy to understand how branches
behave ('else' in this case helps to indent semantically coupled
lines).

> +}


> +       /*
> +        * Applying V4L2 control value only happens
> +        * when power is on for streaming
> +        */

Respect English grammar and punctuation, i.e. don't forget periods at
the end of sentences in multi-line comments.

...

> +       endpoint =
> +               fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> +                                               FWNODE_GRAPH_ENDPOINT_NEXT);
> +       if (!endpoint) {

Strictly speaking dev_fwnode(dev) might be NULL or an error pointer. I
dunno how the graph is implemented there and if it's possible to get
an error pointer out of it. At least this probably needs to be aligned
there at some point.

> +               dev_err(dev, "endpoint node not found\n");
> +               return -EINVAL;
> +       }

...

> +static const struct dev_pm_ops gc08a3_pm_ops = {
> +       RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL)
> +};

There is a DEFINE_* PM macro, use it.

...

> +

Redundant blank line.

> +module_i2c_driver(gc08a3_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-27 12:46   ` Andy Shevchenko
@ 2024-02-27 13:26     ` Sakari Ailus
  2024-02-28  3:19     ` Zhi Mao (毛智)
  2024-03-03  2:46     ` Zhi Mao (毛智)
  2 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2024-02-27 13:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Zhi Mao, mchehab, robh+dt, krzysztof.kozlowski+dt,
	laurent.pinchart, shengnan.wang, yaya.chang, 10572168,
	Project_Global_Chrome_Upstream_Group, yunkec, conor+dt,
	matthias.bgg, angelogioacchino.delregno, jacopo.mondi,
	hverkuil-cisco, heiko, jernej.skrabec, macromorgan, linus.walleij,
	hdegoede, tomi.valkeinen, gerald.loacker, bingbu.cao, dan.scally,
	linux-media, devicetree, linux-kernel, linux-arm-kernel,
	linux-mediatek

Hi Andy,

On Tue, Feb 27, 2024 at 02:46:54PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 3:33 AM Zhi Mao <zhi.mao@mediatek.com> wrote:
> >
> > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor.
> 
> ...
> 
> > +/*
> > + * gc08a3.c - gc08a3 sensor driver
> 
> Drop the filename from the file, it's impractical (esp. if the file
> will be renamed for some reason in the future).
> 
> > + *
> > + * Copyright 2023 MediaTek

You could update the year.

...

> 
> > +       endpoint =
> > +               fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> > +                                               FWNODE_GRAPH_ENDPOINT_NEXT);
> > +       if (!endpoint) {
> 
> Strictly speaking dev_fwnode(dev) might be NULL or an error pointer. I
> dunno how the graph is implemented there and if it's possible to get
> an error pointer out of it. At least this probably needs to be aligned
> there at some point.

This is fine---the fwnode API returns errors (for functions that can) for
NULL or error pointer fwnodes.

The patches are in my tree already, please post a patch on top of this that
I can squash.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-27 12:46   ` Andy Shevchenko
  2024-02-27 13:26     ` Sakari Ailus
@ 2024-02-28  3:19     ` Zhi Mao (毛智)
  2024-02-28  9:22       ` Andy Shevchenko
  2024-03-03  2:46     ` Zhi Mao (毛智)
  2 siblings, 1 reply; 12+ messages in thread
From: Zhi Mao (毛智) @ 2024-02-28  3:19 UTC (permalink / raw)
  To: andy.shevchenko@gmail.com
  Cc: heiko@sntech.de, tomi.valkeinen@ideasonboard.com,
	robh+dt@kernel.org, yunkec@chromium.org,
	linux-kernel@vger.kernel.org, dan.scally@ideasonboard.com,
	gerald.loacker@wolfvision.net,
	Shengnan Wang (王圣男), hdegoede@redhat.com,
	linux-media@vger.kernel.org, linus.walleij@linaro.org,
	linux-arm-kernel@lists.infradead.org,
	Yaya Chang (張雅清), mchehab@kernel.org,
	jacopo.mondi@ideasonboard.com, jernej.skrabec@gmail.com,
	linux-mediatek@lists.infradead.org, bingbu.cao@intel.com,
	sakari.ailus@linux.intel.com,
	Project_Global_Chrome_Upstream_Group, 10572168@qq.com,
	conor+dt@kernel.org, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	laurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,
	angelogioacchino.delregno@collabora.com, macromorgan@hotmail.com

Hi Andy,

Thanks for your review.

On Tue, 2024-02-27 at 14:46 +0200, Andy Shevchenko wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Tue, Feb 27, 2024 at 3:33 AM Zhi Mao <zhi.mao@mediatek.com>
> wrote:
> >
> > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor.
> 
> ...
> 
> 
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/units.h>
> 
> This is a semi-random list of inclusions. Please, follow the IWYU
> (Include What You Use) principle. *At least* this list misses the
> following:  array_size.h, bits.h, container_of.h, device.h, err.h,
> i2c.h, math64.h, module.h, mod_devicetable.h, property.h, types.h.
> 
> ...
After checking these header files, we find:
1."container_of.h" is included by "media-entity.h", while "media-
entity.h" is inculded by "v4l2-subdev.h"
2."device.h" is included by v4l2-dev.h, while "v4l2-dev.h" is inculded
by "v4l2-subdev.h"
3."types.h" is inclded by "v4l2-cci.h"
4.remaining header files which mentioned above are also similar, they
by some other header files
I think we just need include those necessary header files(such as
"v4l2-subdev.h"/"v4l2-cci.h"...) in sensor driver. 
If we add these header files which mentioned above, I am afraid it will
cause "repeated inclusion header file" issue.
As this driver code can be compiled pass, so there is not miss any
header files.
Another, I also reviewed some other sensor driver code(such as
gc0a08/gc2145 and imx/ov), they are all the same.
Can we keep this coding style and follow with most of those 
image sensor driver?

> 
> ...
> 
> > +static const struct dev_pm_ops gc08a3_pm_ops = {
> > +       RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL)
> > +};
> 
> There is a DEFINE_* PM macro, use it.
> 
static DEFINE_SIMPLE_DEV_PM_OPS(gc08a3_pm_ops,
				gc08a3_power_off,
				gc08a3_power_on);
Do you mean, we shoule use this Macro? 
> ...
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-28  3:19     ` Zhi Mao (毛智)
@ 2024-02-28  9:22       ` Andy Shevchenko
  2024-02-28 12:02         ` sakari.ailus
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2024-02-28  9:22 UTC (permalink / raw)
  To: Zhi Mao (毛智)
  Cc: heiko@sntech.de, tomi.valkeinen@ideasonboard.com,
	robh+dt@kernel.org, yunkec@chromium.org,
	linux-kernel@vger.kernel.org, dan.scally@ideasonboard.com,
	gerald.loacker@wolfvision.net,
	Shengnan Wang (王圣男), hdegoede@redhat.com,
	linux-media@vger.kernel.org, linus.walleij@linaro.org,
	linux-arm-kernel@lists.infradead.org,
	Yaya Chang (張雅清), mchehab@kernel.org,
	jacopo.mondi@ideasonboard.com, jernej.skrabec@gmail.com,
	linux-mediatek@lists.infradead.org, bingbu.cao@intel.com,
	sakari.ailus@linux.intel.com,
	Project_Global_Chrome_Upstream_Group, 10572168@qq.com,
	conor+dt@kernel.org, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	laurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,
	angelogioacchino.delregno@collabora.com, macromorgan@hotmail.com

On Wed, Feb 28, 2024 at 5:19 AM Zhi Mao (毛智) <zhi.mao@mediatek.com> wrote:
> On Tue, 2024-02-27 at 14:46 +0200, Andy Shevchenko wrote:
> >  On Tue, Feb 27, 2024 at 3:33 AM Zhi Mao <zhi.mao@mediatek.com>
> > wrote:

...

> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/units.h>
> >
> > This is a semi-random list of inclusions. Please, follow the IWYU
> > (Include What You Use) principle. *At least* this list misses the
> > following:  array_size.h, bits.h, container_of.h, device.h, err.h,
> > i2c.h, math64.h, module.h, mod_devicetable.h, property.h, types.h.

> After checking these header files, we find:
> 1."container_of.h" is included by "media-entity.h", while "media-
> entity.h" is inculded by "v4l2-subdev.h"
> 2."device.h" is included by v4l2-dev.h, while "v4l2-dev.h" is inculded
> by "v4l2-subdev.h"
> 3."types.h" is inclded by "v4l2-cci.h"
> 4.remaining header files which mentioned above are also similar, they
> by some other header files
> I think we just need include those necessary header files(such as
> "v4l2-subdev.h"/"v4l2-cci.h"...) in sensor driver.

That's bad. You should read again what the IWYU principle means.

> If we add these header files which mentioned above, I am afraid it will
> cause "repeated inclusion header file" issue.

How would it be possible? Are some headers broken?

> As this driver code can be compiled pass, so there is not miss any
> header files.

There are _a lot_ of missing headers. Your compilation works by luck.

> Another, I also reviewed some other sensor driver code(such as
> gc0a08/gc2145 and imx/ov), they are all the same.

They are also problematic. So what?

> Can we keep this coding style and follow with most of those
> image sensor driver?

Why? We do NOT want to continue developers to avoid decreasing their
technical debts.

...

> > > +static const struct dev_pm_ops gc08a3_pm_ops = {
> > > +       RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL)
> > > +};
> >
> > There is a DEFINE_* PM macro, use it.
> >
> static DEFINE_SIMPLE_DEV_PM_OPS(gc08a3_pm_ops,
> gc08a3_power_off,
> gc08a3_power_on);
> Do you mean, we shoule use this Macro?

It depends on what type of sleep you want to provide. There are
different types of DEFINE_*() macros in the PM headers.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-28  9:22       ` Andy Shevchenko
@ 2024-02-28 12:02         ` sakari.ailus
  2024-02-28 17:17           ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: sakari.ailus @ 2024-02-28 12:02 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Zhi Mao (毛智), heiko@sntech.de,
	tomi.valkeinen@ideasonboard.com, robh+dt@kernel.org,
	yunkec@chromium.org, linux-kernel@vger.kernel.org,
	dan.scally@ideasonboard.com, gerald.loacker@wolfvision.net,
	Shengnan Wang (王圣男), hdegoede@redhat.com,
	linux-media@vger.kernel.org, linus.walleij@linaro.org,
	linux-arm-kernel@lists.infradead.org,
	Yaya Chang (張雅清), mchehab@kernel.org,
	jacopo.mondi@ideasonboard.com, jernej.skrabec@gmail.com,
	linux-mediatek@lists.infradead.org, bingbu.cao@intel.com,
	Project_Global_Chrome_Upstream_Group, 10572168@qq.com,
	conor+dt@kernel.org, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	laurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,
	angelogioacchino.delregno@collabora.com, macromorgan@hotmail.com

Hi Andy, Zhi,

On Wed, Feb 28, 2024 at 11:22:13AM +0200, Andy Shevchenko wrote:
> > Another, I also reviewed some other sensor driver code(such as
> > gc0a08/gc2145 and imx/ov), they are all the same.
> 
> They are also problematic. So what?

The situation is the same in a large number of drivers, also outside the
media tree. It's also not trivial to figure out which header should be
included and it tends to be that if it compiles without warnings,
developers won't bother trying to figure out what should still be changed.

I wonder if this could be automated by using the C pre-processor and a
small Perl script. :-)

> > Can we keep this coding style and follow with most of those
> > image sensor driver?
> 
> Why? We do NOT want to continue developers to avoid decreasing their
> technical debts.

I agree. People tend to copy the code from existing drivers and argue "it's
just a driver, why bother?". That code will be soon found in a dozen other
new driver patchsets and every time must brought up in review and fixed,
leading to extra work for reviewers and developers. We've had quite a lot
of work improving the current (sensor etc.) drivers and fixing up the use
of bad patterns but this area has so far received little attention.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-28 12:02         ` sakari.ailus
@ 2024-02-28 17:17           ` Andy Shevchenko
  2024-02-28 17:33             ` Kieran Bingham
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2024-02-28 17:17 UTC (permalink / raw)
  To: sakari.ailus@linux.intel.com
  Cc: Zhi Mao (毛智), heiko@sntech.de,
	tomi.valkeinen@ideasonboard.com, robh+dt@kernel.org,
	yunkec@chromium.org, linux-kernel@vger.kernel.org,
	dan.scally@ideasonboard.com, gerald.loacker@wolfvision.net,
	Shengnan Wang (王圣男), hdegoede@redhat.com,
	linux-media@vger.kernel.org, linus.walleij@linaro.org,
	linux-arm-kernel@lists.infradead.org,
	Yaya Chang (張雅清), mchehab@kernel.org,
	jacopo.mondi@ideasonboard.com, jernej.skrabec@gmail.com,
	linux-mediatek@lists.infradead.org, bingbu.cao@intel.com,
	Project_Global_Chrome_Upstream_Group, 10572168@qq.com,
	conor+dt@kernel.org, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	laurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,
	angelogioacchino.delregno@collabora.com, macromorgan@hotmail.com

On Wed, Feb 28, 2024 at 2:03 PM sakari.ailus@linux.intel.com
<sakari.ailus@linux.intel.com> wrote:
> On Wed, Feb 28, 2024 at 11:22:13AM +0200, Andy Shevchenko wrote:
> > > Another, I also reviewed some other sensor driver code(such as
> > > gc0a08/gc2145 and imx/ov), they are all the same.
> >
> > They are also problematic. So what?
>
> The situation is the same in a large number of drivers, also outside the
> media tree. It's also not trivial to figure out which header should be
> included and it tends to be that if it compiles without warnings,
> developers won't bother trying to figure out what should still be changed.

I spent about 20 minutes reading the code. Yes, for inexperienced
developers it can take 3x longer, but still, much longer time was
spent on writing that code to begin with. So, the headers are a part
of the evolution of the code. And now it's pure technical debt with no
excuses to not fulfill.

TL;DR: It's (rightfully) assumed that the developer *knows* their code.

> I wonder if this could be automated by using the C preprocessor and a
> small Perl script. :-)

There is a Google project coming from Clang people or so, but I have
no idea of the state of affairs.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-28 17:17           ` Andy Shevchenko
@ 2024-02-28 17:33             ` Kieran Bingham
  2024-02-28 18:06               ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Kieran Bingham @ 2024-02-28 17:33 UTC (permalink / raw)
  To: Andy Shevchenko, sakari.ailus; +Cc: Zhi Mao 毛智, heiko

Quoting Andy Shevchenko (2024-02-28 17:17:12)
> On Wed, Feb 28, 2024 at 2:03 PM sakari.ailus@linux.intel.com
> <sakari.ailus@linux.intel.com> wrote:
> > On Wed, Feb 28, 2024 at 11:22:13AM +0200, Andy Shevchenko wrote:
> > > > Another, I also reviewed some other sensor driver code(such as
> > > > gc0a08/gc2145 and imx/ov), they are all the same.
> > >
> > > They are also problematic. So what?
> >
> > The situation is the same in a large number of drivers, also outside the
> > media tree. It's also not trivial to figure out which header should be
> > included and it tends to be that if it compiles without warnings,
> > developers won't bother trying to figure out what should still be changed.
> 
> I spent about 20 minutes reading the code. Yes, for inexperienced
> developers it can take 3x longer, but still, much longer time was
> spent on writing that code to begin with. So, the headers are a part
> of the evolution of the code. And now it's pure technical debt with no
> excuses to not fulfill.
> 
> TL;DR: It's (rightfully) assumed that the developer *knows* their code.
> 
> > I wonder if this could be automated by using the C preprocessor and a
> > small Perl script. :-)
> 
> There is a Google project coming from Clang people or so, but I have
> no idea of the state of affairs.

I've used iwyu (include-what-you-use) successfully on libcamera. But I
don't know how this would translate to working on the kernel, with a
more complex build system.

But indeed, tooling already exists [0] to do these checks, and is packaged
in distros [1]

[0] https://include-what-you-use.org/
[1] https://manpages.ubuntu.com/manpages/focal/man1/iwyu_tool.1.html

--
Kieran


> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-28 17:33             ` Kieran Bingham
@ 2024-02-28 18:06               ` Andy Shevchenko
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2024-02-28 18:06 UTC (permalink / raw)
  To: Kieran Bingham, Jonathan Cameron
  Cc: sakari.ailus, Zhi Mao 毛智, heiko@sntech.de,
	tomi.valkeinen@ideasonboard.com, robh+dt@kernel.org,
	yunkec@chromium.org, linux-kernel@vger.kernel.org,
	dan.scally@ideasonboard.com, gerald.loacker@wolfvision.net,
	Shengnan Wang 王圣男, hdegoede@redhat.com,
	linux-media@vger.kernel.org, linus.walleij@linaro.org,
	linux-arm-kernel@lists.infradead.org,
	Yaya Chang 張雅清, mchehab@kernel.org,
	jacopo.mondi@ideasonboard.com, jernej.skrabec@gmail.com,
	linux-mediatek@lists.infradead.org, bingbu.cao@intel.com,
	Project_Global_Chrome_Upstream_Group, 10572168@qq.com,
	conor+dt@kernel.org, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	laurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,
	angelogioacchino.delregno@collabora.com, macromorgan@hotmail.com

On Wed, Feb 28, 2024 at 7:34 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> Quoting Andy Shevchenko (2024-02-28 17:17:12)
> > On Wed, Feb 28, 2024 at 2:03 PM sakari.ailus@linux.intel.com
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Wed, Feb 28, 2024 at 11:22:13AM +0200, Andy Shevchenko wrote:
> > > > > Another, I also reviewed some other sensor driver code(such as
> > > > > gc0a08/gc2145 and imx/ov), they are all the same.
> > > >
> > > > They are also problematic. So what?
> > >
> > > The situation is the same in a large number of drivers, also outside the
> > > media tree. It's also not trivial to figure out which header should be
> > > included and it tends to be that if it compiles without warnings,
> > > developers won't bother trying to figure out what should still be changed.
> >
> > I spent about 20 minutes reading the code. Yes, for inexperienced
> > developers it can take 3x longer, but still, much longer time was
> > spent on writing that code to begin with. So, the headers are a part
> > of the evolution of the code. And now it's pure technical debt with no
> > excuses to not fulfill.
> >
> > TL;DR: It's (rightfully) assumed that the developer *knows* their code.
> >
> > > I wonder if this could be automated by using the C preprocessor and a
> > > small Perl script. :-)
> >
> > There is a Google project coming from Clang people or so, but I have
> > no idea of the state of affairs.
>
> I've used iwyu (include-what-you-use) successfully on libcamera. But I
> don't know how this would translate to working on the kernel, with a
> more complex build system.

Jonathan tried that once. It needs to be heavily tweaked to be used in
the kernel project.

> But indeed, tooling already exists [0] to do these checks, and is packaged
> in distros [1]
>
> [0] https://include-what-you-use.org/
> [1] https://manpages.ubuntu.com/manpages/focal/man1/iwyu_tool.1.html

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver
  2024-02-27 12:46   ` Andy Shevchenko
  2024-02-27 13:26     ` Sakari Ailus
  2024-02-28  3:19     ` Zhi Mao (毛智)
@ 2024-03-03  2:46     ` Zhi Mao (毛智)
  2 siblings, 0 replies; 12+ messages in thread
From: Zhi Mao (毛智) @ 2024-03-03  2:46 UTC (permalink / raw)
  To: andy.shevchenko@gmail.com
  Cc: heiko@sntech.de, tomi.valkeinen@ideasonboard.com,
	robh+dt@kernel.org, yunkec@chromium.org,
	linux-kernel@vger.kernel.org, dan.scally@ideasonboard.com,
	gerald.loacker@wolfvision.net,
	Shengnan Wang (王圣男), hdegoede@redhat.com,
	linux-media@vger.kernel.org, linus.walleij@linaro.org,
	linux-arm-kernel@lists.infradead.org,
	Yaya Chang (張雅清), mchehab@kernel.org,
	jacopo.mondi@ideasonboard.com, jernej.skrabec@gmail.com,
	linux-mediatek@lists.infradead.org, bingbu.cao@intel.com,
	sakari.ailus@linux.intel.com,
	Project_Global_Chrome_Upstream_Group, 10572168@qq.com,
	conor+dt@kernel.org, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	laurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,
	angelogioacchino.delregno@collabora.com, macromorgan@hotmail.com

Hi Andy,

Thanks for your review.


On Tue, 2024-02-27 at 14:46 +0200, Andy Shevchenko wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Tue, Feb 27, 2024 at 3:33 AM Zhi Mao <zhi.mao@mediatek.com>
> wrote:
> >
> > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor.
> 
> ...
> 
> > +/*
> > + * gc08a3.c - gc08a3 sensor driver
> 
> Drop the filename from the file, it's impractical (esp. if the file
> will be renamed for some reason in the future).
> 
fixed in patch:v7.

> > + *
> > + * Copyright 2023 MediaTek
> > + *
> > + * Zhi Mao <zhi.mao@mediatek.com>
> > + */
> 
> ...
> 
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/units.h>
> 
> This is a semi-random list of inclusions. Please, follow the IWYU
> (Include What You Use) principle. *At least* this list misses the
> following:  array_size.h, bits.h, container_of.h, device.h, err.h,
> i2c.h, math64.h, module.h, mod_devicetable.h, property.h, types.h.
> 
fixed in patch:v7.

> ...
> 
> > +       /*update crop info to subdev state*/
> 
> Missing spaces.
> 
fixed in patch:v7.

> ...
> 
> > +       /*update fmt info to subdev state*/
> 
> Ditto.
> 
fixed in patch:v7.

> ...
> 
> > +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32
> pattern_menu)
> > +{
> > +       u32 pattern = 0;
> 
> No, please use the default case for this assignment.
> 
fixed in patch:v7.

> > +       int ret;
> > +
> > +       if (pattern_menu) {
> > +               switch (pattern_menu) {
> > +               case 1:
> > +                       pattern = 0x00;
> > +                       break;
> > +               case 2:
> > +                       pattern = 0x10;
> > +                       break;
> > +               case 3:
> > +               case 4:
> > +               case 5:
> > +               case 6:
> > +               case 7:
> > +                       pattern = pattern_menu + 1;
> > +                       break;
> > +               }
> > +
> > +               ret = cci_write(gc08a3->regmap,
> GC08A3_REG_TEST_PATTERN_IDX,
> > +                               pattern, NULL);
> > +               if (ret)
> > +                       return ret;
> > +
> 
> > +               ret = cci_write(gc08a3->regmap,
> GC08A3_REG_TEST_PATTERN_EN,
> > +                               GC08A3_TEST_PATTERN_EN, NULL);
> > +               if (ret)
> > +                       return ret;
> > +       } else {
> > +               ret = cci_write(gc08a3->regmap,
> GC08A3_REG_TEST_PATTERN_EN,
> > +                               0x00, NULL);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> 
> 'else' is redundant, but you can deduplicate code above with
> 
>   return cci_write(...);
> } else {
>   return cci_write(...);
> }
> 
> Of course you can go even further, but I think with the above it will
> be balanced to the way that it's easy to understand how branches
> behave ('else' in this case helps to indent semantically coupled
> lines).
> 
fixed in patch:v7.

> > +}
> 
> 
> > +       /*
> > +        * Applying V4L2 control value only happens
> > +        * when power is on for streaming
> > +        */
> 
> Respect English grammar and punctuation, i.e. don't forget periods at
> the end of sentences in multi-line comments.
> 
fixed in patch:v7.

> ...
> 
> > +       endpoint =
> > +               fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0,
> 0,
> >
> +                                               FWNODE_GRAPH_ENDPOINT
> _NEXT);
> > +       if (!endpoint) {
> 
> Strictly speaking dev_fwnode(dev) might be NULL or an error pointer.
> I
> dunno how the graph is implemented there and if it's possible to get
> an error pointer out of it. At least this probably needs to be
> aligned
> there at some point.
> 
Mr.Sakari has explained this comments.
"This is fine---the fwnode API returns errors (for functions that can)
for NULL or error pointer fwnodes."

> > +               dev_err(dev, "endpoint node not found\n");
> > +               return -EINVAL;
> > +       }
> 
> ...
> 
> > +static const struct dev_pm_ops gc08a3_pm_ops = {
> > +       RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL)
> > +};
> 
> There is a DEFINE_* PM macro, use it.
> 
fixed in patch:v7.

> ...
> 
> > +
> 
> Redundant blank line.
> 
fixed in patch:v7.

> > +module_i2c_driver(gc08a3_i2c_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-03-03  2:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-27  1:32 [PATCH v6 0/2] media: i2c: Add support for GC08A3 sensor Zhi Mao
2024-02-27  1:32 ` [PATCH v6 1/2] media: dt-bindings: i2c: add GalaxyCore GC08A3 image sensor Zhi Mao
2024-02-27  1:32 ` [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver Zhi Mao
2024-02-27 12:46   ` Andy Shevchenko
2024-02-27 13:26     ` Sakari Ailus
2024-02-28  3:19     ` Zhi Mao (毛智)
2024-02-28  9:22       ` Andy Shevchenko
2024-02-28 12:02         ` sakari.ailus
2024-02-28 17:17           ` Andy Shevchenko
2024-02-28 17:33             ` Kieran Bingham
2024-02-28 18:06               ` Andy Shevchenko
2024-03-03  2:46     ` Zhi Mao (毛智)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).