[PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements

public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements
@ 2023-09-24 15:32 Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 01/20] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
                   ` (19 more replies)
  0 siblings, 20 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Hello,

This patch series is a collection of miscellaneous cleanups and
improvements to the imx219 driver.

Most notably, the series starts with converting the driver to the new
CCI helpers ([1]) in 01/20. Unlike the IMX290, IMX296, IMX297 and IMX327
that have little-endian register values, this sensor adheres to the CCI
specification, so the conversion was possible. It caused a regression
that I had a bit of trouble tracking though, which showed that merging
conversion to the CCI helpers without testing would be dangerous.

Patch 05/20 fixes what I believe is an issue with the test pattern
generator configuration in 640x480 mode, and should be the only
functional change in the series.

For details about other patches, please see their individual commit
logs. Compared to v3, patch 15/20 has been added, and the series has
been rebased on top of a merge of Sakari's sensors pull request for v6.7
and a v6.6 fixes branch that will appear in v6.6-rc3. This was necessary
as both of those branches contain imx219 changes that conflict with this
series. As a result of the rebase, patch 15/20 from v3 got dropped.

The changes have been tested on a Raspberry Pi 4, with all the modes
supproted by the driver.

[1] https://lore.kernel.org/linux-media/20230627125109.52354-1-hdegoede@redhat.com/

Laurent Pinchart (20):
  media: i2c: imx219: Convert to CCI register access helpers
  media: i2c: imx219: Drop unused macros
  media: i2c: imx219: Replace register addresses with macros
  media: i2c: imx219: Drop IMX219_REG_CSI_LANE_MODE from common regs
    array
  media: i2c: imx219: Fix test pattern window for 640x480 mode
  media: i2c: imx219: Set mode registers programmatically
  media: i2c: imx219: Merge format and binning setting functions
  media: i2c: imx219: Initialize ycbcr_enc
  media: i2c: imx219: Use active crop rectangle to configure registers
  media: i2c: imx219: Infer binning settings from format and crop
  media: i2c: imx219: Access height from active format in
    imx219_set_ctrl
  media: i2c: imx219: Don't store the current mode in the imx219
    structure
  media: i2c: imx219: Drop IMX219_VTS_* macros
  media: i2c: imx219: Group functions by purpose
  media: i2c: imx219: Remove unneeded goto
  media: i2c: imx219: Implement .init_cfg() using .set_fmt()
  media: i2c: imx219: Separate horizontal and vertical binning
  media: i2c: imx219: Calculate crop rectangle dynamically
  media: i2c: imx219: Name all subdev state variables 'state'
  media: i2c: imx219: Move variables to inner scope

 drivers/media/i2c/Kconfig  |    1 +
 drivers/media/i2c/imx219.c | 1467 ++++++++++++++++--------------------
 2 files changed, 643 insertions(+), 825 deletions(-)

base-commit: 76d7b38d896f7bc6c65c4bd053df060f3f3dcb90
-- 
Regards,

Laurent Pinchart

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

* [PATCH v4 01/20] media: i2c: imx219: Convert to CCI register access helpers
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 02/20] media: i2c: imx219: Drop unused macros Laurent Pinchart
                   ` (18 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Use the new common CCI register access helpers to replace the private
register access helpers in the imx219 driver. This simplifies the driver
by reducing the amount of code.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/imx219.c | 515 ++++++++++++++++---------------------
 2 files changed, 221 insertions(+), 295 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 74ff833ff48c..53b443be5a59 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -99,6 +99,7 @@ config VIDEO_IMX214
 
 config VIDEO_IMX219
 	tristate "Sony IMX219 sensor support"
+	select V4L2_CCI_I2C
 	help
 	  This is a Video4Linux2 sensor driver for the Sony
 	  IMX219 camera.
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index af29398c657d..9d84f09bcced 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -21,40 +21,49 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+
+#include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mediabus.h>
-#include <asm/unaligned.h>
 
-#define IMX219_REG_VALUE_08BIT		1
-#define IMX219_REG_VALUE_16BIT		2
+/* Chip ID */
+#define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
+#define IMX219_CHIP_ID			0x0219
 
-#define IMX219_REG_MODE_SELECT		0x0100
+#define IMX219_REG_MODE_SELECT		CCI_REG8(0x0100)
 #define IMX219_MODE_STANDBY		0x00
 #define IMX219_MODE_STREAMING		0x01
 
-/* Chip ID */
-#define IMX219_REG_CHIP_ID		0x0000
-#define IMX219_CHIP_ID			0x0219
-
-/* External clock frequency is 24.0M */
-#define IMX219_XCLK_FREQ		24000000
-
-/* Pixel rate is fixed for all the modes */
-#define IMX219_PIXEL_RATE		182400000
-#define IMX219_PIXEL_RATE_4LANE		280800000
-
-#define IMX219_DEFAULT_LINK_FREQ	456000000
-#define IMX219_DEFAULT_LINK_FREQ_4LANE	363000000
-
-#define IMX219_REG_CSI_LANE_MODE	0x0114
+#define IMX219_REG_CSI_LANE_MODE	CCI_REG8(0x0114)
 #define IMX219_CSI_2_LANE_MODE		0x01
 #define IMX219_CSI_4_LANE_MODE		0x03
 
+/* Analog gain control */
+#define IMX219_REG_ANALOG_GAIN		CCI_REG8(0x0157)
+#define IMX219_ANA_GAIN_MIN		0
+#define IMX219_ANA_GAIN_MAX		232
+#define IMX219_ANA_GAIN_STEP		1
+#define IMX219_ANA_GAIN_DEFAULT		0x0
+
+/* Digital gain control */
+#define IMX219_REG_DIGITAL_GAIN		CCI_REG16(0x0158)
+#define IMX219_DGTL_GAIN_MIN		0x0100
+#define IMX219_DGTL_GAIN_MAX		0x0fff
+#define IMX219_DGTL_GAIN_DEFAULT	0x0100
+#define IMX219_DGTL_GAIN_STEP		1
+
+/* Exposure control */
+#define IMX219_REG_EXPOSURE		CCI_REG16(0x015a)
+#define IMX219_EXPOSURE_MIN		4
+#define IMX219_EXPOSURE_STEP		1
+#define IMX219_EXPOSURE_DEFAULT		0x640
+#define IMX219_EXPOSURE_MAX		65535
+
 /* V_TIMING internal */
-#define IMX219_REG_VTS			0x0160
+#define IMX219_REG_VTS			CCI_REG16(0x0160)
 #define IMX219_VTS_15FPS		0x0dc6
 #define IMX219_VTS_30FPS_1080P		0x06e3
 #define IMX219_VTS_30FPS_BINNED		0x06e3
@@ -72,37 +81,16 @@
 /* HBLANK control - read only */
 #define IMX219_PPL_DEFAULT		3448
 
-/* Exposure control */
-#define IMX219_REG_EXPOSURE		0x015a
-#define IMX219_EXPOSURE_MIN		4
-#define IMX219_EXPOSURE_STEP		1
-#define IMX219_EXPOSURE_DEFAULT		0x640
-#define IMX219_EXPOSURE_MAX		65535
-
-/* Analog gain control */
-#define IMX219_REG_ANALOG_GAIN		0x0157
-#define IMX219_ANA_GAIN_MIN		0
-#define IMX219_ANA_GAIN_MAX		232
-#define IMX219_ANA_GAIN_STEP		1
-#define IMX219_ANA_GAIN_DEFAULT		0x0
-
-/* Digital gain control */
-#define IMX219_REG_DIGITAL_GAIN		0x0158
-#define IMX219_DGTL_GAIN_MIN		0x0100
-#define IMX219_DGTL_GAIN_MAX		0x0fff
-#define IMX219_DGTL_GAIN_DEFAULT	0x0100
-#define IMX219_DGTL_GAIN_STEP		1
-
-#define IMX219_REG_ORIENTATION		0x0172
+#define IMX219_REG_ORIENTATION		CCI_REG8(0x0172)
 
 /* Binning  Mode */
-#define IMX219_REG_BINNING_MODE		0x0174
+#define IMX219_REG_BINNING_MODE		CCI_REG16(0x0174)
 #define IMX219_BINNING_NONE		0x0000
 #define IMX219_BINNING_2X2		0x0101
 #define IMX219_BINNING_2X2_ANALOG	0x0303
 
 /* Test Pattern Control */
-#define IMX219_REG_TEST_PATTERN		0x0600
+#define IMX219_REG_TEST_PATTERN		CCI_REG16(0x0600)
 #define IMX219_TEST_PATTERN_DISABLE	0
 #define IMX219_TEST_PATTERN_SOLID_COLOR	1
 #define IMX219_TEST_PATTERN_COLOR_BARS	2
@@ -110,10 +98,10 @@
 #define IMX219_TEST_PATTERN_PN9		4
 
 /* Test pattern colour components */
-#define IMX219_REG_TESTP_RED		0x0602
-#define IMX219_REG_TESTP_GREENR		0x0604
-#define IMX219_REG_TESTP_BLUE		0x0606
-#define IMX219_REG_TESTP_GREENB		0x0608
+#define IMX219_REG_TESTP_RED		CCI_REG16(0x0602)
+#define IMX219_REG_TESTP_GREENR		CCI_REG16(0x0604)
+#define IMX219_REG_TESTP_BLUE		CCI_REG16(0x0606)
+#define IMX219_REG_TESTP_GREENB		CCI_REG16(0x0608)
 #define IMX219_TESTP_COLOUR_MIN		0
 #define IMX219_TESTP_COLOUR_MAX		0x03ff
 #define IMX219_TESTP_COLOUR_STEP	1
@@ -122,6 +110,16 @@
 #define IMX219_TESTP_BLUE_DEFAULT	0
 #define IMX219_TESTP_GREENB_DEFAULT	0
 
+/* External clock frequency is 24.0M */
+#define IMX219_XCLK_FREQ		24000000
+
+/* Pixel rate is fixed for all the modes */
+#define IMX219_PIXEL_RATE		182400000
+#define IMX219_PIXEL_RATE_4LANE		280800000
+
+#define IMX219_DEFAULT_LINK_FREQ	456000000
+#define IMX219_DEFAULT_LINK_FREQ_4LANE	363000000
+
 /* IMX219 native and active pixel array size. */
 #define IMX219_NATIVE_WIDTH		3296U
 #define IMX219_NATIVE_HEIGHT		2480U
@@ -130,14 +128,9 @@
 #define IMX219_PIXEL_ARRAY_WIDTH	3280U
 #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
 
-struct imx219_reg {
-	u16 address;
-	u8 val;
-};
-
 struct imx219_reg_list {
 	unsigned int num_of_regs;
-	const struct imx219_reg *regs;
+	const struct cci_reg_sequence *regs;
 };
 
 /* Mode : resolution and related config&values */
@@ -160,53 +153,53 @@ struct imx219_mode {
 	bool binning;
 };
 
-static const struct imx219_reg imx219_common_regs[] = {
-	{0x0100, 0x00},	/* Mode Select */
+static const struct cci_reg_sequence imx219_common_regs[] = {
+	{ CCI_REG8(0x0100), 0x00 },	/* Mode Select */
 
 	/* To Access Addresses 3000-5fff, send the following commands */
-	{0x30eb, 0x0c},
-	{0x30eb, 0x05},
-	{0x300a, 0xff},
-	{0x300b, 0xff},
-	{0x30eb, 0x05},
-	{0x30eb, 0x09},
+	{ CCI_REG8(0x30eb), 0x0c },
+	{ CCI_REG8(0x30eb), 0x05 },
+	{ CCI_REG8(0x300a), 0xff },
+	{ CCI_REG8(0x300b), 0xff },
+	{ CCI_REG8(0x30eb), 0x05 },
+	{ CCI_REG8(0x30eb), 0x09 },
 
 	/* PLL Clock Table */
-	{0x0301, 0x05},	/* VTPXCK_DIV */
-	{0x0303, 0x01},	/* VTSYSCK_DIV */
-	{0x0304, 0x03},	/* PREPLLCK_VT_DIV 0x03 = AUTO set */
-	{0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */
-	{0x0306, 0x00},	/* PLL_VT_MPY */
-	{0x0307, 0x39},
-	{0x030b, 0x01},	/* OP_SYS_CLK_DIV */
-	{0x030c, 0x00},	/* PLL_OP_MPY */
-	{0x030d, 0x72},
+	{ CCI_REG8(0x0301), 0x05 },	/* VTPXCK_DIV */
+	{ CCI_REG8(0x0303), 0x01 },	/* VTSYSCK_DIV */
+	{ CCI_REG8(0x0304), 0x03 },	/* PREPLLCK_VT_DIV 0x03 = AUTO set */
+	{ CCI_REG8(0x0305), 0x03 },	/* PREPLLCK_OP_DIV 0x03 = AUTO set */
+	{ CCI_REG8(0x0306), 0x00 },	/* PLL_VT_MPY */
+	{ CCI_REG8(0x0307), 0x39 },
+	{ CCI_REG8(0x030b), 0x01 },	/* OP_SYS_CLK_DIV */
+	{ CCI_REG8(0x030c), 0x00 },	/* PLL_OP_MPY */
+	{ CCI_REG8(0x030d), 0x72 },
 
 	/* Undocumented registers */
-	{0x455e, 0x00},
-	{0x471e, 0x4b},
-	{0x4767, 0x0f},
-	{0x4750, 0x14},
-	{0x4540, 0x00},
-	{0x47b4, 0x14},
-	{0x4713, 0x30},
-	{0x478b, 0x10},
-	{0x478f, 0x10},
-	{0x4793, 0x10},
-	{0x4797, 0x0e},
-	{0x479b, 0x0e},
+	{ CCI_REG8(0x455e), 0x00 },
+	{ CCI_REG8(0x471e), 0x4b },
+	{ CCI_REG8(0x4767), 0x0f },
+	{ CCI_REG8(0x4750), 0x14 },
+	{ CCI_REG8(0x4540), 0x00 },
+	{ CCI_REG8(0x47b4), 0x14 },
+	{ CCI_REG8(0x4713), 0x30 },
+	{ CCI_REG8(0x478b), 0x10 },
+	{ CCI_REG8(0x478f), 0x10 },
+	{ CCI_REG8(0x4793), 0x10 },
+	{ CCI_REG8(0x4797), 0x0e },
+	{ CCI_REG8(0x479b), 0x0e },
 
 	/* Frame Bank Register Group "A" */
-	{0x0162, 0x0d},	/* Line_Length_A */
-	{0x0163, 0x78},
-	{0x0170, 0x01}, /* X_ODD_INC_A */
-	{0x0171, 0x01}, /* Y_ODD_INC_A */
+	{ CCI_REG8(0x0162), 0x0d },	/* Line_Length_A */
+	{ CCI_REG8(0x0163), 0x78 },
+	{ CCI_REG8(0x0170), 0x01 },	/* X_ODD_INC_A */
+	{ CCI_REG8(0x0171), 0x01 },	/* Y_ODD_INC_A */
 
 	/* Output setup registers */
-	{0x0114, 0x01},	/* CSI 2-Lane Mode */
-	{0x0128, 0x00},	/* DPHY Auto Mode */
-	{0x012a, 0x18},	/* EXCK_Freq */
-	{0x012b, 0x00},
+	{ CCI_REG8(0x0114), 0x01 },	/* CSI 2-Lane Mode */
+	{ CCI_REG8(0x0128), 0x00 },	/* DPHY Auto Mode */
+	{ CCI_REG8(0x012a), 0x18 },	/* EXCK_Freq */
+	{ CCI_REG8(0x012b), 0x00 },
 };
 
 /*
@@ -214,92 +207,92 @@ static const struct imx219_reg imx219_common_regs[] = {
  * driver.
  * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
  */
-static const struct imx219_reg mode_3280x2464_regs[] = {
-	{0x0164, 0x00},
-	{0x0165, 0x00},
-	{0x0166, 0x0c},
-	{0x0167, 0xcf},
-	{0x0168, 0x00},
-	{0x0169, 0x00},
-	{0x016a, 0x09},
-	{0x016b, 0x9f},
-	{0x016c, 0x0c},
-	{0x016d, 0xd0},
-	{0x016e, 0x09},
-	{0x016f, 0xa0},
-	{0x0624, 0x0c},
-	{0x0625, 0xd0},
-	{0x0626, 0x09},
-	{0x0627, 0xa0},
+static const struct cci_reg_sequence mode_3280x2464_regs[] = {
+	{ CCI_REG8(0x0164), 0x00 },
+	{ CCI_REG8(0x0165), 0x00 },
+	{ CCI_REG8(0x0166), 0x0c },
+	{ CCI_REG8(0x0167), 0xcf },
+	{ CCI_REG8(0x0168), 0x00 },
+	{ CCI_REG8(0x0169), 0x00 },
+	{ CCI_REG8(0x016a), 0x09 },
+	{ CCI_REG8(0x016b), 0x9f },
+	{ CCI_REG8(0x016c), 0x0c },
+	{ CCI_REG8(0x016d), 0xd0 },
+	{ CCI_REG8(0x016e), 0x09 },
+	{ CCI_REG8(0x016f), 0xa0 },
+	{ CCI_REG8(0x0624), 0x0c },
+	{ CCI_REG8(0x0625), 0xd0 },
+	{ CCI_REG8(0x0626), 0x09 },
+	{ CCI_REG8(0x0627), 0xa0 },
 };
 
-static const struct imx219_reg mode_1920_1080_regs[] = {
-	{0x0164, 0x02},
-	{0x0165, 0xa8},
-	{0x0166, 0x0a},
-	{0x0167, 0x27},
-	{0x0168, 0x02},
-	{0x0169, 0xb4},
-	{0x016a, 0x06},
-	{0x016b, 0xeb},
-	{0x016c, 0x07},
-	{0x016d, 0x80},
-	{0x016e, 0x04},
-	{0x016f, 0x38},
-	{0x0624, 0x07},
-	{0x0625, 0x80},
-	{0x0626, 0x04},
-	{0x0627, 0x38},
+static const struct cci_reg_sequence mode_1920_1080_regs[] = {
+	{ CCI_REG8(0x0164), 0x02 },
+	{ CCI_REG8(0x0165), 0xa8 },
+	{ CCI_REG8(0x0166), 0x0a },
+	{ CCI_REG8(0x0167), 0x27 },
+	{ CCI_REG8(0x0168), 0x02 },
+	{ CCI_REG8(0x0169), 0xb4 },
+	{ CCI_REG8(0x016a), 0x06 },
+	{ CCI_REG8(0x016b), 0xeb },
+	{ CCI_REG8(0x016c), 0x07 },
+	{ CCI_REG8(0x016d), 0x80 },
+	{ CCI_REG8(0x016e), 0x04 },
+	{ CCI_REG8(0x016f), 0x38 },
+	{ CCI_REG8(0x0624), 0x07 },
+	{ CCI_REG8(0x0625), 0x80 },
+	{ CCI_REG8(0x0626), 0x04 },
+	{ CCI_REG8(0x0627), 0x38 },
 };
 
-static const struct imx219_reg mode_1640_1232_regs[] = {
-	{0x0164, 0x00},
-	{0x0165, 0x00},
-	{0x0166, 0x0c},
-	{0x0167, 0xcf},
-	{0x0168, 0x00},
-	{0x0169, 0x00},
-	{0x016a, 0x09},
-	{0x016b, 0x9f},
-	{0x016c, 0x06},
-	{0x016d, 0x68},
-	{0x016e, 0x04},
-	{0x016f, 0xd0},
-	{0x0624, 0x06},
-	{0x0625, 0x68},
-	{0x0626, 0x04},
-	{0x0627, 0xd0},
+static const struct cci_reg_sequence mode_1640_1232_regs[] = {
+	{ CCI_REG8(0x0164), 0x00 },
+	{ CCI_REG8(0x0165), 0x00 },
+	{ CCI_REG8(0x0166), 0x0c },
+	{ CCI_REG8(0x0167), 0xcf },
+	{ CCI_REG8(0x0168), 0x00 },
+	{ CCI_REG8(0x0169), 0x00 },
+	{ CCI_REG8(0x016a), 0x09 },
+	{ CCI_REG8(0x016b), 0x9f },
+	{ CCI_REG8(0x016c), 0x06 },
+	{ CCI_REG8(0x016d), 0x68 },
+	{ CCI_REG8(0x016e), 0x04 },
+	{ CCI_REG8(0x016f), 0xd0 },
+	{ CCI_REG8(0x0624), 0x06 },
+	{ CCI_REG8(0x0625), 0x68 },
+	{ CCI_REG8(0x0626), 0x04 },
+	{ CCI_REG8(0x0627), 0xd0 },
 };
 
-static const struct imx219_reg mode_640_480_regs[] = {
-	{0x0164, 0x03},
-	{0x0165, 0xe8},
-	{0x0166, 0x08},
-	{0x0167, 0xe7},
-	{0x0168, 0x02},
-	{0x0169, 0xf0},
-	{0x016a, 0x06},
-	{0x016b, 0xaf},
-	{0x016c, 0x02},
-	{0x016d, 0x80},
-	{0x016e, 0x01},
-	{0x016f, 0xe0},
-	{0x0624, 0x06},
-	{0x0625, 0x68},
-	{0x0626, 0x04},
-	{0x0627, 0xd0},
+static const struct cci_reg_sequence mode_640_480_regs[] = {
+	{ CCI_REG8(0x0164), 0x03 },
+	{ CCI_REG8(0x0165), 0xe8 },
+	{ CCI_REG8(0x0166), 0x08 },
+	{ CCI_REG8(0x0167), 0xe7 },
+	{ CCI_REG8(0x0168), 0x02 },
+	{ CCI_REG8(0x0169), 0xf0 },
+	{ CCI_REG8(0x016a), 0x06 },
+	{ CCI_REG8(0x016b), 0xaf },
+	{ CCI_REG8(0x016c), 0x02 },
+	{ CCI_REG8(0x016d), 0x80 },
+	{ CCI_REG8(0x016e), 0x01 },
+	{ CCI_REG8(0x016f), 0xe0 },
+	{ CCI_REG8(0x0624), 0x06 },
+	{ CCI_REG8(0x0625), 0x68 },
+	{ CCI_REG8(0x0626), 0x04 },
+	{ CCI_REG8(0x0627), 0xd0 },
 };
 
-static const struct imx219_reg raw8_framefmt_regs[] = {
-	{0x018c, 0x08},
-	{0x018d, 0x08},
-	{0x0309, 0x08},
+static const struct cci_reg_sequence raw8_framefmt_regs[] = {
+	{ CCI_REG8(0x018c), 0x08 },
+	{ CCI_REG8(0x018d), 0x08 },
+	{ CCI_REG8(0x0309), 0x08 },
 };
 
-static const struct imx219_reg raw10_framefmt_regs[] = {
-	{0x018c, 0x0a},
-	{0x018d, 0x0a},
-	{0x0309, 0x0a},
+static const struct cci_reg_sequence raw10_framefmt_regs[] = {
+	{ CCI_REG8(0x018c), 0x0a },
+	{ CCI_REG8(0x018d), 0x0a },
+	{ CCI_REG8(0x0309), 0x0a },
 };
 
 static const s64 imx219_link_freq_menu[] = {
@@ -460,6 +453,7 @@ struct imx219 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
+	struct regmap *regmap;
 	struct clk *xclk; /* system clock to IMX219 */
 	u32 xclk_freq;
 
@@ -488,78 +482,6 @@ static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
 	return container_of(_sd, struct imx219, sd);
 }
 
-/* Read registers up to 2 at a time */
-static int imx219_read_reg(struct imx219 *imx219, u16 reg, u32 len, u32 *val)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	struct i2c_msg msgs[2];
-	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
-	u8 data_buf[4] = { 0, };
-	int ret;
-
-	if (len > 4)
-		return -EINVAL;
-
-	/* Write register address */
-	msgs[0].addr = client->addr;
-	msgs[0].flags = 0;
-	msgs[0].len = ARRAY_SIZE(addr_buf);
-	msgs[0].buf = addr_buf;
-
-	/* Read data from register */
-	msgs[1].addr = client->addr;
-	msgs[1].flags = I2C_M_RD;
-	msgs[1].len = len;
-	msgs[1].buf = &data_buf[4 - len];
-
-	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
-	if (ret != ARRAY_SIZE(msgs))
-		return -EIO;
-
-	*val = get_unaligned_be32(data_buf);
-
-	return 0;
-}
-
-/* Write registers up to 2 at a time */
-static int imx219_write_reg(struct imx219 *imx219, u16 reg, u32 len, u32 val)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	u8 buf[6];
-
-	if (len > 4)
-		return -EINVAL;
-
-	put_unaligned_be16(reg, buf);
-	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
-	if (i2c_master_send(client, buf, len + 2) != len + 2)
-		return -EIO;
-
-	return 0;
-}
-
-/* Write a list of registers */
-static int imx219_write_regs(struct imx219 *imx219,
-			     const struct imx219_reg *regs, u32 len)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	unsigned int i;
-	int ret;
-
-	for (i = 0; i < len; i++) {
-		ret = imx219_write_reg(imx219, regs[i].address, 1, regs[i].val);
-		if (ret) {
-			dev_err_ratelimited(&client->dev,
-					    "Failed to write reg 0x%4.4x. error = %d\n",
-					    regs[i].address, ret);
-
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 /* Get bayer order based on flip setting. */
 static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 {
@@ -583,7 +505,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct imx219 *imx219 =
 		container_of(ctrl->handler, struct imx219, ctrl_handler);
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	int ret;
+	int ret = 0;
 
 	if (ctrl->id == V4L2_CID_VBLANK) {
 		int exposure_max, exposure_def;
@@ -607,48 +529,45 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_ANALOGUE_GAIN:
-		ret = imx219_write_reg(imx219, IMX219_REG_ANALOG_GAIN,
-				       IMX219_REG_VALUE_08BIT, ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_ANALOG_GAIN,
+			  ctrl->val, &ret);
 		break;
 	case V4L2_CID_EXPOSURE:
-		ret = imx219_write_reg(imx219, IMX219_REG_EXPOSURE,
-				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
+			  ctrl->val, &ret);
 		break;
 	case V4L2_CID_DIGITAL_GAIN:
-		ret = imx219_write_reg(imx219, IMX219_REG_DIGITAL_GAIN,
-				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
+			  ctrl->val, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN:
-		ret = imx219_write_reg(imx219, IMX219_REG_TEST_PATTERN,
-				       IMX219_REG_VALUE_16BIT,
-				       imx219_test_pattern_val[ctrl->val]);
+		cci_write(imx219->regmap, IMX219_REG_TEST_PATTERN,
+			  imx219_test_pattern_val[ctrl->val], &ret);
 		break;
 	case V4L2_CID_HFLIP:
 	case V4L2_CID_VFLIP:
-		ret = imx219_write_reg(imx219, IMX219_REG_ORIENTATION, 1,
-				       imx219->hflip->val |
-				       imx219->vflip->val << 1);
+		cci_write(imx219->regmap, IMX219_REG_ORIENTATION,
+			  imx219->hflip->val | imx219->vflip->val << 1, &ret);
 		break;
 	case V4L2_CID_VBLANK:
-		ret = imx219_write_reg(imx219, IMX219_REG_VTS,
-				       IMX219_REG_VALUE_16BIT,
-				       imx219->mode->height + ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_VTS,
+			  imx219->mode->height + ctrl->val, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN_RED:
-		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_RED,
-				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_TESTP_RED,
+			  ctrl->val, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN_GREENR:
-		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENR,
-				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_TESTP_GREENR,
+			  ctrl->val, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN_BLUE:
-		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_BLUE,
-				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_TESTP_BLUE,
+			  ctrl->val, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN_GREENB:
-		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENB,
-				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		cci_write(imx219->regmap, IMX219_REG_TESTP_GREENB,
+			  ctrl->val, &ret);
 		break;
 	default:
 		dev_info(&client->dev,
@@ -799,15 +718,15 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 	case MEDIA_BUS_FMT_SGRBG8_1X8:
 	case MEDIA_BUS_FMT_SGBRG8_1X8:
 	case MEDIA_BUS_FMT_SBGGR8_1X8:
-		return imx219_write_regs(imx219, raw8_framefmt_regs,
-					ARRAY_SIZE(raw8_framefmt_regs));
+		return cci_multi_reg_write(imx219->regmap, raw8_framefmt_regs,
+					   ARRAY_SIZE(raw8_framefmt_regs), NULL);
 
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
 	case MEDIA_BUS_FMT_SGRBG10_1X10:
 	case MEDIA_BUS_FMT_SGBRG10_1X10:
 	case MEDIA_BUS_FMT_SBGGR10_1X10:
-		return imx219_write_regs(imx219, raw10_framefmt_regs,
-					ARRAY_SIZE(raw10_framefmt_regs));
+		return cci_multi_reg_write(imx219->regmap, raw10_framefmt_regs,
+					   ARRAY_SIZE(raw10_framefmt_regs), NULL);
 	}
 
 	return -EINVAL;
@@ -816,28 +735,24 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 static int imx219_set_binning(struct imx219 *imx219,
 			      const struct v4l2_mbus_framefmt *format)
 {
-	if (!imx219->mode->binning) {
-		return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
-					IMX219_REG_VALUE_16BIT,
-					IMX219_BINNING_NONE);
-	}
+	if (!imx219->mode->binning)
+		return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
+				 IMX219_BINNING_NONE, NULL);
 
 	switch (format->code) {
 	case MEDIA_BUS_FMT_SRGGB8_1X8:
 	case MEDIA_BUS_FMT_SGRBG8_1X8:
 	case MEDIA_BUS_FMT_SGBRG8_1X8:
 	case MEDIA_BUS_FMT_SBGGR8_1X8:
-		return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
-					IMX219_REG_VALUE_16BIT,
-					IMX219_BINNING_2X2_ANALOG);
+		return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
+				 IMX219_BINNING_2X2_ANALOG, NULL);
 
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
 	case MEDIA_BUS_FMT_SGRBG10_1X10:
 	case MEDIA_BUS_FMT_SGBRG10_1X10:
 	case MEDIA_BUS_FMT_SBGGR10_1X10:
-		return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
-					IMX219_REG_VALUE_16BIT,
-					IMX219_BINNING_2X2);
+		return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
+				 IMX219_BINNING_2X2, NULL);
 	}
 
 	return -EINVAL;
@@ -876,9 +791,9 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 
 static int imx219_configure_lanes(struct imx219 *imx219)
 {
-	return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE,
-				IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ?
-				IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
+	return cci_write(imx219->regmap, IMX219_REG_CSI_LANE_MODE,
+			 imx219->lanes == 2 ? IMX219_CSI_2_LANE_MODE :
+			 IMX219_CSI_4_LANE_MODE, NULL);
 };
 
 static int imx219_start_streaming(struct imx219 *imx219,
@@ -894,7 +809,8 @@ static int imx219_start_streaming(struct imx219 *imx219,
 		return ret;
 
 	/* Send all registers that are common to all modes */
-	ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs));
+	ret = cci_multi_reg_write(imx219->regmap, imx219_common_regs,
+				  ARRAY_SIZE(imx219_common_regs), NULL);
 	if (ret) {
 		dev_err(&client->dev, "%s failed to send mfg header\n", __func__);
 		goto err_rpm_put;
@@ -909,7 +825,8 @@ static int imx219_start_streaming(struct imx219 *imx219,
 
 	/* Apply default values of current mode */
 	reg_list = &imx219->mode->reg_list;
-	ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
+	ret = cci_multi_reg_write(imx219->regmap, reg_list->regs,
+				  reg_list->num_of_regs, NULL);
 	if (ret) {
 		dev_err(&client->dev, "%s failed to set mode\n", __func__);
 		goto err_rpm_put;
@@ -936,8 +853,8 @@ static int imx219_start_streaming(struct imx219 *imx219,
 		goto err_rpm_put;
 
 	/* set stream on register */
-	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
-			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STREAMING, NULL);
 	if (ret)
 		goto err_rpm_put;
 
@@ -958,8 +875,8 @@ static void imx219_stop_streaming(struct imx219 *imx219)
 	int ret;
 
 	/* set stream off register */
-	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
-			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STANDBY, NULL);
 	if (ret)
 		dev_err(&client->dev, "%s failed to set stream\n", __func__);
 
@@ -1058,10 +975,9 @@ static int imx219_identify_module(struct imx219 *imx219)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
 	int ret;
-	u32 val;
+	u64 val;
 
-	ret = imx219_read_reg(imx219, IMX219_REG_CHIP_ID,
-			      IMX219_REG_VALUE_16BIT, &val);
+	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
 	if (ret) {
 		dev_err(&client->dev, "failed to read chip id %x\n",
 			IMX219_CHIP_ID);
@@ -1069,7 +985,7 @@ static int imx219_identify_module(struct imx219 *imx219)
 	}
 
 	if (val != IMX219_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
 			IMX219_CHIP_ID, val);
 		return -EIO;
 	}
@@ -1293,6 +1209,13 @@ static int imx219_probe(struct i2c_client *client)
 	if (imx219_check_hwcfg(dev, imx219))
 		return -EINVAL;
 
+	imx219->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(imx219->regmap)) {
+		ret = PTR_ERR(imx219->regmap);
+		dev_err(dev, "failed to initialize CCI: %d\n", ret);
+		return ret;
+	}
+
 	/* Get system clock (xclk) */
 	imx219->xclk = devm_clk_get(dev, NULL);
 	if (IS_ERR(imx219->xclk)) {
@@ -1336,17 +1259,19 @@ static int imx219_probe(struct i2c_client *client)
 	 * streaming is started, so upon power up switch the modes to:
 	 * streaming -> standby
 	 */
-	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
-			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STREAMING, NULL);
 	if (ret < 0)
 		goto error_power_off;
+
 	usleep_range(100, 110);
 
 	/* put sensor back to standby mode */
-	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
-			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STANDBY, NULL);
 	if (ret < 0)
 		goto error_power_off;
+
 	usleep_range(100, 110);
 
 	ret = imx219_init_controls(imx219);
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 02/20] media: i2c: imx219: Drop unused macros
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 01/20] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 03/20] media: i2c: imx219: Replace register addresses with macros Laurent Pinchart
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Drop a handful of macros that are not used and don't provide any value.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 9d84f09bcced..a9f929fc58b0 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -72,12 +72,6 @@
 
 #define IMX219_VBLANK_MIN		4
 
-/*Frame Length Line*/
-#define IMX219_FLL_MIN			0x08a6
-#define IMX219_FLL_MAX			0xffff
-#define IMX219_FLL_STEP			1
-#define IMX219_FLL_DEFAULT		0x0c98
-
 /* HBLANK control - read only */
 #define IMX219_PPL_DEFAULT		3448
 
@@ -105,10 +99,6 @@
 #define IMX219_TESTP_COLOUR_MIN		0
 #define IMX219_TESTP_COLOUR_MAX		0x03ff
 #define IMX219_TESTP_COLOUR_STEP	1
-#define IMX219_TESTP_RED_DEFAULT	IMX219_TESTP_COLOUR_MAX
-#define IMX219_TESTP_GREENR_DEFAULT	0
-#define IMX219_TESTP_BLUE_DEFAULT	0
-#define IMX219_TESTP_GREENB_DEFAULT	0
 
 /* External clock frequency is 24.0M */
 #define IMX219_XCLK_FREQ		24000000
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 03/20] media: i2c: imx219: Replace register addresses with macros
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 01/20] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 02/20] media: i2c: imx219: Drop unused macros Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 04/20] media: i2c: imx219: Drop IMX219_REG_CSI_LANE_MODE from common regs array Laurent Pinchart
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Define macros for all the known registers used in the register arrays,
and use them to replace the numerical addresses. This improves
readability.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Changes since v2:

- Use IMX219_XCLK_FREQ in imx219_common_regs
---
 drivers/media/i2c/imx219.c | 169 ++++++++++++++++++-------------------
 1 file changed, 81 insertions(+), 88 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index a9f929fc58b0..530b03d78e34 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -41,6 +41,13 @@
 #define IMX219_CSI_2_LANE_MODE		0x01
 #define IMX219_CSI_4_LANE_MODE		0x03
 
+#define IMX219_REG_DPHY_CTRL		CCI_REG8(0x0128)
+#define IMX219_DPHY_CTRL_TIMING_AUTO	0
+#define IMX219_DPHY_CTRL_TIMING_MANUAL	1
+
+#define IMX219_REG_EXCK_FREQ		CCI_REG16(0x012a)
+#define IMX219_EXCK_FREQ(n)		((n) * 256)		/* n expressed in MHz */
+
 /* Analog gain control */
 #define IMX219_REG_ANALOG_GAIN		CCI_REG8(0x0157)
 #define IMX219_ANA_GAIN_MIN		0
@@ -75,6 +82,15 @@
 /* HBLANK control - read only */
 #define IMX219_PPL_DEFAULT		3448
 
+#define IMX219_REG_LINE_LENGTH_A	CCI_REG16(0x0162)
+#define IMX219_REG_X_ADD_STA_A		CCI_REG16(0x0164)
+#define IMX219_REG_X_ADD_END_A		CCI_REG16(0x0166)
+#define IMX219_REG_Y_ADD_STA_A		CCI_REG16(0x0168)
+#define IMX219_REG_Y_ADD_END_A		CCI_REG16(0x016a)
+#define IMX219_REG_X_OUTPUT_SIZE	CCI_REG16(0x016c)
+#define IMX219_REG_Y_OUTPUT_SIZE	CCI_REG16(0x016e)
+#define IMX219_REG_X_ODD_INC_A		CCI_REG8(0x0170)
+#define IMX219_REG_Y_ODD_INC_A		CCI_REG8(0x0171)
 #define IMX219_REG_ORIENTATION		CCI_REG8(0x0172)
 
 /* Binning  Mode */
@@ -83,6 +99,18 @@
 #define IMX219_BINNING_2X2		0x0101
 #define IMX219_BINNING_2X2_ANALOG	0x0303
 
+#define IMX219_REG_CSI_DATA_FORMAT_A	CCI_REG16(0x018c)
+
+/* PLL Settings */
+#define IMX219_REG_VTPXCK_DIV		CCI_REG8(0x0301)
+#define IMX219_REG_VTSYCK_DIV		CCI_REG8(0x0303)
+#define IMX219_REG_PREPLLCK_VT_DIV	CCI_REG8(0x0304)
+#define IMX219_REG_PREPLLCK_OP_DIV	CCI_REG8(0x0305)
+#define IMX219_REG_PLL_VT_MPY		CCI_REG16(0x0306)
+#define IMX219_REG_OPPXCK_DIV		CCI_REG8(0x0309)
+#define IMX219_REG_OPSYCK_DIV		CCI_REG8(0x030b)
+#define IMX219_REG_PLL_OP_MPY		CCI_REG16(0x030c)
+
 /* Test Pattern Control */
 #define IMX219_REG_TEST_PATTERN		CCI_REG16(0x0600)
 #define IMX219_TEST_PATTERN_DISABLE	0
@@ -100,6 +128,9 @@
 #define IMX219_TESTP_COLOUR_MAX		0x03ff
 #define IMX219_TESTP_COLOUR_STEP	1
 
+#define IMX219_REG_TP_WINDOW_WIDTH	CCI_REG16(0x0624)
+#define IMX219_REG_TP_WINDOW_HEIGHT	CCI_REG16(0x0626)
+
 /* External clock frequency is 24.0M */
 #define IMX219_XCLK_FREQ		24000000
 
@@ -144,7 +175,7 @@ struct imx219_mode {
 };
 
 static const struct cci_reg_sequence imx219_common_regs[] = {
-	{ CCI_REG8(0x0100), 0x00 },	/* Mode Select */
+	{ IMX219_REG_MODE_SELECT, 0x00 },	/* Mode Select */
 
 	/* To Access Addresses 3000-5fff, send the following commands */
 	{ CCI_REG8(0x30eb), 0x0c },
@@ -155,15 +186,13 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
 	{ CCI_REG8(0x30eb), 0x09 },
 
 	/* PLL Clock Table */
-	{ CCI_REG8(0x0301), 0x05 },	/* VTPXCK_DIV */
-	{ CCI_REG8(0x0303), 0x01 },	/* VTSYSCK_DIV */
-	{ CCI_REG8(0x0304), 0x03 },	/* PREPLLCK_VT_DIV 0x03 = AUTO set */
-	{ CCI_REG8(0x0305), 0x03 },	/* PREPLLCK_OP_DIV 0x03 = AUTO set */
-	{ CCI_REG8(0x0306), 0x00 },	/* PLL_VT_MPY */
-	{ CCI_REG8(0x0307), 0x39 },
-	{ CCI_REG8(0x030b), 0x01 },	/* OP_SYS_CLK_DIV */
-	{ CCI_REG8(0x030c), 0x00 },	/* PLL_OP_MPY */
-	{ CCI_REG8(0x030d), 0x72 },
+	{ IMX219_REG_VTPXCK_DIV, 5 },
+	{ IMX219_REG_VTSYCK_DIV, 1 },
+	{ IMX219_REG_PREPLLCK_VT_DIV, 3 },	/* 0x03 = AUTO set */
+	{ IMX219_REG_PREPLLCK_OP_DIV, 3 },	/* 0x03 = AUTO set */
+	{ IMX219_REG_PLL_VT_MPY, 57 },
+	{ IMX219_REG_OPSYCK_DIV, 1 },
+	{ IMX219_REG_PLL_OP_MPY, 114 },
 
 	/* Undocumented registers */
 	{ CCI_REG8(0x455e), 0x00 },
@@ -180,16 +209,14 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
 	{ CCI_REG8(0x479b), 0x0e },
 
 	/* Frame Bank Register Group "A" */
-	{ CCI_REG8(0x0162), 0x0d },	/* Line_Length_A */
-	{ CCI_REG8(0x0163), 0x78 },
-	{ CCI_REG8(0x0170), 0x01 },	/* X_ODD_INC_A */
-	{ CCI_REG8(0x0171), 0x01 },	/* Y_ODD_INC_A */
+	{ IMX219_REG_LINE_LENGTH_A, 3448 },
+	{ IMX219_REG_X_ODD_INC_A, 1 },
+	{ IMX219_REG_Y_ODD_INC_A, 1 },
 
 	/* Output setup registers */
-	{ CCI_REG8(0x0114), 0x01 },	/* CSI 2-Lane Mode */
-	{ CCI_REG8(0x0128), 0x00 },	/* DPHY Auto Mode */
-	{ CCI_REG8(0x012a), 0x18 },	/* EXCK_Freq */
-	{ CCI_REG8(0x012b), 0x00 },
+	{ IMX219_REG_CSI_LANE_MODE, IMX219_CSI_2_LANE_MODE },
+	{ IMX219_REG_DPHY_CTRL, IMX219_DPHY_CTRL_TIMING_AUTO },
+	{ IMX219_REG_EXCK_FREQ, IMX219_EXCK_FREQ(IMX219_XCLK_FREQ / 1000000) },
 };
 
 /*
@@ -198,91 +225,57 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
  * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
  */
 static const struct cci_reg_sequence mode_3280x2464_regs[] = {
-	{ CCI_REG8(0x0164), 0x00 },
-	{ CCI_REG8(0x0165), 0x00 },
-	{ CCI_REG8(0x0166), 0x0c },
-	{ CCI_REG8(0x0167), 0xcf },
-	{ CCI_REG8(0x0168), 0x00 },
-	{ CCI_REG8(0x0169), 0x00 },
-	{ CCI_REG8(0x016a), 0x09 },
-	{ CCI_REG8(0x016b), 0x9f },
-	{ CCI_REG8(0x016c), 0x0c },
-	{ CCI_REG8(0x016d), 0xd0 },
-	{ CCI_REG8(0x016e), 0x09 },
-	{ CCI_REG8(0x016f), 0xa0 },
-	{ CCI_REG8(0x0624), 0x0c },
-	{ CCI_REG8(0x0625), 0xd0 },
-	{ CCI_REG8(0x0626), 0x09 },
-	{ CCI_REG8(0x0627), 0xa0 },
+	{ IMX219_REG_X_ADD_STA_A, 0 },
+	{ IMX219_REG_X_ADD_END_A, 3279 },
+	{ IMX219_REG_Y_ADD_STA_A, 0 },
+	{ IMX219_REG_Y_ADD_END_A, 2463 },
+	{ IMX219_REG_X_OUTPUT_SIZE, 3280 },
+	{ IMX219_REG_Y_OUTPUT_SIZE, 2464 },
+	{ IMX219_REG_TP_WINDOW_WIDTH, 3280 },
+	{ IMX219_REG_TP_WINDOW_HEIGHT, 2464 },
 };
 
 static const struct cci_reg_sequence mode_1920_1080_regs[] = {
-	{ CCI_REG8(0x0164), 0x02 },
-	{ CCI_REG8(0x0165), 0xa8 },
-	{ CCI_REG8(0x0166), 0x0a },
-	{ CCI_REG8(0x0167), 0x27 },
-	{ CCI_REG8(0x0168), 0x02 },
-	{ CCI_REG8(0x0169), 0xb4 },
-	{ CCI_REG8(0x016a), 0x06 },
-	{ CCI_REG8(0x016b), 0xeb },
-	{ CCI_REG8(0x016c), 0x07 },
-	{ CCI_REG8(0x016d), 0x80 },
-	{ CCI_REG8(0x016e), 0x04 },
-	{ CCI_REG8(0x016f), 0x38 },
-	{ CCI_REG8(0x0624), 0x07 },
-	{ CCI_REG8(0x0625), 0x80 },
-	{ CCI_REG8(0x0626), 0x04 },
-	{ CCI_REG8(0x0627), 0x38 },
+	{ IMX219_REG_X_ADD_STA_A, 680 },
+	{ IMX219_REG_X_ADD_END_A, 2599 },
+	{ IMX219_REG_Y_ADD_STA_A, 692 },
+	{ IMX219_REG_Y_ADD_END_A, 1771 },
+	{ IMX219_REG_X_OUTPUT_SIZE, 1920 },
+	{ IMX219_REG_Y_OUTPUT_SIZE, 1080 },
+	{ IMX219_REG_TP_WINDOW_WIDTH, 1920 },
+	{ IMX219_REG_TP_WINDOW_HEIGHT, 1080 },
 };
 
 static const struct cci_reg_sequence mode_1640_1232_regs[] = {
-	{ CCI_REG8(0x0164), 0x00 },
-	{ CCI_REG8(0x0165), 0x00 },
-	{ CCI_REG8(0x0166), 0x0c },
-	{ CCI_REG8(0x0167), 0xcf },
-	{ CCI_REG8(0x0168), 0x00 },
-	{ CCI_REG8(0x0169), 0x00 },
-	{ CCI_REG8(0x016a), 0x09 },
-	{ CCI_REG8(0x016b), 0x9f },
-	{ CCI_REG8(0x016c), 0x06 },
-	{ CCI_REG8(0x016d), 0x68 },
-	{ CCI_REG8(0x016e), 0x04 },
-	{ CCI_REG8(0x016f), 0xd0 },
-	{ CCI_REG8(0x0624), 0x06 },
-	{ CCI_REG8(0x0625), 0x68 },
-	{ CCI_REG8(0x0626), 0x04 },
-	{ CCI_REG8(0x0627), 0xd0 },
+	{ IMX219_REG_X_ADD_STA_A, 0 },
+	{ IMX219_REG_X_ADD_END_A, 3279 },
+	{ IMX219_REG_Y_ADD_STA_A, 0 },
+	{ IMX219_REG_Y_ADD_END_A, 2463 },
+	{ IMX219_REG_X_OUTPUT_SIZE, 1640 },
+	{ IMX219_REG_Y_OUTPUT_SIZE, 1232 },
+	{ IMX219_REG_TP_WINDOW_WIDTH, 1640 },
+	{ IMX219_REG_TP_WINDOW_HEIGHT, 1232 },
 };
 
 static const struct cci_reg_sequence mode_640_480_regs[] = {
-	{ CCI_REG8(0x0164), 0x03 },
-	{ CCI_REG8(0x0165), 0xe8 },
-	{ CCI_REG8(0x0166), 0x08 },
-	{ CCI_REG8(0x0167), 0xe7 },
-	{ CCI_REG8(0x0168), 0x02 },
-	{ CCI_REG8(0x0169), 0xf0 },
-	{ CCI_REG8(0x016a), 0x06 },
-	{ CCI_REG8(0x016b), 0xaf },
-	{ CCI_REG8(0x016c), 0x02 },
-	{ CCI_REG8(0x016d), 0x80 },
-	{ CCI_REG8(0x016e), 0x01 },
-	{ CCI_REG8(0x016f), 0xe0 },
-	{ CCI_REG8(0x0624), 0x06 },
-	{ CCI_REG8(0x0625), 0x68 },
-	{ CCI_REG8(0x0626), 0x04 },
-	{ CCI_REG8(0x0627), 0xd0 },
+	{ IMX219_REG_X_ADD_STA_A, 1000 },
+	{ IMX219_REG_X_ADD_END_A, 2279 },
+	{ IMX219_REG_Y_ADD_STA_A, 752 },
+	{ IMX219_REG_Y_ADD_END_A, 1711 },
+	{ IMX219_REG_X_OUTPUT_SIZE, 640 },
+	{ IMX219_REG_Y_OUTPUT_SIZE, 480 },
+	{ IMX219_REG_TP_WINDOW_WIDTH, 1640 },
+	{ IMX219_REG_TP_WINDOW_HEIGHT, 1232 },
 };
 
 static const struct cci_reg_sequence raw8_framefmt_regs[] = {
-	{ CCI_REG8(0x018c), 0x08 },
-	{ CCI_REG8(0x018d), 0x08 },
-	{ CCI_REG8(0x0309), 0x08 },
+	{ IMX219_REG_CSI_DATA_FORMAT_A, 0x0808 },
+	{ IMX219_REG_OPPXCK_DIV, 8 },
 };
 
 static const struct cci_reg_sequence raw10_framefmt_regs[] = {
-	{ CCI_REG8(0x018c), 0x0a },
-	{ CCI_REG8(0x018d), 0x0a },
-	{ CCI_REG8(0x0309), 0x0a },
+	{ IMX219_REG_CSI_DATA_FORMAT_A, 0x0a0a },
+	{ IMX219_REG_OPPXCK_DIV, 10 },
 };
 
 static const s64 imx219_link_freq_menu[] = {
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 04/20] media: i2c: imx219: Drop IMX219_REG_CSI_LANE_MODE from common regs array
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (2 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 03/20] media: i2c: imx219: Replace register addresses with macros Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 05/20] media: i2c: imx219: Fix test pattern window for 640x480 mode Laurent Pinchart
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

The IMX219_REG_CSI_LANE_MODE is configured twice, once with a hardcoded
value in the imx219_common_regs registers array, and once with the value
appropriate for the system in imx219_configure_lanes(). The latter is
enough, drop the former.

Fixes: ceddfd4493b3 ("media: i2c: imx219: Support four-lane operation")
Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 530b03d78e34..a14de8f3dee7 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -214,7 +214,6 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
 	{ IMX219_REG_Y_ODD_INC_A, 1 },
 
 	/* Output setup registers */
-	{ IMX219_REG_CSI_LANE_MODE, IMX219_CSI_2_LANE_MODE },
 	{ IMX219_REG_DPHY_CTRL, IMX219_DPHY_CTRL_TIMING_AUTO },
 	{ IMX219_REG_EXCK_FREQ, IMX219_EXCK_FREQ(IMX219_XCLK_FREQ / 1000000) },
 };
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 05/20] media: i2c: imx219: Fix test pattern window for 640x480 mode
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (3 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 04/20] media: i2c: imx219: Drop IMX219_REG_CSI_LANE_MODE from common regs array Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 06/20] media: i2c: imx219: Set mode registers programmatically Laurent Pinchart
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

The 640x480 mode specifies incorrect values for the TP_WINDOW_WIDTH and
TP_WINDOW_HEIGHT registers, which likely got copied from the 1640x1232
mode. They should be identical to the X_OUTPUT_SIZE and Y_OUTPUT_SIZE
registers as for all the other modes, to avoid cropping the test
pattern. Fix them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index a14de8f3dee7..cf55c59cbb7f 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -263,8 +263,8 @@ static const struct cci_reg_sequence mode_640_480_regs[] = {
 	{ IMX219_REG_Y_ADD_END_A, 1711 },
 	{ IMX219_REG_X_OUTPUT_SIZE, 640 },
 	{ IMX219_REG_Y_OUTPUT_SIZE, 480 },
-	{ IMX219_REG_TP_WINDOW_WIDTH, 1640 },
-	{ IMX219_REG_TP_WINDOW_HEIGHT, 1232 },
+	{ IMX219_REG_TP_WINDOW_WIDTH, 640 },
+	{ IMX219_REG_TP_WINDOW_HEIGHT, 480 },
 };
 
 static const struct cci_reg_sequence raw8_framefmt_regs[] = {
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 06/20] media: i2c: imx219: Set mode registers programmatically
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (4 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 05/20] media: i2c: imx219: Fix test pattern window for 640x480 mode Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 07/20] media: i2c: imx219: Merge format and binning setting functions Laurent Pinchart
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Replace the per-mode register arrays with code that sets the same
register values using the mode definitions. This avoids duplicating
information in two different places.

The error check for invalid formats in imx219_set_framefmt() is dropped
as the format is guaranteed to be valid.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 134 ++++++++++---------------------------
 1 file changed, 36 insertions(+), 98 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index cf55c59cbb7f..927ab106cea6 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -149,11 +149,6 @@
 #define IMX219_PIXEL_ARRAY_WIDTH	3280U
 #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
 
-struct imx219_reg_list {
-	unsigned int num_of_regs;
-	const struct cci_reg_sequence *regs;
-};
-
 /* Mode : resolution and related config&values */
 struct imx219_mode {
 	/* Frame width */
@@ -167,9 +162,6 @@ struct imx219_mode {
 	/* V-timing */
 	unsigned int vts_def;
 
-	/* Default register values */
-	struct imx219_reg_list reg_list;
-
 	/* 2x2 binning is used */
 	bool binning;
 };
@@ -218,65 +210,6 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
 	{ IMX219_REG_EXCK_FREQ, IMX219_EXCK_FREQ(IMX219_XCLK_FREQ / 1000000) },
 };
 
-/*
- * Register sets lifted off the i2C interface from the Raspberry Pi firmware
- * driver.
- * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
- */
-static const struct cci_reg_sequence mode_3280x2464_regs[] = {
-	{ IMX219_REG_X_ADD_STA_A, 0 },
-	{ IMX219_REG_X_ADD_END_A, 3279 },
-	{ IMX219_REG_Y_ADD_STA_A, 0 },
-	{ IMX219_REG_Y_ADD_END_A, 2463 },
-	{ IMX219_REG_X_OUTPUT_SIZE, 3280 },
-	{ IMX219_REG_Y_OUTPUT_SIZE, 2464 },
-	{ IMX219_REG_TP_WINDOW_WIDTH, 3280 },
-	{ IMX219_REG_TP_WINDOW_HEIGHT, 2464 },
-};
-
-static const struct cci_reg_sequence mode_1920_1080_regs[] = {
-	{ IMX219_REG_X_ADD_STA_A, 680 },
-	{ IMX219_REG_X_ADD_END_A, 2599 },
-	{ IMX219_REG_Y_ADD_STA_A, 692 },
-	{ IMX219_REG_Y_ADD_END_A, 1771 },
-	{ IMX219_REG_X_OUTPUT_SIZE, 1920 },
-	{ IMX219_REG_Y_OUTPUT_SIZE, 1080 },
-	{ IMX219_REG_TP_WINDOW_WIDTH, 1920 },
-	{ IMX219_REG_TP_WINDOW_HEIGHT, 1080 },
-};
-
-static const struct cci_reg_sequence mode_1640_1232_regs[] = {
-	{ IMX219_REG_X_ADD_STA_A, 0 },
-	{ IMX219_REG_X_ADD_END_A, 3279 },
-	{ IMX219_REG_Y_ADD_STA_A, 0 },
-	{ IMX219_REG_Y_ADD_END_A, 2463 },
-	{ IMX219_REG_X_OUTPUT_SIZE, 1640 },
-	{ IMX219_REG_Y_OUTPUT_SIZE, 1232 },
-	{ IMX219_REG_TP_WINDOW_WIDTH, 1640 },
-	{ IMX219_REG_TP_WINDOW_HEIGHT, 1232 },
-};
-
-static const struct cci_reg_sequence mode_640_480_regs[] = {
-	{ IMX219_REG_X_ADD_STA_A, 1000 },
-	{ IMX219_REG_X_ADD_END_A, 2279 },
-	{ IMX219_REG_Y_ADD_STA_A, 752 },
-	{ IMX219_REG_Y_ADD_END_A, 1711 },
-	{ IMX219_REG_X_OUTPUT_SIZE, 640 },
-	{ IMX219_REG_Y_OUTPUT_SIZE, 480 },
-	{ IMX219_REG_TP_WINDOW_WIDTH, 640 },
-	{ IMX219_REG_TP_WINDOW_HEIGHT, 480 },
-};
-
-static const struct cci_reg_sequence raw8_framefmt_regs[] = {
-	{ IMX219_REG_CSI_DATA_FORMAT_A, 0x0808 },
-	{ IMX219_REG_OPPXCK_DIV, 8 },
-};
-
-static const struct cci_reg_sequence raw10_framefmt_regs[] = {
-	{ IMX219_REG_CSI_DATA_FORMAT_A, 0x0a0a },
-	{ IMX219_REG_OPPXCK_DIV, 10 },
-};
-
 static const s64 imx219_link_freq_menu[] = {
 	IMX219_DEFAULT_LINK_FREQ,
 };
@@ -372,10 +305,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 2464
 		},
 		.vts_def = IMX219_VTS_15FPS,
-		.reg_list = {
-			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
-			.regs = mode_3280x2464_regs,
-		},
 		.binning = false,
 	},
 	{
@@ -389,10 +318,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 1080
 		},
 		.vts_def = IMX219_VTS_30FPS_1080P,
-		.reg_list = {
-			.num_of_regs = ARRAY_SIZE(mode_1920_1080_regs),
-			.regs = mode_1920_1080_regs,
-		},
 		.binning = false,
 	},
 	{
@@ -406,10 +331,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 2464
 		},
 		.vts_def = IMX219_VTS_30FPS_BINNED,
-		.reg_list = {
-			.num_of_regs = ARRAY_SIZE(mode_1640_1232_regs),
-			.regs = mode_1640_1232_regs,
-		},
 		.binning = true,
 	},
 	{
@@ -423,10 +344,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 960
 		},
 		.vts_def = IMX219_VTS_30FPS_640x480,
-		.reg_list = {
-			.num_of_regs = ARRAY_SIZE(mode_640_480_regs),
-			.regs = mode_640_480_regs,
-		},
 		.binning = true,
 	},
 };
