Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Wed, 27 Sep 2023 15:54:09 +0200
From:   Jacopo Mondi <jacopo.mondi@ideasonboard.com>
To:     Alain Volmat <alain.volmat@foss.st.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Andrey Skvortsov <andrej.skvortzov@gmail.com>,
        Rob Herring <robh@kernel.org>, linux-media@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] media: i2c: gc2145: Galaxy Core GC2145 sensor support
Message-ID: <c2jflrljuoiudmqqjzthumwxf3fpeepr2zh4alvunoyo4hmfn2@eapdhi43qilj>
References: <20230926092825.819229-1-alain.volmat@foss.st.com>
 <20230926092825.819229-3-alain.volmat@foss.st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230926092825.819229-3-alain.volmat@foss.st.com>
Precedence: bulk

Hi Alain

On Tue, Sep 26, 2023 at 11:28:20AM +0200, Alain Volmat wrote:
> Addition of support for the Galaxy Core GC2145 XVGA sensor.
> The sensor supports both DVP and CSI-2 interfaces however for
> the time being only CSI-2 is implemented.
>
> Configurations is currently based on initialization scripts
> coming from Galaxy Core and for that purpose only 3 static
> resolutions are supported with static framerates.
>  - 640x480 (30fps)
>  - 1280x720 (30fps)
>  - 1600x1200 (20fps)
>
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> ---
>  MAINTAINERS                |    8 +
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/gc2145.c | 1591 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 1612 insertions(+)
>  create mode 100644 drivers/media/i2c/gc2145.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..c595335812c7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8600,6 +8600,14 @@ F:	kernel/futex/*
>  F:	tools/perf/bench/futex*
>  F:	tools/testing/selftests/futex/
>
> +GALAXYCORE GC2145 SENSOR DRIVER
> +M:	Alain Volmat <alain.volmat@foss.st.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git
> +F:	Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> +F:	drivers/media/i2c/gc2145.c
> +
>  GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER
>  M:	Tim Harvey <tharvey@gateworks.com>
>  S:	Maintained
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 74ff833ff48c..78e5c8cbf916 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -50,6 +50,18 @@ config VIDEO_AR0521
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ar0521.
>
> +config VIDEO_GC2145
> +	tristate "GalaxyCore GC2145 sensor support"
> +	depends on I2C && VIDEO_DEV
> +	select VIDEO_V4L2_SUBDEV_API
> +	select V4L2_FWNODE

The top-level menu already:

	select V4L2_FWNODE
	select VIDEO_V4L2_SUBDEV_API

you can drop these two.

> +	help
> +	  This is a V4L2 sensor-level driver for GalaxyCore GC2145
> +	  2 Mpixel camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called gc2145.
> +
>  config VIDEO_HI556
>  	tristate "Hynix Hi-556 sensor support"
>  	help
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 80b00d39b48f..e74eded89428 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_VIDEO_DW9719) += dw9719.o
>  obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
>  obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
>  obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
> +obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
>  obj-$(CONFIG_VIDEO_HI556) += hi556.o
>  obj-$(CONFIG_VIDEO_HI846) += hi846.o
>  obj-$(CONFIG_VIDEO_HI847) += hi847.o
> diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
> new file mode 100644
> index 000000000000..d0b065011732
> --- /dev/null
> +++ b/drivers/media/i2c/gc2145.c
> @@ -0,0 +1,1591 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * A V4L2 driver for Galaxycore GC2145 camera.
> + * Copyright (C) 2022, STMicroelectronics SA

2023 ?

> + *
> + * Inspired from the imx219.c driver
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>

I would have added an empy line here. Up to you

> +#include <media/mipi-csi2.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mediabus.h>
> +
> +/* Chip ID */
> +#define GC2145_CHIP_ID		0x2145
> +
> +/* Page 0 */
> +#define GC2145_REG_HBLANK_HIGH	0x05
> +#define GC2145_REG_HBLANK_LOW	0x06
> +#define GC2145_REG_VBLANK_HIGH	0x07
> +#define GC2145_REG_VBLANK_LOW	0x08
> +#define GC2145_REG_ROW_START_HIGH	0x09
> +#define GC2145_REG_ROW_START_LOW	0x0a
> +#define GC2145_REG_COL_START_HIGH	0x0b
> +#define GC2145_REG_COL_START_LOW	0x0c
> +#define GC2145_REG_WIN_HEIGHT_HIGH	0x0d
> +#define GC2145_REG_WIN_HEIGHT_LOW	0x0e
> +#define GC2145_REG_WIN_WIDTH_HIGH	0x0f
> +#define GC2145_REG_WIN_WIDTH_LOW	0x10
> +#define GC2145_REG_ANALOG_MODE1	0x17
> +#define GC2145_REG_OUTPUT_FMT	0x84
> +#define GC2145_REG_SYNC_MODE	0x86
> +#define GC2145_SYNC_MODE_COL_SWITCH	BIT(4)
> +#define GC2145_SYNC_MODE_ROW_SWITCH	BIT(5)
> +#define GC2145_REG_DEBUG_MODE2	0x8c
> +#define GC2145_REG_DEBUG_MODE3	0x8d
> +#define GC2145_REG_CROP_ENABLE	0x90
> +#define GC2145_REG_CROP_Y_HIGH	0x91
> +#define GC2145_REG_CROP_Y_LOW	0x92
> +#define GC2145_REG_CROP_X_HIGH	0x93
> +#define GC2145_REG_CROP_X_LOW	0x94
> +#define GC2145_REG_CROP_HEIGHT_HIGH	0x95
> +#define GC2145_REG_CROP_HEIGHT_LOW	0x96
> +#define GC2145_REG_CROP_WIDTH_HIGH	0x97
> +#define GC2145_REG_CROP_WIDTH_LOW	0x98
> +#define GC2145_REG_CHIP_ID	0xf0
> +#define GC2145_REG_PAD_IO	0xf2
> +#define GC2145_REG_PAGE_SELECT	0xfe
> +/* Page 3 */
> +#define GC2145_REG_FIFO_FULL_LVL_LOW	0x04
> +#define GC2145_REG_FIFO_FULL_LVL_HIGH	0x05
> +#define GC2145_REG_MIPI_DT	0x11
> +#define GC2145_REG_LWC_LOW	0x12
> +#define GC2145_REG_LWC_HIGH	0x13
> +#define GC2145_REG_FIFO_GATE_MODE	0x17
> +
> +/* External clock frequency is 24.0MHz */
> +#define GC2145_XCLK_FREQ	(24 * HZ_PER_MHZ)
> +
> +#define GC2145_NATIVE_WIDTH	1616U
> +#define GC2145_NATIVE_HEIGHT	1232U
> +
> +struct gc2145_reg {
> +	unsigned char address;
> +	unsigned char val;
> +};
> +
> +struct gc2145_reg_list {
> +	unsigned int num_of_regs;
> +	const struct gc2145_reg *regs;
> +};
> +
> +/**
> + * struct gc2145_mode - GC2145 mode description
> + * @width: frame width (in pixel)
> + * @height: frame height (in pixel)
> + * @frame_interval: interval (fractionnal) between 2 frames
> + * @reg_list: registers config sequence to enter into the mode
> + * @pixel_rate: pixel_rate associated with the mode
> + * @crop: window area captured
> + * @hblank: horizontal blanking setting of this mode
> + */
> +struct gc2145_mode {
> +	unsigned int width;
> +	unsigned int height;
> +	struct v4l2_fract frame_interval;
> +	struct gc2145_reg_list reg_list;
> +	unsigned long pixel_rate;
> +	struct v4l2_rect crop;
> +	u32 hblank;
> +};
> +
> +static const struct gc2145_reg common_regs[] = {
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	/* SH Delay */
> +	{0x12, 0x2e},
> +	/* Flip */
> +	{GC2145_REG_ANALOG_MODE1, 0x14},
> +	/* Analog Conf */
> +	{0x18, 0x22}, {0x19, 0x0e}, {0x1a, 0x01}, {0x1b, 0x4b},
> +	{0x1c, 0x07}, {0x1d, 0x10}, {0x1e, 0x88}, {0x1f, 0x78},
> +	{0x20, 0x03}, {0x21, 0x40}, {0x22, 0xa0}, {0x24, 0x16},
> +	{0x25, 0x01}, {0x26, 0x10}, {0x2d, 0x60}, {0x30, 0x01},
> +	{0x31, 0x90}, {0x33, 0x06}, {0x34, 0x01},
> +	{0x80, 0x7f}, {0x81, 0x26}, {0x82, 0xfa}, {0x83, 0x00},
> +	{0x84, 0x02}, {0x86, 0x02}, {0x88, 0x03}, {0x89, 0x03},
> +	{0x85, 0x08}, {0x8a, 0x00}, {0x8b, 0x00}, {0xb0, 0x55},
> +	{0xc3, 0x00}, {0xc4, 0x80}, {0xc5, 0x90}, {0xc6, 0x3b},
> +	{0xc7, 0x46},
> +	/* BLK */
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	{0x40, 0x42}, {0x41, 0x00}, {0x43, 0x5b}, {0x5e, 0x00},
> +	{0x5f, 0x00}, {0x60, 0x00}, {0x61, 0x00}, {0x62, 0x00},
> +	{0x63, 0x00}, {0x64, 0x00}, {0x65, 0x00}, {0x66, 0x20},
> +	{0x67, 0x20}, {0x68, 0x20}, {0x69, 0x20}, {0x76, 0x00},
> +	{0x6a, 0x08}, {0x6b, 0x08}, {0x6c, 0x08}, {0x6d, 0x08},
> +	{0x6e, 0x08}, {0x6f, 0x08}, {0x70, 0x08}, {0x71, 0x08},
> +	{0x76, 0x00}, {0x72, 0xf0}, {0x7e, 0x3c}, {0x7f, 0x00},
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0x48, 0x15}, {0x49, 0x00}, {0x4b, 0x0b},
> +	/* AEC */
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	{0x03, 0x04}, {0x04, 0xe2},
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0x01, 0x04}, {0x02, 0xc0}, {0x03, 0x04}, {0x04, 0x90},
> +	{0x05, 0x30}, {0x06, 0x90}, {0x07, 0x30}, {0x08, 0x80},
> +	{0x09, 0x00}, {0x0a, 0x82}, {0x0b, 0x11}, {0x0c, 0x10},
> +	{0x11, 0x10}, {0x13, 0x7b}, {0x17, 0x00}, {0x1c, 0x11},
> +	{0x1e, 0x61}, {0x1f, 0x35}, {0x20, 0x40}, {0x22, 0x40},
> +	{0x23, 0x20},
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0x0f, 0x04},
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0x12, 0x35}, {0x15, 0xb0}, {0x10, 0x31}, {0x3e, 0x28},
> +	{0x3f, 0xb0}, {0x40, 0x90}, {0x41, 0x0f},
> +	/* INTPEE */
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0x90, 0x6c}, {0x91, 0x03}, {0x92, 0xcb}, {0x94, 0x33},
> +	{0x95, 0x84}, {0x97, 0x65}, {0xa2, 0x11},
> +	/* DNDD */
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0x80, 0xc1}, {0x81, 0x08}, {0x82, 0x05}, {0x83, 0x08},
> +	{0x84, 0x0a}, {0x86, 0xf0}, {0x87, 0x50}, {0x88, 0x15},
> +	{0x89, 0xb0}, {0x8a, 0x30}, {0x8b, 0x10},
> +	/* ASDE */
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0x21, 0x04},
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0xa3, 0x50}, {0xa4, 0x20}, {0xa5, 0x40}, {0xa6, 0x80},
> +	{0xab, 0x40}, {0xae, 0x0c}, {0xb3, 0x46}, {0xb4, 0x64},
> +	{0xb6, 0x38}, {0xb7, 0x01}, {0xb9, 0x2b}, {0x3c, 0x04},
> +	{0x3d, 0x15}, {0x4b, 0x06}, {0x4c, 0x20},
> +	/* Gamma */
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0x10, 0x09}, {0x11, 0x0d}, {0x12, 0x13}, {0x13, 0x19},
> +	{0x14, 0x27}, {0x15, 0x37}, {0x16, 0x45}, {0x17, 0x53},
> +	{0x18, 0x69}, {0x19, 0x7d}, {0x1a, 0x8f}, {0x1b, 0x9d},
> +	{0x1c, 0xa9}, {0x1d, 0xbd}, {0x1e, 0xcd}, {0x1f, 0xd9},
> +	{0x20, 0xe3}, {0x21, 0xea}, {0x22, 0xef}, {0x23, 0xf5},
> +	{0x24, 0xf9}, {0x25, 0xff},
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	{0xc6, 0x20}, {0xc7, 0x2b},
> +	/* Gamma 2 */
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0x26, 0x0f}, {0x27, 0x14}, {0x28, 0x19}, {0x29, 0x1e},
> +	{0x2a, 0x27}, {0x2b, 0x33}, {0x2c, 0x3b}, {0x2d, 0x45},
> +	{0x2e, 0x59}, {0x2f, 0x69}, {0x30, 0x7c}, {0x31, 0x89},
> +	{0x32, 0x98}, {0x33, 0xae}, {0x34, 0xc0}, {0x35, 0xcf},
> +	{0x36, 0xda}, {0x37, 0xe2}, {0x38, 0xe9}, {0x39, 0xf3},
> +	{0x3a, 0xf9}, {0x3b, 0xff},
> +	/* YCP */
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0xd1, 0x32}, {0xd2, 0x32}, {0xd3, 0x40}, {0xd6, 0xf0},
> +	{0xd7, 0x10}, {0xd8, 0xda}, {0xdd, 0x14}, {0xde, 0x86},
> +	{0xed, 0x80}, {0xee, 0x00}, {0xef, 0x3f}, {0xd8, 0xd8},
> +	/* ABS */
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0x9f, 0x40},
> +	/* LSC */
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0xc2, 0x14}, {0xc3, 0x0d}, {0xc4, 0x0c}, {0xc8, 0x15},
> +	{0xc9, 0x0d}, {0xca, 0x0a}, {0xbc, 0x24}, {0xbd, 0x10},
> +	{0xbe, 0x0b}, {0xb6, 0x25}, {0xb7, 0x16}, {0xb8, 0x15},
> +	{0xc5, 0x00}, {0xc6, 0x00}, {0xc7, 0x00}, {0xcb, 0x00},
> +	{0xcc, 0x00}, {0xcd, 0x00}, {0xbf, 0x07}, {0xc0, 0x00},
> +	{0xc1, 0x00}, {0xb9, 0x00}, {0xba, 0x00}, {0xbb, 0x00},
> +	{0xaa, 0x01}, {0xab, 0x01}, {0xac, 0x00}, {0xad, 0x05},
> +	{0xae, 0x06}, {0xaf, 0x0e}, {0xb0, 0x0b}, {0xb1, 0x07},
> +	{0xb2, 0x06}, {0xb3, 0x17}, {0xb4, 0x0e}, {0xb5, 0x0e},
> +	{0xd0, 0x09}, {0xd1, 0x00}, {0xd2, 0x00}, {0xd6, 0x08},
> +	{0xd7, 0x00}, {0xd8, 0x00}, {0xd9, 0x00}, {0xda, 0x00},
> +	{0xdb, 0x00}, {0xd3, 0x0a}, {0xd4, 0x00}, {0xd5, 0x00},
> +	{0xa4, 0x00}, {0xa5, 0x00}, {0xa6, 0x77}, {0xa7, 0x77},
> +	{0xa8, 0x77}, {0xa9, 0x77}, {0xa1, 0x80}, {0xa2, 0x80},
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0xdf, 0x0d}, {0xdc, 0x25}, {0xdd, 0x30}, {0xe0, 0x77},
> +	{0xe1, 0x80}, {0xe2, 0x77}, {0xe3, 0x90}, {0xe6, 0x90},
> +	{0xe7, 0xa0}, {0xe8, 0x90}, {0xe9, 0xa0},
> +	/* AWB */
> +	/* measure window */
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	{0xec, 0x06}, {0xed, 0x04}, {0xee, 0x60}, {0xef, 0x90},
> +	{0xb6, 0x01},
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0x4f, 0x00}, {0x4f, 0x00}, {0x4b, 0x01}, {0x4f, 0x00},
> +	{0x4c, 0x01}, {0x4d, 0x71}, {0x4e, 0x01},
> +	{0x4c, 0x01}, {0x4d, 0x91}, {0x4e, 0x01},
> +	{0x4c, 0x01}, {0x4d, 0x70}, {0x4e, 0x01},
> +	{0x4c, 0x01}, {0x4d, 0x90}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0xb0}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0x8f}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0x6f}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0xaf}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0xd0}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0xf0}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0xcf}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0xef}, {0x4e, 0x02},
> +	{0x4c, 0x01}, {0x4d, 0x6e}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x8e}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xae}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xce}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x4d}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x6d}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x8d}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xad}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xcd}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x4c}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x6c}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x8c}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xac}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xcc}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xcb}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x4b}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x6b}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x8b}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0xab}, {0x4e, 0x03},
> +	{0x4c, 0x01}, {0x4d, 0x8a}, {0x4e, 0x04},
> +	{0x4c, 0x01}, {0x4d, 0xaa}, {0x4e, 0x04},
> +	{0x4c, 0x01}, {0x4d, 0xca}, {0x4e, 0x04},
> +	{0x4c, 0x01}, {0x4d, 0xca}, {0x4e, 0x04},
> +	{0x4c, 0x01}, {0x4d, 0xc9}, {0x4e, 0x04},
> +	{0x4c, 0x01}, {0x4d, 0x8a}, {0x4e, 0x04},
> +	{0x4c, 0x01}, {0x4d, 0x89}, {0x4e, 0x04},
> +	{0x4c, 0x01}, {0x4d, 0xa9}, {0x4e, 0x04},
> +	{0x4c, 0x02}, {0x4d, 0x0b}, {0x4e, 0x05},
> +	{0x4c, 0x02}, {0x4d, 0x0a}, {0x4e, 0x05},
> +	{0x4c, 0x01}, {0x4d, 0xeb}, {0x4e, 0x05},
> +	{0x4c, 0x01}, {0x4d, 0xea}, {0x4e, 0x05},
> +	{0x4c, 0x02}, {0x4d, 0x09}, {0x4e, 0x05},
> +	{0x4c, 0x02}, {0x4d, 0x29}, {0x4e, 0x05},
> +	{0x4c, 0x02}, {0x4d, 0x2a}, {0x4e, 0x05},
> +	{0x4c, 0x02}, {0x4d, 0x4a}, {0x4e, 0x05},
> +	{0x4c, 0x02}, {0x4d, 0x8a}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0x49}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0x69}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0x89}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0xa9}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0x48}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0x68}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0x69}, {0x4e, 0x06},
> +	{0x4c, 0x02}, {0x4d, 0xca}, {0x4e, 0x07},
> +	{0x4c, 0x02}, {0x4d, 0xc9}, {0x4e, 0x07},
> +	{0x4c, 0x02}, {0x4d, 0xe9}, {0x4e, 0x07},
> +	{0x4c, 0x03}, {0x4d, 0x09}, {0x4e, 0x07},
> +	{0x4c, 0x02}, {0x4d, 0xc8}, {0x4e, 0x07},
> +	{0x4c, 0x02}, {0x4d, 0xe8}, {0x4e, 0x07},
> +	{0x4c, 0x02}, {0x4d, 0xa7}, {0x4e, 0x07},
> +	{0x4c, 0x02}, {0x4d, 0xc7}, {0x4e, 0x07},
> +	{0x4c, 0x02}, {0x4d, 0xe7}, {0x4e, 0x07},
> +	{0x4c, 0x03}, {0x4d, 0x07}, {0x4e, 0x07},
> +	{0x4f, 0x01},
> +	{0x50, 0x80}, {0x51, 0xa8}, {0x52, 0x47}, {0x53, 0x38},
> +	{0x54, 0xc7}, {0x56, 0x0e}, {0x58, 0x08}, {0x5b, 0x00},
> +	{0x5c, 0x74}, {0x5d, 0x8b}, {0x61, 0xdb}, {0x62, 0xb8},
> +	{0x63, 0x86}, {0x64, 0xc0}, {0x65, 0x04}, {0x67, 0xa8},
> +	{0x68, 0xb0}, {0x69, 0x00}, {0x6a, 0xa8}, {0x6b, 0xb0},
> +	{0x6c, 0xaf}, {0x6d, 0x8b}, {0x6e, 0x50}, {0x6f, 0x18},
> +	{0x73, 0xf0}, {0x70, 0x0d}, {0x71, 0x60}, {0x72, 0x80},
> +	{0x74, 0x01}, {0x75, 0x01}, {0x7f, 0x0c}, {0x76, 0x70},
> +	{0x77, 0x58}, {0x78, 0xa0}, {0x79, 0x5e}, {0x7a, 0x54},
> +	{0x7b, 0x58},
> +	/* CC */
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0xc0, 0x01}, {0xc1, 0x44}, {0xc2, 0xfd}, {0xc3, 0x04},
> +	{0xc4, 0xf0}, {0xc5, 0x48}, {0xc6, 0xfd}, {0xc7, 0x46},
> +	{0xc8, 0xfd}, {0xc9, 0x02}, {0xca, 0xe0}, {0xcb, 0x45},
> +	{0xcc, 0xec}, {0xcd, 0x48}, {0xce, 0xf0}, {0xcf, 0xf0},
> +	{0xe3, 0x0c}, {0xe4, 0x4b}, {0xe5, 0xe0},
> +	/* ABS */
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	{0x9f, 0x40},
> +	/* Dark sun */
> +	{GC2145_REG_PAGE_SELECT, 0x02},
> +	{0x40, 0xbf}, {0x46, 0xcf},
> +};
> +
> +#define GC2145_640_480_PIXELRATE	(60 * HZ_PER_MHZ)
> +static const struct gc2145_reg mode_640_480_regs[] = {
> +	{GC2145_REG_PAGE_SELECT, 0xf0}, {GC2145_REG_PAGE_SELECT, 0xf0},
> +	{GC2145_REG_PAGE_SELECT, 0xf0}, {0xfc, 0x06},
> +	{0xf6, 0x00}, {0xf7, 0x1d}, {0xf8, 0x86}, {0xfa, 0x00},
> +	{0xf9, 0x8e},
> +	/* Disable PAD IO */
> +	{GC2145_REG_PAD_IO, 0x00},
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	/* Row/Col start - 0/0 */
> +	{GC2145_REG_ROW_START_HIGH, 0x00}, {GC2145_REG_ROW_START_LOW, 0x00},
> +	{GC2145_REG_COL_START_HIGH, 0x00}, {GC2145_REG_COL_START_LOW, 0x00},
> +	/* Window size 1216/1618 */
> +	{GC2145_REG_WIN_HEIGHT_HIGH, 0x04}, {GC2145_REG_WIN_HEIGHT_LOW, 0xc0},
> +	{GC2145_REG_WIN_WIDTH_HIGH, 0x06}, {GC2145_REG_WIN_WIDTH_LOW, 0x52},
> +	/* Scalar more */
> +	{0xfd, 0x01}, {0xfa, 0x00},
> +	/* Crop 640-480@0-0 */
> +	{GC2145_REG_CROP_ENABLE, 0x01},
> +	{GC2145_REG_CROP_Y_HIGH, 0x00}, {GC2145_REG_CROP_Y_LOW, 0x00},
> +	{GC2145_REG_CROP_X_HIGH, 0x00}, {GC2145_REG_CROP_X_LOW, 0x00},
> +	{GC2145_REG_CROP_HEIGHT_HIGH, 0x01}, {GC2145_REG_CROP_HEIGHT_LOW, 0xe0},
> +	{GC2145_REG_CROP_WIDTH_HIGH, 0x02}, {GC2145_REG_CROP_WIDTH_LOW, 0x80},
> +	/* Subsampling configuration */
> +	{0x99, 0x55}, {0x9a, 0x06}, {0x9b, 0x01},
> +	{0x9c, 0x23}, {0x9d, 0x00}, {0x9e, 0x00}, {0x9f, 0x01},
> +	{0xa0, 0x23}, {0xa1, 0x00}, {0xa2, 0x00},
> +	/* Framerate */
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	{GC2145_REG_HBLANK_HIGH, 0x01}, {GC2145_REG_HBLANK_LOW, 0x30},
> +	{GC2145_REG_VBLANK_HIGH, 0x00}, {GC2145_REG_VBLANK_LOW, 0x0c},
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	/* AEC anti-flicker */
> +	{0x25, 0x01}, {0x26, 0x75},
> +	/* AEC exposure level 1-5 */
> +	{0x27, 0x04}, {0x28, 0x5f}, {0x29, 0x04}, {0x2a, 0x5f},
> +	{0x2b, 0x04}, {0x2c, 0x5f}, {0x2d, 0x04}, {0x2e, 0x5f},
> +};
> +
> +#define GC2145_1280_720_PIXELRATE	(96 * HZ_PER_MHZ)
> +static const struct gc2145_reg mode_1280_720_regs[] = {
> +	{GC2145_REG_PAGE_SELECT, 0xf0}, {GC2145_REG_PAGE_SELECT, 0xf0},
> +	{GC2145_REG_PAGE_SELECT, 0xf0}, {0xfc, 0x06},
> +	{0xf6, 0x00}, {0xf7, 0x1d}, {0xf8, 0x83}, {0xfa, 0x00},
> +	{0xf9, 0x8e},
> +	/* Disable PAD IO */
> +	{GC2145_REG_PAD_IO, 0x00},
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	/* Row/Col start - 240/160 */
> +	{GC2145_REG_ROW_START_HIGH, 0x00}, {GC2145_REG_ROW_START_LOW, 0xf0},
> +	{GC2145_REG_COL_START_HIGH, 0x00}, {GC2145_REG_COL_START_LOW, 0xa0},
> +	/* Window size 736/1296 */
> +	{GC2145_REG_WIN_HEIGHT_HIGH, 0x02}, {GC2145_REG_WIN_HEIGHT_LOW, 0xe0},
> +	{GC2145_REG_WIN_WIDTH_HIGH, 0x05}, {GC2145_REG_WIN_WIDTH_LOW, 0x10},
> +	/* Crop 1280-720@0-0 */
> +	{GC2145_REG_CROP_ENABLE, 0x01},
> +	{GC2145_REG_CROP_Y_HIGH, 0x00}, {GC2145_REG_CROP_Y_LOW, 0x00},
> +	{GC2145_REG_CROP_X_HIGH, 0x00}, {GC2145_REG_CROP_X_LOW, 0x00},
> +	{GC2145_REG_CROP_HEIGHT_HIGH, 0x02}, {GC2145_REG_CROP_HEIGHT_LOW, 0xd0},
> +	{GC2145_REG_CROP_WIDTH_HIGH, 0x05}, {GC2145_REG_CROP_WIDTH_LOW, 0x00},
> +	/* Framerate */
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	{GC2145_REG_HBLANK_HIGH, 0x01}, {GC2145_REG_HBLANK_LOW, 0x56},
> +	{GC2145_REG_VBLANK_HIGH, 0x00}, {GC2145_REG_VBLANK_LOW, 0x11},
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	/* AEC anti-flicker */
> +	{0x25, 0x00}, {0x26, 0xe6},
> +	/* AEC exposure level 1-5 */
> +	{0x27, 0x02}, {0x28, 0xb2}, {0x29, 0x02}, {0x2a, 0xb2},
> +	{0x2b, 0x02}, {0x2c, 0xb2}, {0x2d, 0x02}, {0x2e, 0xb2},
> +};
> +
> +#define GC2145_1600_1200_PIXELRATE	(72 * HZ_PER_MHZ)
> +static const struct gc2145_reg mode_1600_1200_regs[] = {
> +	{GC2145_REG_PAGE_SELECT, 0xf0}, {GC2145_REG_PAGE_SELECT, 0xf0},
> +	{GC2145_REG_PAGE_SELECT, 0xf0}, {0xfc, 0x06},
> +	{0xf6, 0x00}, {0xf7, 0x1d}, {0xf8, 0x84}, {0xfa, 0x00},
> +	{0xf9, 0x8e},
> +	/* Disable PAD IO */
> +	{GC2145_REG_PAD_IO, 0x00},
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	/* Row/Col start - 0/0 */
> +	{GC2145_REG_ROW_START_HIGH, 0x00}, {GC2145_REG_ROW_START_LOW, 0x00},
> +	{GC2145_REG_COL_START_HIGH, 0x00}, {GC2145_REG_COL_START_LOW, 0x00},
> +	/* Window size: 1216/1618 */
> +	{GC2145_REG_WIN_HEIGHT_HIGH, 0x04}, {GC2145_REG_WIN_HEIGHT_LOW, 0xc0},
> +	{GC2145_REG_WIN_WIDTH_HIGH, 0x06}, {GC2145_REG_WIN_WIDTH_LOW, 0x52},
> +	/* Crop 1600-1200@0-0 */
> +	{GC2145_REG_CROP_ENABLE, 0x01},
> +	{GC2145_REG_CROP_Y_HIGH, 0x00}, {GC2145_REG_CROP_Y_LOW, 0x00},
> +	{GC2145_REG_CROP_X_HIGH, 0x00}, {GC2145_REG_CROP_X_LOW, 0x00},
> +	{GC2145_REG_CROP_HEIGHT_HIGH, 0x04}, {GC2145_REG_CROP_HEIGHT_LOW, 0xb0},
> +	{GC2145_REG_CROP_WIDTH_HIGH, 0x06}, {GC2145_REG_CROP_WIDTH_LOW, 0x40},
> +	/* Framerate */
> +	{GC2145_REG_PAGE_SELECT, 0x00},
> +	{GC2145_REG_HBLANK_HIGH, 0x01}, {GC2145_REG_HBLANK_LOW, 0x56},
> +	{GC2145_REG_VBLANK_HIGH, 0x00}, {GC2145_REG_VBLANK_LOW, 0x10},
> +	{GC2145_REG_PAGE_SELECT, 0x01},
> +	/* AEC anti-flicker */
> +	{0x25, 0x00}, {0x26, 0xfa},
> +	/* AEC exposure level 1-5 */
> +	{0x27, 0x04}, {0x28, 0xe2}, {0x29, 0x04}, {0x2a, 0xe2},
> +	{0x2b, 0x04}, {0x2c, 0xe2}, {0x2d, 0x04}, {0x2e, 0xe2},
> +};
> +
> +/* Regulators supplies */
> +static const char * const gc2145_supply_name[] = {
> +	"IOVDD", /* Digital I/O (1.7-3V) suppply */
> +	"AVDD",  /* Analog (2.7-3V) supply */
> +	"DVDD",  /* Digital Core (1.7-1.9V) supply */
> +};
> +
> +#define GC2145_NUM_SUPPLIES ARRAY_SIZE(gc2145_supply_name)
> +
> +/* Mode configs */
> +#define GC2145_MODE_640X480	0
> +#define GC2145_MODE_1280X720	1
> +#define GC2145_MODE_1600X1200	2
> +static const struct gc2145_mode supported_modes[] = {
> +	{
> +		/* 640x480 30fps mode */
> +		.width = 640,
> +		.height = 480,
> +		.frame_interval = {
> +			.numerator = 1,
> +			.denominator = 30,
> +		},
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> +			.regs = mode_640_480_regs,
> +		},
> +		.pixel_rate = GC2145_640_480_PIXELRATE,
> +		.crop = {
> +			.top = 0,
> +			.left = 0,
> +			.width = 640,
> +			.height = 480,
> +		},
> +		.hblank = 0x130,
> +	},
> +	{
> +		/* 1280x720 30fps mode */
> +		.width = 1280,
> +		.height = 720,
> +		.frame_interval = {
> +			.numerator = 1,
> +			.denominator = 30,
> +		},
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280_720_regs),
> +			.regs = mode_1280_720_regs,
> +		},
> +		.pixel_rate = GC2145_1280_720_PIXELRATE,
> +		.crop = {
> +			.top = 160,
> +			.left = 240,
> +			.width = 1280,
> +			.height = 720,
> +		},
> +		.hblank = 0x156,
> +	},
> +	{
> +		/* 1600x1200 20fps mode */
> +		.width = 1600,
> +		.height = 1200,
> +		.frame_interval = {
> +			.numerator = 1,
> +			.denominator = 20,
> +		},
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1600_1200_regs),
> +			.regs = mode_1600_1200_regs,
> +		},
> +		.pixel_rate = GC2145_1600_1200_PIXELRATE,
> +		.crop = {
> +			.top = 0,
> +			.left = 0,
> +			.width = 1600,
> +			.height = 1200,
> +		},
> +		.hblank = 0x156,
> +	},
> +};
> +
> +/**
> + * struct gc2145_format - GC2145 pixel format description
> + * @code: media bus (MBUS) associated code
> + * @colorspace: V4L2 colospace
> + * @datatype: MIPI CSI2 data type
> + * @output_fmt: GC2145 output format
> + * @row_col_switch: control of GC2145 row/col switch feature
> + */
> +struct gc2145_format {
> +	unsigned int code;
> +	unsigned int colorspace;
> +	unsigned char datatype;
> +	unsigned char output_fmt;
> +	unsigned char row_col_switch;

Never initialized in the supported_formats[] list. Is this intentional ?

> +};
> +
> +/* All supported formats */
> +#define GC2145_DEFAULT_MBUS_FORMAT	MEDIA_BUS_FMT_YUYV8_2X8

