* [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements
@ 2023-08-15 18:24 Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 01/12] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
` (11 more replies)
0 siblings, 12 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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/12. 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 04/12 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.
The series depends on two fixes for the imx219 driver ([2]) that should
make it to v6.6, while the 12 patches here are v6.7 material.
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/
[2] https://lore.kernel.org/linux-media/20230814193435.24158-1-laurent.pinchart@ideasonboard.com
Laurent Pinchart (12):
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: 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
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/imx219.c | 668 +++++++++++++------------------------
2 files changed, 241 insertions(+), 428 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 01/12] media: i2c: imx219: Convert to CCI register access helpers
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 02/12] media: i2c: imx219: Drop unused macros Laurent Pinchart
` (10 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Dave Stevenson, Jacopo Mondi, Lad Prabhakar, Hans de Goede
Use the new comon 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>
---
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 226454b6a90d..f7cea5c53ead 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -118,6 +118,7 @@ config VIDEO_IMX219
depends on I2C && VIDEO_DEV
select MEDIA_CONTROLLER
select VIDEO_V4L2_SUBDEV_API
+ select V4L2_CCI_I2C
select V4L2_FWNODE
help
This is a Video4Linux2 sensor driver for the Sony
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 2822ce8b065b..39695b0e6537 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;
@@ -491,78 +485,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)
{
@@ -586,7 +508,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;
@@ -610,48 +532,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,
@@ -802,15 +721,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;
@@ -819,28 +738,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;
@@ -879,9 +794,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,
@@ -897,7 +812,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;
@@ -912,7 +828,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;
@@ -939,8 +856,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;
@@ -961,8 +878,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__);
@@ -1101,10 +1018,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);
@@ -1112,7 +1028,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;
}
@@ -1336,6 +1252,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)) {
@@ -1379,17 +1302,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] 16+ messages in thread
* [PATCH v1 02/12] media: i2c: imx219: Drop unused macros
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 01/12] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 03/12] media: i2c: imx219: Replace register addresses with macros Laurent Pinchart
` (9 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 39695b0e6537..26ee33c09e6a 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] 16+ messages in thread
* [PATCH v1 03/12] media: i2c: imx219: Replace register addresses with macros
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 01/12] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 02/12] media: i2c: imx219: Drop unused macros Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 04/12] media: i2c: imx219: Fix test pattern window for 640x480 mode Laurent Pinchart
` (8 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 26ee33c09e6a..70a4cb4e152c 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(24) },
};
/*
@@ -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] 16+ messages in thread
* [PATCH v1 04/12] media: i2c: imx219: Fix test pattern window for 640x480 mode
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (2 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 03/12] media: i2c: imx219: Replace register addresses with macros Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 05/12] media: i2c: imx219: Set mode registers programmatically Laurent Pinchart
` (7 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 70a4cb4e152c..a0b746447df2 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -264,8 +264,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] 16+ messages in thread
* [PATCH v1 05/12] media: i2c: imx219: Set mode registers programmatically
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (3 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 04/12] media: i2c: imx219: Fix test pattern window for 640x480 mode Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 06/12] media: i2c: imx219: Merge format and binning setting functions Laurent Pinchart
` (6 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 a0b746447df2..9f1656bc3302 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;
};
@@ -219,65 +211,6 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
{ IMX219_REG_EXCK_FREQ, IMX219_EXCK_FREQ(24) },
};
-/*
- * 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,
};
@@ -373,10 +306,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,
},
{
@@ -390,10 +319,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,
},
{
@@ -407,10 +332,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,
},
{
@@ -424,10 +345,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,
},
};
@@ -699,23 +616,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,
@@ -787,7 +734,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);
@@ -809,15 +755,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] 16+ messages in thread
* [PATCH v1 06/12] media: i2c: imx219: Merge format and binning setting functions
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (4 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 05/12] media: i2c: imx219: Set mode registers programmatically Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc Laurent Pinchart
` (5 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 9f1656bc3302..8c61b748d9a5 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -618,6 +618,7 @@ static int imx219_set_framefmt(struct imx219 *imx219,
{
const struct imx219_mode *mode = imx219->mode;
unsigned int bpp;
+ u16 bin_mode;
int ret = 0;
switch (format->code) {
@@ -648,6 +649,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,
@@ -665,32 +675,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)
@@ -764,13 +748,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] 16+ messages in thread
* [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (5 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 06/12] media: i2c: imx219: Merge format and binning setting functions Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-17 11:00 ` Dave Stevenson
2023-08-15 18:24 ` [PATCH v1 08/12] media: i2c: imx219: Use active crop rectangle to configure registers Laurent Pinchart
` (4 subsequent siblings)
11 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 8c61b748d9a5..976014ed7711 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -499,6 +499,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_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
fmt->xfer_func = V4L2_XFER_FUNC_NONE;
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 08/12] media: i2c: imx219: Use active crop rectangle to configure registers
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (6 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 09/12] media: i2c: imx219: Infer binning settings from format and crop Laurent Pinchart
` (3 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
drivers/media/i2c/imx219.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 976014ed7711..c9f3b3db4458 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -615,9 +615,9 @@ 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 v4l2_mbus_framefmt *format,
+ const struct v4l2_rect *crop)
{
- const struct imx219_mode *mode = imx219->mode;
unsigned int bpp;
u16 bin_mode;
int ret = 0;
@@ -640,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;
@@ -719,6 +717,7 @@ 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 v4l2_rect *crop;
int ret;
ret = pm_runtime_resume_and_get(&client->dev);
@@ -742,7 +741,8 @@ 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);
+ crop = v4l2_subdev_get_pad_crop(&imx219->sd, state, 0);
+ ret = imx219_set_framefmt(imx219, format, crop);
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] 16+ messages in thread
* [PATCH v1 09/12] media: i2c: imx219: Infer binning settings from format and crop
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (7 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 08/12] media: i2c: imx219: Use active crop rectangle to configure registers Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 10/12] media: i2c: imx219: Access height from active format in imx219_set_ctrl Laurent Pinchart
` (2 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 c9f3b3db4458..71edb100e877 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[] = {
@@ -306,7 +303,6 @@ static const struct imx219_mode supported_modes[] = {
.height = 2464
},
.vts_def = IMX219_VTS_15FPS,
- .binning = false,
},
{
/* 1080P 30fps cropped */
@@ -319,7 +315,6 @@ static const struct imx219_mode supported_modes[] = {
.height = 1080
},
.vts_def = IMX219_VTS_30FPS_1080P,
- .binning = false,
},
{
/* 2x2 binned 30fps mode */
@@ -332,7 +327,6 @@ static const struct imx219_mode supported_modes[] = {
.height = 2464
},
.vts_def = IMX219_VTS_30FPS_BINNED,
- .binning = true,
},
{
/* 640x480 30fps mode */
@@ -345,7 +339,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] 16+ messages in thread
* [PATCH v1 10/12] media: i2c: imx219: Access height from active format in imx219_set_ctrl
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (8 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 09/12] media: i2c: imx219: Infer binning settings from format and crop Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 11/12] media: i2c: imx219: Don't store the current mode in the imx219 structure Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 12/12] media: i2c: imx219: Drop IMX219_VTS_* macros Laurent Pinchart
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 71edb100e877..85b7e3f4f4d0 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -401,13 +401,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,
@@ -447,7 +452,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] 16+ messages in thread
* [PATCH v1 11/12] media: i2c: imx219: Don't store the current mode in the imx219 structure
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (9 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 10/12] media: i2c: imx219: Access height from active format in imx219_set_ctrl Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 12/12] media: i2c: imx219: Drop IMX219_VTS_* macros Laurent Pinchart
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 85b7e3f4f4d0..c9c68a0f3b46 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -363,9 +363,6 @@ struct imx219 {
struct v4l2_ctrl *vblank;
struct v4l2_ctrl *hblank;
- /* Current mode */
- const struct imx219_mode *mode;
-
/* Streaming on/off */
bool streaming;
@@ -584,7 +581,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,
@@ -967,8 +963,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;
@@ -997,15 +993,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,
@@ -1192,10 +1188,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] 16+ messages in thread
* [PATCH v1 12/12] media: i2c: imx219: Drop IMX219_VTS_* macros
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
` (10 preceding siblings ...)
2023-08-15 18:24 ` [PATCH v1 11/12] media: i2c: imx219: Don't store the current mode in the imx219 structure Laurent Pinchart
@ 2023-08-15 18:24 ` Laurent Pinchart
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-15 18:24 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>
---
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 c9c68a0f3b46..3a742716ddd6 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
@@ -302,7 +298,7 @@ static const struct imx219_mode supported_modes[] = {
.width = 3280,
.height = 2464
},
- .vts_def = IMX219_VTS_15FPS,
+ .vts_def = 3526,
},
{
/* 1080P 30fps cropped */
@@ -314,7 +310,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 */
@@ -326,7 +322,7 @@ static const struct imx219_mode supported_modes[] = {
.width = 3280,
.height = 2464
},
- .vts_def = IMX219_VTS_30FPS_BINNED,
+ .vts_def = 1763,
},
{
/* 640x480 30fps mode */
@@ -338,7 +334,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] 16+ messages in thread
* Re: [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc
2023-08-15 18:24 ` [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc Laurent Pinchart
@ 2023-08-17 11:00 ` Dave Stevenson
2023-08-17 11:59 ` Laurent Pinchart
0 siblings, 1 reply; 16+ messages in thread
From: Dave Stevenson @ 2023-08-17 11:00 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Sakari Ailus, Jacopo Mondi, Lad Prabhakar, Hans de Goede
Hi Laurent
Thanks for the patch
On Tue, 15 Aug 2023 at 19:24, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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>
> ---
> 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 8c61b748d9a5..976014ed7711 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -499,6 +499,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_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> fmt->xfer_func = V4L2_XFER_FUNC_NONE;
In [1] for imx290 you requested that I change from using the
V4L2_MAP_xxx_DEFAULT macros to hardcode them, and now you're mixing
and matching the two in the same driver.
Could we have some consistency please? Personally I don't mind which
is used, but mixing and matching within a driver feels wrong.
(If there is a genuine desire for V4L2_MAP_xxx_DEFAULT or hardcoding
in sensor drivers, it'd be nice if it was documented to avoid
additional review cycles).
Also just noting that you seem not to be using get_maintainers for
your patches as I appear not to have been included.
Dave
[1] https://patchwork.linuxtv.org/project/linux-media/patch/20230131192016.3476937-3-dave.stevenson@raspberrypi.com/#144299
> }
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc
2023-08-17 11:00 ` Dave Stevenson
@ 2023-08-17 11:59 ` Laurent Pinchart
2023-08-21 22:30 ` Laurent Pinchart
0 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-17 11:59 UTC (permalink / raw)
To: Dave Stevenson
Cc: linux-media, Sakari Ailus, Jacopo Mondi, Lad Prabhakar,
Hans de Goede, Hans Verkuil
Hi Dave,
(CC'ing Hans Verkuil).
On Thu, Aug 17, 2023 at 12:00:10PM +0100, Dave Stevenson wrote:
> On Tue, 15 Aug 2023 at 19:24, Laurent Pinchart wrote:
> >
> > 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>
> > ---
> > 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 8c61b748d9a5..976014ed7711 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -499,6 +499,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_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>
> In [1] for imx290 you requested that I change from using the
> V4L2_MAP_xxx_DEFAULT macros to hardcode them, and now you're mixing
> and matching the two in the same driver.
> Could we have some consistency please? Personally I don't mind which
> is used, but mixing and matching within a driver feels wrong.
> (If there is a genuine desire for V4L2_MAP_xxx_DEFAULT or hardcoding
> in sensor drivers, it'd be nice if it was documented to avoid
> additional review cycles).
Absolutely, sorry about this. I'll fix it in v2 and use
V4L2_MAP_YCBCR_ENC_601.
Hans, should we add a V4L2_YCBCR_ENC_NONE for non-YUV formats ?
> Also just noting that you seem not to be using get_maintainers for
> your patches as I appear not to have been included.
I'm not sure how that happened, as I use get_maintainer.pl. I probably
made a mistake somewhere. Sorry about that. I was actually looking
forward to your review of the series :-)
> [1] https://patchwork.linuxtv.org/project/linux-media/patch/20230131192016.3476937-3-dave.stevenson@raspberrypi.com/#144299
>
> > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc
2023-08-17 11:59 ` Laurent Pinchart
@ 2023-08-21 22:30 ` Laurent Pinchart
0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-08-21 22:30 UTC (permalink / raw)
To: Dave Stevenson
Cc: linux-media, Sakari Ailus, Jacopo Mondi, Lad Prabhakar,
Hans de Goede, Hans Verkuil
Hi Dave,
On Thu, Aug 17, 2023 at 02:59:25PM +0300, Laurent Pinchart wrote:
> Hi Dave,
>
> (CC'ing Hans Verkuil).
>
> On Thu, Aug 17, 2023 at 12:00:10PM +0100, Dave Stevenson wrote:
> > On Tue, 15 Aug 2023 at 19:24, Laurent Pinchart wrote:
> > >
> > > 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>
> > > ---
> > > 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 8c61b748d9a5..976014ed7711 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -499,6 +499,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_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > > fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> >
> > In [1] for imx290 you requested that I change from using the
> > V4L2_MAP_xxx_DEFAULT macros to hardcode them, and now you're mixing
> > and matching the two in the same driver.
> > Could we have some consistency please? Personally I don't mind which
> > is used, but mixing and matching within a driver feels wrong.
> > (If there is a genuine desire for V4L2_MAP_xxx_DEFAULT or hardcoding
> > in sensor drivers, it'd be nice if it was documented to avoid
> > additional review cycles).
>
> Absolutely, sorry about this. I'll fix it in v2 and use
> V4L2_MAP_YCBCR_ENC_601.
>
> Hans, should we add a V4L2_YCBCR_ENC_NONE for non-YUV formats ?
>
> > Also just noting that you seem not to be using get_maintainers for
> > your patches as I appear not to have been included.
>
> I'm not sure how that happened, as I use get_maintainer.pl. I probably
> made a mistake somewhere. Sorry about that. I was actually looking
> forward to your review of the series :-)
Actually, you were on CC, so I'm not sure why you haven't received the
series.
> > [1] https://patchwork.linuxtv.org/project/linux-media/patch/20230131192016.3476937-3-dave.stevenson@raspberrypi.com/#144299
> >
> > > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-08-21 22:30 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-15 18:24 [PATCH v1 00/12] media: i2c: imx219: Miscellaneous cleanups and improvements Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 01/12] media: i2c: imx219: Convert to CCI register access helpers Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 02/12] media: i2c: imx219: Drop unused macros Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 03/12] media: i2c: imx219: Replace register addresses with macros Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 04/12] media: i2c: imx219: Fix test pattern window for 640x480 mode Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 05/12] media: i2c: imx219: Set mode registers programmatically Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 06/12] media: i2c: imx219: Merge format and binning setting functions Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 07/12] media: i2c: imx219: Initialize ycbcr_enc Laurent Pinchart
2023-08-17 11:00 ` Dave Stevenson
2023-08-17 11:59 ` Laurent Pinchart
2023-08-21 22:30 ` Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 08/12] media: i2c: imx219: Use active crop rectangle to configure registers Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 09/12] media: i2c: imx219: Infer binning settings from format and crop Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 10/12] media: i2c: imx219: Access height from active format in imx219_set_ctrl Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 11/12] media: i2c: imx219: Don't store the current mode in the imx219 structure Laurent Pinchart
2023-08-15 18:24 ` [PATCH v1 12/12] media: i2c: imx219: Drop IMX219_VTS_* macros Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).