There are four modes, and each mode has a table of registers.
Some of the registers are common to all modes, so create new
tables for these common registers to reduce duplicate code.
Signed-off-by: Adam Ford <[email protected]>
Reviewed-by: Dave Stevenson <[email protected]>
---
V4: No Change
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 77bd79a5954e..7f44d62047b6 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -145,23 +145,61 @@ struct imx219_mode {
struct imx219_reg_list reg_list;
};
-/*
- * 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 imx219_reg mode_3280x2464_regs[] = {
- {0x0100, 0x00},
+static const struct imx219_reg imx219_common_regs[] = {
+ {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},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
+
+ /* 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},
+
+ /* 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},
+
+ /* Frame Bank Register Group "A" */
+ {0x0162, 0x0d}, /* Line_Length_A */
+ {0x0163, 0x78},
+ {0x0170, 0x01}, /* X_ODD_INC_A */
+ {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},
+};
+
+/*
+ * 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 imx219_reg mode_3280x2464_regs[] = {
{0x0164, 0x00},
{0x0165, 0x00},
{0x0166, 0x0c},
@@ -174,53 +212,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
{0x016d, 0xd0},
{0x016e, 0x09},
{0x016f, 0xa0},
- {0x0170, 0x01},
- {0x0171, 0x01},
- {0x0174, 0x00},
+ {0x0174, 0x00}, /* No-Binning */
{0x0175, 0x00},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x0c},
{0x0625, 0xd0},
{0x0626, 0x09},
{0x0627, 0xa0},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
- {0x0162, 0x0d},
- {0x0163, 0x78},
};
static const struct imx219_reg mode_1920_1080_regs[] = {
- {0x0100, 0x00},
- {0x30eb, 0x05},
- {0x30eb, 0x0c},
- {0x300a, 0xff},
- {0x300b, 0xff},
- {0x30eb, 0x05},
- {0x30eb, 0x09},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
- {0x012b, 0x00},
- {0x0162, 0x0d},
- {0x0163, 0x78},
{0x0164, 0x02},
{0x0165, 0xa8},
{0x0166, 0x0a},
@@ -233,49 +233,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
{0x016d, 0x80},
{0x016e, 0x04},
{0x016f, 0x38},
- {0x0170, 0x01},
- {0x0171, 0x01},
- {0x0174, 0x00},
+ {0x0174, 0x00}, /* No-Binning */
{0x0175, 0x00},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x07},
{0x0625, 0x80},
{0x0626, 0x04},
{0x0627, 0x38},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
};
static const struct imx219_reg mode_1640_1232_regs[] = {
- {0x0100, 0x00},
- {0x30eb, 0x0c},
- {0x30eb, 0x05},
- {0x300a, 0xff},
- {0x300b, 0xff},
- {0x30eb, 0x05},
- {0x30eb, 0x09},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
- {0x012b, 0x00},
{0x0164, 0x00},
{0x0165, 0x00},
{0x0166, 0x0c},
@@ -288,53 +254,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
{0x016d, 0x68},
{0x016e, 0x04},
{0x016f, 0xd0},
- {0x0170, 0x01},
- {0x0171, 0x01},
- {0x0174, 0x01},
+ {0x0174, 0x01}, /* x2-Binning */
{0x0175, 0x01},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x06},
{0x0625, 0x68},
{0x0626, 0x04},
{0x0627, 0xd0},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
- {0x0162, 0x0d},
- {0x0163, 0x78},
};
static const struct imx219_reg mode_640_480_regs[] = {
- {0x0100, 0x00},
- {0x30eb, 0x05},
- {0x30eb, 0x0c},
- {0x300a, 0xff},
- {0x300b, 0xff},
- {0x30eb, 0x05},
- {0x30eb, 0x09},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
- {0x012b, 0x00},
- {0x0162, 0x0d},
- {0x0163, 0x78},
{0x0164, 0x03},
{0x0165, 0xe8},
{0x0166, 0x08},
@@ -347,35 +275,12 @@ static const struct imx219_reg mode_640_480_regs[] = {
{0x016d, 0x80},
{0x016e, 0x01},
{0x016f, 0xe0},
- {0x0170, 0x01},
- {0x0171, 0x01},
- {0x0174, 0x03},
+ {0x0174, 0x03}, /* x2-analog binning */
{0x0175, 0x03},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x06},
{0x0625, 0x68},
{0x0626, 0x04},
{0x0627, 0xd0},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
};
static const struct imx219_reg raw8_framefmt_regs[] = {
@@ -1041,6 +946,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
if (ret < 0)
return ret;
+ /* Send all registers that are common to all modes */
+ ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs));
+ if (ret) {
+ dev_err(&client->dev, "%s failed to send mfg header\n", __func__);
+ goto err_rpm_put;
+ }
+
/* 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);
--
2.34.1
The imx219 camera is capable of either two-lane or four-lane
operation. When operating in four-lane, both the pixel rate and
link frequency change. Regardless of the mode, however, both
frequencies remain fixed.
Helper functions are needed to read and set pixel and link frequencies
which also reduces the number of fixed registers in the table of modes.
Signed-off-by: Adam Ford <[email protected]>
---
V4: Restore check for nr_of_link_frequencies and update commit message
V3: Keep the helper function doing the link and lane parsing to
keep th probe function small.
V2: Replace if-else statements with ternary operator
Fix 4-lane Link Rate.
Fix checking the link rate so only the link rate for
the given number of lanes is permitted.
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 7f44d62047b6..b5fa4986470a 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -42,10 +42,16 @@
/* External clock frequency is 24.0M */
#define IMX219_XCLK_FREQ 24000000
-/* Pixel rate is fixed at 182.4M for all the modes */
+/* 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_CSI_2_LANE_MODE 0x01
+#define IMX219_CSI_4_LANE_MODE 0x03
/* V_TIMING internal */
#define IMX219_REG_VTS 0x0160
@@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
IMX219_DEFAULT_LINK_FREQ,
};
+static const s64 imx219_link_freq_4lane_menu[] = {
+ IMX219_DEFAULT_LINK_FREQ_4LANE,
+};
+
static const char * const imx219_test_pattern_menu[] = {
"Disabled",
"Color Bars",
@@ -474,6 +484,9 @@ struct imx219 {
/* Streaming on/off */
bool streaming;
+
+ /* Two or Four lanes */
+ u8 lanes;
};
static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
@@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
return -EINVAL;
}
+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);
+};
+
static int imx219_start_streaming(struct imx219 *imx219)
{
struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
@@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
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 default values of current mode */
reg_list = &imx219->mode->reg_list;
ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
@@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
.open = imx219_open,
};
+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)
{
@@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
/* By default, PIXEL_RATE is read only */
imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
V4L2_CID_PIXEL_RATE,
- IMX219_PIXEL_RATE,
- IMX219_PIXEL_RATE, 1,
- IMX219_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_link_freq_menu);
+ (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;
@@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
mutex_destroy(&imx219->mutex);
}
-static int imx219_check_hwcfg(struct device *dev)
+static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
{
struct fwnode_handle *endpoint;
struct v4l2_fwnode_endpoint ep_cfg = {
@@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
}
/* Check the number of MIPI CSI2 data lanes */
- if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
- dev_err(dev, "only 2 data lanes are currently supported\n");
+ if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
+ ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
+ dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
goto error_out;
}
+ imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
/* Check the link frequency set in device tree */
if (!ep_cfg.nr_of_link_frequencies) {
@@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
}
if (ep_cfg.nr_of_link_frequencies != 1 ||
- ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
+ (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
+ IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
dev_err(dev, "Link frequency not supported: %lld\n",
ep_cfg.link_frequencies[0]);
goto error_out;
@@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
/* Check the hardware configuration in device tree */
- if (imx219_check_hwcfg(dev))
+ if (imx219_check_hwcfg(dev, imx219))
return -EINVAL;
/* Get system clock (xclk) */
--
2.34.1
On Tue, Dec 20, 2022 at 6:08 AM Adam Ford <[email protected]> wrote:
>
> The imx219 camera is capable of either two-lane or four-lane
> operation. When operating in four-lane, both the pixel rate and
> link frequency change. Regardless of the mode, however, both
> frequencies remain fixed.
>
> Helper functions are needed to read and set pixel and link frequencies
> which also reduces the number of fixed registers in the table of modes.
>
> Signed-off-by: Adam Ford <[email protected]>
Gentle nudge on this.
> ---
> V4: Restore check for nr_of_link_frequencies and update commit message
> V3: Keep the helper function doing the link and lane parsing to
> keep th probe function small.
>
> V2: Replace if-else statements with ternary operator
> Fix 4-lane Link Rate.
> Fix checking the link rate so only the link rate for
> the given number of lanes is permitted.
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 7f44d62047b6..b5fa4986470a 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -42,10 +42,16 @@
> /* External clock frequency is 24.0M */
> #define IMX219_XCLK_FREQ 24000000
>
> -/* Pixel rate is fixed at 182.4M for all the modes */
> +/* 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_CSI_2_LANE_MODE 0x01
> +#define IMX219_CSI_4_LANE_MODE 0x03
>
> /* V_TIMING internal */
> #define IMX219_REG_VTS 0x0160
> @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
> IMX219_DEFAULT_LINK_FREQ,
> };
>
> +static const s64 imx219_link_freq_4lane_menu[] = {
> + IMX219_DEFAULT_LINK_FREQ_4LANE,
> +};
> +
> static const char * const imx219_test_pattern_menu[] = {
> "Disabled",
> "Color Bars",
> @@ -474,6 +484,9 @@ struct imx219 {
>
> /* Streaming on/off */
> bool streaming;
> +
> + /* Two or Four lanes */
> + u8 lanes;
> };
>
> static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> return -EINVAL;
> }
>
> +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);
> +};
> +
> static int imx219_start_streaming(struct imx219 *imx219)
> {
> struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> 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 default values of current mode */
> reg_list = &imx219->mode->reg_list;
> ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> .open = imx219_open,
> };
>
> +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)
> {
> @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
> /* By default, PIXEL_RATE is read only */
> imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> V4L2_CID_PIXEL_RATE,
> - IMX219_PIXEL_RATE,
> - IMX219_PIXEL_RATE, 1,
> - IMX219_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_link_freq_menu);
> + (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;
>
> @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
> mutex_destroy(&imx219->mutex);
> }
>
> -static int imx219_check_hwcfg(struct device *dev)
> +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> {
> struct fwnode_handle *endpoint;
> struct v4l2_fwnode_endpoint ep_cfg = {
> @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
> }
>
> /* Check the number of MIPI CSI2 data lanes */
> - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> - dev_err(dev, "only 2 data lanes are currently supported\n");
> + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> + dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> goto error_out;
> }
> + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
>
> /* Check the link frequency set in device tree */
> if (!ep_cfg.nr_of_link_frequencies) {
> @@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
> }
>
> if (ep_cfg.nr_of_link_frequencies != 1 ||
> - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> + (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> dev_err(dev, "Link frequency not supported: %lld\n",
> ep_cfg.link_frequencies[0]);
> goto error_out;
> @@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
> v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
>
> /* Check the hardware configuration in device tree */
> - if (imx219_check_hwcfg(dev))
> + if (imx219_check_hwcfg(dev, imx219))
> return -EINVAL;
>
> /* Get system clock (xclk) */
> --
> 2.34.1
>
Hi Adam
Sorry, for the delay.
On Fri, 13 Jan 2023 at 18:41, Adam Ford <[email protected]> wrote:
>
> On Tue, Dec 20, 2022 at 6:08 AM Adam Ford <[email protected]> wrote:
> >
> > The imx219 camera is capable of either two-lane or four-lane
> > operation. When operating in four-lane, both the pixel rate and
> > link frequency change. Regardless of the mode, however, both
> > frequencies remain fixed.
> >
> > Helper functions are needed to read and set pixel and link frequencies
> > which also reduces the number of fixed registers in the table of modes.
> >
> > Signed-off-by: Adam Ford <[email protected]>
I can't test on 4 lanes, but 2 lane modes still work, and the code
looks reasonable for 4 lanes.
Reviewed-by: Dave Stevenson <[email protected]>
> Gentle nudge on this.
>
> > ---
> > V4: Restore check for nr_of_link_frequencies and update commit message
> > V3: Keep the helper function doing the link and lane parsing to
> > keep th probe function small.
> >
> > V2: Replace if-else statements with ternary operator
> > Fix 4-lane Link Rate.
> > Fix checking the link rate so only the link rate for
> > the given number of lanes is permitted.
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 7f44d62047b6..b5fa4986470a 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -42,10 +42,16 @@
> > /* External clock frequency is 24.0M */
> > #define IMX219_XCLK_FREQ 24000000
> >
> > -/* Pixel rate is fixed at 182.4M for all the modes */
> > +/* 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_CSI_2_LANE_MODE 0x01
> > +#define IMX219_CSI_4_LANE_MODE 0x03
> >
> > /* V_TIMING internal */
> > #define IMX219_REG_VTS 0x0160
> > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
> > IMX219_DEFAULT_LINK_FREQ,
> > };
> >
> > +static const s64 imx219_link_freq_4lane_menu[] = {
> > + IMX219_DEFAULT_LINK_FREQ_4LANE,
> > +};
> > +
> > static const char * const imx219_test_pattern_menu[] = {
> > "Disabled",
> > "Color Bars",
> > @@ -474,6 +484,9 @@ struct imx219 {
> >
> > /* Streaming on/off */
> > bool streaming;
> > +
> > + /* Two or Four lanes */
> > + u8 lanes;
> > };
> >
> > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > return -EINVAL;
> > }
> >
> > +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);
> > +};
> > +
> > static int imx219_start_streaming(struct imx219 *imx219)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > 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 default values of current mode */
> > reg_list = &imx219->mode->reg_list;
> > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> > .open = imx219_open,
> > };
> >
> > +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)
> > {
> > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
> > /* By default, PIXEL_RATE is read only */
> > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > V4L2_CID_PIXEL_RATE,
> > - IMX219_PIXEL_RATE,
> > - IMX219_PIXEL_RATE, 1,
> > - IMX219_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_link_freq_menu);
> > + (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;
> >
> > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
> > mutex_destroy(&imx219->mutex);
> > }
> >
> > -static int imx219_check_hwcfg(struct device *dev)
> > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > {
> > struct fwnode_handle *endpoint;
> > struct v4l2_fwnode_endpoint ep_cfg = {
> > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
> > }
> >
> > /* Check the number of MIPI CSI2 data lanes */
> > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > - dev_err(dev, "only 2 data lanes are currently supported\n");
> > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> > goto error_out;
> > }
> > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> >
> > /* Check the link frequency set in device tree */
> > if (!ep_cfg.nr_of_link_frequencies) {
> > @@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
> > }
> >
> > if (ep_cfg.nr_of_link_frequencies != 1 ||
> > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> > + (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > dev_err(dev, "Link frequency not supported: %lld\n",
> > ep_cfg.link_frequencies[0]);
> > goto error_out;
> > @@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
> > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> >
> > /* Check the hardware configuration in device tree */
> > - if (imx219_check_hwcfg(dev))
> > + if (imx219_check_hwcfg(dev, imx219))
> > return -EINVAL;
> >
> > /* Get system clock (xclk) */
> > --
> > 2.34.1
> >
On Mon, Jan 16, 2023 at 8:42 AM Dave Stevenson
<[email protected]> wrote:
>
> Hi Adam
>
> Sorry, for the delay.
>
> On Fri, 13 Jan 2023 at 18:41, Adam Ford <[email protected]> wrote:
> >
> > On Tue, Dec 20, 2022 at 6:08 AM Adam Ford <[email protected]> wrote:
> > >
> > > The imx219 camera is capable of either two-lane or four-lane
> > > operation. When operating in four-lane, both the pixel rate and
> > > link frequency change. Regardless of the mode, however, both
> > > frequencies remain fixed.
> > >
> > > Helper functions are needed to read and set pixel and link frequencies
> > > which also reduces the number of fixed registers in the table of modes.
> > >
> > > Signed-off-by: Adam Ford <[email protected]>
>
> I can't test on 4 lanes, but 2 lane modes still work, and the code
> looks reasonable for 4 lanes.
Thanks! For what it's worth, I tested this on an Renesas RZ/G2M SoC
which has both a 2-lane and 4-lane interface, and I didn't see any
difference in performance or image quality between the 2-lane and
4-lane modes.
adam
>
> Reviewed-by: Dave Stevenson <[email protected]>
>
>
> > Gentle nudge on this.
> >
> > > ---
> > > V4: Restore check for nr_of_link_frequencies and update commit message
> > > V3: Keep the helper function doing the link and lane parsing to
> > > keep th probe function small.
> > >
> > > V2: Replace if-else statements with ternary operator
> > > Fix 4-lane Link Rate.
> > > Fix checking the link rate so only the link rate for
> > > the given number of lanes is permitted.
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 7f44d62047b6..b5fa4986470a 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -42,10 +42,16 @@
> > > /* External clock frequency is 24.0M */
> > > #define IMX219_XCLK_FREQ 24000000
> > >
> > > -/* Pixel rate is fixed at 182.4M for all the modes */
> > > +/* 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_CSI_2_LANE_MODE 0x01
> > > +#define IMX219_CSI_4_LANE_MODE 0x03
> > >
> > > /* V_TIMING internal */
> > > #define IMX219_REG_VTS 0x0160
> > > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
> > > IMX219_DEFAULT_LINK_FREQ,
> > > };
> > >
> > > +static const s64 imx219_link_freq_4lane_menu[] = {
> > > + IMX219_DEFAULT_LINK_FREQ_4LANE,
> > > +};
> > > +
> > > static const char * const imx219_test_pattern_menu[] = {
> > > "Disabled",
> > > "Color Bars",
> > > @@ -474,6 +484,9 @@ struct imx219 {
> > >
> > > /* Streaming on/off */
> > > bool streaming;
> > > +
> > > + /* Two or Four lanes */
> > > + u8 lanes;
> > > };
> > >
> > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > > return -EINVAL;
> > > }
> > >
> > > +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);
> > > +};
> > > +
> > > static int imx219_start_streaming(struct imx219 *imx219)
> > > {
> > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > 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 default values of current mode */
> > > reg_list = &imx219->mode->reg_list;
> > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> > > .open = imx219_open,
> > > };
> > >
> > > +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)
> > > {
> > > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
> > > /* By default, PIXEL_RATE is read only */
> > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > > V4L2_CID_PIXEL_RATE,
> > > - IMX219_PIXEL_RATE,
> > > - IMX219_PIXEL_RATE, 1,
> > > - IMX219_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_link_freq_menu);
> > > + (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;
> > >
> > > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
> > > mutex_destroy(&imx219->mutex);
> > > }
> > >
> > > -static int imx219_check_hwcfg(struct device *dev)
> > > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > > {
> > > struct fwnode_handle *endpoint;
> > > struct v4l2_fwnode_endpoint ep_cfg = {
> > > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
> > > }
> > >
> > > /* Check the number of MIPI CSI2 data lanes */
> > > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > > - dev_err(dev, "only 2 data lanes are currently supported\n");
> > > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> > > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> > > goto error_out;
> > > }
> > > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> > >
> > > /* Check the link frequency set in device tree */
> > > if (!ep_cfg.nr_of_link_frequencies) {
> > > @@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
> > > }
> > >
> > > if (ep_cfg.nr_of_link_frequencies != 1 ||
> > > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> > > + (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > > dev_err(dev, "Link frequency not supported: %lld\n",
> > > ep_cfg.link_frequencies[0]);
> > > goto error_out;
> > > @@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
> > > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> > >
> > > /* Check the hardware configuration in device tree */
> > > - if (imx219_check_hwcfg(dev))
> > > + if (imx219_check_hwcfg(dev, imx219))
> > > return -EINVAL;
> > >
> > > /* Get system clock (xclk) */
> > > --
> > > 2.34.1
> > >
On Mon, 16 Jan 2023 at 16:36, Adam Ford <[email protected]> wrote:
>
> On Mon, Jan 16, 2023 at 8:42 AM Dave Stevenson
> <[email protected]> wrote:
> >
> > Hi Adam
> >
> > Sorry, for the delay.
> >
> > On Fri, 13 Jan 2023 at 18:41, Adam Ford <[email protected]> wrote:
> > >
> > > On Tue, Dec 20, 2022 at 6:08 AM Adam Ford <[email protected]> wrote:
> > > >
> > > > The imx219 camera is capable of either two-lane or four-lane
> > > > operation. When operating in four-lane, both the pixel rate and
> > > > link frequency change. Regardless of the mode, however, both
> > > > frequencies remain fixed.
> > > >
> > > > Helper functions are needed to read and set pixel and link frequencies
> > > > which also reduces the number of fixed registers in the table of modes.
> > > >
> > > > Signed-off-by: Adam Ford <[email protected]>
> >
> > I can't test on 4 lanes, but 2 lane modes still work, and the code
> > looks reasonable for 4 lanes.
>
> Thanks! For what it's worth, I tested this on an Renesas RZ/G2M SoC
> which has both a 2-lane and 4-lane interface, and I didn't see any
> difference in performance or image quality between the 2-lane and
> 4-lane modes.
It's more that I don't have an IMX219 module that breaks out all 4 lanes.
It still works correctly on 2 lanes, so as long as it works for you on
4 lanes, then I'm happy.
Dave
> adam
> >
> > Reviewed-by: Dave Stevenson <[email protected]>
> >
> >
> > > Gentle nudge on this.
> > >
> > > > ---
> > > > V4: Restore check for nr_of_link_frequencies and update commit message
> > > > V3: Keep the helper function doing the link and lane parsing to
> > > > keep th probe function small.
> > > >
> > > > V2: Replace if-else statements with ternary operator
> > > > Fix 4-lane Link Rate.
> > > > Fix checking the link rate so only the link rate for
> > > > the given number of lanes is permitted.
> > > >
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index 7f44d62047b6..b5fa4986470a 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -42,10 +42,16 @@
> > > > /* External clock frequency is 24.0M */
> > > > #define IMX219_XCLK_FREQ 24000000
> > > >
> > > > -/* Pixel rate is fixed at 182.4M for all the modes */
> > > > +/* 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_CSI_2_LANE_MODE 0x01
> > > > +#define IMX219_CSI_4_LANE_MODE 0x03
> > > >
> > > > /* V_TIMING internal */
> > > > #define IMX219_REG_VTS 0x0160
> > > > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
> > > > IMX219_DEFAULT_LINK_FREQ,
> > > > };
> > > >
> > > > +static const s64 imx219_link_freq_4lane_menu[] = {
> > > > + IMX219_DEFAULT_LINK_FREQ_4LANE,
> > > > +};
> > > > +
> > > > static const char * const imx219_test_pattern_menu[] = {
> > > > "Disabled",
> > > > "Color Bars",
> > > > @@ -474,6 +484,9 @@ struct imx219 {
> > > >
> > > > /* Streaming on/off */
> > > > bool streaming;
> > > > +
> > > > + /* Two or Four lanes */
> > > > + u8 lanes;
> > > > };
> > > >
> > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > +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);
> > > > +};
> > > > +
> > > > static int imx219_start_streaming(struct imx219 *imx219)
> > > > {
> > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > > 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 default values of current mode */
> > > > reg_list = &imx219->mode->reg_list;
> > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > > > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> > > > .open = imx219_open,
> > > > };
> > > >
> > > > +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)
> > > > {
> > > > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
> > > > /* By default, PIXEL_RATE is read only */
> > > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > > > V4L2_CID_PIXEL_RATE,
> > > > - IMX219_PIXEL_RATE,
> > > > - IMX219_PIXEL_RATE, 1,
> > > > - IMX219_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_link_freq_menu);
> > > > + (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;
> > > >
> > > > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
> > > > mutex_destroy(&imx219->mutex);
> > > > }
> > > >
> > > > -static int imx219_check_hwcfg(struct device *dev)
> > > > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > > > {
> > > > struct fwnode_handle *endpoint;
> > > > struct v4l2_fwnode_endpoint ep_cfg = {
> > > > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > }
> > > >
> > > > /* Check the number of MIPI CSI2 data lanes */
> > > > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > > > - dev_err(dev, "only 2 data lanes are currently supported\n");
> > > > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> > > > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > > > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> > > > goto error_out;
> > > > }
> > > > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> > > >
> > > > /* Check the link frequency set in device tree */
> > > > if (!ep_cfg.nr_of_link_frequencies) {
> > > > @@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > }
> > > >
> > > > if (ep_cfg.nr_of_link_frequencies != 1 ||
> > > > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> > > > + (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > > > dev_err(dev, "Link frequency not supported: %lld\n",
> > > > ep_cfg.link_frequencies[0]);
> > > > goto error_out;
> > > > @@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
> > > > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> > > >
> > > > /* Check the hardware configuration in device tree */
> > > > - if (imx219_check_hwcfg(dev))
> > > > + if (imx219_check_hwcfg(dev, imx219))
> > > > return -EINVAL;
> > > >
> > > > /* Get system clock (xclk) */
> > > > --
> > > > 2.34.1
> > > >
On Mon, Jan 16, 2023 at 11:05 AM Dave Stevenson
<[email protected]> wrote:
>
> On Mon, 16 Jan 2023 at 16:36, Adam Ford <[email protected]> wrote:
> >
> > On Mon, Jan 16, 2023 at 8:42 AM Dave Stevenson
> > <[email protected]> wrote:
> > >
> > > Hi Adam
> > >
> > > Sorry, for the delay.
> > >
> > > On Fri, 13 Jan 2023 at 18:41, Adam Ford <[email protected]> wrote:
> > > >
> > > > On Tue, Dec 20, 2022 at 6:08 AM Adam Ford <[email protected]> wrote:
> > > > >
> > > > > The imx219 camera is capable of either two-lane or four-lane
> > > > > operation. When operating in four-lane, both the pixel rate and
> > > > > link frequency change. Regardless of the mode, however, both
> > > > > frequencies remain fixed.
> > > > >
> > > > > Helper functions are needed to read and set pixel and link frequencies
> > > > > which also reduces the number of fixed registers in the table of modes.
> > > > >
> > > > > Signed-off-by: Adam Ford <[email protected]>
> > >
> > > I can't test on 4 lanes, but 2 lane modes still work, and the code
> > > looks reasonable for 4 lanes.
> >
> > Thanks! For what it's worth, I tested this on an Renesas RZ/G2M SoC
> > which has both a 2-lane and 4-lane interface, and I didn't see any
> > difference in performance or image quality between the 2-lane and
> > 4-lane modes.
>
> It's more that I don't have an IMX219 module that breaks out all 4 lanes.
> It still works correctly on 2 lanes, so as long as it works for you on
> 4 lanes, then I'm happy.
>
> Dave
>
> > adam
> > >
> > > Reviewed-by: Dave Stevenson <[email protected]>
> > >
Is there anything that I need to do to get this into the media
staging? I wasn't sure if I missed someone in the CC list.
adam
> > >
> > > > Gentle nudge on this.
> > > >
> > > > > ---
> > > > > V4: Restore check for nr_of_link_frequencies and update commit message
> > > > > V3: Keep the helper function doing the link and lane parsing to
> > > > > keep th probe function small.
> > > > >
> > > > > V2: Replace if-else statements with ternary operator
> > > > > Fix 4-lane Link Rate.
> > > > > Fix checking the link rate so only the link rate for
> > > > > the given number of lanes is permitted.
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > index 7f44d62047b6..b5fa4986470a 100644
> > > > > --- a/drivers/media/i2c/imx219.c
> > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > @@ -42,10 +42,16 @@
> > > > > /* External clock frequency is 24.0M */
> > > > > #define IMX219_XCLK_FREQ 24000000
> > > > >
> > > > > -/* Pixel rate is fixed at 182.4M for all the modes */
> > > > > +/* 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_CSI_2_LANE_MODE 0x01
> > > > > +#define IMX219_CSI_4_LANE_MODE 0x03
> > > > >
> > > > > /* V_TIMING internal */
> > > > > #define IMX219_REG_VTS 0x0160
> > > > > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
> > > > > IMX219_DEFAULT_LINK_FREQ,
> > > > > };
> > > > >
> > > > > +static const s64 imx219_link_freq_4lane_menu[] = {
> > > > > + IMX219_DEFAULT_LINK_FREQ_4LANE,
> > > > > +};
> > > > > +
> > > > > static const char * const imx219_test_pattern_menu[] = {
> > > > > "Disabled",
> > > > > "Color Bars",
> > > > > @@ -474,6 +484,9 @@ struct imx219 {
> > > > >
> > > > > /* Streaming on/off */
> > > > > bool streaming;
> > > > > +
> > > > > + /* Two or Four lanes */
> > > > > + u8 lanes;
> > > > > };
> > > > >
> > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > > > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > +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);
> > > > > +};
> > > > > +
> > > > > static int imx219_start_streaming(struct imx219 *imx219)
> > > > > {
> > > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > > > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > > > 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 default values of current mode */
> > > > > reg_list = &imx219->mode->reg_list;
> > > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > > > > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> > > > > .open = imx219_open,
> > > > > };
> > > > >
> > > > > +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)
> > > > > {
> > > > > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
> > > > > /* By default, PIXEL_RATE is read only */
> > > > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > > > > V4L2_CID_PIXEL_RATE,
> > > > > - IMX219_PIXEL_RATE,
> > > > > - IMX219_PIXEL_RATE, 1,
> > > > > - IMX219_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_link_freq_menu);
> > > > > + (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;
> > > > >
> > > > > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
> > > > > mutex_destroy(&imx219->mutex);
> > > > > }
> > > > >
> > > > > -static int imx219_check_hwcfg(struct device *dev)
> > > > > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > > > > {
> > > > > struct fwnode_handle *endpoint;
> > > > > struct v4l2_fwnode_endpoint ep_cfg = {
> > > > > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > > }
> > > > >
> > > > > /* Check the number of MIPI CSI2 data lanes */
> > > > > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > > > > - dev_err(dev, "only 2 data lanes are currently supported\n");
> > > > > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> > > > > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > > > > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> > > > > goto error_out;
> > > > > }
> > > > > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> > > > >
> > > > > /* Check the link frequency set in device tree */
> > > > > if (!ep_cfg.nr_of_link_frequencies) {
> > > > > @@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > > }
> > > > >
> > > > > if (ep_cfg.nr_of_link_frequencies != 1 ||
> > > > > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> > > > > + (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > > > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > > > > dev_err(dev, "Link frequency not supported: %lld\n",
> > > > > ep_cfg.link_frequencies[0]);
> > > > > goto error_out;
> > > > > @@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
> > > > > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> > > > >
> > > > > /* Check the hardware configuration in device tree */
> > > > > - if (imx219_check_hwcfg(dev))
> > > > > + if (imx219_check_hwcfg(dev, imx219))
> > > > > return -EINVAL;
> > > > >
> > > > > /* Get system clock (xclk) */
> > > > > --
> > > > > 2.34.1
> > > > >
Hi Adam
On Tue, 31 Jan 2023 at 20:52, Adam Ford <[email protected]> wrote:
>
> On Mon, Jan 16, 2023 at 11:05 AM Dave Stevenson
> <[email protected]> wrote:
> >
> > On Mon, 16 Jan 2023 at 16:36, Adam Ford <[email protected]> wrote:
> > >
> > > On Mon, Jan 16, 2023 at 8:42 AM Dave Stevenson
> > > <[email protected]> wrote:
> > > >
> > > > Hi Adam
> > > >
> > > > Sorry, for the delay.
> > > >
> > > > On Fri, 13 Jan 2023 at 18:41, Adam Ford <[email protected]> wrote:
> > > > >
> > > > > On Tue, Dec 20, 2022 at 6:08 AM Adam Ford <[email protected]> wrote:
> > > > > >
> > > > > > The imx219 camera is capable of either two-lane or four-lane
> > > > > > operation. When operating in four-lane, both the pixel rate and
> > > > > > link frequency change. Regardless of the mode, however, both
> > > > > > frequencies remain fixed.
> > > > > >
> > > > > > Helper functions are needed to read and set pixel and link frequencies
> > > > > > which also reduces the number of fixed registers in the table of modes.
> > > > > >
> > > > > > Signed-off-by: Adam Ford <[email protected]>
> > > >
> > > > I can't test on 4 lanes, but 2 lane modes still work, and the code
> > > > looks reasonable for 4 lanes.
> > >
> > > Thanks! For what it's worth, I tested this on an Renesas RZ/G2M SoC
> > > which has both a 2-lane and 4-lane interface, and I didn't see any
> > > difference in performance or image quality between the 2-lane and
> > > 4-lane modes.
> >
> > It's more that I don't have an IMX219 module that breaks out all 4 lanes.
> > It still works correctly on 2 lanes, so as long as it works for you on
> > 4 lanes, then I'm happy.
> >
> > Dave
> >
> > > adam
> > > >
> > > > Reviewed-by: Dave Stevenson <[email protected]>
> > > >
>
> Is there anything that I need to do to get this into the media
> staging? I wasn't sure if I missed someone in the CC list.
Your patches are in Sakari's tree, and he issued a pull request to
Mauro for that about a week back. Those patches should be in 6.3.
Dave
> adam
> > > >
> > > > > Gentle nudge on this.
> > > > >
> > > > > > ---
> > > > > > V4: Restore check for nr_of_link_frequencies and update commit message
> > > > > > V3: Keep the helper function doing the link and lane parsing to
> > > > > > keep th probe function small.
> > > > > >
> > > > > > V2: Replace if-else statements with ternary operator
> > > > > > Fix 4-lane Link Rate.
> > > > > > Fix checking the link rate so only the link rate for
> > > > > > the given number of lanes is permitted.
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > index 7f44d62047b6..b5fa4986470a 100644
> > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > @@ -42,10 +42,16 @@
> > > > > > /* External clock frequency is 24.0M */
> > > > > > #define IMX219_XCLK_FREQ 24000000
> > > > > >
> > > > > > -/* Pixel rate is fixed at 182.4M for all the modes */
> > > > > > +/* 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_CSI_2_LANE_MODE 0x01
> > > > > > +#define IMX219_CSI_4_LANE_MODE 0x03
> > > > > >
> > > > > > /* V_TIMING internal */
> > > > > > #define IMX219_REG_VTS 0x0160
> > > > > > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
> > > > > > IMX219_DEFAULT_LINK_FREQ,
> > > > > > };
> > > > > >
> > > > > > +static const s64 imx219_link_freq_4lane_menu[] = {
> > > > > > + IMX219_DEFAULT_LINK_FREQ_4LANE,
> > > > > > +};
> > > > > > +
> > > > > > static const char * const imx219_test_pattern_menu[] = {
> > > > > > "Disabled",
> > > > > > "Color Bars",
> > > > > > @@ -474,6 +484,9 @@ struct imx219 {
> > > > > >
> > > > > > /* Streaming on/off */
> > > > > > bool streaming;
> > > > > > +
> > > > > > + /* Two or Four lanes */
> > > > > > + u8 lanes;
> > > > > > };
> > > > > >
> > > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > > > > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > +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);
> > > > > > +};
> > > > > > +
> > > > > > static int imx219_start_streaming(struct imx219 *imx219)
> > > > > > {
> > > > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > > > > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > > > > 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 default values of current mode */
> > > > > > reg_list = &imx219->mode->reg_list;
> > > > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > > > > > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> > > > > > .open = imx219_open,
> > > > > > };
> > > > > >
> > > > > > +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)
> > > > > > {
> > > > > > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
> > > > > > /* By default, PIXEL_RATE is read only */
> > > > > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > > > > > V4L2_CID_PIXEL_RATE,
> > > > > > - IMX219_PIXEL_RATE,
> > > > > > - IMX219_PIXEL_RATE, 1,
> > > > > > - IMX219_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_link_freq_menu);
> > > > > > + (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;
> > > > > >
> > > > > > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
> > > > > > mutex_destroy(&imx219->mutex);
> > > > > > }
> > > > > >
> > > > > > -static int imx219_check_hwcfg(struct device *dev)
> > > > > > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > > > > > {
> > > > > > struct fwnode_handle *endpoint;
> > > > > > struct v4l2_fwnode_endpoint ep_cfg = {
> > > > > > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > > > }
> > > > > >
> > > > > > /* Check the number of MIPI CSI2 data lanes */
> > > > > > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > > > > > - dev_err(dev, "only 2 data lanes are currently supported\n");
> > > > > > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> > > > > > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > > > > > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> > > > > > goto error_out;
> > > > > > }
> > > > > > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> > > > > >
> > > > > > /* Check the link frequency set in device tree */
> > > > > > if (!ep_cfg.nr_of_link_frequencies) {
> > > > > > @@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > > > }
> > > > > >
> > > > > > if (ep_cfg.nr_of_link_frequencies != 1 ||
> > > > > > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> > > > > > + (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > > > > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > > > > > dev_err(dev, "Link frequency not supported: %lld\n",
> > > > > > ep_cfg.link_frequencies[0]);
> > > > > > goto error_out;
> > > > > > @@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> > > > > >
> > > > > > /* Check the hardware configuration in device tree */
> > > > > > - if (imx219_check_hwcfg(dev))
> > > > > > + if (imx219_check_hwcfg(dev, imx219))
> > > > > > return -EINVAL;
> > > > > >
> > > > > > /* Get system clock (xclk) */
> > > > > > --
> > > > > > 2.34.1
> > > > > >
On Tue, Jan 31, 2023 at 4:09 PM Dave Stevenson
<[email protected]> wrote:
>
> Hi Adam
>
> On Tue, 31 Jan 2023 at 20:52, Adam Ford <[email protected]> wrote:
> >
> > On Mon, Jan 16, 2023 at 11:05 AM Dave Stevenson
> > <[email protected]> wrote:
> > >
> > > On Mon, 16 Jan 2023 at 16:36, Adam Ford <[email protected]> wrote:
> > > >
> > > > On Mon, Jan 16, 2023 at 8:42 AM Dave Stevenson
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Adam
> > > > >
> > > > > Sorry, for the delay.
> > > > >
> > > > > On Fri, 13 Jan 2023 at 18:41, Adam Ford <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Dec 20, 2022 at 6:08 AM Adam Ford <[email protected]> wrote:
> > > > > > >
> > > > > > > The imx219 camera is capable of either two-lane or four-lane
> > > > > > > operation. When operating in four-lane, both the pixel rate and
> > > > > > > link frequency change. Regardless of the mode, however, both
> > > > > > > frequencies remain fixed.
> > > > > > >
> > > > > > > Helper functions are needed to read and set pixel and link frequencies
> > > > > > > which also reduces the number of fixed registers in the table of modes.
> > > > > > >
> > > > > > > Signed-off-by: Adam Ford <[email protected]>
> > > > >
> > > > > I can't test on 4 lanes, but 2 lane modes still work, and the code
> > > > > looks reasonable for 4 lanes.
> > > >
> > > > Thanks! For what it's worth, I tested this on an Renesas RZ/G2M SoC
> > > > which has both a 2-lane and 4-lane interface, and I didn't see any
> > > > difference in performance or image quality between the 2-lane and
> > > > 4-lane modes.
> > >
> > > It's more that I don't have an IMX219 module that breaks out all 4 lanes.
> > > It still works correctly on 2 lanes, so as long as it works for you on
> > > 4 lanes, then I'm happy.
> > >
> > > Dave
> > >
> > > > adam
> > > > >
> > > > > Reviewed-by: Dave Stevenson <[email protected]>
> > > > >
> >
> > Is there anything that I need to do to get this into the media
> > staging? I wasn't sure if I missed someone in the CC list.
>
> Your patches are in Sakari's tree, and he issued a pull request to
> Mauro for that about a week back. Those patches should be in 6.3.
Awesome. Thank you. I must not have looked in the right place.
adam
>
> Dave
>
> > adam
> > > > >
> > > > > > Gentle nudge on this.
> > > > > >
> > > > > > > ---
> > > > > > > V4: Restore check for nr_of_link_frequencies and update commit message
> > > > > > > V3: Keep the helper function doing the link and lane parsing to
> > > > > > > keep th probe function small.
> > > > > > >
> > > > > > > V2: Replace if-else statements with ternary operator
> > > > > > > Fix 4-lane Link Rate.
> > > > > > > Fix checking the link rate so only the link rate for
> > > > > > > the given number of lanes is permitted.
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > > index 7f44d62047b6..b5fa4986470a 100644
> > > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > > @@ -42,10 +42,16 @@
> > > > > > > /* External clock frequency is 24.0M */
> > > > > > > #define IMX219_XCLK_FREQ 24000000
> > > > > > >
> > > > > > > -/* Pixel rate is fixed at 182.4M for all the modes */
> > > > > > > +/* 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_CSI_2_LANE_MODE 0x01
> > > > > > > +#define IMX219_CSI_4_LANE_MODE 0x03
> > > > > > >
> > > > > > > /* V_TIMING internal */
> > > > > > > #define IMX219_REG_VTS 0x0160
> > > > > > > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = {
> > > > > > > IMX219_DEFAULT_LINK_FREQ,
> > > > > > > };
> > > > > > >
> > > > > > > +static const s64 imx219_link_freq_4lane_menu[] = {
> > > > > > > + IMX219_DEFAULT_LINK_FREQ_4LANE,
> > > > > > > +};
> > > > > > > +
> > > > > > > static const char * const imx219_test_pattern_menu[] = {
> > > > > > > "Disabled",
> > > > > > > "Color Bars",
> > > > > > > @@ -474,6 +484,9 @@ struct imx219 {
> > > > > > >
> > > > > > > /* Streaming on/off */
> > > > > > > bool streaming;
> > > > > > > +
> > > > > > > + /* Two or Four lanes */
> > > > > > > + u8 lanes;
> > > > > > > };
> > > > > > >
> > > > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > > > > > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > > > > > > return -EINVAL;
> > > > > > > }
> > > > > > >
> > > > > > > +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);
> > > > > > > +};
> > > > > > > +
> > > > > > > static int imx219_start_streaming(struct imx219 *imx219)
> > > > > > > {
> > > > > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > > > > > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > > > > > 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 default values of current mode */
> > > > > > > reg_list = &imx219->mode->reg_list;
> > > > > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > > > > > > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> > > > > > > .open = imx219_open,
> > > > > > > };
> > > > > > >
> > > > > > > +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)
> > > > > > > {
> > > > > > > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219)
> > > > > > > /* By default, PIXEL_RATE is read only */
> > > > > > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > > > > > > V4L2_CID_PIXEL_RATE,
> > > > > > > - IMX219_PIXEL_RATE,
> > > > > > > - IMX219_PIXEL_RATE, 1,
> > > > > > > - IMX219_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_link_freq_menu);
> > > > > > > + (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;
> > > > > > >
> > > > > > > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219)
> > > > > > > mutex_destroy(&imx219->mutex);
> > > > > > > }
> > > > > > >
> > > > > > > -static int imx219_check_hwcfg(struct device *dev)
> > > > > > > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > > > > > > {
> > > > > > > struct fwnode_handle *endpoint;
> > > > > > > struct v4l2_fwnode_endpoint ep_cfg = {
> > > > > > > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > > > > }
> > > > > > >
> > > > > > > /* Check the number of MIPI CSI2 data lanes */
> > > > > > > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > > > > > > - dev_err(dev, "only 2 data lanes are currently supported\n");
> > > > > > > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> > > > > > > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > > > > > > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> > > > > > > goto error_out;
> > > > > > > }
> > > > > > > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> > > > > > >
> > > > > > > /* Check the link frequency set in device tree */
> > > > > > > if (!ep_cfg.nr_of_link_frequencies) {
> > > > > > > @@ -1340,7 +1375,8 @@ static int imx219_check_hwcfg(struct device *dev)
> > > > > > > }
> > > > > > >
> > > > > > > if (ep_cfg.nr_of_link_frequencies != 1 ||
> > > > > > > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> > > > > > > + (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > > > > > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > > > > > > dev_err(dev, "Link frequency not supported: %lld\n",
> > > > > > > ep_cfg.link_frequencies[0]);
> > > > > > > goto error_out;
> > > > > > > @@ -1368,7 +1404,7 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> > > > > > >
> > > > > > > /* Check the hardware configuration in device tree */
> > > > > > > - if (imx219_check_hwcfg(dev))
> > > > > > > + if (imx219_check_hwcfg(dev, imx219))
> > > > > > > return -EINVAL;
> > > > > > >
> > > > > > > /* Get system clock (xclk) */
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >