* [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables
@ 2022-06-16 12:31 Adam Ford
2022-06-16 12:31 ` [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation Adam Ford
2022-06-21 15:46 ` [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables Dave Stevenson
0 siblings, 2 replies; 8+ messages in thread
From: Adam Ford @ 2022-06-16 12:31 UTC (permalink / raw)
To: linux-media; +Cc: dave.stevenson, mchehab, linux-kernel, aford, Adam Ford
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>
---
V2: Merge the PLL table into the common table instead of having
two separate, common tables.
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index e10af3f74b38..a43eed143999 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -145,23 +145,60 @@ 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 imx219_common_regs[] = {
+ {0x0100, 0x00}, /* Mode Select */
+
+ /* Access Command Sequence */
{0x30eb, 0x0c},
{0x30eb, 0x05},
{0x300a, 0xff},
{0x300b, 0xff},
{0x30eb, 0x05},
{0x30eb, 0x09},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
+
+ /* PLL Clock Table */
+ {0x0301, 0x05}, /* VTPXCK_DIV */
+ {0x0303, 0x01}, /* VTSYSCK_DIV */
+ {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */
+ {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */
+ {0x0306, 0x00}, /* PLL_VT_MPY */
+ {0x0307, 0x39},
+ {0x030b, 0x01}, /* OP_SYS_CLK_DIV */
+ {0x030c, 0x00}, /* PLL_OP_MPY */
+ {0x030d, 0x72},
+
+ /* Undocumented registers */
+ {0x455e, 0x00},
+ {0x471e, 0x4b},
+ {0x4767, 0x0f},
+ {0x4750, 0x14},
+ {0x4540, 0x00},
+ {0x47b4, 0x14},
+ {0x4713, 0x30},
+ {0x478b, 0x10},
+ {0x478f, 0x10},
+ {0x4793, 0x10},
+ {0x4797, 0x0e},
+ {0x479b, 0x0e},
+
+ /* Frame Bank Register Group "A" */
+ {0x0162, 0x0d}, /* Line_Length_A */
+ {0x0163, 0x78},
+
+ /* Output setup registers */
+ {0x0114, 0x01}, /* CSI 2-Lane Mode */
+ {0x0128, 0x00}, /* DPHY Auto Mode */
+ {0x012a, 0x18}, /* EXCK_Freq */
{0x012b, 0x00},
+};
+
+/*
+ * Register sets lifted off the i2C interface from the Raspberry Pi firmware
+ * driver.
+ * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
+ */
+static const struct imx219_reg mode_3280x2464_regs[] = {
{0x0164, 0x00},
{0x0165, 0x00},
{0x0166, 0x0c},
@@ -176,51 +213,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
{0x016f, 0xa0},
{0x0170, 0x01},
{0x0171, 0x01},
- {0x0174, 0x00},
+ {0x0174, 0x00}, /* No-Binning */
{0x0175, 0x00},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x0c},
{0x0625, 0xd0},
{0x0626, 0x09},
{0x0627, 0xa0},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
- {0x0162, 0x0d},
- {0x0163, 0x78},
};
static const struct imx219_reg mode_1920_1080_regs[] = {
- {0x0100, 0x00},
- {0x30eb, 0x05},
- {0x30eb, 0x0c},
- {0x300a, 0xff},
- {0x300b, 0xff},
- {0x30eb, 0x05},
- {0x30eb, 0x09},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
- {0x012b, 0x00},
- {0x0162, 0x0d},
- {0x0163, 0x78},
{0x0164, 0x02},
{0x0165, 0xa8},
{0x0166, 0x0a},
@@ -235,47 +236,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
{0x016f, 0x38},
{0x0170, 0x01},
{0x0171, 0x01},
- {0x0174, 0x00},
+ {0x0174, 0x00}, /* No-Binning */
{0x0175, 0x00},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x07},
{0x0625, 0x80},
{0x0626, 0x04},
{0x0627, 0x38},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
};
static const struct imx219_reg mode_1640_1232_regs[] = {
- {0x0100, 0x00},
- {0x30eb, 0x0c},
- {0x30eb, 0x05},
- {0x300a, 0xff},
- {0x300b, 0xff},
- {0x30eb, 0x05},
- {0x30eb, 0x09},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
- {0x012b, 0x00},
{0x0164, 0x00},
{0x0165, 0x00},
{0x0166, 0x0c},
@@ -290,51 +259,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
{0x016f, 0xd0},
{0x0170, 0x01},
{0x0171, 0x01},
- {0x0174, 0x01},
+ {0x0174, 0x01}, /* x2-Binning */
{0x0175, 0x01},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x06},
{0x0625, 0x68},
{0x0626, 0x04},
{0x0627, 0xd0},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
- {0x0162, 0x0d},
- {0x0163, 0x78},
};
static const struct imx219_reg mode_640_480_regs[] = {
- {0x0100, 0x00},
- {0x30eb, 0x05},
- {0x30eb, 0x0c},
- {0x300a, 0xff},
- {0x300b, 0xff},
- {0x30eb, 0x05},
- {0x30eb, 0x09},
- {0x0114, 0x01},
- {0x0128, 0x00},
- {0x012a, 0x18},
- {0x012b, 0x00},
- {0x0162, 0x0d},
- {0x0163, 0x78},
{0x0164, 0x03},
{0x0165, 0xe8},
{0x0166, 0x08},
@@ -349,33 +282,12 @@ static const struct imx219_reg mode_640_480_regs[] = {
{0x016f, 0xe0},
{0x0170, 0x01},
{0x0171, 0x01},
- {0x0174, 0x03},
+ {0x0174, 0x03}, /* x2-analog binning */
{0x0175, 0x03},
- {0x0301, 0x05},
- {0x0303, 0x01},
- {0x0304, 0x03},
- {0x0305, 0x03},
- {0x0306, 0x00},
- {0x0307, 0x39},
- {0x030b, 0x01},
- {0x030c, 0x00},
- {0x030d, 0x72},
{0x0624, 0x06},
{0x0625, 0x68},
{0x0626, 0x04},
{0x0627, 0xd0},
- {0x455e, 0x00},
- {0x471e, 0x4b},
- {0x4767, 0x0f},
- {0x4750, 0x14},
- {0x4540, 0x00},
- {0x47b4, 0x14},
- {0x4713, 0x30},
- {0x478b, 0x10},
- {0x478f, 0x10},
- {0x4793, 0x10},
- {0x4797, 0x0e},
- {0x479b, 0x0e},
};
static const struct imx219_reg raw8_framefmt_regs[] = {
@@ -1041,6 +953,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, imx219_common_regs, ARRAY_SIZE(imx219_common_regs));
+ if (ret) {
+ dev_err(&client->dev, "%s failed to send mfg header\n", __func__);
+ goto err_rpm_put;
+ }
+
/* Apply default values of current mode */
reg_list = &imx219->mode->reg_list;
ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation 2022-06-16 12:31 [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables Adam Ford @ 2022-06-16 12:31 ` Adam Ford 2022-06-21 16:05 ` Dave Stevenson 2022-06-21 15:46 ` [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables Dave Stevenson 1 sibling, 1 reply; 8+ messages in thread From: Adam Ford @ 2022-06-16 12:31 UTC (permalink / raw) To: linux-media; +Cc: dave.stevenson, mchehab, linux-kernel, aford, Adam Ford 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. Signed-off-by: Adam Ford <aford173@gmail.com> --- V2: Replace if-else statements with ternary operator Fix 4-lane Link Rate. Fix checking the link rate so only the link rate for the given number of lanes is permitted. diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index a43eed143999..0d9004a5c4d2 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -42,10 +42,16 @@ /* External clock frequency is 24.0M */ #define IMX219_XCLK_FREQ 24000000 -/* Pixel rate is fixed at 182.4M for all the modes */ +/* Pixel rate is fixed for all the modes */ #define IMX219_PIXEL_RATE 182400000 +#define IMX219_PIXEL_RATE_4LANE 280800000 #define IMX219_DEFAULT_LINK_FREQ 456000000 +#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 + +#define IMX219_REG_CSI_LANE_MODE 0x0114 +#define IMX219_CSI_2_LANE_MODE 0x01 +#define IMX219_CSI_4_LANE_MODE 0x03 /* V_TIMING internal */ #define IMX219_REG_VTS 0x0160 @@ -306,6 +312,10 @@ static const s64 imx219_link_freq_menu[] = { IMX219_DEFAULT_LINK_FREQ, }; +static const s64 imx219_link_freq_4lane_menu[] = { + IMX219_DEFAULT_LINK_FREQ_4LANE, +}; + static const char * const imx219_test_pattern_menu[] = { "Disabled", "Color Bars", @@ -481,6 +491,9 @@ struct imx219 { /* Streaming on/off */ bool streaming; + + /* Two or Four lanes */ + u8 lanes; }; static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) @@ -943,6 +956,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, return -EINVAL; } +static int imx219_configure_lanes(struct imx219 *imx219) +{ + return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, + IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ? + IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE); +}; + static int imx219_start_streaming(struct imx219 *imx219) { struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); @@ -960,6 +980,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); @@ -1191,6 +1218,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { .open = imx219_open, }; +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) +{ + return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; +} + /* Initialize control handlers */ static int imx219_init_controls(struct imx219 *imx219) { @@ -1212,15 +1244,16 @@ static int imx219_init_controls(struct imx219 *imx219) /* By default, PIXEL_RATE is read only */ imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_PIXEL_RATE, - IMX219_PIXEL_RATE, - IMX219_PIXEL_RATE, 1, - IMX219_PIXEL_RATE); + imx219_get_pixel_rate(imx219), + imx219_get_pixel_rate(imx219), 1, + imx219_get_pixel_rate(imx219)); imx219->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_LINK_FREQ, ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, - imx219_link_freq_menu); + (imx219->lanes == 2) ? imx219_link_freq_menu : + imx219_link_freq_4lane_menu); if (imx219->link_freq) imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; @@ -1315,67 +1348,76 @@ 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, u64 link_frequency) { - 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 (link_frequency != ((imx219->lanes == 2) ? + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE)) { + dev_err(&client->dev, "Link frequency not supported: %lld\n", + 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; + u64 link_frequency = 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; /* Check the link frequency set in device tree */ - if (!ep_cfg.nr_of_link_frequencies) { + if (ep_cfg.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; } + 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, link_frequency)) return -EINVAL; /* Get system clock (xclk) */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation 2022-06-16 12:31 ` [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation Adam Ford @ 2022-06-21 16:05 ` Dave Stevenson 2022-06-21 17:08 ` Adam Ford 0 siblings, 1 reply; 8+ messages in thread From: Dave Stevenson @ 2022-06-21 16:05 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Mauro Carvalho Chehab, LKML, Adam Ford-BE Hi Adam On Thu, 16 Jun 2022 at 13:31, 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. > > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > V2: Replace if-else statements with ternary operator > Fix 4-lane Link Rate. > Fix checking the link rate so only the link rate for > the given number of lanes is permitted. > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index a43eed143999..0d9004a5c4d2 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -42,10 +42,16 @@ > /* External clock frequency is 24.0M */ > #define IMX219_XCLK_FREQ 24000000 > > -/* Pixel rate is fixed at 182.4M for all the modes */ > +/* Pixel rate is fixed for all the modes */ > #define IMX219_PIXEL_RATE 182400000 > +#define IMX219_PIXEL_RATE_4LANE 280800000 > > #define IMX219_DEFAULT_LINK_FREQ 456000000 > +#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 > + > +#define IMX219_REG_CSI_LANE_MODE 0x0114 > +#define IMX219_CSI_2_LANE_MODE 0x01 > +#define IMX219_CSI_4_LANE_MODE 0x03 > > /* V_TIMING internal */ > #define IMX219_REG_VTS 0x0160 > @@ -306,6 +312,10 @@ static const s64 imx219_link_freq_menu[] = { > IMX219_DEFAULT_LINK_FREQ, > }; > > +static const s64 imx219_link_freq_4lane_menu[] = { > + IMX219_DEFAULT_LINK_FREQ_4LANE, > +}; > + > static const char * const imx219_test_pattern_menu[] = { > "Disabled", > "Color Bars", > @@ -481,6 +491,9 @@ struct imx219 { > > /* Streaming on/off */ > bool streaming; > + > + /* Two or Four lanes */ > + u8 lanes; > }; > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > @@ -943,6 +956,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > return -EINVAL; > } > > +static int imx219_configure_lanes(struct imx219 *imx219) > +{ > + return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, > + IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ? > + IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE); > +}; > + > static int imx219_start_streaming(struct imx219 *imx219) > { > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > @@ -960,6 +980,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); > @@ -1191,6 +1218,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { > .open = imx219_open, > }; > > +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > +{ > + return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > +} > + > /* Initialize control handlers */ > static int imx219_init_controls(struct imx219 *imx219) > { > @@ -1212,15 +1244,16 @@ static int imx219_init_controls(struct imx219 *imx219) > /* By default, PIXEL_RATE is read only */ > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_PIXEL_RATE, > - IMX219_PIXEL_RATE, > - IMX219_PIXEL_RATE, 1, > - IMX219_PIXEL_RATE); > + imx219_get_pixel_rate(imx219), > + imx219_get_pixel_rate(imx219), 1, > + imx219_get_pixel_rate(imx219)); > > imx219->link_freq = > v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_LINK_FREQ, > ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, > - imx219_link_freq_menu); > + (imx219->lanes == 2) ? imx219_link_freq_menu : > + imx219_link_freq_4lane_menu); > if (imx219->link_freq) > imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > @@ -1315,67 +1348,76 @@ 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, u64 link_frequency) > { > - 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 (link_frequency != ((imx219->lanes == 2) ? > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE)) { > + dev_err(&client->dev, "Link frequency not supported: %lld\n", > + 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; > + u64 link_frequency = 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; > > /* Check the link frequency set in device tree */ > - if (!ep_cfg.nr_of_link_frequencies) { > + if (ep_cfg.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; > } > + 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); Having a "goto" to the middle of a function, and then if(ret) return ret; is horrid. Working with diffs has messed this up a bit which is why I hadn't noticed it in the last patch set, but I was about to comment that link_frequency doesn't need to be initialised to 0 above, but it does due to this (except it should take the return path). I'm not quite sure why you need to move the fwnode calls out of imx219_check_hwcfg. Pass in struct imx219 *imx219 and it can check the link_frequency and assign imx219->lanes with the values it was already validating. You can drop the struct device *dev as it is available via imx219->sd->dev, initialised by v4l2_i2c_subdev_init. The v4l2_fwnode_endpoint_free and fwnode_handle_put can then remain at the end of the function as common cleanup code. Rename imx219_check_hwcfg to imx219_read_hwcfg if the name offends as it is now doing more than just checking it. Dave > - > - 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, link_frequency)) > return -EINVAL; > > /* Get system clock (xclk) */ > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation 2022-06-21 16:05 ` Dave Stevenson @ 2022-06-21 17:08 ` Adam Ford 2022-06-21 17:31 ` Dave Stevenson 0 siblings, 1 reply; 8+ messages in thread From: Adam Ford @ 2022-06-21 17:08 UTC (permalink / raw) To: Dave Stevenson Cc: Linux Media Mailing List, Mauro Carvalho Chehab, LKML, Adam Ford-BE On Tue, Jun 21, 2022 at 11:05 AM Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Adam > > On Thu, 16 Jun 2022 at 13:31, 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. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > V2: Replace if-else statements with ternary operator > > Fix 4-lane Link Rate. > > Fix checking the link rate so only the link rate for > > the given number of lanes is permitted. > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index a43eed143999..0d9004a5c4d2 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -42,10 +42,16 @@ > > /* External clock frequency is 24.0M */ > > #define IMX219_XCLK_FREQ 24000000 > > > > -/* Pixel rate is fixed at 182.4M for all the modes */ > > +/* Pixel rate is fixed for all the modes */ > > #define IMX219_PIXEL_RATE 182400000 > > +#define IMX219_PIXEL_RATE_4LANE 280800000 > > > > #define IMX219_DEFAULT_LINK_FREQ 456000000 > > +#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 > > + > > +#define IMX219_REG_CSI_LANE_MODE 0x0114 > > +#define IMX219_CSI_2_LANE_MODE 0x01 > > +#define IMX219_CSI_4_LANE_MODE 0x03 > > > > /* V_TIMING internal */ > > #define IMX219_REG_VTS 0x0160 > > @@ -306,6 +312,10 @@ static const s64 imx219_link_freq_menu[] = { > > IMX219_DEFAULT_LINK_FREQ, > > }; > > > > +static const s64 imx219_link_freq_4lane_menu[] = { > > + IMX219_DEFAULT_LINK_FREQ_4LANE, > > +}; > > + > > static const char * const imx219_test_pattern_menu[] = { > > "Disabled", > > "Color Bars", > > @@ -481,6 +491,9 @@ struct imx219 { > > > > /* Streaming on/off */ > > bool streaming; > > + > > + /* Two or Four lanes */ > > + u8 lanes; > > }; > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > > @@ -943,6 +956,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > > return -EINVAL; > > } > > > > +static int imx219_configure_lanes(struct imx219 *imx219) > > +{ > > + return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, > > + IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ? > > + IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE); > > +}; > > + > > static int imx219_start_streaming(struct imx219 *imx219) > > { > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > @@ -960,6 +980,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); > > @@ -1191,6 +1218,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { > > .open = imx219_open, > > }; > > > > +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > > +{ > > + return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > > +} > > + > > /* Initialize control handlers */ > > static int imx219_init_controls(struct imx219 *imx219) > > { > > @@ -1212,15 +1244,16 @@ static int imx219_init_controls(struct imx219 *imx219) > > /* By default, PIXEL_RATE is read only */ > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > > V4L2_CID_PIXEL_RATE, > > - IMX219_PIXEL_RATE, > > - IMX219_PIXEL_RATE, 1, > > - IMX219_PIXEL_RATE); > > + imx219_get_pixel_rate(imx219), > > + imx219_get_pixel_rate(imx219), 1, > > + imx219_get_pixel_rate(imx219)); > > > > imx219->link_freq = > > v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, > > V4L2_CID_LINK_FREQ, > > ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, > > - imx219_link_freq_menu); > > + (imx219->lanes == 2) ? imx219_link_freq_menu : > > + imx219_link_freq_4lane_menu); > > if (imx219->link_freq) > > imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > @@ -1315,67 +1348,76 @@ 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, u64 link_frequency) > > { > > - 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 (link_frequency != ((imx219->lanes == 2) ? > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE)) { > > + dev_err(&client->dev, "Link frequency not supported: %lld\n", > > + 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; > > + u64 link_frequency = 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; > > > > /* Check the link frequency set in device tree */ > > - if (!ep_cfg.nr_of_link_frequencies) { > > + if (ep_cfg.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; > > } > > + 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); > > Having a "goto" to the middle of a function, and then if(ret) return > ret; is horrid. Working with diffs has messed this up a bit which is > why I hadn't noticed it in the last patch set, but I was about to > comment that link_frequency doesn't need to be initialised to 0 above, > but it does due to this (except it should take the return path). > > I'm not quite sure why you need to move the fwnode calls out of > imx219_check_hwcfg. Pass in struct imx219 *imx219 and it can check the It seemed more appropriate to me for probe to set the values instead of passing it to a helper function to set them. Since the Probe function needed to extract the fwnode info to do this, I moved the calls out of the helper. > link_frequency and assign imx219->lanes with the values it was already > validating. You can drop the struct device *dev as it is available via > imx219->sd->dev, initialised by v4l2_i2c_subdev_init. The > v4l2_fwnode_endpoint_free and fwnode_handle_put can then remain at the > end of the function as common cleanup code. > Rename imx219_check_hwcfg to imx219_read_hwcfg if the name offends as > it is now doing more than just checking it. Would be you ok if we did it all in the probe function and drop this helper function? > > Dave > > > - > > - 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, link_frequency)) > > return -EINVAL; > > > > /* Get system clock (xclk) */ > > -- > > 2.34.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation 2022-06-21 17:08 ` Adam Ford @ 2022-06-21 17:31 ` Dave Stevenson 0 siblings, 0 replies; 8+ messages in thread From: Dave Stevenson @ 2022-06-21 17:31 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Mauro Carvalho Chehab, LKML, Adam Ford-BE On Tue, 21 Jun 2022 at 18:08, Adam Ford <aford173@gmail.com> wrote: > > On Tue, Jun 21, 2022 at 11:05 AM Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Adam > > > > On Thu, 16 Jun 2022 at 13:31, 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. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > --- > > > V2: Replace if-else statements with ternary operator > > > Fix 4-lane Link Rate. > > > Fix checking the link rate so only the link rate for > > > the given number of lanes is permitted. > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > > index a43eed143999..0d9004a5c4d2 100644 > > > --- a/drivers/media/i2c/imx219.c > > > +++ b/drivers/media/i2c/imx219.c > > > @@ -42,10 +42,16 @@ > > > /* External clock frequency is 24.0M */ > > > #define IMX219_XCLK_FREQ 24000000 > > > > > > -/* Pixel rate is fixed at 182.4M for all the modes */ > > > +/* Pixel rate is fixed for all the modes */ > > > #define IMX219_PIXEL_RATE 182400000 > > > +#define IMX219_PIXEL_RATE_4LANE 280800000 > > > > > > #define IMX219_DEFAULT_LINK_FREQ 456000000 > > > +#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 > > > + > > > +#define IMX219_REG_CSI_LANE_MODE 0x0114 > > > +#define IMX219_CSI_2_LANE_MODE 0x01 > > > +#define IMX219_CSI_4_LANE_MODE 0x03 > > > > > > /* V_TIMING internal */ > > > #define IMX219_REG_VTS 0x0160 > > > @@ -306,6 +312,10 @@ static const s64 imx219_link_freq_menu[] = { > > > IMX219_DEFAULT_LINK_FREQ, > > > }; > > > > > > +static const s64 imx219_link_freq_4lane_menu[] = { > > > + IMX219_DEFAULT_LINK_FREQ_4LANE, > > > +}; > > > + > > > static const char * const imx219_test_pattern_menu[] = { > > > "Disabled", > > > "Color Bars", > > > @@ -481,6 +491,9 @@ struct imx219 { > > > > > > /* Streaming on/off */ > > > bool streaming; > > > + > > > + /* Two or Four lanes */ > > > + u8 lanes; > > > }; > > > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > > > @@ -943,6 +956,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > > > return -EINVAL; > > > } > > > > > > +static int imx219_configure_lanes(struct imx219 *imx219) > > > +{ > > > + return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, > > > + IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ? > > > + IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE); > > > +}; > > > + > > > static int imx219_start_streaming(struct imx219 *imx219) > > > { > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > > @@ -960,6 +980,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); > > > @@ -1191,6 +1218,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { > > > .open = imx219_open, > > > }; > > > > > > +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > > > +{ > > > + return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > > > +} > > > + > > > /* Initialize control handlers */ > > > static int imx219_init_controls(struct imx219 *imx219) > > > { > > > @@ -1212,15 +1244,16 @@ static int imx219_init_controls(struct imx219 *imx219) > > > /* By default, PIXEL_RATE is read only */ > > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > > > V4L2_CID_PIXEL_RATE, > > > - IMX219_PIXEL_RATE, > > > - IMX219_PIXEL_RATE, 1, > > > - IMX219_PIXEL_RATE); > > > + imx219_get_pixel_rate(imx219), > > > + imx219_get_pixel_rate(imx219), 1, > > > + imx219_get_pixel_rate(imx219)); > > > > > > imx219->link_freq = > > > v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, > > > V4L2_CID_LINK_FREQ, > > > ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, > > > - imx219_link_freq_menu); > > > + (imx219->lanes == 2) ? imx219_link_freq_menu : > > > + imx219_link_freq_4lane_menu); > > > if (imx219->link_freq) > > > imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > > > @@ -1315,67 +1348,76 @@ 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, u64 link_frequency) > > > { > > > - 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 (link_frequency != ((imx219->lanes == 2) ? > > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE)) { > > > + dev_err(&client->dev, "Link frequency not supported: %lld\n", > > > + 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; > > > + u64 link_frequency = 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; > > > > > > /* Check the link frequency set in device tree */ > > > - if (!ep_cfg.nr_of_link_frequencies) { > > > + if (ep_cfg.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; > > > } > > > + 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); > > > > Having a "goto" to the middle of a function, and then if(ret) return > > ret; is horrid. Working with diffs has messed this up a bit which is > > why I hadn't noticed it in the last patch set, but I was about to > > comment that link_frequency doesn't need to be initialised to 0 above, > > but it does due to this (except it should take the return path). > > > > I'm not quite sure why you need to move the fwnode calls out of > > imx219_check_hwcfg. Pass in struct imx219 *imx219 and it can check the > > It seemed more appropriate to me for probe to set the values instead > of passing it to a helper function to set them. Since the Probe > function needed to extract the fwnode info to do this, I moved the > calls out of the helper. > > > link_frequency and assign imx219->lanes with the values it was already > > validating. You can drop the struct device *dev as it is available via > > imx219->sd->dev, initialised by v4l2_i2c_subdev_init. The > > v4l2_fwnode_endpoint_free and fwnode_handle_put can then remain at the > > end of the function as common cleanup code. > > Rename imx219_check_hwcfg to imx219_read_hwcfg if the name offends as > > it is now doing more than just checking it. > > Would be you ok if we did it all in the probe function and drop this > helper function? imx219_probe was 115 lines. Your changes increased it to 154, and you're now wanting to take it up to ~165, and it still needs some further work to clean up the error handling. Handling the endpoint and extracting the CSI config from it is self-contained as an operation. Why does it need to directly be in probe? The compiler is very likely going to inline the helper, but breaking it up aids readability IMHO. Look at imx334 [1] and it goes further and gets all the GPIO config within a helper function (imx334_parse_hw_config). [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/imx334.c#L776 > > > > Dave > > > > > - > > > - 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, link_frequency)) > > > return -EINVAL; > > > > > > /* Get system clock (xclk) */ > > > -- > > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables 2022-06-16 12:31 [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables Adam Ford 2022-06-16 12:31 ` [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation Adam Ford @ 2022-06-21 15:46 ` Dave Stevenson 2022-06-21 15:58 ` Adam Ford 1 sibling, 1 reply; 8+ messages in thread From: Dave Stevenson @ 2022-06-21 15:46 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Mauro Carvalho Chehab, LKML, Adam Ford-BE Hi Adam Thanks for the patch, and sorry it's taken me a few days to get to it. On Thu, 16 Jun 2022 at 13:31, 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> > --- > V2: Merge the PLL table into the common table instead of having > two separate, common tables. > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index e10af3f74b38..a43eed143999 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -145,23 +145,60 @@ 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 imx219_common_regs[] = { > + {0x0100, 0x00}, /* Mode Select */ > + > + /* Access Command Sequence */ > {0x30eb, 0x0c}, > {0x30eb, 0x05}, > {0x300a, 0xff}, > {0x300b, 0xff}, > {0x30eb, 0x05}, > {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > + > + /* PLL Clock Table */ > + {0x0301, 0x05}, /* VTPXCK_DIV */ > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > + {0x0306, 0x00}, /* PLL_VT_MPY */ > + {0x0307, 0x39}, > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > + {0x030c, 0x00}, /* PLL_OP_MPY */ > + {0x030d, 0x72}, > + > + /* Undocumented registers */ > + {0x455e, 0x00}, > + {0x471e, 0x4b}, > + {0x4767, 0x0f}, > + {0x4750, 0x14}, > + {0x4540, 0x00}, > + {0x47b4, 0x14}, > + {0x4713, 0x30}, > + {0x478b, 0x10}, > + {0x478f, 0x10}, > + {0x4793, 0x10}, > + {0x4797, 0x0e}, > + {0x479b, 0x0e}, > + > + /* Frame Bank Register Group "A" */ > + {0x0162, 0x0d}, /* Line_Length_A */ > + {0x0163, 0x78}, Registers 0x0170 and 0x171 for X_ODD_INC_A and Y_ODD_INC_A are also common to all modes as 0x01. You could have modes with skipping, but currently there are none. > + > + /* Output setup registers */ > + {0x0114, 0x01}, /* CSI 2-Lane Mode */ > + {0x0128, 0x00}, /* DPHY Auto Mode */ > + {0x012a, 0x18}, /* EXCK_Freq */ > {0x012b, 0x00}, > +}; > + > +/* > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > + * driver. > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > + */ > +static const struct imx219_reg mode_3280x2464_regs[] = { > {0x0164, 0x00}, > {0x0165, 0x00}, > {0x0166, 0x0c}, > @@ -176,51 +213,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > {0x016f, 0xa0}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x00}, > + {0x0174, 0x00}, /* No-Binning */ > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x0c}, > {0x0625, 0xd0}, > {0x0626, 0x09}, > {0x0627, 0xa0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > }; > > static const struct imx219_reg mode_1920_1080_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > {0x0164, 0x02}, > {0x0165, 0xa8}, > {0x0166, 0x0a}, > @@ -235,47 +236,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > {0x016f, 0x38}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x00}, > + {0x0174, 0x00}, /* No-Binning */ > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x07}, > {0x0625, 0x80}, > {0x0626, 0x04}, > {0x0627, 0x38}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > }; > > static const struct imx219_reg mode_1640_1232_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x0c}, > - {0x30eb, 0x05}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > {0x0164, 0x00}, > {0x0165, 0x00}, > {0x0166, 0x0c}, > @@ -290,51 +259,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > {0x016f, 0xd0}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x01}, > + {0x0174, 0x01}, /* x2-Binning */ > {0x0175, 0x01}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > {0x0627, 0xd0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > }; > > static const struct imx219_reg mode_640_480_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > {0x0164, 0x03}, > {0x0165, 0xe8}, > {0x0166, 0x08}, > @@ -349,33 +282,12 @@ static const struct imx219_reg mode_640_480_regs[] = { > {0x016f, 0xe0}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x03}, > + {0x0174, 0x03}, /* x2-analog binning */ > {0x0175, 0x03}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > {0x0627, 0xd0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > }; > > static const struct imx219_reg raw8_framefmt_regs[] = { > @@ -1041,6 +953,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > if (ret < 0) > return ret; > > + /* Send the Manufacturing Header common to all modes */ It's a table of common settings, not a manufacturing header. s/Send the Manufacturing Header/Send all registers that are Dave > + ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs)); > + if (ret) { > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > + goto err_rpm_put; > + } > + > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables 2022-06-21 15:46 ` [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables Dave Stevenson @ 2022-06-21 15:58 ` Adam Ford 2022-06-21 16:23 ` Dave Stevenson 0 siblings, 1 reply; 8+ messages in thread From: Adam Ford @ 2022-06-21 15:58 UTC (permalink / raw) To: Dave Stevenson Cc: Linux Media Mailing List, Mauro Carvalho Chehab, LKML, Adam Ford-BE On Tue, Jun 21, 2022 at 10:46 AM Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Adam > > Thanks for the patch, and sorry it's taken me a few days to get to it. > > On Thu, 16 Jun 2022 at 13:31, 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> > > --- > > V2: Merge the PLL table into the common table instead of having > > two separate, common tables. > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index e10af3f74b38..a43eed143999 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -145,23 +145,60 @@ 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 imx219_common_regs[] = { > > + {0x0100, 0x00}, /* Mode Select */ > > + > > + /* Access Command Sequence */ > > {0x30eb, 0x0c}, > > {0x30eb, 0x05}, > > {0x300a, 0xff}, > > {0x300b, 0xff}, > > {0x30eb, 0x05}, > > {0x30eb, 0x09}, > > - {0x0114, 0x01}, > > - {0x0128, 0x00}, > > - {0x012a, 0x18}, > > + > > + /* PLL Clock Table */ > > + {0x0301, 0x05}, /* VTPXCK_DIV */ > > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > + {0x0306, 0x00}, /* PLL_VT_MPY */ > > + {0x0307, 0x39}, > > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > > + {0x030c, 0x00}, /* PLL_OP_MPY */ > > + {0x030d, 0x72}, > > + > > + /* Undocumented registers */ > > + {0x455e, 0x00}, > > + {0x471e, 0x4b}, > > + {0x4767, 0x0f}, > > + {0x4750, 0x14}, > > + {0x4540, 0x00}, > > + {0x47b4, 0x14}, > > + {0x4713, 0x30}, > > + {0x478b, 0x10}, > > + {0x478f, 0x10}, > > + {0x4793, 0x10}, > > + {0x4797, 0x0e}, > > + {0x479b, 0x0e}, > > + > > + /* Frame Bank Register Group "A" */ > > + {0x0162, 0x0d}, /* Line_Length_A */ > > + {0x0163, 0x78}, > > Registers 0x0170 and 0x171 for X_ODD_INC_A and Y_ODD_INC_A are also > common to all modes as 0x01. You could have modes with skipping, but > currently there are none. > > > + > > + /* Output setup registers */ > > + {0x0114, 0x01}, /* CSI 2-Lane Mode */ > > + {0x0128, 0x00}, /* DPHY Auto Mode */ > > + {0x012a, 0x18}, /* EXCK_Freq */ > > {0x012b, 0x00}, > > +}; > > + > > +/* > > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > > + * driver. > > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > > + */ > > +static const struct imx219_reg mode_3280x2464_regs[] = { > > {0x0164, 0x00}, > > {0x0165, 0x00}, > > {0x0166, 0x0c}, > > @@ -176,51 +213,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > {0x016f, 0xa0}, > > {0x0170, 0x01}, > > {0x0171, 0x01}, > > - {0x0174, 0x00}, > > + {0x0174, 0x00}, /* No-Binning */ > > {0x0175, 0x00}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x0c}, > > {0x0625, 0xd0}, > > {0x0626, 0x09}, > > {0x0627, 0xa0}, > > - {0x455e, 0x00}, > > - {0x471e, 0x4b}, > > - {0x4767, 0x0f}, > > - {0x4750, 0x14}, > > - {0x4540, 0x00}, > > - {0x47b4, 0x14}, > > - {0x4713, 0x30}, > > - {0x478b, 0x10}, > > - {0x478f, 0x10}, > > - {0x4793, 0x10}, > > - {0x4797, 0x0e}, > > - {0x479b, 0x0e}, > > - {0x0162, 0x0d}, > > - {0x0163, 0x78}, > > }; > > > > static const struct imx219_reg mode_1920_1080_regs[] = { > > - {0x0100, 0x00}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x0c}, > > - {0x300a, 0xff}, > > - {0x300b, 0xff}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x09}, > > - {0x0114, 0x01}, > > - {0x0128, 0x00}, > > - {0x012a, 0x18}, > > - {0x012b, 0x00}, > > - {0x0162, 0x0d}, > > - {0x0163, 0x78}, > > {0x0164, 0x02}, > > {0x0165, 0xa8}, > > {0x0166, 0x0a}, > > @@ -235,47 +236,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > {0x016f, 0x38}, > > {0x0170, 0x01}, > > {0x0171, 0x01}, > > - {0x0174, 0x00}, > > + {0x0174, 0x00}, /* No-Binning */ > > {0x0175, 0x00}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x07}, > > {0x0625, 0x80}, > > {0x0626, 0x04}, > > {0x0627, 0x38}, > > - {0x455e, 0x00}, > > - {0x471e, 0x4b}, > > - {0x4767, 0x0f}, > > - {0x4750, 0x14}, > > - {0x4540, 0x00}, > > - {0x47b4, 0x14}, > > - {0x4713, 0x30}, > > - {0x478b, 0x10}, > > - {0x478f, 0x10}, > > - {0x4793, 0x10}, > > - {0x4797, 0x0e}, > > - {0x479b, 0x0e}, > > }; > > > > static const struct imx219_reg mode_1640_1232_regs[] = { > > - {0x0100, 0x00}, > > - {0x30eb, 0x0c}, > > - {0x30eb, 0x05}, > > - {0x300a, 0xff}, > > - {0x300b, 0xff}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x09}, > > - {0x0114, 0x01}, > > - {0x0128, 0x00}, > > - {0x012a, 0x18}, > > - {0x012b, 0x00}, > > {0x0164, 0x00}, > > {0x0165, 0x00}, > > {0x0166, 0x0c}, > > @@ -290,51 +259,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > {0x016f, 0xd0}, > > {0x0170, 0x01}, > > {0x0171, 0x01}, > > - {0x0174, 0x01}, > > + {0x0174, 0x01}, /* x2-Binning */ > > {0x0175, 0x01}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x06}, > > {0x0625, 0x68}, > > {0x0626, 0x04}, > > {0x0627, 0xd0}, > > - {0x455e, 0x00}, > > - {0x471e, 0x4b}, > > - {0x4767, 0x0f}, > > - {0x4750, 0x14}, > > - {0x4540, 0x00}, > > - {0x47b4, 0x14}, > > - {0x4713, 0x30}, > > - {0x478b, 0x10}, > > - {0x478f, 0x10}, > > - {0x4793, 0x10}, > > - {0x4797, 0x0e}, > > - {0x479b, 0x0e}, > > - {0x0162, 0x0d}, > > - {0x0163, 0x78}, > > }; > > > > static const struct imx219_reg mode_640_480_regs[] = { > > - {0x0100, 0x00}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x0c}, > > - {0x300a, 0xff}, > > - {0x300b, 0xff}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x09}, > > - {0x0114, 0x01}, > > - {0x0128, 0x00}, > > - {0x012a, 0x18}, > > - {0x012b, 0x00}, > > - {0x0162, 0x0d}, > > - {0x0163, 0x78}, > > {0x0164, 0x03}, > > {0x0165, 0xe8}, > > {0x0166, 0x08}, > > @@ -349,33 +282,12 @@ static const struct imx219_reg mode_640_480_regs[] = { > > {0x016f, 0xe0}, > > {0x0170, 0x01}, > > {0x0171, 0x01}, > > - {0x0174, 0x03}, > > + {0x0174, 0x03}, /* x2-analog binning */ > > {0x0175, 0x03}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x06}, > > {0x0625, 0x68}, > > {0x0626, 0x04}, > > {0x0627, 0xd0}, > > - {0x455e, 0x00}, > > - {0x471e, 0x4b}, > > - {0x4767, 0x0f}, > > - {0x4750, 0x14}, > > - {0x4540, 0x00}, > > - {0x47b4, 0x14}, > > - {0x4713, 0x30}, > > - {0x478b, 0x10}, > > - {0x478f, 0x10}, > > - {0x4793, 0x10}, > > - {0x4797, 0x0e}, > > - {0x479b, 0x0e}, > > }; > > > > static const struct imx219_reg raw8_framefmt_regs[] = { > > @@ -1041,6 +953,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > > if (ret < 0) > > return ret; > > > > + /* Send the Manufacturing Header common to all modes */ > > It's a table of common settings, not a manufacturing header. > s/Send the Manufacturing Header/Send all registers that are I used that term because the data sheet shows the following sequence: {0x30eb, 0x0c}, {0x30eb, 0x05}, {0x300a, 0xff}, {0x300b, 0xff}, {0x30eb, 0x05}, {0x30eb, 0x09}, It's listed as "Manufacturer Specific Registers" to access 0x3000-5fff, the specific sequence as specified in 3-4. The entire table is common, but I tried to put comments into the sections to explain what they do. > > Dave > > > + ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs)); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > > + goto err_rpm_put; > > + } > > + > > /* Apply default values of current mode */ > > reg_list = &imx219->mode->reg_list; > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > -- > > 2.34.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables 2022-06-21 15:58 ` Adam Ford @ 2022-06-21 16:23 ` Dave Stevenson 0 siblings, 0 replies; 8+ messages in thread From: Dave Stevenson @ 2022-06-21 16:23 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Mauro Carvalho Chehab, LKML, Adam Ford-BE On Tue, 21 Jun 2022 at 16:58, Adam Ford <aford173@gmail.com> wrote: > > On Tue, Jun 21, 2022 at 10:46 AM Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Adam > > > > Thanks for the patch, and sorry it's taken me a few days to get to it. > > > > On Thu, 16 Jun 2022 at 13:31, 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> > > > --- > > > V2: Merge the PLL table into the common table instead of having > > > two separate, common tables. > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > > index e10af3f74b38..a43eed143999 100644 > > > --- a/drivers/media/i2c/imx219.c > > > +++ b/drivers/media/i2c/imx219.c > > > @@ -145,23 +145,60 @@ 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 */ I've just noticed that this comment is still outside the table. The registers you list as "Access Command Sequence" are the commands to allow access, the rest are additional configuration. > > > +static const struct imx219_reg imx219_common_regs[] = { > > > + {0x0100, 0x00}, /* Mode Select */ > > > + > > > + /* Access Command Sequence */ > > > {0x30eb, 0x0c}, > > > {0x30eb, 0x05}, > > > {0x300a, 0xff}, > > > {0x300b, 0xff}, > > > {0x30eb, 0x05}, > > > {0x30eb, 0x09}, > > > - {0x0114, 0x01}, > > > - {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > + > > > + /* PLL Clock Table */ > > > + {0x0301, 0x05}, /* VTPXCK_DIV */ > > > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > > > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > > + {0x0306, 0x00}, /* PLL_VT_MPY */ > > > + {0x0307, 0x39}, > > > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > > > + {0x030c, 0x00}, /* PLL_OP_MPY */ > > > + {0x030d, 0x72}, > > > + > > > + /* Undocumented registers */ > > > + {0x455e, 0x00}, > > > + {0x471e, 0x4b}, > > > + {0x4767, 0x0f}, > > > + {0x4750, 0x14}, > > > + {0x4540, 0x00}, > > > + {0x47b4, 0x14}, > > > + {0x4713, 0x30}, > > > + {0x478b, 0x10}, > > > + {0x478f, 0x10}, > > > + {0x4793, 0x10}, > > > + {0x4797, 0x0e}, > > > + {0x479b, 0x0e}, > > > + > > > + /* Frame Bank Register Group "A" */ > > > + {0x0162, 0x0d}, /* Line_Length_A */ > > > + {0x0163, 0x78}, > > > > Registers 0x0170 and 0x171 for X_ODD_INC_A and Y_ODD_INC_A are also > > common to all modes as 0x01. You could have modes with skipping, but > > currently there are none. > > > > > + > > > + /* Output setup registers */ > > > + {0x0114, 0x01}, /* CSI 2-Lane Mode */ > > > + {0x0128, 0x00}, /* DPHY Auto Mode */ > > > + {0x012a, 0x18}, /* EXCK_Freq */ > > > {0x012b, 0x00}, > > > +}; > > > + > > > +/* > > > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > > > + * driver. > > > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > > > + */ > > > +static const struct imx219_reg mode_3280x2464_regs[] = { > > > {0x0164, 0x00}, > > > {0x0165, 0x00}, > > > {0x0166, 0x0c}, > > > @@ -176,51 +213,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > > {0x016f, 0xa0}, > > > {0x0170, 0x01}, > > > {0x0171, 0x01}, > > > - {0x0174, 0x00}, > > > + {0x0174, 0x00}, /* No-Binning */ > > > {0x0175, 0x00}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x0c}, > > > {0x0625, 0xd0}, > > > {0x0626, 0x09}, > > > {0x0627, 0xa0}, > > > - {0x455e, 0x00}, > > > - {0x471e, 0x4b}, > > > - {0x4767, 0x0f}, > > > - {0x4750, 0x14}, > > > - {0x4540, 0x00}, > > > - {0x47b4, 0x14}, > > > - {0x4713, 0x30}, > > > - {0x478b, 0x10}, > > > - {0x478f, 0x10}, > > > - {0x4793, 0x10}, > > > - {0x4797, 0x0e}, > > > - {0x479b, 0x0e}, > > > - {0x0162, 0x0d}, > > > - {0x0163, 0x78}, > > > }; > > > > > > static const struct imx219_reg mode_1920_1080_regs[] = { > > > - {0x0100, 0x00}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x0c}, > > > - {0x300a, 0xff}, > > > - {0x300b, 0xff}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x09}, > > > - {0x0114, 0x01}, > > > - {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > - {0x012b, 0x00}, > > > - {0x0162, 0x0d}, > > > - {0x0163, 0x78}, > > > {0x0164, 0x02}, > > > {0x0165, 0xa8}, > > > {0x0166, 0x0a}, > > > @@ -235,47 +236,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > > {0x016f, 0x38}, > > > {0x0170, 0x01}, > > > {0x0171, 0x01}, > > > - {0x0174, 0x00}, > > > + {0x0174, 0x00}, /* No-Binning */ > > > {0x0175, 0x00}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x07}, > > > {0x0625, 0x80}, > > > {0x0626, 0x04}, > > > {0x0627, 0x38}, > > > - {0x455e, 0x00}, > > > - {0x471e, 0x4b}, > > > - {0x4767, 0x0f}, > > > - {0x4750, 0x14}, > > > - {0x4540, 0x00}, > > > - {0x47b4, 0x14}, > > > - {0x4713, 0x30}, > > > - {0x478b, 0x10}, > > > - {0x478f, 0x10}, > > > - {0x4793, 0x10}, > > > - {0x4797, 0x0e}, > > > - {0x479b, 0x0e}, > > > }; > > > > > > static const struct imx219_reg mode_1640_1232_regs[] = { > > > - {0x0100, 0x00}, > > > - {0x30eb, 0x0c}, > > > - {0x30eb, 0x05}, > > > - {0x300a, 0xff}, > > > - {0x300b, 0xff}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x09}, > > > - {0x0114, 0x01}, > > > - {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > - {0x012b, 0x00}, > > > {0x0164, 0x00}, > > > {0x0165, 0x00}, > > > {0x0166, 0x0c}, > > > @@ -290,51 +259,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > > {0x016f, 0xd0}, > > > {0x0170, 0x01}, > > > {0x0171, 0x01}, > > > - {0x0174, 0x01}, > > > + {0x0174, 0x01}, /* x2-Binning */ > > > {0x0175, 0x01}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x06}, > > > {0x0625, 0x68}, > > > {0x0626, 0x04}, > > > {0x0627, 0xd0}, > > > - {0x455e, 0x00}, > > > - {0x471e, 0x4b}, > > > - {0x4767, 0x0f}, > > > - {0x4750, 0x14}, > > > - {0x4540, 0x00}, > > > - {0x47b4, 0x14}, > > > - {0x4713, 0x30}, > > > - {0x478b, 0x10}, > > > - {0x478f, 0x10}, > > > - {0x4793, 0x10}, > > > - {0x4797, 0x0e}, > > > - {0x479b, 0x0e}, > > > - {0x0162, 0x0d}, > > > - {0x0163, 0x78}, > > > }; > > > > > > static const struct imx219_reg mode_640_480_regs[] = { > > > - {0x0100, 0x00}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x0c}, > > > - {0x300a, 0xff}, > > > - {0x300b, 0xff}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x09}, > > > - {0x0114, 0x01}, > > > - {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > - {0x012b, 0x00}, > > > - {0x0162, 0x0d}, > > > - {0x0163, 0x78}, > > > {0x0164, 0x03}, > > > {0x0165, 0xe8}, > > > {0x0166, 0x08}, > > > @@ -349,33 +282,12 @@ static const struct imx219_reg mode_640_480_regs[] = { > > > {0x016f, 0xe0}, > > > {0x0170, 0x01}, > > > {0x0171, 0x01}, > > > - {0x0174, 0x03}, > > > + {0x0174, 0x03}, /* x2-analog binning */ > > > {0x0175, 0x03}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x06}, > > > {0x0625, 0x68}, > > > {0x0626, 0x04}, > > > {0x0627, 0xd0}, > > > - {0x455e, 0x00}, > > > - {0x471e, 0x4b}, > > > - {0x4767, 0x0f}, > > > - {0x4750, 0x14}, > > > - {0x4540, 0x00}, > > > - {0x47b4, 0x14}, > > > - {0x4713, 0x30}, > > > - {0x478b, 0x10}, > > > - {0x478f, 0x10}, > > > - {0x4793, 0x10}, > > > - {0x4797, 0x0e}, > > > - {0x479b, 0x0e}, > > > }; > > > > > > static const struct imx219_reg raw8_framefmt_regs[] = { > > > @@ -1041,6 +953,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > > > if (ret < 0) > > > return ret; > > > > > > + /* Send the Manufacturing Header common to all modes */ > > > > It's a table of common settings, not a manufacturing header. > > s/Send the Manufacturing Header/Send all registers that are > > I used that term because the data sheet shows the following sequence: > > {0x30eb, 0x0c}, > {0x30eb, 0x05}, > {0x300a, 0xff}, > {0x300b, 0xff}, > {0x30eb, 0x05}, > {0x30eb, 0x09}, > > It's listed as "Manufacturer Specific Registers" to access > 0x3000-5fff, the specific sequence as specified in 3-4. The entire > table is common, but I tried to put comments into the sections to > explain what they do. That little bit is, yes, and you refer to it as such in the comments for imx219_common_regs. At this point you are sending the complete table of imx219_common_regs, which includes registers outside that 0x3000-5fff address space. TBH All registers are manufacturer specific unless you implement the MIPI CCS or SMIA standards (which, it seems, almost no one does). > > > > Dave > > > > > + ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs)); > > > + if (ret) { > > > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > > > + goto err_rpm_put; > > > + } > > > + > > > /* Apply default values of current mode */ > > > reg_list = &imx219->mode->reg_list; > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > > -- > > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-21 17:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-16 12:31 [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables Adam Ford 2022-06-16 12:31 ` [PATCH V2 2/2] media: i2c: imx219: Support four-lane operation Adam Ford 2022-06-21 16:05 ` Dave Stevenson 2022-06-21 17:08 ` Adam Ford 2022-06-21 17:31 ` Dave Stevenson 2022-06-21 15:46 ` [PATCH V2 1/2] media: i2c: imx219: Split common registers from mode tables Dave Stevenson 2022-06-21 15:58 ` Adam Ford 2022-06-21 16:23 ` Dave Stevenson
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.