* [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane
@ 2022-04-12 13:55 Adam Ford
2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw)
To: linux-media
Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford,
laurent.pinchart, Adam Ford, Dave Stevenson,
Mauro Carvalho Chehab, linux-kernel
The driver currently only supports a 2-lane camera, a fixed external
clock (XCLK) at 24MHz, a fixed Pixel Rate of 182.4MHz, and a fixed
link rate of 456MHz. There are a bunch of hard-codec values in a
table of operating modes which expect the above to be true.
According to the datasheet, the driver is capable of operating in
either 4-lane with a pixel rate of 280.8MHz and Linux frequency
of 702MHz or 2-lane configured as stated above. The XCLK can be
anywhere from 6MHz - 27MHz instead of being fixed at 24MHz.
Split up the hard-coded values into smaller helper functions that
dynamically set the registers of the camera based on the XCLK and
desired number of lanes.
This series was tested on a Beacon RZ/G2M streaming video at 640x480
to an LCD with fbdevsink
media-ctl --links "'rcar_csi2 feaa0000.csi2':1->'VIN0 output':0[1]" -d /dev/media1
media-ctl --set-v4l2 "'imx219 2-0010':0[fmt:SRGGB8_1X8/640x480 field:none]" -d /dev/media1
yavta -w '0x009f0905 2048' /dev/v4l-subdev12
gst-launch-1.0 v4l2src device=/dev/video7 ! video/x-bayer,width=640,height=480,format=rggb ! queue ! bayer2rgb ! fbdevsink
Due to hardware limitations, the XCLK is still 24MHz, so anyone
willing to test this series with a different XCLK would be appreciated.
Due to the video format, streaming video at larger resolution was
not feasible, however individual frames captured at 1920x1080 were
successful.
Adam Ford (4):
media: i2c: imx219: Split common registers from mode tables
media: i2c: imx219: Support four-lane operation
media: i2c: imx219: Enable variable XCLK
media: i2c: imx219: Create DPHY helper function
drivers/media/i2c/imx219.c | 340 +++++++++++++++++++++++--------------
1 file changed, 213 insertions(+), 127 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables
2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
@ 2022-04-12 13:55 ` Adam Ford
2022-04-25 16:20 ` Dave Stevenson
2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw)
To: linux-media
Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford,
laurent.pinchart, Adam Ford, Dave Stevenson,
Mauro Carvalho Chehab, linux-kernel
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 <aford173@gmail.com>
---
drivers/media/i2c/imx219.c | 103 ++++++++++++++-----------------------
1 file changed, 39 insertions(+), 64 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index e10af3f74b38..b7cc36b16547 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -145,19 +145,36 @@ 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},
+/* To Access Addresses 3000-5fff, send the following commands */
+static const struct imx219_reg mfg_specific_reg[] = {
+ {0x0100, 0x00}, /* Mode Select */
{0x30eb, 0x0c},
{0x30eb, 0x05},
{0x300a, 0xff},
{0x300b, 0xff},
{0x30eb, 0x05},
{0x30eb, 0x09},
+};
+
+static const struct imx219_reg pll_clk_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},
+};
+
+/*
+ * 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[] = {
{0x0114, 0x01},
{0x0128, 0x00},
{0x012a, 0x18},
@@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
{0x0171, 0x01},
{0x0174, 0x00},
{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},
@@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
};
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},
@@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
{0x0171, 0x01},
{0x0174, 0x00},
{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},
@@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
};
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},
@@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
{0x0171, 0x01},
{0x0174, 0x01},
{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},
@@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
};
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},
@@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = {
{0x0171, 0x01},
{0x0174, 0x03},
{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},
@@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
if (ret < 0)
return ret;
+ /* Send the Manufacturing Header common to all modes */
+ ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg));
+ 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);
@@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219)
goto err_rpm_put;
}
+ /* Configure the PLL clocks */
+ ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table));
+ if (ret) {
+ dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__);
+ goto err_rpm_put;
+ }
+
+
/* Apply customized values from user */
ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
if (ret)
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/4] media: i2c: imx219: Support four-lane operation
2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford
@ 2022-04-12 13:55 ` Adam Ford
2022-04-25 15:39 ` Dave Stevenson
2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw)
To: linux-media
Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford,
laurent.pinchart, Adam Ford, Dave Stevenson,
Mauro Carvalho Chehab, linux-kernel
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.
Since the link frequency and number of lanes is extracted from the
endpoint, move the endpoint handling into the probe function and
out of the imx219_check_hwcfg. This simplifies the imx219_check_hwcfg
just a bit, and places all the parameters into the imx219 structure.
It then allows imx219_check_hwcfg to simply validate the settings.
Signed-off-by: Adam Ford <aford173@gmail.com>
---
drivers/media/i2c/imx219.c | 144 ++++++++++++++++++++++++-------------
1 file changed, 96 insertions(+), 48 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index b7cc36b16547..d13ce5c1ece6 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 702000000
+
+#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
@@ -175,7 +181,6 @@ static const struct imx219_reg pll_clk_table[] = {
* 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
*/
static const struct imx219_reg mode_3280x2464_regs[] = {
- {0x0114, 0x01},
{0x0128, 0x00},
{0x012a, 0x18},
{0x012b, 0x00},
@@ -216,7 +221,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
};
static const struct imx219_reg mode_1920_1080_regs[] = {
- {0x0114, 0x01},
{0x0128, 0x00},
{0x012a, 0x18},
{0x012b, 0x00},
@@ -257,7 +261,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
};
static const struct imx219_reg mode_1640_1232_regs[] = {
- {0x0114, 0x01},
{0x0128, 0x00},
{0x012a, 0x18},
{0x012b, 0x00},
@@ -298,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
};
static const struct imx219_reg mode_640_480_regs[] = {
- {0x0114, 0x01},
{0x0128, 0x00},
{0x012a, 0x18},
{0x012b, 0x00},
@@ -352,6 +354,7 @@ static const struct imx219_reg raw10_framefmt_regs[] = {
static const s64 imx219_link_freq_menu[] = {
IMX219_DEFAULT_LINK_FREQ,
+ IMX219_DEFAULT_LINK_FREQ_4LANE,
};
static const char * const imx219_test_pattern_menu[] = {
@@ -529,6 +532,13 @@ struct imx219 {
/* Streaming on/off */
bool streaming;
+
+ /* Two or Four lanes */
+ u8 lanes;
+
+ /* Link Frequency info */
+ unsigned int nr_of_link_frequencies;
+ u64 link_frequency;
};
static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
@@ -991,6 +1001,20 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
return -EINVAL;
}
+static int imx219_configure_lanes(struct imx219 *imx219)
+{
+ int ret;
+
+ /* imx219->lanes has already been validated to be either 2 or 4 */
+ if (imx219->lanes == 2)
+ ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE,
+ IMX219_REG_VALUE_08BIT, IMX219_CSI_2_LANE_MODE);
+ else
+ ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE,
+ IMX219_REG_VALUE_08BIT, IMX219_CSI_4_LANE_MODE);
+ return ret;
+};
+
static int imx219_start_streaming(struct imx219 *imx219)
{
struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
@@ -1008,6 +1032,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);
@@ -1247,6 +1278,14 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
.open = imx219_open,
};
+static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
+{
+ if (imx219->lanes == 2)
+ return IMX219_PIXEL_RATE;
+ else
+ return IMX219_PIXEL_RATE_4LANE;
+}
+
/* Initialize control handlers */
static int imx219_init_controls(struct imx219 *imx219)
{
@@ -1268,14 +1307,15 @@ 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,
+ ARRAY_SIZE(imx219_link_freq_menu) - 1,
+ (imx219->lanes == 4),
imx219_link_freq_menu);
if (imx219->link_freq)
imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
@@ -1371,67 +1411,75 @@ 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 imx219 *imx219)
{
- struct fwnode_handle *endpoint;
+ struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+
+ /* Check the number of MIPI CSI2 data lanes */
+ if (imx219->lanes != 2 && imx219->lanes != 4) {
+ dev_err(&client->dev, "only 2 or 4 data lanes are currently supported\n");
+ return -EINVAL;
+ }
+
+ if (imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ &&
+ imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ_4LANE) {
+ dev_err(&client->dev, "Link frequency not supported: %lld\n",
+ imx219->link_frequency);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int imx219_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct imx219 *imx219;
struct v4l2_fwnode_endpoint ep_cfg = {
.bus_type = V4L2_MBUS_CSI2_DPHY
};
- int ret = -EINVAL;
+ struct fwnode_handle *endpoint;
+ int ret = 0;
+
+ imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
+ if (!imx219)
+ return -ENOMEM;
+ v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
+
+ /* Fetch the endpoint to extract lanes and link-frequency */
endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
if (!endpoint) {
dev_err(dev, "endpoint node not found\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto fwnode_cleanup;
}
if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
dev_err(dev, "could not parse endpoint\n");
- goto error_out;
+ ret = -EINVAL;
+ goto fwnode_cleanup;
}
- /* Check the number of MIPI CSI2 data lanes */
- if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
- dev_err(dev, "only 2 data lanes are currently supported\n");
- goto error_out;
- }
+ imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
+ imx219->nr_of_link_frequencies = ep_cfg.nr_of_link_frequencies;
- /* Check the link frequency set in device tree */
- if (!ep_cfg.nr_of_link_frequencies) {
+ if (imx219->nr_of_link_frequencies != 1) {
dev_err(dev, "link-frequency property not found in DT\n");
- goto error_out;
- }
-
- if (ep_cfg.nr_of_link_frequencies != 1 ||
- ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
- dev_err(dev, "Link frequency not supported: %lld\n",
- ep_cfg.link_frequencies[0]);
- goto error_out;
+ ret = -EINVAL;
+ goto fwnode_cleanup;
}
+ imx219->link_frequency = ep_cfg.link_frequencies[0];
- ret = 0;
-
-error_out:
+ /* Cleanup endpoint and handle any errors from above */
+fwnode_cleanup:
v4l2_fwnode_endpoint_free(&ep_cfg);
fwnode_handle_put(endpoint);
-
- return ret;
-}
-
-static int imx219_probe(struct i2c_client *client)
-{
- struct device *dev = &client->dev;
- struct imx219 *imx219;
- int ret;
-
- imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
- if (!imx219)
- return -ENOMEM;
-
- v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
+ if (ret)
+ return ret;
/* Check the hardware configuration in device tree */
- if (imx219_check_hwcfg(dev))
+ if (imx219_check_hwcfg(imx219))
return -EINVAL;
/* Get system clock (xclk) */
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] media: i2c: imx219: Enable variable XCLK
2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford
2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford
@ 2022-04-12 13:55 ` Adam Ford
2022-04-25 15:58 ` Dave Stevenson
2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford
2022-04-25 12:08 ` [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
4 siblings, 1 reply; 16+ messages in thread
From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw)
To: linux-media
Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford,
laurent.pinchart, Adam Ford, Dave Stevenson,
Mauro Carvalho Chehab, linux-kernel
The datasheet shows the external clock can be anything
from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and
PREPLLCK_OP_DIV need to change based on the clock, so
create helper functions to set these registers based on
the rate of xclk.
Move the validation of the clock rate into imx219_check_hwcfg
which means delaying the call to it until after xclk has been
determined.
Signed-off-by: Adam Ford <aford173@gmail.com>
---
drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++--------
1 file changed, 63 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index d13ce5c1ece6..08e7d0e72430 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -39,8 +39,12 @@
#define IMX219_REG_CHIP_ID 0x0000
#define IMX219_CHIP_ID 0x0219
-/* External clock frequency is 24.0M */
-#define IMX219_XCLK_FREQ 24000000
+/* Default external clock frequency is 24.0M */
+#define IMX219_XCLK_MIN_FREQ 6000000
+#define IMX219_XCLK_MAX_FREQ 27000000
+#define IMX219_REG_EXCK 0x012a
+#define IMX219_REG_PREPLLCK_VT_DIV 0x0304
+#define IMX219_REG_PREPLLCK_OP_DIV 0x0305
/* Pixel rate is fixed for all the modes */
#define IMX219_PIXEL_RATE 182400000
@@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_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 */
@@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = {
*/
static const struct imx219_reg mode_3280x2464_regs[] = {
{0x0128, 0x00},
- {0x012a, 0x18},
{0x012b, 0x00},
{0x0164, 0x00},
{0x0165, 0x00},
@@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
static const struct imx219_reg mode_1920_1080_regs[] = {
{0x0128, 0x00},
- {0x012a, 0x18},
{0x012b, 0x00},
{0x0162, 0x0d},
{0x0163, 0x78},
@@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
static const struct imx219_reg mode_1640_1232_regs[] = {
{0x0128, 0x00},
- {0x012a, 0x18},
{0x012b, 0x00},
{0x0164, 0x00},
{0x0165, 0x00},
@@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
static const struct imx219_reg mode_640_480_regs[] = {
{0x0128, 0x00},
- {0x012a, 0x18},
{0x012b, 0x00},
{0x0162, 0x0d},
{0x0163, 0x78},
@@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219)
return ret;
};
+static int imx219_set_exck_freq(struct imx219 *imx219)
+{
+ int ret;
+ /* The imx219 registers need MHz not Hz */
+ u8 clk = (u8) (imx219->xclk_freq/1000000U);
+
+ /* Set the clock frequency in MHz */
+ ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
+ IMX219_REG_VALUE_08BIT, clk);
+
+ /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */
+ switch (clk) {
+ case 6 ... 11:
+ ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
+ IMX219_REG_VALUE_08BIT, 0x01);
+ if (ret)
+ return ret;
+ ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
+ IMX219_REG_VALUE_08BIT, 0x01);
+ return ret;
+ case 12 ... 23:
+ ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
+ IMX219_REG_VALUE_08BIT, 0x02);
+ if (ret)
+ return ret;
+
+ ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
+ IMX219_REG_VALUE_08BIT, 0x02);
+
+ return ret;
+ case 24 ... 27:
+ ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
+ IMX219_REG_VALUE_08BIT, 0x03);
+ if (ret)
+ return ret;
+ ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
+ IMX219_REG_VALUE_08BIT, 0x03);
+ return ret;
+ default:
+ /* Should never get here */
+ return -EINVAL;
+ }
+}
+
static int imx219_start_streaming(struct imx219 *imx219)
{
struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
@@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219)
goto err_rpm_put;
}
+ /* Configure clock based on reference clock frequency */
+ imx219_set_exck_freq(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);
@@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219)
return -EINVAL;
}
+ if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) ||
+ imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) {
+ dev_err(&client->dev, "xclk frequency not supported: %d Hz\n",
+ imx219->xclk_freq);
+ return -EINVAL;
+ }
+
return 0;
}
@@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client)
if (ret)
return ret;
- /* Check the hardware configuration in device tree */
- if (imx219_check_hwcfg(imx219))
- return -EINVAL;
-
/* Get system clock (xclk) */
imx219->xclk = devm_clk_get(dev, NULL);
if (IS_ERR(imx219->xclk)) {
@@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client)
}
imx219->xclk_freq = clk_get_rate(imx219->xclk);
- if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
- dev_err(dev, "xclk frequency not supported: %d Hz\n",
- imx219->xclk_freq);
+
+ /* Check the hardware configuration in device tree */
+ if (imx219_check_hwcfg(imx219))
return -EINVAL;
- }
ret = imx219_get_regulators(imx219);
if (ret) {
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/4] media: i2c: imx219: Create DPHY helper function
2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
` (2 preceding siblings ...)
2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford
@ 2022-04-12 13:55 ` Adam Ford
2022-04-25 16:07 ` Dave Stevenson
2022-04-25 12:08 ` [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
4 siblings, 1 reply; 16+ messages in thread
From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw)
To: linux-media
Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford,
laurent.pinchart, Adam Ford, Dave Stevenson,
Mauro Carvalho Chehab, linux-kernel
In the table of modes, each mode sets the DPHY to auto.
Create a helper function which does the same thing while
removing the entry for auto DPHY from ever mode entry.
Signed-off-by: Adam Ford <aford173@gmail.com>
---
drivers/media/i2c/imx219.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 08e7d0e72430..bb0bc1b8d91c 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -35,6 +35,10 @@
#define IMX219_MODE_STANDBY 0x00
#define IMX219_MODE_STREAMING 0x01
+
+#define IMX219_REG_DPHY_CTRL 0x0128
+#define IMX219_DPHY_AUTO 0
+
/* Chip ID */
#define IMX219_REG_CHIP_ID 0x0000
#define IMX219_CHIP_ID 0x0219
@@ -183,7 +187,6 @@ static const struct imx219_reg pll_clk_table[] = {
* 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
*/
static const struct imx219_reg mode_3280x2464_regs[] = {
- {0x0128, 0x00},
{0x012b, 0x00},
{0x0164, 0x00},
{0x0165, 0x00},
@@ -222,7 +225,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
};
static const struct imx219_reg mode_1920_1080_regs[] = {
- {0x0128, 0x00},
{0x012b, 0x00},
{0x0162, 0x0d},
{0x0163, 0x78},
@@ -261,7 +263,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
};
static const struct imx219_reg mode_1640_1232_regs[] = {
- {0x0128, 0x00},
{0x012b, 0x00},
{0x0164, 0x00},
{0x0165, 0x00},
@@ -300,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
};
static const struct imx219_reg mode_640_480_regs[] = {
- {0x0128, 0x00},
{0x012b, 0x00},
{0x0162, 0x0d},
{0x0163, 0x78},
@@ -999,6 +999,15 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
return -EINVAL;
}
+static int imx219_enable_dphy(struct imx219 *imx219, u8 mode)
+{
+ int ret;
+
+ ret = imx219_write_reg(imx219, IMX219_REG_DPHY_CTRL,
+ IMX219_REG_VALUE_08BIT, mode);
+ return ret;
+};
+
static int imx219_configure_lanes(struct imx219 *imx219)
{
int ret;
@@ -1081,6 +1090,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
goto err_rpm_put;
}
+ /* Setup DPHY */
+ ret = imx219_enable_dphy(imx219, IMX219_DPHY_AUTO);
+ if (ret) {
+ dev_err(&client->dev, "%s failed to configure dphy\n", __func__);
+ goto err_rpm_put;
+ }
+
/* Configure clock based on reference clock frequency */
imx219_set_exck_freq(imx219);
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane
2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
` (3 preceding siblings ...)
2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford
@ 2022-04-25 12:08 ` Adam Ford
4 siblings, 0 replies; 16+ messages in thread
From: Adam Ford @ 2022-04-25 12:08 UTC (permalink / raw)
To: linux-media
Cc: Prabhakar Mahadev Lad, Tim Harvey, cstevens, Adam Ford-BE,
Laurent Pinchart, Dave Stevenson, Mauro Carvalho Chehab,
Linux Kernel Mailing List
On Tue, Apr 12, 2022 at 8:55 AM Adam Ford <aford173@gmail.com> wrote:
>
> The driver currently only supports a 2-lane camera, a fixed external
> clock (XCLK) at 24MHz, a fixed Pixel Rate of 182.4MHz, and a fixed
> link rate of 456MHz. There are a bunch of hard-codec values in a
> table of operating modes which expect the above to be true.
>
> According to the datasheet, the driver is capable of operating in
> either 4-lane with a pixel rate of 280.8MHz and Linux frequency
> of 702MHz or 2-lane configured as stated above. The XCLK can be
> anywhere from 6MHz - 27MHz instead of being fixed at 24MHz.
>
> Split up the hard-coded values into smaller helper functions that
> dynamically set the registers of the camera based on the XCLK and
> desired number of lanes.
>
> This series was tested on a Beacon RZ/G2M streaming video at 640x480
> to an LCD with fbdevsink
>
> media-ctl --links "'rcar_csi2 feaa0000.csi2':1->'VIN0 output':0[1]" -d /dev/media1
> media-ctl --set-v4l2 "'imx219 2-0010':0[fmt:SRGGB8_1X8/640x480 field:none]" -d /dev/media1
> yavta -w '0x009f0905 2048' /dev/v4l-subdev12
> gst-launch-1.0 v4l2src device=/dev/video7 ! video/x-bayer,width=640,height=480,format=rggb ! queue ! bayer2rgb ! fbdevsink
>
> Due to hardware limitations, the XCLK is still 24MHz, so anyone
> willing to test this series with a different XCLK would be appreciated.
>
> Due to the video format, streaming video at larger resolution was
> not feasible, however individual frames captured at 1920x1080 were
> successful.
>
Any comments from anyone on this series?
> Adam Ford (4):
> media: i2c: imx219: Split common registers from mode tables
> media: i2c: imx219: Support four-lane operation
> media: i2c: imx219: Enable variable XCLK
> media: i2c: imx219: Create DPHY helper function
>
> drivers/media/i2c/imx219.c | 340 +++++++++++++++++++++++--------------
> 1 file changed, 213 insertions(+), 127 deletions(-)
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] media: i2c: imx219: Support four-lane operation
2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford
@ 2022-04-25 15:39 ` Dave Stevenson
0 siblings, 0 replies; 16+ messages in thread
From: Dave Stevenson @ 2022-04-25 15:39 UTC (permalink / raw)
To: Adam Ford
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML
Hi Adam
Sorry for the delay in reviewing this.
On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> 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.
>
> Since the link frequency and number of lanes is extracted from the
> endpoint, move the endpoint handling into the probe function and
> out of the imx219_check_hwcfg. This simplifies the imx219_check_hwcfg
> just a bit, and places all the parameters into the imx219 structure.
> It then allows imx219_check_hwcfg to simply validate the settings.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> drivers/media/i2c/imx219.c | 144 ++++++++++++++++++++++++-------------
> 1 file changed, 96 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index b7cc36b16547..d13ce5c1ece6 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 702000000
My datasheet table 9-2 lists pll1_vt_freq as being 702MHz, and
Speed/Lane (Ch) is 726Mbps, so a link frequency of 363MHz.
Total output rate is 2.904Gbps, which is the 726Mbps * 4 lanes.
> +
> +#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
> @@ -175,7 +181,6 @@ static const struct imx219_reg pll_clk_table[] = {
> * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
> */
> static const struct imx219_reg mode_3280x2464_regs[] = {
> - {0x0114, 0x01},
> {0x0128, 0x00},
> {0x012a, 0x18},
> {0x012b, 0x00},
> @@ -216,7 +221,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> };
>
> static const struct imx219_reg mode_1920_1080_regs[] = {
> - {0x0114, 0x01},
> {0x0128, 0x00},
> {0x012a, 0x18},
> {0x012b, 0x00},
> @@ -257,7 +261,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> };
>
> static const struct imx219_reg mode_1640_1232_regs[] = {
> - {0x0114, 0x01},
> {0x0128, 0x00},
> {0x012a, 0x18},
> {0x012b, 0x00},
> @@ -298,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> };
>
> static const struct imx219_reg mode_640_480_regs[] = {
> - {0x0114, 0x01},
> {0x0128, 0x00},
> {0x012a, 0x18},
> {0x012b, 0x00},
> @@ -352,6 +354,7 @@ static const struct imx219_reg raw10_framefmt_regs[] = {
>
> static const s64 imx219_link_freq_menu[] = {
> IMX219_DEFAULT_LINK_FREQ,
> + IMX219_DEFAULT_LINK_FREQ_4LANE,
This one always feels weird to me. There is only one link frequency
supported at a time defined by the number of lanes. OK it's a read
only control, but having 2 link_freq_menu arrays of 1 element each,
and choosing the appropriate one seems more logical to me.
> };
>
> static const char * const imx219_test_pattern_menu[] = {
> @@ -529,6 +532,13 @@ struct imx219 {
>
> /* Streaming on/off */
> bool streaming;
> +
> + /* Two or Four lanes */
> + u8 lanes;
> +
> + /* Link Frequency info */
> + unsigned int nr_of_link_frequencies;
> + u64 link_frequency;
> };
>
> static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> @@ -991,6 +1001,20 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> return -EINVAL;
> }
>
> +static int imx219_configure_lanes(struct imx219 *imx219)
> +{
> + int ret;
> +
> + /* imx219->lanes has already been validated to be either 2 or 4 */
> + if (imx219->lanes == 2)
> + ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE,
> + IMX219_REG_VALUE_08BIT, IMX219_CSI_2_LANE_MODE);
> + else
> + ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE,
> + IMX219_REG_VALUE_08BIT, IMX219_CSI_4_LANE_MODE);
> + return ret;
Could be condensed to the one liner (with appropriate wrapping):
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);
> @@ -1008,6 +1032,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);
> @@ -1247,6 +1278,14 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> .open = imx219_open,
> };
>
> +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> +{
> + if (imx219->lanes == 2)
> + return IMX219_PIXEL_RATE;
> + else
> + return IMX219_PIXEL_RATE_4LANE;
Again:
return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
?
> +}
> +
> /* Initialize control handlers */
> static int imx219_init_controls(struct imx219 *imx219)
> {
> @@ -1268,14 +1307,15 @@ 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,
> + ARRAY_SIZE(imx219_link_freq_menu) - 1,
> + (imx219->lanes == 4),
> imx219_link_freq_menu);
> if (imx219->link_freq)
> imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> @@ -1371,67 +1411,75 @@ 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 imx219 *imx219)
> {
> - struct fwnode_handle *endpoint;
> + struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +
> + /* Check the number of MIPI CSI2 data lanes */
> + if (imx219->lanes != 2 && imx219->lanes != 4) {
> + dev_err(&client->dev, "only 2 or 4 data lanes are currently supported\n");
> + return -EINVAL;
> + }
> +
> + if (imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ &&
> + imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ_4LANE) {
This permits choosing 4 lane operation with a link frequency of
IMX219_DEFAULT_LINK_FREQ, or 2 lane operation with a link frequency of
IMX219_DEFAULT_LINK_FREQ_4LANE.
> + dev_err(&client->dev, "Link frequency not supported: %lld\n",
> + imx219->link_frequency);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int imx219_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct imx219 *imx219;
> struct v4l2_fwnode_endpoint ep_cfg = {
> .bus_type = V4L2_MBUS_CSI2_DPHY
> };
> - int ret = -EINVAL;
> + struct fwnode_handle *endpoint;
> + int ret = 0;
> +
> + imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> + if (!imx219)
> + return -ENOMEM;
>
> + v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> +
> + /* Fetch the endpoint to extract lanes and link-frequency */
> endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> if (!endpoint) {
> dev_err(dev, "endpoint node not found\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto fwnode_cleanup;
> }
>
> if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
> dev_err(dev, "could not parse endpoint\n");
> - goto error_out;
> + ret = -EINVAL;
> + goto fwnode_cleanup;
> }
>
> - /* Check the number of MIPI CSI2 data lanes */
> - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> - dev_err(dev, "only 2 data lanes are currently supported\n");
> - goto error_out;
> - }
> + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> + imx219->nr_of_link_frequencies = ep_cfg.nr_of_link_frequencies;
>
> - /* Check the link frequency set in device tree */
> - if (!ep_cfg.nr_of_link_frequencies) {
> + if (imx219->nr_of_link_frequencies != 1) {
> dev_err(dev, "link-frequency property not found in DT\n");
> - goto error_out;
> - }
> -
> - if (ep_cfg.nr_of_link_frequencies != 1 ||
> - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) {
> - dev_err(dev, "Link frequency not supported: %lld\n",
> - ep_cfg.link_frequencies[0]);
> - goto error_out;
> + ret = -EINVAL;
> + goto fwnode_cleanup;
> }
> + imx219->link_frequency = ep_cfg.link_frequencies[0];
imx219->link_frequency is only used within imx219_check_hwcfg, which
is called from imx219_probe.
Pass it as a parameter instead of storing it in the state structure?
Cheers.
Dave
>
> - ret = 0;
> -
> -error_out:
> + /* Cleanup endpoint and handle any errors from above */
> +fwnode_cleanup:
> v4l2_fwnode_endpoint_free(&ep_cfg);
> fwnode_handle_put(endpoint);
> -
> - return ret;
> -}
> -
> -static int imx219_probe(struct i2c_client *client)
> -{
> - struct device *dev = &client->dev;
> - struct imx219 *imx219;
> - int ret;
> -
> - imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> - if (!imx219)
> - return -ENOMEM;
> -
> - v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> + if (ret)
> + return ret;
>
> /* Check the hardware configuration in device tree */
> - if (imx219_check_hwcfg(dev))
> + if (imx219_check_hwcfg(imx219))
> return -EINVAL;
>
> /* Get system clock (xclk) */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK
2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford
@ 2022-04-25 15:58 ` Dave Stevenson
2022-04-25 16:13 ` Dave Stevenson
0 siblings, 1 reply; 16+ messages in thread
From: Dave Stevenson @ 2022-04-25 15:58 UTC (permalink / raw)
To: Adam Ford
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML
Hi Adam
I have no way of testing with an alternate XCLK value, so I'm working
based on the datasheet only.
On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
>
> The datasheet shows the external clock can be anything
> from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and
> PREPLLCK_OP_DIV need to change based on the clock, so
> create helper functions to set these registers based on
> the rate of xclk.
>
> Move the validation of the clock rate into imx219_check_hwcfg
> which means delaying the call to it until after xclk has been
> determined.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++--------
> 1 file changed, 63 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index d13ce5c1ece6..08e7d0e72430 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -39,8 +39,12 @@
> #define IMX219_REG_CHIP_ID 0x0000
> #define IMX219_CHIP_ID 0x0219
>
> -/* External clock frequency is 24.0M */
> -#define IMX219_XCLK_FREQ 24000000
> +/* Default external clock frequency is 24.0M */
> +#define IMX219_XCLK_MIN_FREQ 6000000
> +#define IMX219_XCLK_MAX_FREQ 27000000
> +#define IMX219_REG_EXCK 0x012a
> +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304
> +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305
>
> /* Pixel rate is fixed for all the modes */
> #define IMX219_PIXEL_RATE 182400000
> @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_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 */
> @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = {
> */
> static const struct imx219_reg mode_3280x2464_regs[] = {
> {0x0128, 0x00},
> - {0x012a, 0x18},
> {0x012b, 0x00},
> {0x0164, 0x00},
> {0x0165, 0x00},
> @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
>
> static const struct imx219_reg mode_1920_1080_regs[] = {
> {0x0128, 0x00},
> - {0x012a, 0x18},
> {0x012b, 0x00},
> {0x0162, 0x0d},
> {0x0163, 0x78},
> @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
>
> static const struct imx219_reg mode_1640_1232_regs[] = {
> {0x0128, 0x00},
> - {0x012a, 0x18},
> {0x012b, 0x00},
> {0x0164, 0x00},
> {0x0165, 0x00},
> @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>
> static const struct imx219_reg mode_640_480_regs[] = {
> {0x0128, 0x00},
> - {0x012a, 0x18},
> {0x012b, 0x00},
> {0x0162, 0x0d},
> {0x0163, 0x78},
> @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> return ret;
> };
>
> +static int imx219_set_exck_freq(struct imx219 *imx219)
> +{
> + int ret;
> + /* The imx219 registers need MHz not Hz */
> + u8 clk = (u8) (imx219->xclk_freq/1000000U);
> +
> + /* Set the clock frequency in MHz */
> + ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
> + IMX219_REG_VALUE_08BIT, clk);
> +
> + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */
> + switch (clk) {
> + case 6 ... 11:
> + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> + IMX219_REG_VALUE_08BIT, 0x01);
> + if (ret)
> + return ret;
> + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> + IMX219_REG_VALUE_08BIT, 0x01);
> + return ret;
> + case 12 ... 23:
> + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> + IMX219_REG_VALUE_08BIT, 0x02);
> + if (ret)
> + return ret;
> +
> + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> + IMX219_REG_VALUE_08BIT, 0x02);
> +
> + return ret;
> + case 24 ... 27:
> + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> + IMX219_REG_VALUE_08BIT, 0x03);
> + if (ret)
> + return ret;
> + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> + IMX219_REG_VALUE_08BIT, 0x03);
> + return ret;
> + default:
> + /* Should never get here */
> + return -EINVAL;
> + }
> +}
> +
> static int imx219_start_streaming(struct imx219 *imx219)
> {
> struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219)
> goto err_rpm_put;
> }
>
> + /* Configure clock based on reference clock frequency */
> + imx219_set_exck_freq(imx219);
You're not checking the return value from this function, so any I2C
failures will be ignored.
> +
> /* 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);
> @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219)
> return -EINVAL;
> }
>
> + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) ||
> + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) {
> + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n",
imx219->xclk_freq is unsigned, so %u
> + imx219->xclk_freq);
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> - /* Check the hardware configuration in device tree */
> - if (imx219_check_hwcfg(imx219))
> - return -EINVAL;
> -
> /* Get system clock (xclk) */
> imx219->xclk = devm_clk_get(dev, NULL);
> if (IS_ERR(imx219->xclk)) {
> @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client)
> }
>
> imx219->xclk_freq = clk_get_rate(imx219->xclk);
My bug admittedly, but clk_get_rate returns an unsigned long, but
imx219->xclk_freq is u32.
Ideally imx219->xclk_freq should be unsigned long to match, and the
dev_err I commented on earlier should be %lu.
Cheers.
Dave
> - if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> - dev_err(dev, "xclk frequency not supported: %d Hz\n",
> - imx219->xclk_freq);
> +
> + /* Check the hardware configuration in device tree */
> + if (imx219_check_hwcfg(imx219))
> return -EINVAL;
> - }
>
> ret = imx219_get_regulators(imx219);
> if (ret) {
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] media: i2c: imx219: Create DPHY helper function
2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford
@ 2022-04-25 16:07 ` Dave Stevenson
0 siblings, 0 replies; 16+ messages in thread
From: Dave Stevenson @ 2022-04-25 16:07 UTC (permalink / raw)
To: Adam Ford
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML
Hi Adam
On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
>
> In the table of modes, each mode sets the DPHY to auto.
> Create a helper function which does the same thing while
> removing the entry for auto DPHY from ever mode entry.
s/ever/every
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> drivers/media/i2c/imx219.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 08e7d0e72430..bb0bc1b8d91c 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -35,6 +35,10 @@
> #define IMX219_MODE_STANDBY 0x00
> #define IMX219_MODE_STREAMING 0x01
>
> +
> +#define IMX219_REG_DPHY_CTRL 0x0128
> +#define IMX219_DPHY_AUTO 0
> +
> /* Chip ID */
> #define IMX219_REG_CHIP_ID 0x0000
> #define IMX219_CHIP_ID 0x0219
> @@ -183,7 +187,6 @@ static const struct imx219_reg pll_clk_table[] = {
> * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
> */
> static const struct imx219_reg mode_3280x2464_regs[] = {
> - {0x0128, 0x00},
> {0x012b, 0x00},
> {0x0164, 0x00},
> {0x0165, 0x00},
> @@ -222,7 +225,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> };
>
> static const struct imx219_reg mode_1920_1080_regs[] = {
> - {0x0128, 0x00},
> {0x012b, 0x00},
> {0x0162, 0x0d},
> {0x0163, 0x78},
> @@ -261,7 +263,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> };
>
> static const struct imx219_reg mode_1640_1232_regs[] = {
> - {0x0128, 0x00},
> {0x012b, 0x00},
> {0x0164, 0x00},
> {0x0165, 0x00},
> @@ -300,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> };
>
> static const struct imx219_reg mode_640_480_regs[] = {
> - {0x0128, 0x00},
> {0x012b, 0x00},
> {0x0162, 0x0d},
> {0x0163, 0x78},
> @@ -999,6 +999,15 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> return -EINVAL;
> }
>
> +static int imx219_enable_dphy(struct imx219 *imx219, u8 mode)
> +{
> + int ret;
> +
> + ret = imx219_write_reg(imx219, IMX219_REG_DPHY_CTRL,
> + IMX219_REG_VALUE_08BIT, mode);
Is there a specific reason to extract this one register, but leave the block
{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},
that appear to also be common to all modes.
Other drivers break that lot out into a global registers array that is
always sent, rather than individual register writes.
Having this one register extra write as a new function is actually
likely to increase the size of the module overall, instead of reducing
it.
Dave
> + return ret;
> +};
> +
> static int imx219_configure_lanes(struct imx219 *imx219)
> {
> int ret;
> @@ -1081,6 +1090,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> goto err_rpm_put;
> }
>
> + /* Setup DPHY */
> + ret = imx219_enable_dphy(imx219, IMX219_DPHY_AUTO);
> + if (ret) {
> + dev_err(&client->dev, "%s failed to configure dphy\n", __func__);
> + goto err_rpm_put;
> + }
> +
> /* Configure clock based on reference clock frequency */
> imx219_set_exck_freq(imx219);
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK
2022-04-25 15:58 ` Dave Stevenson
@ 2022-04-25 16:13 ` Dave Stevenson
2022-04-25 17:28 ` Adam Ford
0 siblings, 1 reply; 16+ messages in thread
From: Dave Stevenson @ 2022-04-25 16:13 UTC (permalink / raw)
To: Adam Ford
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML
Hi again
On Mon, 25 Apr 2022 at 16:58, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Adam
>
> I have no way of testing with an alternate XCLK value, so I'm working
> based on the datasheet only.
>
> On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
> >
> > The datasheet shows the external clock can be anything
> > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and
> > PREPLLCK_OP_DIV need to change based on the clock, so
> > create helper functions to set these registers based on
> > the rate of xclk.
> >
> > Move the validation of the clock rate into imx219_check_hwcfg
> > which means delaying the call to it until after xclk has been
> > determined.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++--------
> > 1 file changed, 63 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index d13ce5c1ece6..08e7d0e72430 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -39,8 +39,12 @@
> > #define IMX219_REG_CHIP_ID 0x0000
> > #define IMX219_CHIP_ID 0x0219
> >
> > -/* External clock frequency is 24.0M */
> > -#define IMX219_XCLK_FREQ 24000000
> > +/* Default external clock frequency is 24.0M */
> > +#define IMX219_XCLK_MIN_FREQ 6000000
> > +#define IMX219_XCLK_MAX_FREQ 27000000
> > +#define IMX219_REG_EXCK 0x012a
> > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304
> > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305
> >
> > /* Pixel rate is fixed for all the modes */
> > #define IMX219_PIXEL_RATE 182400000
> > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_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 */
> > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = {
> > */
> > static const struct imx219_reg mode_3280x2464_regs[] = {
> > {0x0128, 0x00},
> > - {0x012a, 0x18},
> > {0x012b, 0x00},
> > {0x0164, 0x00},
> > {0x0165, 0x00},
> > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> >
> > static const struct imx219_reg mode_1920_1080_regs[] = {
> > {0x0128, 0x00},
> > - {0x012a, 0x18},
> > {0x012b, 0x00},
> > {0x0162, 0x0d},
> > {0x0163, 0x78},
> > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> >
> > static const struct imx219_reg mode_1640_1232_regs[] = {
> > {0x0128, 0x00},
> > - {0x012a, 0x18},
> > {0x012b, 0x00},
> > {0x0164, 0x00},
> > {0x0165, 0x00},
> > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >
> > static const struct imx219_reg mode_640_480_regs[] = {
> > {0x0128, 0x00},
> > - {0x012a, 0x18},
> > {0x012b, 0x00},
> > {0x0162, 0x0d},
> > {0x0163, 0x78},
> > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> > return ret;
> > };
> >
> > +static int imx219_set_exck_freq(struct imx219 *imx219)
> > +{
> > + int ret;
> > + /* The imx219 registers need MHz not Hz */
> > + u8 clk = (u8) (imx219->xclk_freq/1000000U);
> > +
> > + /* Set the clock frequency in MHz */
> > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
> > + IMX219_REG_VALUE_08BIT, clk);
In reviewing your other patch I noticed that the EXCK register is
actually a 16 bit value. The integer part is in 0x012a, and the
fractional part (1/256) in 0x012b, which is currently initialised from
the mode tables.
Your division discards the fractional part totally, so if the
configured frequency was 19.2MHz, then it would be programmed
incorrectly.
The value for register 0x012b needs to be computed and set here.
> > +
> > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */
> > + switch (clk) {
> > + case 6 ... 11:
> > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > + IMX219_REG_VALUE_08BIT, 0x01);
> > + if (ret)
> > + return ret;
> > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > + IMX219_REG_VALUE_08BIT, 0x01);
> > + return ret;
> > + case 12 ... 23:
> > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > + IMX219_REG_VALUE_08BIT, 0x02);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > + IMX219_REG_VALUE_08BIT, 0x02);
> > +
> > + return ret;
> > + case 24 ... 27:
> > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > + IMX219_REG_VALUE_08BIT, 0x03);
> > + if (ret)
> > + return ret;
> > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > + IMX219_REG_VALUE_08BIT, 0x03);
> > + return ret;
> > + default:
> > + /* Should never get here */
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static int imx219_start_streaming(struct imx219 *imx219)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > goto err_rpm_put;
> > }
> >
> > + /* Configure clock based on reference clock frequency */
> > + imx219_set_exck_freq(imx219);
>
> You're not checking the return value from this function, so any I2C
> failures will be ignored.
>
> > +
> > /* 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);
> > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219)
> > return -EINVAL;
> > }
> >
> > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) ||
> > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) {
> > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n",
>
> imx219->xclk_freq is unsigned, so %u
>
> > + imx219->xclk_freq);
> > + return -EINVAL;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client)
> > if (ret)
> > return ret;
> >
> > - /* Check the hardware configuration in device tree */
> > - if (imx219_check_hwcfg(imx219))
> > - return -EINVAL;
> > -
> > /* Get system clock (xclk) */
> > imx219->xclk = devm_clk_get(dev, NULL);
> > if (IS_ERR(imx219->xclk)) {
> > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client)
> > }
> >
> > imx219->xclk_freq = clk_get_rate(imx219->xclk);
>
> My bug admittedly, but clk_get_rate returns an unsigned long, but
> imx219->xclk_freq is u32.
> Ideally imx219->xclk_freq should be unsigned long to match, and the
> dev_err I commented on earlier should be %lu.
>
> Cheers.
> Dave
>
> > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> > - dev_err(dev, "xclk frequency not supported: %d Hz\n",
> > - imx219->xclk_freq);
> > +
> > + /* Check the hardware configuration in device tree */
> > + if (imx219_check_hwcfg(imx219))
> > return -EINVAL;
> > - }
> >
> > ret = imx219_get_regulators(imx219);
> > if (ret) {
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables
2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford
@ 2022-04-25 16:20 ` Dave Stevenson
2022-04-25 16:50 ` Adam Ford
0 siblings, 1 reply; 16+ messages in thread
From: Dave Stevenson @ 2022-04-25 16:20 UTC (permalink / raw)
To: Adam Ford
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML
Hi Adam
On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
>
> 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 <aford173@gmail.com>
> ---
> drivers/media/i2c/imx219.c | 103 ++++++++++++++-----------------------
> 1 file changed, 39 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index e10af3f74b38..b7cc36b16547 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -145,19 +145,36 @@ 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},
> +/* To Access Addresses 3000-5fff, send the following commands */
> +static const struct imx219_reg mfg_specific_reg[] = {
> + {0x0100, 0x00}, /* Mode Select */
> {0x30eb, 0x0c},
> {0x30eb, 0x05},
> {0x300a, 0xff},
> {0x300b, 0xff},
> {0x30eb, 0x05},
> {0x30eb, 0x09},
> +};
> +
> +static const struct imx219_reg pll_clk_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},
> +};
(I've come back to this patch last as my first reading was happy with it)
Is there a good reason for making these two tables instead of one with
comments as to what the registers are doing?
As per my comment on patch 4, one table of registers setting these,
the DPHY register, and 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},
{0x0162, 0x0d},
{0x0163, 0x78},
would remove the duplication, reduce the code size, and be slightly
more readable.
Dave
> +/*
> + * 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[] = {
> {0x0114, 0x01},
> {0x0128, 0x00},
> {0x012a, 0x18},
> @@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> {0x0171, 0x01},
> {0x0174, 0x00},
> {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},
> @@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> };
>
> 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},
> @@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> {0x0171, 0x01},
> {0x0174, 0x00},
> {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},
> @@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> };
>
> 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},
> @@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> {0x0171, 0x01},
> {0x0174, 0x01},
> {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},
> @@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> };
>
> 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},
> @@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = {
> {0x0171, 0x01},
> {0x0174, 0x03},
> {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},
> @@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> if (ret < 0)
> return ret;
>
> + /* Send the Manufacturing Header common to all modes */
> + ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg));
> + 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);
> @@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219)
> goto err_rpm_put;
> }
>
> + /* Configure the PLL clocks */
> + ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table));
> + if (ret) {
> + dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__);
> + goto err_rpm_put;
> + }
> +
> +
> /* Apply customized values from user */
> ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> if (ret)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables
2022-04-25 16:20 ` Dave Stevenson
@ 2022-04-25 16:50 ` Adam Ford
2022-04-25 17:17 ` Dave Stevenson
0 siblings, 1 reply; 16+ messages in thread
From: Adam Ford @ 2022-04-25 16:50 UTC (permalink / raw)
To: Dave Stevenson
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML
On Mon, Apr 25, 2022 at 11:20 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Adam
>
> On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
> >
> > 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 <aford173@gmail.com>
> > ---
> > drivers/media/i2c/imx219.c | 103 ++++++++++++++-----------------------
> > 1 file changed, 39 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index e10af3f74b38..b7cc36b16547 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -145,19 +145,36 @@ 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},
> > +/* To Access Addresses 3000-5fff, send the following commands */
> > +static const struct imx219_reg mfg_specific_reg[] = {
> > + {0x0100, 0x00}, /* Mode Select */
> > {0x30eb, 0x0c},
> > {0x30eb, 0x05},
> > {0x300a, 0xff},
> > {0x300b, 0xff},
> > {0x30eb, 0x05},
> > {0x30eb, 0x09},
> > +};
> > +
> > +static const struct imx219_reg pll_clk_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},
> > +};
>
> (I've come back to this patch last as my first reading was happy with it)
> Is there a good reason for making these two tables instead of one with
> comments as to what the registers are doing?
The pll_clk tables were written after the resolution settings before I
split them. I was concerned about having all the common tables in one
place, because registers would be set in a different order than they
were originally. I wasn't sure if the pll clock tables needed to be
set after the resolution or not. It seemed possible to me that it
wouldn't necessarily know how to set the clocks without knowing the
desired resolution. I can certainly merge them together and run some
tests to see if there are regressions. If there are none, I can keep
them in a common block.
I can certainly add comments to indicate what's being done. I had
thought about it.
>
> As per my comment on patch 4, one table of registers setting these,
> the DPHY register, and 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},
> {0x0162, 0x0d},
> {0x0163, 0x78},
> would remove the duplication, reduce the code size, and be slightly
> more readable.
>
> Dave
>
> > +/*
> > + * 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[] = {
> > {0x0114, 0x01},
> > {0x0128, 0x00},
> > {0x012a, 0x18},
> > @@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > {0x0171, 0x01},
> > {0x0174, 0x00},
> > {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},
> > @@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > };
> >
> > 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},
> > @@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > {0x0171, 0x01},
> > {0x0174, 0x00},
> > {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},
> > @@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > };
> >
> > 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},
> > @@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > {0x0171, 0x01},
> > {0x0174, 0x01},
> > {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},
> > @@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > };
> >
> > 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},
> > @@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = {
> > {0x0171, 0x01},
> > {0x0174, 0x03},
> > {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},
> > @@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > if (ret < 0)
> > return ret;
> >
> > + /* Send the Manufacturing Header common to all modes */
> > + ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg));
> > + 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);
> > @@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > goto err_rpm_put;
> > }
> >
> > + /* Configure the PLL clocks */
> > + ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table));
> > + if (ret) {
> > + dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__);
> > + goto err_rpm_put;
> > + }
> > +
> > +
> > /* Apply customized values from user */
> > ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> > if (ret)
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables
2022-04-25 16:50 ` Adam Ford
@ 2022-04-25 17:17 ` Dave Stevenson
0 siblings, 0 replies; 16+ messages in thread
From: Dave Stevenson @ 2022-04-25 17:17 UTC (permalink / raw)
To: Adam Ford
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML
Hi Adam
On Mon, 25 Apr 2022 at 17:51, Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Apr 25, 2022 at 11:20 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Adam
> >
> > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
> > >
> > > 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 <aford173@gmail.com>
> > > ---
> > > drivers/media/i2c/imx219.c | 103 ++++++++++++++-----------------------
> > > 1 file changed, 39 insertions(+), 64 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index e10af3f74b38..b7cc36b16547 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -145,19 +145,36 @@ 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},
> > > +/* To Access Addresses 3000-5fff, send the following commands */
> > > +static const struct imx219_reg mfg_specific_reg[] = {
> > > + {0x0100, 0x00}, /* Mode Select */
> > > {0x30eb, 0x0c},
> > > {0x30eb, 0x05},
> > > {0x300a, 0xff},
> > > {0x300b, 0xff},
> > > {0x30eb, 0x05},
> > > {0x30eb, 0x09},
> > > +};
> > > +
> > > +static const struct imx219_reg pll_clk_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},
> > > +};
> >
> > (I've come back to this patch last as my first reading was happy with it)
> > Is there a good reason for making these two tables instead of one with
> > comments as to what the registers are doing?
>
> The pll_clk tables were written after the resolution settings before I
> split them. I was concerned about having all the common tables in one
> place, because registers would be set in a different order than they
> were originally. I wasn't sure if the pll clock tables needed to be
> set after the resolution or not. It seemed possible to me that it
> wouldn't necessarily know how to set the clocks without knowing the
> desired resolution. I can certainly merge them together and run some
> tests to see if there are regressions. If there are none, I can keep
> them in a common block.
>
> I can certainly add comments to indicate what's being done. I had
> thought about it.
More often PLL settings are set before the resolution configuration.
The initialisation sequence for imx219 was provided by Sony, but I
note that the datasheet that Table 37 "Initialization sequence with
XCLR" lists:
- (1) to (3): Refer power up sequence timing diagram
- (4) Set PLL parameters. Basic setting. Set Readout mode. Set MIPI parameters
- (5) Start streaming with 0x0100 (mode_select = 1)
So I really wouldn't be concerned about setting the PLL registers
before the resolution. The PLL configuration is mode independent as
the pixel rate and link frequency are effectively fixed.
Dave
> >
> > As per my comment on patch 4, one table of registers setting these,
> > the DPHY register, and 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},
> > {0x0162, 0x0d},
> > {0x0163, 0x78},
> > would remove the duplication, reduce the code size, and be slightly
> > more readable.
> >
> > Dave
> >
> > > +/*
> > > + * 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[] = {
> > > {0x0114, 0x01},
> > > {0x0128, 0x00},
> > > {0x012a, 0x18},
> > > @@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > > {0x0171, 0x01},
> > > {0x0174, 0x00},
> > > {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},
> > > @@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > > };
> > >
> > > 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},
> > > @@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > > {0x0171, 0x01},
> > > {0x0174, 0x00},
> > > {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},
> > > @@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > > };
> > >
> > > 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},
> > > @@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > > {0x0171, 0x01},
> > > {0x0174, 0x01},
> > > {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},
> > > @@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > > };
> > >
> > > 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},
> > > @@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = {
> > > {0x0171, 0x01},
> > > {0x0174, 0x03},
> > > {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},
> > > @@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > if (ret < 0)
> > > return ret;
> > >
> > > + /* Send the Manufacturing Header common to all modes */
> > > + ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg));
> > > + 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);
> > > @@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > goto err_rpm_put;
> > > }
> > >
> > > + /* Configure the PLL clocks */
> > > + ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table));
> > > + if (ret) {
> > > + dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__);
> > > + goto err_rpm_put;
> > > + }
> > > +
> > > +
> > > /* Apply customized values from user */
> > > ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> > > if (ret)
> > > --
> > > 2.34.1
> > >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK
2022-04-25 16:13 ` Dave Stevenson
@ 2022-04-25 17:28 ` Adam Ford
2022-04-25 17:57 ` Dave Stevenson
0 siblings, 1 reply; 16+ messages in thread
From: Adam Ford @ 2022-04-25 17:28 UTC (permalink / raw)
To: Dave Stevenson
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML
On Mon, Apr 25, 2022 at 11:13 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi again
>
> On Mon, 25 Apr 2022 at 16:58, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Adam
> >
> > I have no way of testing with an alternate XCLK value, so I'm working
> > based on the datasheet only.
> >
> > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
> > >
> > > The datasheet shows the external clock can be anything
> > > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and
> > > PREPLLCK_OP_DIV need to change based on the clock, so
> > > create helper functions to set these registers based on
> > > the rate of xclk.
> > >
> > > Move the validation of the clock rate into imx219_check_hwcfg
> > > which means delaying the call to it until after xclk has been
> > > determined.
> > >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++--------
> > > 1 file changed, 63 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index d13ce5c1ece6..08e7d0e72430 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -39,8 +39,12 @@
> > > #define IMX219_REG_CHIP_ID 0x0000
> > > #define IMX219_CHIP_ID 0x0219
> > >
> > > -/* External clock frequency is 24.0M */
> > > -#define IMX219_XCLK_FREQ 24000000
> > > +/* Default external clock frequency is 24.0M */
> > > +#define IMX219_XCLK_MIN_FREQ 6000000
> > > +#define IMX219_XCLK_MAX_FREQ 27000000
> > > +#define IMX219_REG_EXCK 0x012a
> > > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304
> > > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305
> > >
> > > /* Pixel rate is fixed for all the modes */
> > > #define IMX219_PIXEL_RATE 182400000
> > > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_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 */
> > > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = {
> > > */
> > > static const struct imx219_reg mode_3280x2464_regs[] = {
> > > {0x0128, 0x00},
> > > - {0x012a, 0x18},
> > > {0x012b, 0x00},
> > > {0x0164, 0x00},
> > > {0x0165, 0x00},
> > > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > >
> > > static const struct imx219_reg mode_1920_1080_regs[] = {
> > > {0x0128, 0x00},
> > > - {0x012a, 0x18},
> > > {0x012b, 0x00},
> > > {0x0162, 0x0d},
> > > {0x0163, 0x78},
> > > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > >
> > > static const struct imx219_reg mode_1640_1232_regs[] = {
> > > {0x0128, 0x00},
> > > - {0x012a, 0x18},
> > > {0x012b, 0x00},
> > > {0x0164, 0x00},
> > > {0x0165, 0x00},
> > > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > >
> > > static const struct imx219_reg mode_640_480_regs[] = {
> > > {0x0128, 0x00},
> > > - {0x012a, 0x18},
> > > {0x012b, 0x00},
> > > {0x0162, 0x0d},
> > > {0x0163, 0x78},
> > > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> > > return ret;
> > > };
> > >
> > > +static int imx219_set_exck_freq(struct imx219 *imx219)
> > > +{
> > > + int ret;
> > > + /* The imx219 registers need MHz not Hz */
> > > + u8 clk = (u8) (imx219->xclk_freq/1000000U);
> > > +
> > > + /* Set the clock frequency in MHz */
> > > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
> > > + IMX219_REG_VALUE_08BIT, clk);
>
> In reviewing your other patch I noticed that the EXCK register is
> actually a 16 bit value. The integer part is in 0x012a, and the
> fractional part (1/256) in 0x012b, which is currently initialised from
> the mode tables.
> Your division discards the fractional part totally, so if the
> configured frequency was 19.2MHz, then it would be programmed
> incorrectly.
>
> The value for register 0x012b needs to be computed and set here.
That makes sense.
I am understanding your comment about register 0x12b, would the
example frequency of 19.2MHz translate to 51 for register 12b?
Part of me thinks I should drop this patch because I have no real way
to test it, and I don't like writing 'theoretical' code.
adam
>
> > > +
> > > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */
> > > + switch (clk) {
> > > + case 6 ... 11:
> > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > + IMX219_REG_VALUE_08BIT, 0x01);
> > > + if (ret)
> > > + return ret;
> > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > + IMX219_REG_VALUE_08BIT, 0x01);
> > > + return ret;
> > > + case 12 ... 23:
> > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > + IMX219_REG_VALUE_08BIT, 0x02);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > + IMX219_REG_VALUE_08BIT, 0x02);
> > > +
> > > + return ret;
> > > + case 24 ... 27:
> > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > + IMX219_REG_VALUE_08BIT, 0x03);
> > > + if (ret)
> > > + return ret;
> > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > + IMX219_REG_VALUE_08BIT, 0x03);
> > > + return ret;
> > > + default:
> > > + /* Should never get here */
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > static int imx219_start_streaming(struct imx219 *imx219)
> > > {
> > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > goto err_rpm_put;
> > > }
> > >
> > > + /* Configure clock based on reference clock frequency */
> > > + imx219_set_exck_freq(imx219);
> >
> > You're not checking the return value from this function, so any I2C
> > failures will be ignored.
> >
> > > +
> > > /* 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);
> > > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219)
> > > return -EINVAL;
> > > }
> > >
> > > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) ||
> > > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) {
> > > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n",
> >
> > imx219->xclk_freq is unsigned, so %u
> >
> > > + imx219->xclk_freq);
> > > + return -EINVAL;
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client)
> > > if (ret)
> > > return ret;
> > >
> > > - /* Check the hardware configuration in device tree */
> > > - if (imx219_check_hwcfg(imx219))
> > > - return -EINVAL;
> > > -
> > > /* Get system clock (xclk) */
> > > imx219->xclk = devm_clk_get(dev, NULL);
> > > if (IS_ERR(imx219->xclk)) {
> > > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client)
> > > }
> > >
> > > imx219->xclk_freq = clk_get_rate(imx219->xclk);
> >
> > My bug admittedly, but clk_get_rate returns an unsigned long, but
> > imx219->xclk_freq is u32.
> > Ideally imx219->xclk_freq should be unsigned long to match, and the
> > dev_err I commented on earlier should be %lu.
> >
> > Cheers.
> > Dave
> >
> > > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> > > - dev_err(dev, "xclk frequency not supported: %d Hz\n",
> > > - imx219->xclk_freq);
> > > +
> > > + /* Check the hardware configuration in device tree */
> > > + if (imx219_check_hwcfg(imx219))
> > > return -EINVAL;
> > > - }
> > >
> > > ret = imx219_get_regulators(imx219);
> > > if (ret) {
> > > --
> > > 2.34.1
> > >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK
2022-04-25 17:28 ` Adam Ford
@ 2022-04-25 17:57 ` Dave Stevenson
0 siblings, 0 replies; 16+ messages in thread
From: Dave Stevenson @ 2022-04-25 17:57 UTC (permalink / raw)
To: Adam Ford
Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens,
Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML
On Mon, 25 Apr 2022 at 18:28, Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Apr 25, 2022 at 11:13 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi again
> >
> > On Mon, 25 Apr 2022 at 16:58, Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> > >
> > > Hi Adam
> > >
> > > I have no way of testing with an alternate XCLK value, so I'm working
> > > based on the datasheet only.
> > >
> > > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote:
> > > >
> > > > The datasheet shows the external clock can be anything
> > > > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and
> > > > PREPLLCK_OP_DIV need to change based on the clock, so
> > > > create helper functions to set these registers based on
> > > > the rate of xclk.
> > > >
> > > > Move the validation of the clock rate into imx219_check_hwcfg
> > > > which means delaying the call to it until after xclk has been
> > > > determined.
> > > >
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > ---
> > > > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++--------
> > > > 1 file changed, 63 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index d13ce5c1ece6..08e7d0e72430 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -39,8 +39,12 @@
> > > > #define IMX219_REG_CHIP_ID 0x0000
> > > > #define IMX219_CHIP_ID 0x0219
> > > >
> > > > -/* External clock frequency is 24.0M */
> > > > -#define IMX219_XCLK_FREQ 24000000
> > > > +/* Default external clock frequency is 24.0M */
> > > > +#define IMX219_XCLK_MIN_FREQ 6000000
> > > > +#define IMX219_XCLK_MAX_FREQ 27000000
> > > > +#define IMX219_REG_EXCK 0x012a
> > > > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304
> > > > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305
> > > >
> > > > /* Pixel rate is fixed for all the modes */
> > > > #define IMX219_PIXEL_RATE 182400000
> > > > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_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 */
> > > > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = {
> > > > */
> > > > static const struct imx219_reg mode_3280x2464_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0164, 0x00},
> > > > {0x0165, 0x00},
> > > > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > > >
> > > > static const struct imx219_reg mode_1920_1080_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0162, 0x0d},
> > > > {0x0163, 0x78},
> > > > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > > >
> > > > static const struct imx219_reg mode_1640_1232_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0164, 0x00},
> > > > {0x0165, 0x00},
> > > > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > > >
> > > > static const struct imx219_reg mode_640_480_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0162, 0x0d},
> > > > {0x0163, 0x78},
> > > > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> > > > return ret;
> > > > };
> > > >
> > > > +static int imx219_set_exck_freq(struct imx219 *imx219)
> > > > +{
> > > > + int ret;
> > > > + /* The imx219 registers need MHz not Hz */
> > > > + u8 clk = (u8) (imx219->xclk_freq/1000000U);
> > > > +
> > > > + /* Set the clock frequency in MHz */
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
> > > > + IMX219_REG_VALUE_08BIT, clk);
> >
> > In reviewing your other patch I noticed that the EXCK register is
> > actually a 16 bit value. The integer part is in 0x012a, and the
> > fractional part (1/256) in 0x012b, which is currently initialised from
> > the mode tables.
> > Your division discards the fractional part totally, so if the
> > configured frequency was 19.2MHz, then it would be programmed
> > incorrectly.
> >
> > The value for register 0x012b needs to be computed and set here.
>
> That makes sense.
> I am understanding your comment about register 0x12b, would the
> example frequency of 19.2MHz translate to 51 for register 12b?
Yes, AIUI 19.2MHz would be 0x13 0x33 (decimal 19 and 51 / 256).
The datasheet lists the default as being a 7.6MHz clock with register
values 0x07 0x99 (7 and 153/256, or 7.5976MHz)
> Part of me thinks I should drop this patch because I have no real way
> to test it, and I don't like writing 'theoretical' code.
I have some reservations over it for the same reasons.
If you haven't got an actual use case for it, then drop it for now.
Dave
> adam
> >
> > > > +
> > > > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */
> > > > + switch (clk) {
> > > > + case 6 ... 11:
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x01);
> > > > + if (ret)
> > > > + return ret;
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x01);
> > > > + return ret;
> > > > + case 12 ... 23:
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x02);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x02);
> > > > +
> > > > + return ret;
> > > > + case 24 ... 27:
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x03);
> > > > + if (ret)
> > > > + return ret;
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x03);
> > > > + return ret;
> > > > + default:
> > > > + /* Should never get here */
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > > > static int imx219_start_streaming(struct imx219 *imx219)
> > > > {
> > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > > goto err_rpm_put;
> > > > }
> > > >
> > > > + /* Configure clock based on reference clock frequency */
> > > > + imx219_set_exck_freq(imx219);
> > >
> > > You're not checking the return value from this function, so any I2C
> > > failures will be ignored.
> > >
> > > > +
> > > > /* 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);
> > > > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219)
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) ||
> > > > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) {
> > > > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n",
> > >
> > > imx219->xclk_freq is unsigned, so %u
> > >
> > > > + imx219->xclk_freq);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - /* Check the hardware configuration in device tree */
> > > > - if (imx219_check_hwcfg(imx219))
> > > > - return -EINVAL;
> > > > -
> > > > /* Get system clock (xclk) */
> > > > imx219->xclk = devm_clk_get(dev, NULL);
> > > > if (IS_ERR(imx219->xclk)) {
> > > > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client)
> > > > }
> > > >
> > > > imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > >
> > > My bug admittedly, but clk_get_rate returns an unsigned long, but
> > > imx219->xclk_freq is u32.
> > > Ideally imx219->xclk_freq should be unsigned long to match, and the
> > > dev_err I commented on earlier should be %lu.
> > >
> > > Cheers.
> > > Dave
> > >
> > > > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> > > > - dev_err(dev, "xclk frequency not supported: %d Hz\n",
> > > > - imx219->xclk_freq);
> > > > +
> > > > + /* Check the hardware configuration in device tree */
> > > > + if (imx219_check_hwcfg(imx219))
> > > > return -EINVAL;
> > > > - }
> > > >
> > > > ret = imx219_get_regulators(imx219);
> > > > if (ret) {
> > > > --
> > > > 2.34.1
> > > >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK
@ 2022-04-13 2:52 kernel test robot
0 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2022-04-13 2:52 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 21197 bytes --]
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220412135534.2796158-4-aford173@gmail.com>
References: <20220412135534.2796158-4-aford173@gmail.com>
TO: Adam Ford <aford173@gmail.com>
TO: linux-media(a)vger.kernel.org
CC: prabhakar.mahadev-lad.rj(a)bp.renesas.com
CC: tharvey(a)gateworks.com
CC: cstevens(a)beaconembedded.com
CC: aford(a)beaconembedded.com
CC: laurent.pinchart(a)ideasonboard.com
CC: Adam Ford <aford173@gmail.com>
CC: Dave Stevenson <dave.stevenson@raspberrypi.com>
CC: Mauro Carvalho Chehab <mchehab@kernel.org>
CC: linux-kernel(a)vger.kernel.org
Hi Adam,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.18-rc2 next-20220412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Adam-Ford/media-i2c-imx219-Enable-variable-xclk-and-4-lane/20220412-215748
base: git://linuxtv.org/media_tree.git master
:::::: branch date: 13 hours ago
:::::: commit date: 13 hours ago
config: x86_64-randconfig-c007-20220411 (https://download.01.org/0day-ci/archive/20220413/202204131055.oqAU9K1D-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fe2478d44e4f7f191c43fef629ac7a23d0251e72)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7e77bfbf6a383bd37e05915762cb9263ecf88f55
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Adam-Ford/media-i2c-imx219-Enable-variable-xclk-and-4-lane/20220412-215748
git checkout 7e77bfbf6a383bd37e05915762cb9263ecf88f55
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
include/linux/minmax.h:28:26: note: expanded from macro '__cmp'
#define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
^
drivers/hwmon/lm75.h:27:14: note: '__UNIQUE_ID___x242' is < '__UNIQUE_ID___y243'
int ntemp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
^
include/linux/minmax.h:137:32: note: expanded from macro 'clamp_val'
#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:124:36: note: expanded from macro 'clamp_t'
#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:104:27: note: expanded from macro 'min_t'
#define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:38:3: note: expanded from macro '__careful_cmp'
__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:33:3: note: expanded from macro '__cmp_once'
__cmp(unique_x, unique_y, op); })
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:28:26: note: expanded from macro '__cmp'
#define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
^~~
drivers/hwmon/lm75.h:27:14: note: '?' condition is true
int ntemp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
^
include/linux/minmax.h:137:32: note: expanded from macro 'clamp_val'
#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
^
include/linux/minmax.h:124:36: note: expanded from macro 'clamp_t'
#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
^
include/linux/minmax.h:104:27: note: expanded from macro 'min_t'
#define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
^
include/linux/minmax.h:38:3: note: expanded from macro '__careful_cmp'
__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
^
include/linux/minmax.h:33:3: note: expanded from macro '__cmp_once'
__cmp(unique_x, unique_y, op); })
^
include/linux/minmax.h:28:26: note: expanded from macro '__cmp'
#define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
^
drivers/hwmon/lm75.h:29:12: note: 'ntemp' is < 0
ntemp += (ntemp < 0 ? -250 : 250);
^~~~~
drivers/hwmon/lm75.h:29:12: note: '?' condition is true
drivers/hwmon/lm75.h:30:29: note: The result of the left shift is undefined because the left operand is negative
return (u16)((ntemp / 500) << 7);
~~~~~~~~~~~~~ ^
Suppressed 42 warnings (41 in non-user code, 1 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
23 warnings generated.
drivers/hwmon/ltc2947-core.c:295:2: warning: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memset(alarms, 0, sizeof(alarms));
^
include/linux/fortify-string.h:272:25: note: expanded from macro 'memset'
#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:265:2: note: expanded from macro '__fortify_memset_chk'
__underlying_memset(p, c, __fortify_size); \
^~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:47:29: note: expanded from macro '__underlying_memset'
#define __underlying_memset __builtin_memset
^~~~~~~~~~~~~~~~
drivers/hwmon/ltc2947-core.c:295:2: note: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11
memset(alarms, 0, sizeof(alarms));
^
include/linux/fortify-string.h:272:25: note: expanded from macro 'memset'
#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:265:2: note: expanded from macro '__fortify_memset_chk'
__underlying_memset(p, c, __fortify_size); \
^~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:47:29: note: expanded from macro '__underlying_memset'
#define __underlying_memset __builtin_memset
^~~~~~~~~~~~~~~~
drivers/hwmon/ltc2947-core.c:336:9: warning: Call to function 'sprintf' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'sprintf_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
return sprintf(buf, "%lld\n", val);
^~~~~~~
drivers/hwmon/ltc2947-core.c:336:9: note: Call to function 'sprintf' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'sprintf_s' in case of C11
return sprintf(buf, "%lld\n", val);
^~~~~~~
Suppressed 21 warnings (21 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
41 warnings generated.
Suppressed 41 warnings (41 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
41 warnings generated.
Suppressed 41 warnings (41 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
41 warnings generated.
Suppressed 41 warnings (41 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
41 warnings generated.
Suppressed 41 warnings (41 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
44 warnings generated.
>> drivers/media/i2c/imx219.c:1023:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/imx219.c:1023:2: note: Value stored to 'ret' is never read
ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Suppressed 43 warnings (43 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
44 warnings generated.
drivers/media/i2c/imx258.c:781:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/imx258.c:781:3: note: Value stored to 'ret' is never read
ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Suppressed 43 warnings (43 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
44 warnings generated.
Suppressed 44 warnings (43 in non-user code, 1 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
43 warnings generated.
Suppressed 43 warnings (43 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
59 warnings generated.
drivers/scsi/ch.c:247:2: warning: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memset(cmd,0,sizeof(cmd));
^
include/linux/fortify-string.h:272:25: note: expanded from macro 'memset'
#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:265:2: note: expanded from macro '__fortify_memset_chk'
__underlying_memset(p, c, __fortify_size); \
^~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:47:29: note: expanded from macro '__underlying_memset'
#define __underlying_memset __builtin_memset
^~~~~~~~~~~~~~~~
drivers/scsi/ch.c:247:2: note: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11
memset(cmd,0,sizeof(cmd));
^
include/linux/fortify-string.h:272:25: note: expanded from macro 'memset'
#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:265:2: note: expanded from macro '__fortify_memset_chk'
__underlying_memset(p, c, __fortify_size); \
^~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:47:29: note: expanded from macro '__underlying_memset'
#define __underlying_memset __builtin_memset
^~~~~~~~~~~~~~~~
drivers/scsi/ch.c:264:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memcpy(data,buffer+16,16);
^
include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
#define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
__underlying_##op(p, q, __fortify_size); \
^~~~~~~~~~~~~~~~~
note: expanded from here
include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
#define __underlying_memcpy __builtin_memcpy
^~~~~~~~~~~~~~~~
drivers/scsi/ch.c:264:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
memcpy(data,buffer+16,16);
^
include/linux/fortify-string.h:369:26: note: expanded from macro 'memcpy'
#define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:362:2: note: expanded from macro '__fortify_memcpy_chk'
__underlying_##op(p, q, __fortify_size); \
^~~~~~~~~~~~~~~~~
note: expanded from here
include/linux/fortify-string.h:45:29: note: expanded from macro '__underlying_memcpy'
#define __underlying_memcpy __builtin_memcpy
^~~~~~~~~~~~~~~~
drivers/scsi/ch.c:284:2: warning: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memset(cmd,0,sizeof(cmd));
^
include/linux/fortify-string.h:272:25: note: expanded from macro 'memset'
#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:265:2: note: expanded from macro '__fortify_memset_chk'
__underlying_memset(p, c, __fortify_size); \
^~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:47:29: note: expanded from macro '__underlying_memset'
#define __underlying_memset __builtin_memset
^~~~~~~~~~~~~~~~
drivers/scsi/ch.c:284:2: note: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11
memset(cmd,0,sizeof(cmd));
^
include/linux/fortify-string.h:272:25: note: expanded from macro 'memset'
#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:265:2: note: expanded from macro '__fortify_memset_chk'
__underlying_memset(p, c, __fortify_size); \
^~~~~~~~~~~~~~~~~~~
include/linux/fortify-string.h:47:29: note: expanded from macro '__underlying_memset'
#define __underlying_memset __builtin_memset
^~~~~~~~~~~~~~~~
drivers/scsi/ch.c:304:2: warning: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memset(cmd,0,sizeof(cmd));
^
vim +/ret +1023 drivers/media/i2c/imx219.c
72b5174b2b64255 Adam Ford 2022-04-12 1015
7e77bfbf6a383bd Adam Ford 2022-04-12 1016 static int imx219_set_exck_freq(struct imx219 *imx219)
7e77bfbf6a383bd Adam Ford 2022-04-12 1017 {
7e77bfbf6a383bd Adam Ford 2022-04-12 1018 int ret;
7e77bfbf6a383bd Adam Ford 2022-04-12 1019 /* The imx219 registers need MHz not Hz */
7e77bfbf6a383bd Adam Ford 2022-04-12 1020 u8 clk = (u8) (imx219->xclk_freq/1000000U);
7e77bfbf6a383bd Adam Ford 2022-04-12 1021
7e77bfbf6a383bd Adam Ford 2022-04-12 1022 /* Set the clock frequency in MHz */
7e77bfbf6a383bd Adam Ford 2022-04-12 @1023 ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
7e77bfbf6a383bd Adam Ford 2022-04-12 1024 IMX219_REG_VALUE_08BIT, clk);
7e77bfbf6a383bd Adam Ford 2022-04-12 1025
7e77bfbf6a383bd Adam Ford 2022-04-12 1026 /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */
7e77bfbf6a383bd Adam Ford 2022-04-12 1027 switch (clk) {
7e77bfbf6a383bd Adam Ford 2022-04-12 1028 case 6 ... 11:
7e77bfbf6a383bd Adam Ford 2022-04-12 1029 ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
7e77bfbf6a383bd Adam Ford 2022-04-12 1030 IMX219_REG_VALUE_08BIT, 0x01);
7e77bfbf6a383bd Adam Ford 2022-04-12 1031 if (ret)
7e77bfbf6a383bd Adam Ford 2022-04-12 1032 return ret;
7e77bfbf6a383bd Adam Ford 2022-04-12 1033 ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
7e77bfbf6a383bd Adam Ford 2022-04-12 1034 IMX219_REG_VALUE_08BIT, 0x01);
7e77bfbf6a383bd Adam Ford 2022-04-12 1035 return ret;
7e77bfbf6a383bd Adam Ford 2022-04-12 1036 case 12 ... 23:
7e77bfbf6a383bd Adam Ford 2022-04-12 1037 ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
7e77bfbf6a383bd Adam Ford 2022-04-12 1038 IMX219_REG_VALUE_08BIT, 0x02);
7e77bfbf6a383bd Adam Ford 2022-04-12 1039 if (ret)
7e77bfbf6a383bd Adam Ford 2022-04-12 1040 return ret;
7e77bfbf6a383bd Adam Ford 2022-04-12 1041
7e77bfbf6a383bd Adam Ford 2022-04-12 1042 ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
7e77bfbf6a383bd Adam Ford 2022-04-12 1043 IMX219_REG_VALUE_08BIT, 0x02);
7e77bfbf6a383bd Adam Ford 2022-04-12 1044
7e77bfbf6a383bd Adam Ford 2022-04-12 1045 return ret;
7e77bfbf6a383bd Adam Ford 2022-04-12 1046 case 24 ... 27:
7e77bfbf6a383bd Adam Ford 2022-04-12 1047 ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
7e77bfbf6a383bd Adam Ford 2022-04-12 1048 IMX219_REG_VALUE_08BIT, 0x03);
7e77bfbf6a383bd Adam Ford 2022-04-12 1049 if (ret)
7e77bfbf6a383bd Adam Ford 2022-04-12 1050 return ret;
7e77bfbf6a383bd Adam Ford 2022-04-12 1051 ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
7e77bfbf6a383bd Adam Ford 2022-04-12 1052 IMX219_REG_VALUE_08BIT, 0x03);
7e77bfbf6a383bd Adam Ford 2022-04-12 1053 return ret;
7e77bfbf6a383bd Adam Ford 2022-04-12 1054 default:
7e77bfbf6a383bd Adam Ford 2022-04-12 1055 /* Should never get here */
7e77bfbf6a383bd Adam Ford 2022-04-12 1056 return -EINVAL;
7e77bfbf6a383bd Adam Ford 2022-04-12 1057 }
7e77bfbf6a383bd Adam Ford 2022-04-12 1058 }
7e77bfbf6a383bd Adam Ford 2022-04-12 1059
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-04-25 17:57 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford
2022-04-25 16:20 ` Dave Stevenson
2022-04-25 16:50 ` Adam Ford
2022-04-25 17:17 ` Dave Stevenson
2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford
2022-04-25 15:39 ` Dave Stevenson
2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford
2022-04-25 15:58 ` Dave Stevenson
2022-04-25 16:13 ` Dave Stevenson
2022-04-25 17:28 ` Adam Ford
2022-04-25 17:57 ` Dave Stevenson
2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford
2022-04-25 16:07 ` Dave Stevenson
2022-04-25 12:08 ` [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
2022-04-13 2:52 [PATCH 3/4] media: i2c: imx219: Enable variable XCLK kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.