@@ -695,23 +612,53 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 static int imx219_set_framefmt(struct imx219 *imx219,
 			       const struct v4l2_mbus_framefmt *format)
 {
+	const struct imx219_mode *mode = imx219->mode;
+	unsigned int bpp;
+	int ret = 0;
+
 	switch (format->code) {
 	case MEDIA_BUS_FMT_SRGGB8_1X8:
 	case MEDIA_BUS_FMT_SGRBG8_1X8:
 	case MEDIA_BUS_FMT_SGBRG8_1X8:
 	case MEDIA_BUS_FMT_SBGGR8_1X8:
-		return cci_multi_reg_write(imx219->regmap, raw8_framefmt_regs,
-					   ARRAY_SIZE(raw8_framefmt_regs), NULL);
+		bpp = 8;
+		break;
 
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
 	case MEDIA_BUS_FMT_SGRBG10_1X10:
 	case MEDIA_BUS_FMT_SGBRG10_1X10:
 	case MEDIA_BUS_FMT_SBGGR10_1X10:
-		return cci_multi_reg_write(imx219->regmap, raw10_framefmt_regs,
-					   ARRAY_SIZE(raw10_framefmt_regs), NULL);
+	default:
+		bpp = 10;
+		break;
 	}
 
-	return -EINVAL;
+	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
+		  mode->crop.left - IMX219_PIXEL_ARRAY_LEFT, &ret);
+	cci_write(imx219->regmap, IMX219_REG_X_ADD_END_A,
+		  mode->crop.left - IMX219_PIXEL_ARRAY_LEFT + mode->crop.width - 1,
+		  &ret);
+	cci_write(imx219->regmap, IMX219_REG_Y_ADD_STA_A,
+		  mode->crop.top - IMX219_PIXEL_ARRAY_TOP, &ret);
+	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
+		  mode->crop.top - IMX219_PIXEL_ARRAY_TOP + mode->crop.height - 1,
+		  &ret);
+
+	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
+		  format->width, &ret);
+	cci_write(imx219->regmap, IMX219_REG_Y_OUTPUT_SIZE,
+		  format->height, &ret);
+
+	cci_write(imx219->regmap, IMX219_REG_TP_WINDOW_WIDTH,
+		  format->width, &ret);
+	cci_write(imx219->regmap, IMX219_REG_TP_WINDOW_HEIGHT,
+		  format->height, &ret);
+
+	cci_write(imx219->regmap, IMX219_REG_CSI_DATA_FORMAT_A,
+		  (bpp << 8) | bpp, &ret);
+	cci_write(imx219->regmap, IMX219_REG_OPPXCK_DIV, bpp, &ret);
+
+	return ret;
 }
 
 static int imx219_set_binning(struct imx219 *imx219,
@@ -783,7 +730,6 @@ static int imx219_start_streaming(struct imx219 *imx219,
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
 	const struct v4l2_mbus_framefmt *format;
-	const struct imx219_reg_list *reg_list;
 	int ret;
 
 	ret = pm_runtime_resume_and_get(&client->dev);
@@ -805,15 +751,7 @@ static int imx219_start_streaming(struct imx219 *imx219,
 		goto err_rpm_put;
 	}
 
-	/* Apply default values of current mode */
-	reg_list = &imx219->mode->reg_list;
-	ret = cci_multi_reg_write(imx219->regmap, reg_list->regs,
-				  reg_list->num_of_regs, NULL);
-	if (ret) {
-		dev_err(&client->dev, "%s failed to set mode\n", __func__);
-		goto err_rpm_put;
-	}
-
+	/* Apply format and crop settings. */
 	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
 	ret = imx219_set_framefmt(imx219, format);
 	if (ret) {
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 07/20] media: i2c: imx219: Merge format and binning setting functions
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (5 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 06/20] media: i2c: imx219: Set mode registers programmatically Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 08/20] media: i2c: imx219: Initialize ycbcr_enc Laurent Pinchart
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

The imx219_set_binning() function sets registers based on the bpp value,
which is computed in imx219_set_framefmt(). As both functions are called
from the same place consecutively, and set registers based on the
selected mode, merge them together to simplify the code.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Changes since v2:

- Store bin_mode in a u64
---
 drivers/media/i2c/imx219.c | 43 +++++++++-----------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 927ab106cea6..140924445a61 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -614,6 +614,7 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 {
 	const struct imx219_mode *mode = imx219->mode;
 	unsigned int bpp;
+	u64 bin_mode;
 	int ret = 0;
 
 	switch (format->code) {
@@ -644,6 +645,15 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 		  mode->crop.top - IMX219_PIXEL_ARRAY_TOP + mode->crop.height - 1,
 		  &ret);
 
+	if (!imx219->mode->binning)
+		bin_mode = IMX219_BINNING_NONE;
+	else if (bpp == 8)
+		bin_mode = IMX219_BINNING_2X2_ANALOG;
+	else
+		bin_mode = IMX219_BINNING_2X2;
+
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret);
+
 	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
 		  format->width, &ret);
 	cci_write(imx219->regmap, IMX219_REG_Y_OUTPUT_SIZE,
@@ -661,32 +671,6 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 	return ret;
 }
 
-static int imx219_set_binning(struct imx219 *imx219,
-			      const struct v4l2_mbus_framefmt *format)
-{
-	if (!imx219->mode->binning)
-		return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
-				 IMX219_BINNING_NONE, NULL);
-
-	switch (format->code) {
-	case MEDIA_BUS_FMT_SRGGB8_1X8:
-	case MEDIA_BUS_FMT_SGRBG8_1X8:
-	case MEDIA_BUS_FMT_SGBRG8_1X8:
-	case MEDIA_BUS_FMT_SBGGR8_1X8:
-		return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
-				 IMX219_BINNING_2X2_ANALOG, NULL);
-
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-	case MEDIA_BUS_FMT_SGRBG10_1X10:
-	case MEDIA_BUS_FMT_SGBRG10_1X10:
-	case MEDIA_BUS_FMT_SBGGR10_1X10:
-		return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
-				 IMX219_BINNING_2X2, NULL);
-	}
-
-	return -EINVAL;
-}
-
 static int imx219_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_selection *sel)
@@ -760,13 +744,6 @@ static int imx219_start_streaming(struct imx219 *imx219,
 		goto err_rpm_put;
 	}
 
-	ret = imx219_set_binning(imx219, format);
-	if (ret) {
-		dev_err(&client->dev, "%s failed to set binning: %d\n",
-			__func__, ret);
-		goto err_rpm_put;
-	}
-
 	/* Apply customized values from user */
 	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
 	if (ret)
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 08/20] media: i2c: imx219: Initialize ycbcr_enc
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (6 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 07/20] media: i2c: imx219: Merge format and binning setting functions Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 09/20] media: i2c: imx219: Use active crop rectangle to configure registers Laurent Pinchart
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

While the ycbcr_enc field doesn't apply to raw formats, leaving it
uninitialized makes the driver behave in a less deterministic way. Fix
it by picking the default value for the colorspace.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Changes since v1:

- Use V4L2_YCBCR_ENC_601
---
 drivers/media/i2c/imx219.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 140924445a61..2e1fbeef11a8 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -495,6 +495,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
 	fmt->height = mode->height;
 	fmt->field = V4L2_FIELD_NONE;
 	fmt->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
 	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
 }
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 09/20] media: i2c: imx219: Use active crop rectangle to configure registers
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (7 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 08/20] media: i2c: imx219: Initialize ycbcr_enc Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:32 ` [PATCH v4 10/20] media: i2c: imx219: Infer binning settings from format and crop Laurent Pinchart
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Configure the crop-related registers from the values stored in the
active crop rectangle instead of the mode structure. This removes usage
of the mode from the imx219_set_framefmt(). No functional change is
intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
Changes since v2:

- Pass state to imx219_set_framefmt()
---
 drivers/media/i2c/imx219.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 2e1fbeef11a8..562cfee87b24 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -611,13 +611,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 }
 
 static int imx219_set_framefmt(struct imx219 *imx219,
-			       const struct v4l2_mbus_framefmt *format)
+			       struct v4l2_subdev_state *state)
 {
-	const struct imx219_mode *mode = imx219->mode;
+	const struct v4l2_mbus_framefmt *format;
+	const struct v4l2_rect *crop;
 	unsigned int bpp;
 	u64 bin_mode;
 	int ret = 0;
 
+	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
+	crop = v4l2_subdev_get_pad_crop(&imx219->sd, state, 0);
+
 	switch (format->code) {
 	case MEDIA_BUS_FMT_SRGGB8_1X8:
 	case MEDIA_BUS_FMT_SGRBG8_1X8:
@@ -636,15 +640,13 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 	}
 
 	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
-		  mode->crop.left - IMX219_PIXEL_ARRAY_LEFT, &ret);
+		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
 	cci_write(imx219->regmap, IMX219_REG_X_ADD_END_A,
-		  mode->crop.left - IMX219_PIXEL_ARRAY_LEFT + mode->crop.width - 1,
-		  &ret);
+		  crop->left - IMX219_PIXEL_ARRAY_LEFT + crop->width - 1, &ret);
 	cci_write(imx219->regmap, IMX219_REG_Y_ADD_STA_A,
-		  mode->crop.top - IMX219_PIXEL_ARRAY_TOP, &ret);
+		  crop->top - IMX219_PIXEL_ARRAY_TOP, &ret);
 	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
-		  mode->crop.top - IMX219_PIXEL_ARRAY_TOP + mode->crop.height - 1,
-		  &ret);
+		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
 
 	if (!imx219->mode->binning)
 		bin_mode = IMX219_BINNING_NONE;
@@ -714,7 +716,6 @@ static int imx219_start_streaming(struct imx219 *imx219,
 				  struct v4l2_subdev_state *state)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	const struct v4l2_mbus_framefmt *format;
 	int ret;
 
 	ret = pm_runtime_resume_and_get(&client->dev);
@@ -737,8 +738,7 @@ static int imx219_start_streaming(struct imx219 *imx219,
 	}
 
 	/* Apply format and crop settings. */
-	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
-	ret = imx219_set_framefmt(imx219, format);
+	ret = imx219_set_framefmt(imx219, state);
 	if (ret) {
 		dev_err(&client->dev, "%s failed to set frame format: %d\n",
 			__func__, ret);
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 10/20] media: i2c: imx219: Infer binning settings from format and crop
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (8 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 09/20] media: i2c: imx219: Use active crop rectangle to configure registers Laurent Pinchart
@ 2023-09-24 15:32 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 11/20] media: i2c: imx219: Access height from active format in imx219_set_ctrl Laurent Pinchart
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:32 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Compare the format and crop rectangle dimensions to infer binning
settings, instead of storing the binning mode in the imx219_mode
structure. This removes duplicate information from the mode.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 562cfee87b24..bc836cd13ef5 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -161,9 +161,6 @@ struct imx219_mode {
 
 	/* V-timing */
 	unsigned int vts_def;
-
-	/* 2x2 binning is used */
-	bool binning;
 };
 
 static const struct cci_reg_sequence imx219_common_regs[] = {
@@ -305,7 +302,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 2464
 		},
 		.vts_def = IMX219_VTS_15FPS,
-		.binning = false,
 	},
 	{
 		/* 1080P 30fps cropped */
@@ -318,7 +314,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 1080
 		},
 		.vts_def = IMX219_VTS_30FPS_1080P,
-		.binning = false,
 	},
 	{
 		/* 2x2 binned 30fps mode */
@@ -331,7 +326,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 2464
 		},
 		.vts_def = IMX219_VTS_30FPS_BINNED,
-		.binning = true,
 	},
 	{
 		/* 640x480 30fps mode */
@@ -344,7 +338,6 @@ static const struct imx219_mode supported_modes[] = {
 			.height = 960
 		},
 		.vts_def = IMX219_VTS_30FPS_640x480,
-		.binning = true,
 	},
 };
 
@@ -648,7 +641,7 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
 		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
 
-	if (!imx219->mode->binning)
+	if (format->width == crop->width && format->height == crop->height)
 		bin_mode = IMX219_BINNING_NONE;
 	else if (bpp == 8)
 		bin_mode = IMX219_BINNING_2X2_ANALOG;
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 11/20] media: i2c: imx219: Access height from active format in imx219_set_ctrl
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (9 preceding siblings ...)
  2023-09-24 15:32 ` [PATCH v4 10/20] media: i2c: imx219: Infer binning settings from format and crop Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 12/20] media: i2c: imx219: Don't store the current mode in the imx219 structure Laurent Pinchart
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Use the active format height instead of the mode height in
imx219_set_ctrl(). This prepares for dropping the mode field from the
imx219 structure.

The state is retrieved using v4l2_subdev_get_locked_active_state() as
the subdev active state and the control handler share the same lock.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index bc836cd13ef5..f4f684bda4fa 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -397,13 +397,18 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct imx219 *imx219 =
 		container_of(ctrl->handler, struct imx219, ctrl_handler);
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	const struct v4l2_mbus_framefmt *format;
+	struct v4l2_subdev_state *state;
 	int ret = 0;
 
+	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
+	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
+
 	if (ctrl->id == V4L2_CID_VBLANK) {
 		int exposure_max, exposure_def;
 
 		/* Update max exposure while meeting expected vblanking */
-		exposure_max = imx219->mode->height + ctrl->val - 4;
+		exposure_max = format->height + ctrl->val - 4;
 		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
 			exposure_max : IMX219_EXPOSURE_DEFAULT;
 		__v4l2_ctrl_modify_range(imx219->exposure,
@@ -443,7 +448,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_VBLANK:
 		cci_write(imx219->regmap, IMX219_REG_VTS,
-			  imx219->mode->height + ctrl->val, &ret);
+			  format->height + ctrl->val, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN_RED:
 		cci_write(imx219->regmap, IMX219_REG_TESTP_RED,
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 12/20] media: i2c: imx219: Don't store the current mode in the imx219 structure
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (10 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 11/20] media: i2c: imx219: Access height from active format in imx219_set_ctrl Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 13/20] media: i2c: imx219: Drop IMX219_VTS_* macros Laurent Pinchart
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

The mode field of the imx219 structure is only used in
imx219_init_controls(), after the probe function sets it to point to the
default mode. Use the default mode directly when initializing controls,
and drop the mode field from the imx219 structure.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index f4f684bda4fa..82e5d1dad14a 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -362,9 +362,6 @@ struct imx219 {
 	struct v4l2_ctrl *vblank;
 	struct v4l2_ctrl *hblank;
 
-	/* Current mode */
-	const struct imx219_mode *mode;
-
 	/* Two or Four lanes */
 	u8 lanes;
 };
@@ -580,7 +577,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	*crop = mode->crop;
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		imx219->mode = mode;
 		/* Update limits and set FPS to default */
 		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
 					 IMX219_VTS_MAX - mode->height, 1,
@@ -923,8 +919,8 @@ static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
 static int imx219_init_controls(struct imx219 *imx219)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	const struct imx219_mode *mode = &supported_modes[0];
 	struct v4l2_ctrl_handler *ctrl_hdlr;
-	unsigned int height = imx219->mode->height;
 	struct v4l2_fwnode_device_properties props;
 	int exposure_max, exposure_def, hblank;
 	int i, ret;
@@ -953,15 +949,15 @@ static int imx219_init_controls(struct imx219 *imx219)
 	/* Initial vblank/hblank/exposure parameters based on current mode */
 	imx219->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
 					   V4L2_CID_VBLANK, IMX219_VBLANK_MIN,
-					   IMX219_VTS_MAX - height, 1,
-					   imx219->mode->vts_def - height);
-	hblank = IMX219_PPL_DEFAULT - imx219->mode->width;
+					   IMX219_VTS_MAX - mode->height, 1,
+					   mode->vts_def - mode->height);
+	hblank = IMX219_PPL_DEFAULT - mode->width;
 	imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
 					   V4L2_CID_HBLANK, hblank, hblank,
 					   1, hblank);
 	if (imx219->hblank)
 		imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	exposure_max = imx219->mode->vts_def - 4;
+	exposure_max = mode->vts_def - 4;
 	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
 		exposure_max : IMX219_EXPOSURE_DEFAULT;
 	imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
@@ -1148,10 +1144,8 @@ static int imx219_probe(struct i2c_client *client)
 	if (ret)
 		goto error_power_off;
 
-	/* Set default mode to max resolution */
-	imx219->mode = &supported_modes[0];
-
-	/* sensor doesn't enter LP-11 state upon power up until and unless
+	/*
+	 * Sensor doesn't enter LP-11 state upon power up until and unless
 	 * streaming is started, so upon power up switch the modes to:
 	 * streaming -> standby
 	 */
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 13/20] media: i2c: imx219: Drop IMX219_VTS_* macros
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (11 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 12/20] media: i2c: imx219: Don't store the current mode in the imx219 structure Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 14/20] media: i2c: imx219: Group functions by purpose Laurent Pinchart
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

The IMX219_VTS_* macros define default VTS values for the modes
supported by the driver. They are used in a single place, and hinder
readability compared to using the value directly as a decimal number.
Drop them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 82e5d1dad14a..76551e6db22a 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -71,10 +71,6 @@
 
 /* V_TIMING internal */
 #define IMX219_REG_VTS			CCI_REG16(0x0160)