Seems like this is not used and you use MEDIA_BUS_FMT_RGB565_2X8_BE
in init_cfg()

> +static const struct gc2145_format supported_formats[] = {
> +	{
> +		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.datatype	= MIPI_CSI2_DT_YUV422_8B,
> +		.output_fmt	= 0x00,
> +	},
> +	{
> +		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.datatype	= MIPI_CSI2_DT_YUV422_8B,
> +		.output_fmt	= 0x01,
> +	},
> +	{
> +		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.datatype	= MIPI_CSI2_DT_YUV422_8B,
> +		.output_fmt	= 0x02,
> +	},
> +	{
> +		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.datatype	= MIPI_CSI2_DT_YUV422_8B,
> +		.output_fmt	= 0x03,
> +	},
> +	{
> +		.code		= MEDIA_BUS_FMT_RGB565_2X8_BE,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.datatype	= MIPI_CSI2_DT_RGB565,
> +		.output_fmt	= 0x06,
> +	},
> +};
> +
> +struct gc2145_ctrls {
> +	struct v4l2_ctrl_handler handler;
> +	struct v4l2_ctrl *pixel_rate;
> +	struct v4l2_ctrl *hblank;
> +	struct v4l2_ctrl *test_pattern;
> +	struct v4l2_ctrl *hflip;
> +	struct v4l2_ctrl *vflip;
> +};
> +
> +struct gc2145 {
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +
> +	struct clk *xclk;
> +
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *powerdown_gpio;
> +	struct regulator_bulk_data supplies[GC2145_NUM_SUPPLIES];
> +
> +	/* V4L2 controls */
> +	struct gc2145_ctrls ctrls;
> +
> +	/* Current mode */
> +	const struct gc2145_mode *mode;
> +
> +	/* Streaming on/off */
> +	bool streaming;
> +};
> +
> +static inline struct gc2145 *to_gc2145(struct v4l2_subdev *_sd)
> +{
> +	return container_of(_sd, struct gc2145, sd);
> +}
> +
> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of(ctrl->handler, struct gc2145,
> +			     ctrls.handler)->sd;
> +}
> +
> +static int gc2145_read_reg(struct gc2145 *gc2145, u8 addr, u8 *data,
> +			   int data_size)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +	struct i2c_msg msg[2];
> +	int ret;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = client->flags;
> +	msg[0].buf = &addr;
> +	msg[0].len = 1;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = client->flags | I2C_M_RD;
> +	msg[1].buf = data;
> +	msg[1].len = data_size;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "%s: error %d: start_index=%x, data_size=%d\n",
> +			__func__, ret, (u32)addr, data_size);
> +		return ret;
> +	}
> +
> +	dev_dbg(&client->dev, "[rd %02x] => %*ph\n", (u32)addr, data_size,
> +		data);
> +
> +	return 0;
> +}
> +
> +static int gc2145_write_reg(struct gc2145 *gc2145, u8 addr, u8 data)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +	struct i2c_msg msg;
> +	u8 buf[] = { addr, data };
> +	int ret;
> +
> +	msg.addr = client->addr;
> +	msg.flags = client->flags;
> +	msg.buf = buf;
> +	msg.len = sizeof(buf);
> +
> +	dev_dbg(&client->dev, "[wr %02x] <= %02xh\n", (u32)addr, data);
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "%s: error %d: addr=%x, data=%02xh\n",
> +			__func__, ret, (u32)addr, data);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int gc2145_mod_reg(struct gc2145 *gc2145, u16 reg, u8 mask, u8 val)
> +{
> +	u8 readval;
> +	int ret;
> +
> +	ret = gc2145_read_reg(gc2145, reg, &readval, 1);
> +	if (ret)
> +		return ret;
> +
> +	readval &= ~mask;
> +	val &= mask;
> +	val |= readval;
> +
> +	return gc2145_write_reg(gc2145, reg, val);
> +}
> +
> +/* Write a list of registers */
> +static int gc2145_write_regs(struct gc2145 *gc2145,
> +			     const struct gc2145_reg *regs, u32 len)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < len; i++) {
> +		ret = gc2145_write_reg(gc2145, regs[i].address, regs[i].val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

From a quick look at the documentation, this sensor seems to be
compatible with MIPI CCI. The newly merged CCI helpers would provide
read/write functions for you. If possible I would suggest to use them

> +
> +static const struct gc2145_format *
> +gc2145_get_format_code(struct gc2145 *gc2145, u32 code)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
> +		if (supported_formats[i].code == code)
> +			break;
> +
> +	if (i >= ARRAY_SIZE(supported_formats))
> +		i = 0;
> +
> +	return &supported_formats[i];
> +}
> +
> +static void gc2145_update_pad_format(struct gc2145 *gc2145,
> +				     const struct gc2145_mode *mode,
> +				     struct v4l2_mbus_framefmt *fmt, u32 code)
> +{
> +	fmt->code = code;
> +	fmt->width = mode->width;
> +	fmt->height = mode->height;
> +	fmt->field = V4L2_FIELD_NONE;
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, fmt->colorspace,
> +							  fmt->ycbcr_enc);
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +}
> +
> +static int gc2145_init_cfg(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_state *state)
> +{
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_rect *crop;
> +
> +	/* Initialize try_fmt */
> +	format = v4l2_subdev_get_pad_format(sd, state, 0);
> +	gc2145_update_pad_format(gc2145,
> +				 &supported_modes[0], format,
> +				 MEDIA_BUS_FMT_RGB565_2X8_BE);
> +
> +	/* Initialize crop rectangle. */
> +	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> +	*crop = supported_modes[0].crop;
> +
> +	return 0;
> +}
> +
> +static int gc2145_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP: {
> +		sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> +		return 0;
> +	}