-#define IMX219_VTS_15FPS		0x0dc6
-#define IMX219_VTS_30FPS_1080P		0x06e3
-#define IMX219_VTS_30FPS_BINNED		0x06e3
-#define IMX219_VTS_30FPS_640x480	0x06e3
 #define IMX219_VTS_MAX			0xffff
 
 #define IMX219_VBLANK_MIN		4
@@ -301,7 +297,7 @@ static const struct imx219_mode supported_modes[] = {
 			.width = 3280,
 			.height = 2464
 		},
-		.vts_def = IMX219_VTS_15FPS,
+		.vts_def = 3526,
 	},
 	{
 		/* 1080P 30fps cropped */
@@ -313,7 +309,7 @@ static const struct imx219_mode supported_modes[] = {
 			.width = 1920,
 			.height = 1080
 		},
-		.vts_def = IMX219_VTS_30FPS_1080P,
+		.vts_def = 1763,
 	},
 	{
 		/* 2x2 binned 30fps mode */
@@ -325,7 +321,7 @@ static const struct imx219_mode supported_modes[] = {
 			.width = 3280,
 			.height = 2464
 		},
-		.vts_def = IMX219_VTS_30FPS_BINNED,
+		.vts_def = 1763,
 	},
 	{
 		/* 640x480 30fps mode */
@@ -337,7 +333,7 @@ static const struct imx219_mode supported_modes[] = {
 			.width = 1280,
 			.height = 960
 		},
-		.vts_def = IMX219_VTS_30FPS_640x480,
+		.vts_def = 1763,
 	},
 };
 
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 14/20] media: i2c: imx219: Group functions by purpose
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (12 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 13/20] media: i2c: imx219: Drop IMX219_VTS_* macros Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 15/20] media: i2c: imx219: Remove unneeded goto Laurent Pinchart
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Move functions around to group them by purpose, in order to improve
readability. No functional change is intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 639 +++++++++++++++++++------------------
 1 file changed, 327 insertions(+), 312 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 76551e6db22a..dfc5ce574227 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -385,6 +385,10 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 	return imx219_mbus_formats[i];
 }
 
+/* -----------------------------------------------------------------------------
+ * Controls
+ */
+
 static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct imx219 *imx219 =
@@ -476,6 +480,303 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
 	.s_ctrl = imx219_set_ctrl,
 };
 
+static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
+{
+	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
+}
+
+/* Initialize control handlers */
+static int imx219_init_controls(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	const struct imx219_mode *mode = &supported_modes[0];
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	struct v4l2_fwnode_device_properties props;
+	int exposure_max, exposure_def, hblank;
+	int i, ret;
+
+	ctrl_hdlr = &imx219->ctrl_handler;
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
+	if (ret)
+		return ret;
+
+	/* By default, PIXEL_RATE is read only */
+	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       imx219_get_pixel_rate(imx219),
+					       imx219_get_pixel_rate(imx219), 1,
+					       imx219_get_pixel_rate(imx219));
+
+	imx219->link_freq =
+		v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops,
+				       V4L2_CID_LINK_FREQ,
+				       ARRAY_SIZE(imx219_link_freq_menu) - 1, 0,
+				       (imx219->lanes == 2) ? imx219_link_freq_menu :
+				       imx219_link_freq_4lane_menu);
+	if (imx219->link_freq)
+		imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	/* Initial vblank/hblank/exposure parameters based on current mode */
+	imx219->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					   V4L2_CID_VBLANK, IMX219_VBLANK_MIN,
+					   IMX219_VTS_MAX - mode->height, 1,
+					   mode->vts_def - mode->height);
+	hblank = IMX219_PPL_DEFAULT - mode->width;
+	imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					   V4L2_CID_HBLANK, hblank, hblank,
+					   1, hblank);
+	if (imx219->hblank)
+		imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	exposure_max = mode->vts_def - 4;
+	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+		exposure_max : IMX219_EXPOSURE_DEFAULT;
+	imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					     V4L2_CID_EXPOSURE,
+					     IMX219_EXPOSURE_MIN, exposure_max,
+					     IMX219_EXPOSURE_STEP,
+					     exposure_def);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+			  IMX219_ANA_GAIN_MIN, IMX219_ANA_GAIN_MAX,
+			  IMX219_ANA_GAIN_STEP, IMX219_ANA_GAIN_DEFAULT);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+			  IMX219_DGTL_GAIN_MIN, IMX219_DGTL_GAIN_MAX,
+			  IMX219_DGTL_GAIN_STEP, IMX219_DGTL_GAIN_DEFAULT);
+
+	imx219->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (imx219->hflip)
+		imx219->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	imx219->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (imx219->vflip)
+		imx219->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx219_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx219_test_pattern_menu) - 1,
+				     0, 0, imx219_test_pattern_menu);
+	for (i = 0; i < 4; i++) {
+		/*
+		 * The assumption is that
+		 * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
+		 * V4L2_CID_TEST_PATTERN_BLUE   == V4L2_CID_TEST_PATTERN_RED + 2
+		 * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
+		 */
+		v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+				  V4L2_CID_TEST_PATTERN_RED + i,
+				  IMX219_TESTP_COLOUR_MIN,
+				  IMX219_TESTP_COLOUR_MAX,
+				  IMX219_TESTP_COLOUR_STEP,
+				  IMX219_TESTP_COLOUR_MAX);
+		/* The "Solid color" pattern is white by default */
+	}
+
+	if (ctrl_hdlr->error) {
+		ret = ctrl_hdlr->error;
+		dev_err(&client->dev, "%s control init failed (%d)\n",
+			__func__, ret);
+		goto error;
+	}
+
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		goto error;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx219_ctrl_ops,
+					      &props);
+	if (ret)
+		goto error;
+
+	imx219->sd.ctrl_handler = ctrl_hdlr;
+
+	return 0;
+
+error:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+
+	return ret;
+}
+
+static void imx219_free_controls(struct imx219 *imx219)
+{
+	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
+}
+
+/* -----------------------------------------------------------------------------
+ * Subdev operations
+ */
+
+static int imx219_set_framefmt(struct imx219 *imx219,
+			       struct v4l2_subdev_state *state)
+{
+	const struct v4l2_mbus_framefmt *format;
+	const struct v4l2_rect *crop;
+	unsigned int bpp;
+	u64 bin_mode;
+	int ret = 0;
+
+	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
+	crop = v4l2_subdev_get_pad_crop(&imx219->sd, state, 0);
+
+	switch (format->code) {
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+		bpp = 8;
+		break;
+
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+	default:
+		bpp = 10;
+		break;
+	}
+
+	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
+		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
+	cci_write(imx219->regmap, IMX219_REG_X_ADD_END_A,
+		  crop->left - IMX219_PIXEL_ARRAY_LEFT + crop->width - 1, &ret);
+	cci_write(imx219->regmap, IMX219_REG_Y_ADD_STA_A,
+		  crop->top - IMX219_PIXEL_ARRAY_TOP, &ret);
+	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
+		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
+
+	if (format->width == crop->width && format->height == crop->height)
+		bin_mode = IMX219_BINNING_NONE;
+	else if (bpp == 8)
+		bin_mode = IMX219_BINNING_2X2_ANALOG;
+	else
+		bin_mode = IMX219_BINNING_2X2;
+
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret);
+
+	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
+		  format->width, &ret);
+	cci_write(imx219->regmap, IMX219_REG_Y_OUTPUT_SIZE,
+		  format->height, &ret);
+
+	cci_write(imx219->regmap, IMX219_REG_TP_WINDOW_WIDTH,
+		  format->width, &ret);
+	cci_write(imx219->regmap, IMX219_REG_TP_WINDOW_HEIGHT,
+		  format->height, &ret);
+
+	cci_write(imx219->regmap, IMX219_REG_CSI_DATA_FORMAT_A,
+		  (bpp << 8) | bpp, &ret);
+	cci_write(imx219->regmap, IMX219_REG_OPPXCK_DIV, bpp, &ret);
+
+	return ret;
+}
+
+static int imx219_configure_lanes(struct imx219 *imx219)
+{
+	return cci_write(imx219->regmap, IMX219_REG_CSI_LANE_MODE,
+			 imx219->lanes == 2 ? IMX219_CSI_2_LANE_MODE :
+			 IMX219_CSI_4_LANE_MODE, NULL);
+};
+
+static int imx219_start_streaming(struct imx219 *imx219,
+				  struct v4l2_subdev_state *state)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&client->dev);
+	if (ret < 0)
+		return ret;
+
+	/* Send all registers that are common to all modes */
+	ret = cci_multi_reg_write(imx219->regmap, imx219_common_regs,
+				  ARRAY_SIZE(imx219_common_regs), NULL);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to send mfg header\n", __func__);
+		goto err_rpm_put;
+	}
+
+	/* Configure two or four Lane mode */
+	ret = imx219_configure_lanes(imx219);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to configure lanes\n", __func__);
+		goto err_rpm_put;
+	}
+
+	/* Apply format and crop settings. */
+	ret = imx219_set_framefmt(imx219, state);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to set frame format: %d\n",
+			__func__, ret);
+		goto err_rpm_put;
+	}
+
+	/* Apply customized values from user */
+	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
+	if (ret)
+		goto err_rpm_put;
+
+	/* set stream on register */
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STREAMING, NULL);
+	if (ret)
+		goto err_rpm_put;
+
+	/* vflip and hflip cannot change during streaming */
+	__v4l2_ctrl_grab(imx219->vflip, true);
+	__v4l2_ctrl_grab(imx219->hflip, true);
+
+	return 0;
+
+err_rpm_put:
+	pm_runtime_put(&client->dev);
+	return ret;
+}
+
+static void imx219_stop_streaming(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+
+	/* set stream off register */
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STANDBY, NULL);
+	if (ret)
+		dev_err(&client->dev, "%s failed to set stream\n", __func__);
+
+	__v4l2_ctrl_grab(imx219->vflip, false);
+	__v4l2_ctrl_grab(imx219->hflip, false);
+
+	pm_runtime_put(&client->dev);
+}
+
+static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_subdev_state *state;
+	int ret = 0;
+
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+
+	if (enable) {
+		/*
+		 * Apply default & customized values
+		 * and then start streaming.
+		 */
+		ret = imx219_start_streaming(imx219, state);
+		if (ret)
+			goto unlock;
+	} else {
+		imx219_stop_streaming(imx219);
+	}
+
+unlock:
+	v4l2_subdev_unlock_state(state);
+	return ret;
+}
+
 static void imx219_update_pad_format(struct imx219 *imx219,
 				     const struct imx219_mode *mode,
 				     struct v4l2_mbus_framefmt *fmt, u32 code)
@@ -600,70 +901,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int imx219_set_framefmt(struct imx219 *imx219,
-			       struct v4l2_subdev_state *state)
-{
-	const struct v4l2_mbus_framefmt *format;
-	const struct v4l2_rect *crop;
-	unsigned int bpp;
-	u64 bin_mode;
-	int ret = 0;
-
-	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
-	crop = v4l2_subdev_get_pad_crop(&imx219->sd, state, 0);
-
-	switch (format->code) {
-	case MEDIA_BUS_FMT_SRGGB8_1X8:
-	case MEDIA_BUS_FMT_SGRBG8_1X8:
-	case MEDIA_BUS_FMT_SGBRG8_1X8:
-	case MEDIA_BUS_FMT_SBGGR8_1X8:
-		bpp = 8;
-		break;
-
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-	case MEDIA_BUS_FMT_SGRBG10_1X10:
-	case MEDIA_BUS_FMT_SGBRG10_1X10:
-	case MEDIA_BUS_FMT_SBGGR10_1X10:
-	default:
-		bpp = 10;
-		break;
-	}
-
-	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
-		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
-	cci_write(imx219->regmap, IMX219_REG_X_ADD_END_A,
-		  crop->left - IMX219_PIXEL_ARRAY_LEFT + crop->width - 1, &ret);
-	cci_write(imx219->regmap, IMX219_REG_Y_ADD_STA_A,
-		  crop->top - IMX219_PIXEL_ARRAY_TOP, &ret);
-	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
-		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
-
-	if (format->width == crop->width && format->height == crop->height)
-		bin_mode = IMX219_BINNING_NONE;
-	else if (bpp == 8)
-		bin_mode = IMX219_BINNING_2X2_ANALOG;
-	else
-		bin_mode = IMX219_BINNING_2X2;
-
-	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret);
-
-	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
-		  format->width, &ret);
-	cci_write(imx219->regmap, IMX219_REG_Y_OUTPUT_SIZE,
-		  format->height, &ret);
-
-	cci_write(imx219->regmap, IMX219_REG_TP_WINDOW_WIDTH,
-		  format->width, &ret);
-	cci_write(imx219->regmap, IMX219_REG_TP_WINDOW_HEIGHT,
-		  format->height, &ret);
-
-	cci_write(imx219->regmap, IMX219_REG_CSI_DATA_FORMAT_A,
-		  (bpp << 8) | bpp, &ret);
-	cci_write(imx219->regmap, IMX219_REG_OPPXCK_DIV, bpp, &ret);
-
-	return ret;
-}
-
 static int imx219_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_selection *sel)
@@ -695,111 +932,35 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 	return -EINVAL;
 }
 
-static int imx219_configure_lanes(struct imx219 *imx219)
-{
-	return cci_write(imx219->regmap, IMX219_REG_CSI_LANE_MODE,
-			 imx219->lanes == 2 ? IMX219_CSI_2_LANE_MODE :
-			 IMX219_CSI_4_LANE_MODE, NULL);
+static const struct v4l2_subdev_core_ops imx219_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
-static int imx219_start_streaming(struct imx219 *imx219,
-				  struct v4l2_subdev_state *state)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	int ret;
+static const struct v4l2_subdev_video_ops imx219_video_ops = {
+	.s_stream = imx219_set_stream,
+};
 
-	ret = pm_runtime_resume_and_get(&client->dev);
-	if (ret < 0)
-		return ret;
+static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
+	.init_cfg = imx219_init_cfg,
+	.enum_mbus_code = imx219_enum_mbus_code,
+	.get_fmt = v4l2_subdev_get_fmt,
+	.set_fmt = imx219_set_pad_format,
+	.get_selection = imx219_get_selection,
+	.enum_frame_size = imx219_enum_frame_size,
+};
 
-	/* Send all registers that are common to all modes */
-	ret = cci_multi_reg_write(imx219->regmap, imx219_common_regs,
-				  ARRAY_SIZE(imx219_common_regs), NULL);
-	if (ret) {
-		dev_err(&client->dev, "%s failed to send mfg header\n", __func__);
-		goto err_rpm_put;
-	}
+static const struct v4l2_subdev_ops imx219_subdev_ops = {
+	.core = &imx219_core_ops,
+	.video = &imx219_video_ops,
+	.pad = &imx219_pad_ops,
+};
 
-	/* Configure two or four Lane mode */
-	ret = imx219_configure_lanes(imx219);
-	if (ret) {
-		dev_err(&client->dev, "%s failed to configure lanes\n", __func__);
-		goto err_rpm_put;
-	}
 