Do you need {} ?

> +
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = GC2145_NATIVE_WIDTH;
> +		sel->r.height = GC2145_NATIVE_HEIGHT;
> +
> +		return 0;
> +
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = 1600;
> +		sel->r.height = 1200;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int gc2145_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *sd_state,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index >= ARRAY_SIZE(supported_formats))
> +		return -EINVAL;
> +
> +	code->code = supported_formats[code->index].code;
> +	return 0;
> +}
> +
> +static int gc2145_enum_frame_size(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *sd_state,
> +				  struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	const struct gc2145_format *gc2145_format;
> +	u32 code;
> +
> +	if (fse->index >= ARRAY_SIZE(supported_modes))
> +		return -EINVAL;
> +
> +	gc2145_format = gc2145_get_format_code(gc2145, fse->code);
> +	code = gc2145_format->code;
> +	if (fse->code != code)
> +		return -EINVAL;

Is this correct ? Should you check if fse->code matches the currently
applied format code or should you just check if fse->code is supported
at al ? I recall the API doc was not super-clear about this, am I
wrong ?

> +
> +	fse->min_width = supported_modes[fse->index].width;
> +	fse->max_width = fse->min_width;
> +	fse->min_height = supported_modes[fse->index].height;
> +	fse->max_height = fse->min_height;
> +
> +	return 0;
> +}
> +
> +static int
> +gc2145_enum_frame_interval(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_state *sd_state,
> +			   struct v4l2_subdev_frame_interval_enum *fie)
> +{
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	const struct gc2145_format *gc2145_format;
> +	u32 code, i;
> +
> +	/* Only supporting a unique framerate per resolution currently */
> +	if (fie->index > 0)
> +		return -EINVAL;
> +
> +	gc2145_format = gc2145_get_format_code(gc2145, fie->code);
> +	code = gc2145_format->code;
> +	if (fie->code != code)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
> +		if (supported_modes[i].width == fie->width &&
> +		    supported_modes[i].height == fie->height)
> +			break;
> +
> +	if (i >= ARRAY_SIZE(supported_modes))
> +		return -EINVAL;
> +
> +	fie->interval.numerator = supported_modes[i].frame_interval.numerator;
> +	fie->interval.denominator =
> +		supported_modes[i].frame_interval.denominator;
> +
> +	return 0;
> +}
> +
> +static int gc2145_set_pad_format(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *sd_state,
> +				 struct v4l2_subdev_format *fmt)
> +{
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	const struct gc2145_mode *mode;
> +	const struct gc2145_format *gc2145_fmt;
> +	struct v4l2_mbus_framefmt *framefmt;
> +	struct gc2145_ctrls *ctrls = &gc2145->ctrls;
> +	struct v4l2_rect *crop;
> +
> +	gc2145_fmt = gc2145_get_format_code(gc2145, fmt->format.code);
> +	mode = v4l2_find_nearest_size(supported_modes,
> +				      ARRAY_SIZE(supported_modes),
> +				      width, height,
> +				      fmt->format.width, fmt->format.height);
> +
> +	/* In RAW mode, VGA is not possible so use 720p instead */
> +	if (gc2145_fmt->colorspace == V4L2_COLORSPACE_RAW &&
> +	    mode == &supported_modes[GC2145_MODE_640X480])
> +		mode = &supported_modes[GC2145_MODE_1280X720];
> +
> +	gc2145_update_pad_format(gc2145, mode, &fmt->format, gc2145_fmt->code);
> +	framefmt = v4l2_subdev_get_pad_format(sd, sd_state, fmt->pad);
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> +	    (gc2145->mode != mode || framefmt->code != fmt->format.code)) {

For consistency see  6cb042908b3e ("media: i2c: imx219: Perform a full
mode set unconditionally")

> +		gc2145->mode = mode;
> +		/* Update pixel_rate based on the mode */
> +		__v4l2_ctrl_s_ctrl_int64(ctrls->pixel_rate, mode->pixel_rate);
> +		/* Update hblank based on the mode */
> +		__v4l2_ctrl_modify_range(ctrls->hblank, mode->hblank,
> +					 mode->hblank, 1, mode->hblank);
> +		__v4l2_ctrl_s_ctrl(ctrls->hblank, mode->hblank);
> +	}
> +	*framefmt = fmt->format;
> +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, fmt->pad);
> +	*crop = mode->crop;
> +
> +	return 0;
> +}
> +
> +static const struct gc2145_reg common_mipi_regs[] = {
> +	{GC2145_REG_PAGE_SELECT, 0x03},
> +	{0x01, 0x87}, {0x02, 0x22}, {0x03, 0x10},
> +	{0x06, 0x88}, {0x10, 0x95}, {0x15, 0x10}, {0x22, 0x04},
> +	{0x23, 0x10}, {0x24, 0x10}, {0x25, 0x10}, {0x26, 0x05},
> +	{0x21, 0x10}, {0x29, 0x03}, {0x2a, 0x0a}, {0x2b, 0x06},

I supposed these are undocumented in your documentation as well, right ?


> +};
> +
> +static int gc2145_config_mipi_mode(struct gc2145 *gc2145,
> +				   const struct gc2145_format *gc2145_format)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +	u16 lwc, fifo_full_lvl, fifo_gate_mode;
> +	int ret;
> +
> +	/* Common MIPI settings */
> +	ret = gc2145_write_regs(gc2145, common_mipi_regs,
> +				ARRAY_SIZE(common_mipi_regs));
> +	if (ret) {
> +		dev_err(&client->dev, "%s failed to set mode - MIPI\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Adjust the MIPI buffer settings.
> +	 * For YUV/RGB, LWC = image width * 2
> +	 * For RAW8, LWC = image width
> +	 * For RAW10, LWC = image width * 1.25

The multiplication factor is then just bpp expressed in bytes, right ?

> +	 */
> +	if (gc2145_format->colorspace != V4L2_COLORSPACE_RAW)
> +		lwc = gc2145->mode->width * 2;
> +	else if (gc2145_format->code == MEDIA_BUS_FMT_SGRBG8_1X8 ||
> +		 gc2145_format->code == MEDIA_BUS_FMT_SRGGB8_1X8 ||
> +		 gc2145_format->code == MEDIA_BUS_FMT_SBGGR8_1X8 ||
> +		 gc2145_format->code == MEDIA_BUS_FMT_SGBRG8_1X8)
> +		lwc = gc2145->mode->width * 1;

The driver does not support RAW formats, right ?

> +	else
> +		lwc = gc2145->mode->width + (gc2145->mode->width / 4);
> +
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_LWC_HIGH, lwc >> 8);
> +	if (ret)
> +		return ret;
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_LWC_LOW, (lwc & 0xff));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Adjust the MIPI Fifo Full Level
> +	 * 640x480 RGB: 0x0190
> +	 * 1280x720 / 1600x1200 (aka no scaler) non RAW: 0x0001
> +	 * 1600x1200 RAW: 0x0190
> +	 */
> +	if (gc2145_format->colorspace != V4L2_COLORSPACE_RAW) {

Ditto, no RAW formats are supported. This applies to all other uses of
V4L2_COLORSPACE_RAW in the driver

> +		if (gc2145->mode->width == 1280 || gc2145->mode->width == 1600)
> +			fifo_full_lvl = 0x0001;
> +		else
> +			fifo_full_lvl = 0x0190;
> +	} else {
> +		fifo_full_lvl = 0x0190;
> +	}
> +
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_FIFO_FULL_LVL_HIGH,
> +			       fifo_full_lvl >> 8);
> +	if (ret)
> +		return ret;
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_FIFO_FULL_LVL_LOW,
> +			       fifo_full_lvl & 0xff);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the fifo gate mode / MIPI wdiv set */
> +	if (gc2145_format->colorspace == V4L2_COLORSPACE_RAW)
> +		fifo_gate_mode = 0xf1;
> +	else
> +		fifo_gate_mode = 0xf0;
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_FIFO_GATE_MODE,
> +			       fifo_gate_mode);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the MIPI data type */
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_MIPI_DT,
> +			       gc2145_format->datatype);
> +	return ret;
> +}
> +
> +static int gc2145_start_streaming(struct gc2145 *gc2145,
> +				  struct v4l2_subdev_state *state)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +	const struct gc2145_format *gc2145_format;
> +	struct v4l2_mbus_framefmt *fmt;
> +	u8 sync_mode;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&client->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * TODO - configuration of the sensor will have to be moved into
> +	 * gc2145_set_pad_format once we have a way to configure without
> +	 * starting the sensor.  In such case, only streaming start will
> +	 * be done here.
> +	 */

Are you sure ? It seems correct to me to update the sensor
configuration before starting the streaming, isn't it ?

> +
> +	/* Apply default values of current mode */
> +	ret = gc2145_write_regs(gc2145, gc2145->mode->reg_list.regs,
> +				gc2145->mode->reg_list.num_of_regs);
> +	if (ret) {
> +		dev_err(&client->dev, "%s failed to set mode - pre ISP\n",
> +			__func__);
> +		goto err_rpm_put;
> +	}
> +
> +	ret = gc2145_write_regs(gc2145, common_regs,
> +				ARRAY_SIZE(common_regs));
> +	if (ret) {
> +		dev_err(&client->dev, "%s failed to set common regs\n",
> +			__func__);
> +		goto err_rpm_put;
> +	}
> +
> +	fmt = v4l2_subdev_get_pad_format(&gc2145->sd, state, 0);
> +	gc2145_format = gc2145_get_format_code(gc2145, fmt->code);
> +
> +	/* Set the output format */
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_PAGE_SELECT, 0x00);
> +	if (ret)
> +		return ret;
> +
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_OUTPUT_FMT,
> +			       gc2145_format->output_fmt);
> +	if (ret)
> +		return ret;
> +
> +	ret = gc2145_read_reg(gc2145, GC2145_REG_SYNC_MODE, &sync_mode, 1);
> +	if (ret)
> +		return ret;
> +
> +	sync_mode &= ~(GC2145_SYNC_MODE_COL_SWITCH |
> +		       GC2145_SYNC_MODE_ROW_SWITCH);
> +	sync_mode |= gc2145_format->row_col_switch;

As said, this is fixed to 0. Did you get what COL/ROW_SWITCH is meant
to be used for ?

> +
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_SYNC_MODE, sync_mode);
> +	if (ret)
> +		return ret;
> +
> +	/* Perform MIPI specific configuration */
> +	ret = gc2145_config_mipi_mode(gc2145, gc2145_format);
> +	if (ret)
> +		return ret;
> +
> +	/* Come back on page 0 by default */
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_PAGE_SELECT, 0x00);
> +	if (ret)
> +		return ret;
> +
> +	/* Apply customized values from user */
> +	ret =  __v4l2_ctrl_handler_setup(&gc2145->ctrls.handler);
> +	if (ret)
> +		goto err_rpm_put;
> +
> +	return 0;
> +
> +err_rpm_put:
> +	pm_runtime_put(&client->dev);
> +	return ret;
> +}
> +
> +static void gc2145_stop_streaming(struct gc2145 *gc2145)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +
> +	/*
> +	 * TODO - once we have a way to turn off only streaming of the
> +	 * sensor, we will have to do it here.

Right, what register write "starts" the streaming ?

> +	 */
> +
> +	pm_runtime_put(&client->dev);
> +}
> +
> +static int gc2145_g_frame_interval(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +
> +	fi->interval = gc2145->mode->frame_interval;
> +
> +	return 0;
> +}
> +
> +static int gc2145_set_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> +	if (gc2145->streaming == enable)
> +		goto unlock;
> +
> +	if (enable) {
> +		/*
> +		 * Apply default & customized values
> +		 * and then start streaming.
> +		 */
> +		ret = gc2145_start_streaming(gc2145, state);
> +		if (ret)
> +			goto unlock;
> +	} else {
> +		gc2145_stop_streaming(gc2145);
> +	}
> +
> +	gc2145->streaming = enable;
> +
> +unlock:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
> +/* Power/clock management functions */
> +static int gc2145_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(GC2145_NUM_SUPPLIES, gc2145->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(gc2145->xclk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		goto reg_off;
> +	}
> +
> +	/* TODO - Following delays should be tuned */
> +	usleep_range(10000, 12000);

wow, 10 to 12 msec ?

The datasheet version I have reports "TBD" as suggested delays :/


> +	gpiod_set_value_cansleep(gc2145->powerdown_gpio, 0);
> +	usleep_range(10000, 12000);
> +	gpiod_set_value_cansleep(gc2145->reset_gpio, 0);
> +	usleep_range(40000, 50000);
> +
> +	return 0;
> +
> +reg_off:
> +	regulator_bulk_disable(GC2145_NUM_SUPPLIES, gc2145->supplies);
> +
> +	return ret;
> +}
> +
> +static int gc2145_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +
> +	gpiod_set_value_cansleep(gc2145->powerdown_gpio, 1);
> +	gpiod_set_value_cansleep(gc2145->reset_gpio, 1);
> +	regulator_bulk_disable(GC2145_NUM_SUPPLIES, gc2145->supplies);
> +	clk_disable_unprepare(gc2145->xclk);

7.2.2 Power Off Sequence shows clock being stopped before disabling
regulators.

> +
> +	return 0;
> +}
> +
> +static int gc2145_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +
> +	if (gc2145->streaming)
> +		gc2145_stop_streaming(gc2145);
> +
> +	return 0;
> +}
> +
> +static int gc2145_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	struct v4l2_subdev_state *state;
> +	int ret;
> +
> +	if (gc2145->streaming) {
> +		state = v4l2_subdev_lock_and_get_active_state(sd);
> +		ret = gc2145_start_streaming(gc2145, state);
> +		v4l2_subdev_unlock_state(state);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	gc2145_stop_streaming(gc2145);
> +	gc2145->streaming = false;
> +
> +	return ret;
> +}
> +
> +static int gc2145_get_regulators(struct gc2145 *gc2145)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +	unsigned int i;
> +
> +	for (i = 0; i < GC2145_NUM_SUPPLIES; i++)
> +		gc2145->supplies[i].supply = gc2145_supply_name[i];
> +
> +	return devm_regulator_bulk_get(&client->dev, GC2145_NUM_SUPPLIES,
> +				       gc2145->supplies);
> +}
> +
> +/* Verify chip ID */
> +static int gc2145_identify_module(struct gc2145 *gc2145)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +	int ret;
> +	u16 chip_id;
> +
> +	ret = gc2145_read_reg(gc2145, GC2145_REG_CHIP_ID, (u8 *)&chip_id, 2);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read chip id (%d)\n", ret);
> +		return ret;
> +	}
> +	chip_id = be16_to_cpu(chip_id);
> +
> +	if (chip_id != GC2145_CHIP_ID) {
> +		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
> +			GC2145_CHIP_ID, chip_id);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static const char * const test_pattern_menu[] = {
> +	"Disabled",
> +	"Colored patterns",
> +	"Uniform white",
> +	"Uniform yellow",
> +	"Uniform cyan",
> +	"Uniform green",
> +	"Uniform magenta",
> +	"Uniform red",
> +	"Uniform black",
> +};
> +
> +#define GC2145_TEST_PATTERN_ENABLE	BIT(0)
> +#define GC2145_TEST_PATTERN_UXGA	BIT(3)
> +
> +#define GC2145_TEST_UNIFORM		BIT(3)
> +#define GC2145_TEST_WHITE		(4 << 4)
> +#define GC2145_TEST_YELLOW		(8 << 4)
> +#define GC2145_TEST_CYAN		(9 << 4)
> +#define GC2145_TEST_GREEN		(6 << 4)
> +#define GC2145_TEST_MAGENTA		(10 << 4)
> +#define GC2145_TEST_RED			(5 << 4)
> +#define GC2145_TEST_BLACK		(0)
> +
> +static const u8 test_pattern_val[] = {
> +	0,
> +	GC2145_TEST_PATTERN_ENABLE,
> +	GC2145_TEST_UNIFORM | GC2145_TEST_WHITE,
> +	GC2145_TEST_UNIFORM | GC2145_TEST_YELLOW,
> +	GC2145_TEST_UNIFORM | GC2145_TEST_CYAN,
> +	GC2145_TEST_UNIFORM | GC2145_TEST_GREEN,
> +	GC2145_TEST_UNIFORM | GC2145_TEST_MAGENTA,
> +	GC2145_TEST_UNIFORM | GC2145_TEST_RED,
> +	GC2145_TEST_UNIFORM | GC2145_TEST_BLACK,
> +};
> +
> +static const struct v4l2_subdev_core_ops gc2145_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_video_ops gc2145_video_ops = {
> +	.g_frame_interval = gc2145_g_frame_interval,
> +	/*
> +	 * Currently frame_interval can't be changed hence s_frame_interval
> +	 * does same as g_frame_interval
> +	 */
> +	.s_frame_interval = gc2145_g_frame_interval,
> +	.s_stream = gc2145_set_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops gc2145_pad_ops = {
> +	.init_cfg = gc2145_init_cfg,
> +	.enum_mbus_code = gc2145_enum_mbus_code,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.set_fmt = gc2145_set_pad_format,
> +	.get_selection = gc2145_get_selection,
> +	.enum_frame_size = gc2145_enum_frame_size,
> +	.enum_frame_interval = gc2145_enum_frame_interval,
> +};
> +
> +static const struct v4l2_subdev_ops gc2145_subdev_ops = {
> +	.core = &gc2145_core_ops,
> +	.video = &gc2145_video_ops,
> +	.pad = &gc2145_pad_ops,
> +};
> +
> +static int gc2145_set_ctrl_test_pattern(struct gc2145 *gc2145, int value)
> +{
> +	int ret;
> +
> +	if (!value) {
> +		/* Disable test pattern */
> +		ret = gc2145_write_reg(gc2145, GC2145_REG_DEBUG_MODE2, 0);
> +		if (ret)
> +			return ret;
> +
> +		return gc2145_write_reg(gc2145, GC2145_REG_DEBUG_MODE3, 0);
> +	}
> +
> +	/* Enable test pattern, colored or uniform */
> +	ret = gc2145_write_reg(gc2145, GC2145_REG_DEBUG_MODE2,
> +			       GC2145_TEST_PATTERN_ENABLE |
> +			       GC2145_TEST_PATTERN_UXGA);
> +	if (ret)
> +		return ret;
> +
> +	if (!(test_pattern_val[value] & GC2145_TEST_UNIFORM))
> +		return gc2145_write_reg(gc2145, GC2145_REG_DEBUG_MODE3, 0);
> +
> +	/* Uniform */
> +	return gc2145_write_reg(gc2145, GC2145_REG_DEBUG_MODE3,
> +				test_pattern_val[value]);
> +}
> +
> +static int gc2145_set_ctrl_hflip(struct gc2145 *gc2145, int value)
> +{
> +	return gc2145_mod_reg(gc2145, GC2145_REG_ANALOG_MODE1,
> +			      BIT(0), (value ? BIT(0) : 0));
> +}
> +
> +static int gc2145_set_ctrl_vflip(struct gc2145 *gc2145, int value)
> +{
> +	return gc2145_mod_reg(gc2145, GC2145_REG_ANALOG_MODE1,
> +			      BIT(1), (value ? BIT(1) : 0));
> +}
> +
> +static int gc2145_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&client->dev);