-	/* Apply format and crop settings. */
-	ret = imx219_set_framefmt(imx219, state);
-	if (ret) {
-		dev_err(&client->dev, "%s failed to set frame format: %d\n",
-			__func__, ret);
-		goto err_rpm_put;
-	}
+/* -----------------------------------------------------------------------------
+ * Power management
+ */
 
-	/* Apply customized values from user */
-	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
-	if (ret)
-		goto err_rpm_put;
-
-	/* set stream on register */
-	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
-			IMX219_MODE_STREAMING, NULL);
-	if (ret)
-		goto err_rpm_put;
-
-	/* vflip and hflip cannot change during streaming */
-	__v4l2_ctrl_grab(imx219->vflip, true);
-	__v4l2_ctrl_grab(imx219->hflip, true);
-
-	return 0;
-
-err_rpm_put:
-	pm_runtime_put(&client->dev);
-	return ret;
-}
-
-static void imx219_stop_streaming(struct imx219 *imx219)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	int ret;
-
-	/* set stream off register */
-	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
-			IMX219_MODE_STANDBY, NULL);
-	if (ret)
-		dev_err(&client->dev, "%s failed to set stream\n", __func__);
-
-	__v4l2_ctrl_grab(imx219->vflip, false);
-	__v4l2_ctrl_grab(imx219->hflip, false);
-
-	pm_runtime_put(&client->dev);
-}
-
-static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
-{
-	struct imx219 *imx219 = to_imx219(sd);
-	struct v4l2_subdev_state *state;
-	int ret = 0;
-
-	state = v4l2_subdev_lock_and_get_active_state(sd);
-
-	if (enable) {
-		/*
-		 * Apply default & customized values
-		 * and then start streaming.
-		 */
-		ret = imx219_start_streaming(imx219, state);
-		if (ret)
-			goto unlock;
-	} else {
-		imx219_stop_streaming(imx219);
-	}
-
-unlock:
-	v4l2_subdev_unlock_state(state);
-	return ret;
-}
-
-/* Power/clock management functions */
 static int imx219_power_on(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -845,6 +1006,10 @@ static int imx219_power_off(struct device *dev)
 	return 0;
 }
 
+/* -----------------------------------------------------------------------------
+ * Probe & remove
+ */
+
 static int imx219_get_regulators(struct imx219 *imx219)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
@@ -881,156 +1046,6 @@ static int imx219_identify_module(struct imx219 *imx219)
 	return 0;
 }
 
-static const struct v4l2_subdev_core_ops imx219_core_ops = {
-	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
-	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
-};
-
-static const struct v4l2_subdev_video_ops imx219_video_ops = {
-	.s_stream = imx219_set_stream,
-};
-
-static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
-	.init_cfg = imx219_init_cfg,
-	.enum_mbus_code = imx219_enum_mbus_code,
-	.get_fmt = v4l2_subdev_get_fmt,
-	.set_fmt = imx219_set_pad_format,
-	.get_selection = imx219_get_selection,
-	.enum_frame_size = imx219_enum_frame_size,
-};
-
-static const struct v4l2_subdev_ops imx219_subdev_ops = {
-	.core = &imx219_core_ops,
-	.video = &imx219_video_ops,
-	.pad = &imx219_pad_ops,
-};
-
-
-static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
-{
-	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
-}
-
-/* Initialize control handlers */
-static int imx219_init_controls(struct imx219 *imx219)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	const struct imx219_mode *mode = &supported_modes[0];
-	struct v4l2_ctrl_handler *ctrl_hdlr;
-	struct v4l2_fwnode_device_properties props;
-	int exposure_max, exposure_def, hblank;
-	int i, ret;
-
-	ctrl_hdlr = &imx219->ctrl_handler;
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
-	if (ret)
-		return ret;
-
-	/* By default, PIXEL_RATE is read only */
-	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
-					       V4L2_CID_PIXEL_RATE,
-					       imx219_get_pixel_rate(imx219),
-					       imx219_get_pixel_rate(imx219), 1,
-					       imx219_get_pixel_rate(imx219));
-
-	imx219->link_freq =
-		v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops,
-				       V4L2_CID_LINK_FREQ,
-				       ARRAY_SIZE(imx219_link_freq_menu) - 1, 0,
-				       (imx219->lanes == 2) ? imx219_link_freq_menu :
-				       imx219_link_freq_4lane_menu);
-	if (imx219->link_freq)
-		imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
-	/* Initial vblank/hblank/exposure parameters based on current mode */
-	imx219->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
-					   V4L2_CID_VBLANK, IMX219_VBLANK_MIN,
-					   IMX219_VTS_MAX - mode->height, 1,
-					   mode->vts_def - mode->height);
-	hblank = IMX219_PPL_DEFAULT - mode->width;
-	imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
-					   V4L2_CID_HBLANK, hblank, hblank,
-					   1, hblank);
-	if (imx219->hblank)
-		imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	exposure_max = mode->vts_def - 4;
-	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
-		exposure_max : IMX219_EXPOSURE_DEFAULT;
-	imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
-					     V4L2_CID_EXPOSURE,
-					     IMX219_EXPOSURE_MIN, exposure_max,
-					     IMX219_EXPOSURE_STEP,
-					     exposure_def);
-
-	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
-			  IMX219_ANA_GAIN_MIN, IMX219_ANA_GAIN_MAX,
-			  IMX219_ANA_GAIN_STEP, IMX219_ANA_GAIN_DEFAULT);
-
-	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
-			  IMX219_DGTL_GAIN_MIN, IMX219_DGTL_GAIN_MAX,
-			  IMX219_DGTL_GAIN_STEP, IMX219_DGTL_GAIN_DEFAULT);
-
-	imx219->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
-					  V4L2_CID_HFLIP, 0, 1, 1, 0);
-	if (imx219->hflip)
-		imx219->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
-
-	imx219->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
-					  V4L2_CID_VFLIP, 0, 1, 1, 0);
-	if (imx219->vflip)
-		imx219->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
-
-	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx219_ctrl_ops,
-				     V4L2_CID_TEST_PATTERN,
-				     ARRAY_SIZE(imx219_test_pattern_menu) - 1,
-				     0, 0, imx219_test_pattern_menu);
-	for (i = 0; i < 4; i++) {
-		/*
-		 * The assumption is that
-		 * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
-		 * V4L2_CID_TEST_PATTERN_BLUE   == V4L2_CID_TEST_PATTERN_RED + 2
-		 * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
-		 */
-		v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
-				  V4L2_CID_TEST_PATTERN_RED + i,
-				  IMX219_TESTP_COLOUR_MIN,
-				  IMX219_TESTP_COLOUR_MAX,
-				  IMX219_TESTP_COLOUR_STEP,
-				  IMX219_TESTP_COLOUR_MAX);
-		/* The "Solid color" pattern is white by default */
-	}
-
-	if (ctrl_hdlr->error) {
-		ret = ctrl_hdlr->error;
-		dev_err(&client->dev, "%s control init failed (%d)\n",
-			__func__, ret);
-		goto error;
-	}
-
-	ret = v4l2_fwnode_device_parse(&client->dev, &props);
-	if (ret)
-		goto error;
-
-	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx219_ctrl_ops,
-					      &props);
-	if (ret)
-		goto error;
-
-	imx219->sd.ctrl_handler = ctrl_hdlr;
-
-	return 0;
-
-error:
-	v4l2_ctrl_handler_free(ctrl_hdlr);
-
-	return ret;
-}
-
-static void imx219_free_controls(struct imx219 *imx219)
-{
-	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
-}
-
 static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
 {
 	struct fwnode_handle *endpoint;
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 15/20] media: i2c: imx219: Remove unneeded goto
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (13 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 14/20] media: i2c: imx219: Group functions by purpose Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-25 10:44   ` Dave Stevenson
  2023-09-24 15:33 ` [PATCH v4 16/20] media: i2c: imx219: Implement .init_cfg() using .set_fmt() Laurent Pinchart
                   ` (4 subsequent siblings)
  19 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Simplify the imx219_set_stream() by removing an unneeded goto statement,
and its corresponding error label. The natural flow of the function is
correct.

While at it, drop a useless comment.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index dfc5ce574227..a12ebf288dd2 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -760,19 +760,11 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
 
 	state = v4l2_subdev_lock_and_get_active_state(sd);
 
-	if (enable) {
-		/*
-		 * Apply default & customized values
-		 * and then start streaming.
-		 */
+	if (enable)
 		ret = imx219_start_streaming(imx219, state);
-		if (ret)
-			goto unlock;
-	} else {
+	else
 		imx219_stop_streaming(imx219);
-	}
 
-unlock:
 	v4l2_subdev_unlock_state(state);
 	return ret;
 }
-- 
Regards,

Laurent Pinchart


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

* Re: [PATCH v4 15/20] media: i2c: imx219: Remove unneeded goto
  2023-09-24 15:33 ` [PATCH v4 15/20] media: i2c: imx219: Remove unneeded goto Laurent Pinchart
@ 2023-09-25 10:44   ` Dave Stevenson
  0 siblings, 0 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-09-25 10:44 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, Sakari Ailus, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

On Sun, 24 Sept 2023 at 16:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Simplify the imx219_set_stream() by removing an unneeded goto statement,
> and its corresponding error label. The natural flow of the function is
> correct.
>
> While at it, drop a useless comment.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/imx219.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index dfc5ce574227..a12ebf288dd2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -760,19 +760,11 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
>
>         state = v4l2_subdev_lock_and_get_active_state(sd);
>
> -       if (enable) {
> -               /*
> -                * Apply default & customized values
> -                * and then start streaming.
> -                */
> +       if (enable)
>                 ret = imx219_start_streaming(imx219, state);
> -               if (ret)
> -                       goto unlock;
> -       } else {
> +       else
>                 imx219_stop_streaming(imx219);
> -       }
>
> -unlock:
>         v4l2_subdev_unlock_state(state);
>         return ret;
>  }
> --
> Regards,
>
> Laurent Pinchart
>

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

* [PATCH v4 16/20] media: i2c: imx219: Implement .init_cfg() using .set_fmt()
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (14 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 15/20] media: i2c: imx219: Remove unneeded goto Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 17/20] media: i2c: imx219: Separate horizontal and vertical binning Laurent Pinchart
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Instead of duplicating the logical implemented in the .set_fmt()
operation in .init_cfg(), call .set_fmt() directly. This centralizes the
format and crop rectangle calculations in a single place.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 40 +++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index a12ebf288dd2..6bfdceaf5044 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -784,28 +784,6 @@ static void imx219_update_pad_format(struct imx219 *imx219,
 	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
 }
 
-static int imx219_init_cfg(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *state)
-{
-	struct imx219 *imx219 = to_imx219(sd);
-	struct v4l2_mbus_framefmt *format;
-	struct v4l2_rect *crop;
-
-	/* Initialize the format. */
-	format = v4l2_subdev_get_pad_format(sd, state, 0);
-	imx219_update_pad_format(imx219, &supported_modes[0], format,
-				 MEDIA_BUS_FMT_SRGGB10_1X10);
-
-	/* Initialize the crop rectangle. */
-	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
-	crop->top = IMX219_PIXEL_ARRAY_TOP;
-	crop->left = IMX219_PIXEL_ARRAY_LEFT;
-	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
-	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
-
-	return 0;
-}
-
 static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -924,6 +902,24 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 	return -EINVAL;
 }
 
+static int imx219_init_cfg(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_state *state)
+{
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = 0,
+		.format = {
+			.code = MEDIA_BUS_FMT_SRGGB10_1X10,
+			.width = supported_modes[0].width,
+			.height = supported_modes[0].height,
+		},
+	};
+
+	imx219_set_pad_format(sd, state, &fmt);
+
+	return 0;
+}
+
 static const struct v4l2_subdev_core_ops imx219_core_ops = {
 	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 17/20] media: i2c: imx219: Separate horizontal and vertical binning
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (15 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 16/20] media: i2c: imx219: Implement .init_cfg() using .set_fmt() Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically Laurent Pinchart
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

The IMX219 has distinct binning registers for the horizontal and
vertical directions. Calculate their value and write them separately.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 39 ++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 6bfdceaf5044..bf1c2a1dad95 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -90,10 +90,11 @@
 #define IMX219_REG_ORIENTATION		CCI_REG8(0x0172)
 
 /* Binning  Mode */
-#define IMX219_REG_BINNING_MODE		CCI_REG16(0x0174)
-#define IMX219_BINNING_NONE		0x0000
-#define IMX219_BINNING_2X2		0x0101
-#define IMX219_BINNING_2X2_ANALOG	0x0303
+#define IMX219_REG_BINNING_MODE_H	CCI_REG8(0x0174)
+#define IMX219_REG_BINNING_MODE_V	CCI_REG8(0x0175)
+#define IMX219_BINNING_NONE		0x00
+#define IMX219_BINNING_X2		0x01
+#define IMX219_BINNING_X2_ANALOG	0x03
 
 #define IMX219_REG_CSI_DATA_FORMAT_A	CCI_REG16(0x018c)
 
@@ -615,7 +616,7 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 	const struct v4l2_mbus_framefmt *format;
 	const struct v4l2_rect *crop;
 	unsigned int bpp;
-	u64 bin_mode;
+	u64 bin_h, bin_v;
 	int ret = 0;
 
 	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
@@ -647,14 +648,28 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
 		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
 
-	if (format->width == crop->width && format->height == crop->height)
-		bin_mode = IMX219_BINNING_NONE;
-	else if (bpp == 8)
-		bin_mode = IMX219_BINNING_2X2_ANALOG;
-	else
-		bin_mode = IMX219_BINNING_2X2;
+	switch (crop->width / format->width) {
+	case 1:
+	default:
+		bin_h = IMX219_BINNING_NONE;
+		break;
+	case 2:
+		bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
+		break;
+	}
 
-	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret);
+	switch (crop->height / format->height) {
+	case 1:
+	default:
+		bin_v = IMX219_BINNING_NONE;
+		break;
+	case 2:
+		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
+		break;
+	}
+
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
 
 	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
 		  format->width, &ret);
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (16 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 17/20] media: i2c: imx219: Separate horizontal and vertical binning Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2025-04-15 14:20   ` Dave Stevenson
  2023-09-24 15:33 ` [PATCH v4 19/20] media: i2c: imx219: Name all subdev state variables 'state' Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 20/20] media: i2c: imx219: Move variables to inner scope Laurent Pinchart
  19 siblings, 1 reply; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Calculate the crop rectangle size and location dynamically when setting
the format, instead of storing it in the imx219_mode structure. This
removes duplicated information from the mode, to guarantee consistency.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
Changes since v2:

- Handle horizontal and vertical binning separately
---
 drivers/media/i2c/imx219.c | 45 +++++++++++++-------------------------
 1 file changed, 15 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index bf1c2a1dad95..2b88c5b8a7bf 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
@@ -153,9 +154,6 @@ struct imx219_mode {
 	/* Frame height */
 	unsigned int height;
 
-	/* Analog crop rectangle. */
-	struct v4l2_rect crop;
-
 	/* V-timing */
 	unsigned int vts_def;
 };
@@ -292,48 +290,24 @@ static const struct imx219_mode supported_modes[] = {
 		/* 8MPix 15fps mode */
 		.width = 3280,
 		.height = 2464,
-		.crop = {
-			.left = IMX219_PIXEL_ARRAY_LEFT,
-			.top = IMX219_PIXEL_ARRAY_TOP,
-			.width = 3280,
-			.height = 2464
-		},
 		.vts_def = 3526,
 	},
 	{
 		/* 1080P 30fps cropped */
 		.width = 1920,
 		.height = 1080,
-		.crop = {
-			.left = 688,
-			.top = 700,
-			.width = 1920,
-			.height = 1080
-		},
 		.vts_def = 1763,
 	},
 	{
 		/* 2x2 binned 30fps mode */
 		.width = 1640,
 		.height = 1232,
-		.crop = {
-			.left = IMX219_PIXEL_ARRAY_LEFT,
-			.top = IMX219_PIXEL_ARRAY_TOP,
-			.width = 3280,
-			.height = 2464
-		},
 		.vts_def = 1763,
 	},
 	{
 		/* 640x480 30fps mode */
 		.width = 640,
 		.height = 480,
-		.crop = {
-			.left = 1008,
-			.top = 760,
-			.width = 1280,
-			.height = 960
-		},
 		.vts_def = 1763,
 	},
 };
@@ -844,6 +818,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	int exposure_max, exposure_def, hblank;
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
+	unsigned int bin_h, bin_v;
 
 	mode = v4l2_find_nearest_size(supported_modes,
 				      ARRAY_SIZE(supported_modes),
@@ -853,10 +828,20 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
 
 	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
-	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
-
 	*format = fmt->format;
-	*crop = mode->crop;
+
+	/*
+	 * Use binning to maximize the crop rectangle size, and centre it in the
+	 * sensor.
+	 */
+	bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
+	bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
+
+	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
+	crop->width = format->width * bin_h;
+	crop->height = format->height * bin_v;
+	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
+	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		/* Update limits and set FPS to default */
-- 
Regards,

Laurent Pinchart


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

* Re: [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically
  2023-09-24 15:33 ` [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically Laurent Pinchart
@ 2025-04-15 14:20   ` Dave Stevenson
  2025-04-25 12:06     ` Laurent Pinchart
  0 siblings, 1 reply; 24+ messages in thread
From: Dave Stevenson @ 2025-04-15 14:20 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, Sakari Ailus, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Hi Laurent

On Sun, 24 Sept 2023 at 16:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Calculate the crop rectangle size and location dynamically when setting
> the format, instead of storing it in the imx219_mode structure. This
> removes duplicated information from the mode, to guarantee consistency.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> Changes since v2:
>
> - Handle horizontal and vertical binning separately
> ---
>  drivers/media/i2c/imx219.c | 45 +++++++++++++-------------------------
>  1 file changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index bf1c2a1dad95..2b88c5b8a7bf 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -18,6 +18,7 @@
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> @@ -153,9 +154,6 @@ struct imx219_mode {
>         /* Frame height */
>         unsigned int height;
>
> -       /* Analog crop rectangle. */
> -       struct v4l2_rect crop;
> -
>         /* V-timing */
>         unsigned int vts_def;
>  };
> @@ -292,48 +290,24 @@ static const struct imx219_mode supported_modes[] = {
>                 /* 8MPix 15fps mode */
>                 .width = 3280,
>                 .height = 2464,
> -               .crop = {
> -                       .left = IMX219_PIXEL_ARRAY_LEFT,
> -                       .top = IMX219_PIXEL_ARRAY_TOP,
> -                       .width = 3280,
> -                       .height = 2464
> -               },
>                 .vts_def = 3526,
>         },
>         {
>                 /* 1080P 30fps cropped */
>                 .width = 1920,
>                 .height = 1080,
> -               .crop = {
> -                       .left = 688,
> -                       .top = 700,
> -                       .width = 1920,
> -                       .height = 1080
> -               },
>                 .vts_def = 1763,
>         },
>         {
>                 /* 2x2 binned 30fps mode */
>                 .width = 1640,
>                 .height = 1232,
> -               .crop = {
> -                       .left = IMX219_PIXEL_ARRAY_LEFT,
> -                       .top = IMX219_PIXEL_ARRAY_TOP,
> -                       .width = 3280,
> -                       .height = 2464
> -               },
>                 .vts_def = 1763,
>         },
>         {
>                 /* 640x480 30fps mode */
>                 .width = 640,
>                 .height = 480,
> -               .crop = {
> -                       .left = 1008,
> -                       .top = 760,
> -                       .width = 1280,
> -                       .height = 960
> -               },
>                 .vts_def = 1763,
>         },
>  };
> @@ -844,6 +818,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         int exposure_max, exposure_def, hblank;
>         struct v4l2_mbus_framefmt *format;
>         struct v4l2_rect *crop;
> +       unsigned int bin_h, bin_v;
>
>         mode = v4l2_find_nearest_size(supported_modes,
>                                       ARRAY_SIZE(supported_modes),
> @@ -853,10 +828,20 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
>
>         format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> -       crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> -
>         *format = fmt->format;
> -       *crop = mode->crop;
> +
> +       /*
> +        * Use binning to maximize the crop rectangle size, and centre it in the
> +        * sensor.
> +        */
> +       bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
> +       bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);