You shouldn't resume the sensor when setting controls but rather check
if the sensor is powered up with:

	if (pm_runtime_get_if_in_use(&client->dev) == 0)
		return 0;

and as you call  __v4l2_ctrl_handler_setup() already at s_stream(1)
time, controls will be applied before streaming start


> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = gc2145_set_ctrl_test_pattern(gc2145, ctrl->val);
> +		break;
> +	case V4L2_CID_HFLIP:
> +		ret = gc2145_set_ctrl_hflip(gc2145, ctrl->val);
> +		break;
> +	case V4L2_CID_VFLIP:
> +		ret = gc2145_set_ctrl_vflip(gc2145, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put(&client->dev);
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops gc2145_ctrl_ops = {
> +	.s_ctrl = gc2145_s_ctrl,
> +};
> +
> +/* Initialize control handlers */
> +static int gc2145_init_controls(struct gc2145 *gc2145)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&gc2145->sd);
> +	const struct v4l2_ctrl_ops *ops = &gc2145_ctrl_ops;
> +	struct gc2145_ctrls *ctrls = &gc2145->ctrls;
> +	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> +	struct v4l2_fwnode_device_properties props;
> +	int ret;
> +
> +	ret = v4l2_ctrl_handler_init(hdl, 12);
> +	if (ret)
> +		return ret;
> +
> +	ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> +					      0, INT_MAX, 1,

You know the supported pixel rates. Why use INT_MAX ?

> +					      supported_modes[0].pixel_rate);
> +	ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;

isn't PIXEL_RATE RO by default ?

> +
> +	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> +					  supported_modes[0].hblank,
> +					  supported_modes[0].hblank, 1,
> +					  supported_modes[0].hblank);

Register P0:0x05 "HBlanking" is 12 bits long.
Is the max value BIT(13) - 1 ?

I have not been able to find a min value.

However registering hblank without vblank kind of defeats the purpose
of using the two controls to compute the frame duration..

> +	ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	ctrls->test_pattern =
> +		v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> +					     ARRAY_SIZE(test_pattern_menu) - 1,
> +					     0, 0, test_pattern_menu);
> +	ctrls->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP,
> +					 0, 1, 1, 0);
> +	ctrls->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP,
> +					 0, 1, 1, 0);
> +
> +	if (hdl->error) {
> +		ret = hdl->error;
> +		dev_err(&client->dev, "control init failed (%d)\n", ret);
> +		goto error;
> +	}
> +
> +	ret = v4l2_fwnode_device_parse(&client->dev, &props);
> +	if (ret)
> +		goto error;
> +
> +	ret = v4l2_ctrl_new_fwnode_properties(hdl, &gc2145_ctrl_ops,
> +					      &props);
> +	if (ret)
> +		goto error;
> +
> +	gc2145->sd.ctrl_handler = hdl;
> +
> +	return 0;
> +
> +error:
> +	v4l2_ctrl_handler_free(hdl);
> +
> +	return ret;
> +}
> +
> +static void gc2145_free_controls(struct gc2145 *gc2145)
> +{
> +	v4l2_ctrl_handler_free(&gc2145->ctrls.handler);

One liners could be called directly maybe..

> +}
> +
> +static int gc2145_check_hwcfg(struct device *dev)
> +{
> +	struct fwnode_handle *endpoint;
> +	struct v4l2_fwnode_endpoint ep_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	int ret = -EINVAL;
> +
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
> +		dev_err(dev, "could not parse endpoint\n");
> +		goto error_out;

no need to v4l2_fwnode_endpoint_free() if
v4l2_fwnode_endpoint_alloc_parse() fails.

This could be

        ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
        fwnode_handle_put(endpoint);
        if (ret)
                return ret;


> +	}
> +
> +	/* Check the number of MIPI CSI2 data lanes */
> +	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> +		dev_err(dev, "only 2 data lanes are currently supported\n");
> +		goto error_out;
> +	}
> +
> +	ret = 0;
> +
> +error_out:
> +	v4l2_fwnode_endpoint_free(&ep_cfg);
> +	fwnode_handle_put(endpoint);
> +
> +	return ret;
> +}
> +
> +static int gc2145_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct gc2145 *gc2145;
> +	int ret;
> +	unsigned int xclk_freq;

You can easily declare this 2 lines up (I know, reverse-xmas-tree
nitpicking)

> +
> +	gc2145 = devm_kzalloc(&client->dev, sizeof(*gc2145), GFP_KERNEL);
> +	if (!gc2145)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&gc2145->sd, client, &gc2145_subdev_ops);
> +
> +	/* Check the hardware configuration in device tree */
> +	if (gc2145_check_hwcfg(dev))
> +		return -EINVAL;
> +
> +	/* Get system clock (xclk) */
> +	gc2145->xclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(gc2145->xclk)) {
> +		dev_err(dev, "failed to get xclk\n");
> +		return PTR_ERR(gc2145->xclk);
> +	}
> +
> +	xclk_freq = clk_get_rate(gc2145->xclk);
> +	if (xclk_freq != GC2145_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = gc2145_get_regulators(gc2145);
> +	if (ret) {
> +		dev_err(dev, "failed to get regulators\n");

Can dev_err_probe help here ?

> +		return ret;
> +	}
> +
> +	/* Request optional enable pin */
> +	gc2145->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +
> +	/* Request optional enable pin */
> +	gc2145->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +							 GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for gc2145_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = gc2145_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = gc2145_identify_module(gc2145);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode */
> +	gc2145->mode = &supported_modes[0];
> +
> +	ret = gc2145_init_controls(gc2145);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize subdev */
> +	gc2145->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
> +	gc2145->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	gc2145->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&gc2145->sd.entity, 1, &gc2145->pad);
> +	if (ret) {
> +		dev_err(dev, "failed to init entity pads: %d\n", ret);
> +		goto error_handler_free;
> +	}
> +
> +	gc2145->sd.state_lock = gc2145->ctrls.handler.lock;
> +	ret = v4l2_subdev_init_finalize(&gc2145->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "subdev init error: %d\n", ret);
> +		goto error_media_entity;
> +	}
> +
> +	ret = v4l2_async_register_subdev_sensor(&gc2145->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> +		goto error_subdev_cleanup;
> +	}
> +
> +	/* Enable runtime PM and turn off the device */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_idle(dev);
> +
> +	return 0;
> +
> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&gc2145->sd);
> +
> +error_media_entity:
> +	media_entity_cleanup(&gc2145->sd.entity);
> +
> +error_handler_free:
> +	gc2145_free_controls(gc2145);
> +
> +error_power_off:
> +	gc2145_power_off(dev);
> +
> +	return ret;
> +}
> +
> +static void gc2145_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct gc2145 *gc2145 = to_gc2145(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);

Do you need to call
	v4l2_subdev_cleanup(&gc2145->sd);

here as well ?

> +	gc2145_free_controls(gc2145);
> +
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		gc2145_power_off(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +}
> +
> +static const struct of_device_id gc2145_dt_ids[] = {
> +	{ .compatible = "galaxycore,gc2145" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, gc2145_dt_ids);
> +
> +static const struct dev_pm_ops gc2145_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(gc2145_suspend, gc2145_resume)

You can drop these function and the "streaming" flag

Thanks
  j

See: [PATCH 00/57] media: i2c: Reduce cargo cult

> +	SET_RUNTIME_PM_OPS(gc2145_power_off, gc2145_power_on, NULL)
> +};
> +
> +static struct i2c_driver gc2145_i2c_driver = {
> +	.driver = {
> +		.name = "gc2145",
> +		.of_match_table	= gc2145_dt_ids,
> +		.pm = pm_ptr(&gc2145_pm_ops),
> +	},
> +	.probe = gc2145_probe,
> +	.remove = gc2145_remove,
> +};
> +
> +module_i2c_driver(gc2145_i2c_driver);
> +
> +MODULE_AUTHOR("Alain Volmat <alain.volmat@foss.st.com");
> +MODULE_DESCRIPTION("GalaxyCore GC2145 sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>