This patch missed 6.6 as the previous LTS, so we've only just noticed
this regression with 6.12. It's also present in 6.15-rc2.

The 1920x1080 mode will give you bin_h of 1, but bin_v of 2 as 1080 <
(2480/2). You therefore get a weirdly stretched image from the sensor.

I accept that having a nicely generic cropping/binning configuration
is ideal, but currently this is a mode based driver, and there is no
API to allow for configuring generic cropping/binning configuration.
Until that API exists and can be implemented for previously mode-based
drivers without regression, I see the only real options being either
- move the binning setup back into struct imx219_mode and defined in
supported modes
- set the two binning values to be the same as the minimum of the two
computed values.
Which would be preferred?

Thanks
  Dave

> +
> +       crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> +       crop->width = format->width * bin_h;
> +       crop->height = format->height * bin_v;
> +       crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
> +       crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
>
>         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>                 /* Update limits and set FPS to default */
> --
> Regards,
>
> Laurent Pinchart
>

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

* Re: [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically
  2025-04-15 14:20   ` Dave Stevenson
@ 2025-04-25 12:06     ` Laurent Pinchart
  0 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2025-04-25 12:06 UTC (permalink / raw)
  To: Dave Stevenson
  Cc: linux-media, Sakari Ailus, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Hi Dave,

On Tue, Apr 15, 2025 at 03:20:40PM +0100, Dave Stevenson wrote:
> On Sun, 24 Sept 2023 at 16:33, Laurent Pinchart wrote:
> >
> > Calculate the crop rectangle size and location dynamically when setting
> > the format, instead of storing it in the imx219_mode structure. This
> > removes duplicated information from the mode, to guarantee consistency.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Handle horizontal and vertical binning separately
> > ---
> >  drivers/media/i2c/imx219.c | 45 +++++++++++++-------------------------
> >  1 file changed, 15 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index bf1c2a1dad95..2b88c5b8a7bf 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> > +#include <linux/minmax.h>
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> > @@ -153,9 +154,6 @@ struct imx219_mode {
> >         /* Frame height */
> >         unsigned int height;
> >
> > -       /* Analog crop rectangle. */
> > -       struct v4l2_rect crop;
> > -
> >         /* V-timing */
> >         unsigned int vts_def;
> >  };
> > @@ -292,48 +290,24 @@ static const struct imx219_mode supported_modes[] = {
> >                 /* 8MPix 15fps mode */
> >                 .width = 3280,
> >                 .height = 2464,
> > -               .crop = {
> > -                       .left = IMX219_PIXEL_ARRAY_LEFT,
> > -                       .top = IMX219_PIXEL_ARRAY_TOP,
> > -                       .width = 3280,
> > -                       .height = 2464
> > -               },
> >                 .vts_def = 3526,
> >         },
> >         {
> >                 /* 1080P 30fps cropped */
> >                 .width = 1920,
> >                 .height = 1080,
> > -               .crop = {
> > -                       .left = 688,
> > -                       .top = 700,
> > -                       .width = 1920,
> > -                       .height = 1080
> > -               },
> >                 .vts_def = 1763,
> >         },
> >         {
> >                 /* 2x2 binned 30fps mode */
> >                 .width = 1640,
> >                 .height = 1232,
> > -               .crop = {
> > -                       .left = IMX219_PIXEL_ARRAY_LEFT,
> > -                       .top = IMX219_PIXEL_ARRAY_TOP,
> > -                       .width = 3280,
> > -                       .height = 2464
> > -               },
> >                 .vts_def = 1763,
> >         },
> >         {
> >                 /* 640x480 30fps mode */
> >                 .width = 640,
> >                 .height = 480,
> > -               .crop = {
> > -                       .left = 1008,
> > -                       .top = 760,
> > -                       .width = 1280,
> > -                       .height = 960
> > -               },
> >                 .vts_def = 1763,
> >         },
> >  };
> > @@ -844,6 +818,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         int exposure_max, exposure_def, hblank;
> >         struct v4l2_mbus_framefmt *format;
> >         struct v4l2_rect *crop;
> > +       unsigned int bin_h, bin_v;
> >
> >         mode = v4l2_find_nearest_size(supported_modes,
> >                                       ARRAY_SIZE(supported_modes),
> > @@ -853,10 +828,20 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
> >
> >         format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > -       crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > -
> >         *format = fmt->format;
> > -       *crop = mode->crop;
> > +
> > +       /*
> > +        * Use binning to maximize the crop rectangle size, and centre it in the
> > +        * sensor.
> > +        */
> > +       bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
> > +       bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
> 
> This patch missed 6.6 as the previous LTS, so we've only just noticed
> this regression with 6.12. It's also present in 6.15-rc2.
> 
> The 1920x1080 mode will give you bin_h of 1, but bin_v of 2 as 1080 <
> (2480/2). You therefore get a weirdly stretched image from the sensor.
> 
> I accept that having a nicely generic cropping/binning configuration
> is ideal, but currently this is a mode based driver, and there is no
> API to allow for configuring generic cropping/binning configuration.
> Until that API exists and can be implemented for previously mode-based
> drivers without regression, I see the only real options being either
> - move the binning setup back into struct imx219_mode and defined in
> supported modes
> - set the two binning values to be the same as the minimum of the two
> computed values.
> Which would be preferred?

Different horizontal and vertical binning factors seem to be a niche use
case to me, so I think I'd prefer the second option. It should also be
the simplest to implement. Would you like to send a patch ?

> > +
> > +       crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > +       crop->width = format->width * bin_h;
> > +       crop->height = format->height * bin_v;
> > +       crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
> > +       crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
> >
> >         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >                 /* Update limits and set FPS to default */

-- 
Regards,

Laurent Pinchart

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

* [PATCH v4 19/20] media: i2c: imx219: Name all subdev state variables 'state'
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (17 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  2023-09-24 15:33 ` [PATCH v4 20/20] media: i2c: imx219: Move variables to inner scope Laurent Pinchart
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

Subdev state variables are named with a mix of 'state' and 'sd_state'
through the driver. To improve consistency, name them all 'state'.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 2b88c5b8a7bf..358a6d77d88c 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -774,7 +774,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
 }
 
 static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
-				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_state *state,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
 	struct imx219 *imx219 = to_imx219(sd);
@@ -788,7 +788,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
 }
 
 static int imx219_enum_frame_size(struct v4l2_subdev *sd,
-				  struct v4l2_subdev_state *sd_state,
+				  struct v4l2_subdev_state *state,
 				  struct v4l2_subdev_frame_size_enum *fse)
 {
 	struct imx219 *imx219 = to_imx219(sd);
@@ -810,7 +810,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 }
 
 static int imx219_set_pad_format(struct v4l2_subdev *sd,
-				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_state *state,
 				 struct v4l2_subdev_format *fmt)
 {
 	struct imx219 *imx219 = to_imx219(sd);
@@ -827,7 +827,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 
 	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
 
-	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
+	format = v4l2_subdev_get_pad_format(sd, state, 0);
 	*format = fmt->format;
 
 	/*
@@ -837,7 +837,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
 	bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
 
-	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
+	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
 	crop->width = format->width * bin_h;
 	crop->height = format->height * bin_v;
 	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
@@ -872,12 +872,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 }
 
 static int imx219_get_selection(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_state *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);
+		sel->r = *v4l2_subdev_get_pad_crop(sd, state, 0);
 		return 0;
 	}
 
-- 
Regards,

Laurent Pinchart


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

* [PATCH v4 20/20] media: i2c: imx219: Move variables to inner scope
  2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
                   ` (18 preceding siblings ...)
  2023-09-24 15:33 ` [PATCH v4 19/20] media: i2c: imx219: Name all subdev state variables 'state' Laurent Pinchart
@ 2023-09-24 15:33 ` Laurent Pinchart
  19 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-09-24 15:33 UTC (permalink / raw)
  To: linux-media
  Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar,
	Hans de Goede

The exposure_max, exposure_def and hblank variables are only used in an
inner scope in the imx219_set_pad_format() function. Move them to that
scope to keep them closer to their usage and improve readability.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 358a6d77d88c..8436880dcf7a 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -815,7 +815,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 {
 	struct imx219 *imx219 = to_imx219(sd);
 	const struct imx219_mode *mode;
-	int exposure_max, exposure_def, hblank;
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
 	unsigned int bin_h, bin_v;
@@ -844,6 +843,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+		int exposure_max;
+		int exposure_def;
+		int hblank;
+
 		/* Update limits and set FPS to default */
 		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
 					 IMX219_VTS_MAX - mode->height, 1,
-- 
Regards,

Laurent Pinchart


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

end of thread, other threads:[~2025-04-25 12:06 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-24 15:32 [PATCH v4 00/20] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 01/20] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 02/20] media: i2c: imx219: Drop unused macros Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 03/20] media: i2c: imx219: Replace register addresses with macros Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 04/20] media: i2c: imx219: Drop IMX219_REG_CSI_LANE_MODE from common regs array Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 05/20] media: i2c: imx219: Fix test pattern window for 640x480 mode Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 06/20] media: i2c: imx219: Set mode registers programmatically Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 07/20] media: i2c: imx219: Merge format and binning setting functions Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 08/20] media: i2c: imx219: Initialize ycbcr_enc Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 09/20] media: i2c: imx219: Use active crop rectangle to configure registers Laurent Pinchart
2023-09-24 15:32 ` [PATCH v4 10/20] media: i2c: imx219: Infer binning settings from format and crop Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 11/20] media: i2c: imx219: Access height from active format in imx219_set_ctrl Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 12/20] media: i2c: imx219: Don't store the current mode in the imx219 structure Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 13/20] media: i2c: imx219: Drop IMX219_VTS_* macros Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 14/20] media: i2c: imx219: Group functions by purpose Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 15/20] media: i2c: imx219: Remove unneeded goto Laurent Pinchart
2023-09-25 10:44   ` Dave Stevenson
2023-09-24 15:33 ` [PATCH v4 16/20] media: i2c: imx219: Implement .init_cfg() using .set_fmt() Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 17/20] media: i2c: imx219: Separate horizontal and vertical binning Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically Laurent Pinchart
2025-04-15 14:20   ` Dave Stevenson
2025-04-25 12:06     ` Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 19/20] media: i2c: imx219: Name all subdev state variables 'state' Laurent Pinchart
2023-09-24 15:33 ` [PATCH v4 20/20] media: i2c: imx219: Move variables to inner scope Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox