| Message ID | 20220709135052.3850913-1-aford173@gmail.com |
|---|---|
| State | Accepted |
| Commit | 8508455961d5a9e8907bcfd8dcd58f19d9b6ce47 |
| Headers | show |
| Series | [1/2] media: i2c: imx219: Split common registers from mode tables | expand |
On Sat, Jul 9, 2022 at 8:51 AM 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. > Dave, I've tried to address your concerns in this V3 series which is down to 2 patches. Any chance you can review them? I've tested the 4-lane mode on both an imx8mp (still images) and a Renesas RZ/G2M (streaming video) and both can capture in 4-lane mode. adam > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > V3: Change some of the comments to better reflect their description > Add a few more entries to the imx219_common_regs table. > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index e10af3f74b38..faa5dab3c2ec 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -145,23 +145,61 @@ struct imx219_mode { > struct imx219_reg_list reg_list; > }; > > -/* > - * Register sets lifted off the i2C interface from the Raspberry Pi firmware > - * driver. > - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > - */ > -static const struct imx219_reg mode_3280x2464_regs[] = { > - {0x0100, 0x00}, > +static const struct imx219_reg imx219_common_regs[] = { > + {0x0100, 0x00}, /* Mode Select */ > + > + /* To Access Addresses 3000-5fff, send the following commands */ > {0x30eb, 0x0c}, > {0x30eb, 0x05}, > {0x300a, 0xff}, > {0x300b, 0xff}, > {0x30eb, 0x05}, > {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > + > + /* PLL Clock Table */ > + {0x0301, 0x05}, /* VTPXCK_DIV */ > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > + {0x0306, 0x00}, /* PLL_VT_MPY */ > + {0x0307, 0x39}, > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > + {0x030c, 0x00}, /* PLL_OP_MPY */ > + {0x030d, 0x72}, > + > + /* Undocumented registers */ > + {0x455e, 0x00}, > + {0x471e, 0x4b}, > + {0x4767, 0x0f}, > + {0x4750, 0x14}, > + {0x4540, 0x00}, > + {0x47b4, 0x14}, > + {0x4713, 0x30}, > + {0x478b, 0x10}, > + {0x478f, 0x10}, > + {0x4793, 0x10}, > + {0x4797, 0x0e}, > + {0x479b, 0x0e}, > + > + /* Frame Bank Register Group "A" */ > + {0x0162, 0x0d}, /* Line_Length_A */ > + {0x0163, 0x78}, > + {0x0170, 0x01}, /* X_ODD_INC_A */ > + {0x0171, 0x01}, /* Y_ODD_INC_A */ > + > + /* Output setup registers */ > + {0x0114, 0x01}, /* CSI 2-Lane Mode */ > + {0x0128, 0x00}, /* DPHY Auto Mode */ > + {0x012a, 0x18}, /* EXCK_Freq */ > {0x012b, 0x00}, > +}; > + > +/* > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > + * driver. > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > + */ > +static const struct imx219_reg mode_3280x2464_regs[] = { > {0x0164, 0x00}, > {0x0165, 0x00}, > {0x0166, 0x0c}, > @@ -174,53 +212,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > {0x016d, 0xd0}, > {0x016e, 0x09}, > {0x016f, 0xa0}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x00}, > + {0x0174, 0x00}, /* No-Binning */ > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x0c}, > {0x0625, 0xd0}, > {0x0626, 0x09}, > {0x0627, 0xa0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > }; > > static const struct imx219_reg mode_1920_1080_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > {0x0164, 0x02}, > {0x0165, 0xa8}, > {0x0166, 0x0a}, > @@ -233,49 +233,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > {0x016d, 0x80}, > {0x016e, 0x04}, > {0x016f, 0x38}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x00}, > + {0x0174, 0x00}, /* No-Binning */ > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x07}, > {0x0625, 0x80}, > {0x0626, 0x04}, > {0x0627, 0x38}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > }; > > static const struct imx219_reg mode_1640_1232_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x0c}, > - {0x30eb, 0x05}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > {0x0164, 0x00}, > {0x0165, 0x00}, > {0x0166, 0x0c}, > @@ -288,53 +254,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > {0x016d, 0x68}, > {0x016e, 0x04}, > {0x016f, 0xd0}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x01}, > + {0x0174, 0x01}, /* x2-Binning */ > {0x0175, 0x01}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > {0x0627, 0xd0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > }; > > static const struct imx219_reg mode_640_480_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > {0x0164, 0x03}, > {0x0165, 0xe8}, > {0x0166, 0x08}, > @@ -347,35 +275,12 @@ static const struct imx219_reg mode_640_480_regs[] = { > {0x016d, 0x80}, > {0x016e, 0x01}, > {0x016f, 0xe0}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x03}, > + {0x0174, 0x03}, /* x2-analog binning */ > {0x0175, 0x03}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > {0x0627, 0xd0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > }; > > static const struct imx219_reg raw8_framefmt_regs[] = { > @@ -1041,6 +946,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > if (ret < 0) > return ret; > > + /* Send all registers that are common to all modes */ > + ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs)); > + if (ret) { > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > + goto err_rpm_put; > + } > + > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > -- > 2.34.1 >
Hi Adam Sorry for the delay in reviewing. On Sat, 9 Jul 2022 at 14:51, 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. This paragraph isn't relevant any more. > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > V3: Keep the helper function doing the link and lane parsing to > keep th probe function small. > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index faa5dab3c2ec..bb4125e7e113 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -42,10 +42,16 @@ > /* External clock frequency is 24.0M */ > #define IMX219_XCLK_FREQ 24000000 > > -/* Pixel rate is fixed at 182.4M for all the modes */ > +/* Pixel rate is fixed for all the modes */ > #define IMX219_PIXEL_RATE 182400000 > +#define IMX219_PIXEL_RATE_4LANE 280800000 > > #define IMX219_DEFAULT_LINK_FREQ 456000000 > +#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 > + > +#define IMX219_REG_CSI_LANE_MODE 0x0114 > +#define IMX219_CSI_2_LANE_MODE 0x01 > +#define IMX219_CSI_4_LANE_MODE 0x03 > > /* V_TIMING internal */ > #define IMX219_REG_VTS 0x0160 > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = { > IMX219_DEFAULT_LINK_FREQ, > }; > > +static const s64 imx219_link_freq_4lane_menu[] = { > + IMX219_DEFAULT_LINK_FREQ_4LANE, > +}; > + > static const char * const imx219_test_pattern_menu[] = { > "Disabled", > "Color Bars", > @@ -474,6 +484,9 @@ struct imx219 { > > /* Streaming on/off */ > bool streaming; > + > + /* Two or Four lanes */ > + u8 lanes; > }; > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > return -EINVAL; > } > > +static int imx219_configure_lanes(struct imx219 *imx219) > +{ > + return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, > + IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ? > + IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE); > +}; > + > static int imx219_start_streaming(struct imx219 *imx219) > { > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > goto err_rpm_put; > } > > + /* Configure two or four Lane mode */ > + ret = imx219_configure_lanes(imx219); > + if (ret) { > + dev_err(&client->dev, "%s failed to configure lanes\n", __func__); > + goto err_rpm_put; > + } > + > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { > .open = imx219_open, > }; > > +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > +{ > + return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > +} > + > /* Initialize control handlers */ > static int imx219_init_controls(struct imx219 *imx219) > { > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219) > /* By default, PIXEL_RATE is read only */ > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_PIXEL_RATE, > - IMX219_PIXEL_RATE, > - IMX219_PIXEL_RATE, 1, > - IMX219_PIXEL_RATE); > + imx219_get_pixel_rate(imx219), > + imx219_get_pixel_rate(imx219), 1, > + imx219_get_pixel_rate(imx219)); > > imx219->link_freq = > v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_LINK_FREQ, > ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, > - imx219_link_freq_menu); > + (imx219->lanes == 2) ? imx219_link_freq_menu : > + imx219_link_freq_4lane_menu); > if (imx219->link_freq) > imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219) > mutex_destroy(&imx219->mutex); > } > > -static int imx219_check_hwcfg(struct device *dev) > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219) > { > struct fwnode_handle *endpoint; > struct v4l2_fwnode_endpoint ep_cfg = { > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev) > } > > /* Check the number of MIPI CSI2 data lanes */ > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) { > - dev_err(dev, "only 2 data lanes are currently supported\n"); > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 && > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) { > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n"); > goto error_out; > } > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes; > > /* Check the link frequency set in device tree */ > if (!ep_cfg.nr_of_link_frequencies) { > @@ -1339,8 +1374,8 @@ static int imx219_check_hwcfg(struct device *dev) > goto error_out; > } > > - if (ep_cfg.nr_of_link_frequencies != 1 || > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) { > + if (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ? > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE)) { You've lost the check of ep_cfg.nr_of_link_frequencies != 1 in this update. Otherwise it looks good to me. Dave > dev_err(dev, "Link frequency not supported: %lld\n", > ep_cfg.link_frequencies[0]); > goto error_out; > @@ -1368,7 +1403,7 @@ static int imx219_probe(struct i2c_client *client) > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops); > > /* Check the hardware configuration in device tree */ > - if (imx219_check_hwcfg(dev)) > + if (imx219_check_hwcfg(dev, imx219)) > return -EINVAL; > > /* Get system clock (xclk) */ > -- > 2.34.1 >
On Sat, 9 Jul 2022 at 14:51, 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> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > V3: Change some of the comments to better reflect their description > Add a few more entries to the imx219_common_regs table. > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index e10af3f74b38..faa5dab3c2ec 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -145,23 +145,61 @@ struct imx219_mode { > struct imx219_reg_list reg_list; > }; > > -/* > - * Register sets lifted off the i2C interface from the Raspberry Pi firmware > - * driver. > - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > - */ > -static const struct imx219_reg mode_3280x2464_regs[] = { > - {0x0100, 0x00}, > +static const struct imx219_reg imx219_common_regs[] = { > + {0x0100, 0x00}, /* Mode Select */ > + > + /* To Access Addresses 3000-5fff, send the following commands */ > {0x30eb, 0x0c}, > {0x30eb, 0x05}, > {0x300a, 0xff}, > {0x300b, 0xff}, > {0x30eb, 0x05}, > {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > + > + /* PLL Clock Table */ > + {0x0301, 0x05}, /* VTPXCK_DIV */ > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > + {0x0306, 0x00}, /* PLL_VT_MPY */ > + {0x0307, 0x39}, > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > + {0x030c, 0x00}, /* PLL_OP_MPY */ > + {0x030d, 0x72}, > + > + /* Undocumented registers */ > + {0x455e, 0x00}, > + {0x471e, 0x4b}, > + {0x4767, 0x0f}, > + {0x4750, 0x14}, > + {0x4540, 0x00}, > + {0x47b4, 0x14}, > + {0x4713, 0x30}, > + {0x478b, 0x10}, > + {0x478f, 0x10}, > + {0x4793, 0x10}, > + {0x4797, 0x0e}, > + {0x479b, 0x0e}, > + > + /* Frame Bank Register Group "A" */ > + {0x0162, 0x0d}, /* Line_Length_A */ > + {0x0163, 0x78}, > + {0x0170, 0x01}, /* X_ODD_INC_A */ > + {0x0171, 0x01}, /* Y_ODD_INC_A */ > + > + /* Output setup registers */ > + {0x0114, 0x01}, /* CSI 2-Lane Mode */ > + {0x0128, 0x00}, /* DPHY Auto Mode */ > + {0x012a, 0x18}, /* EXCK_Freq */ > {0x012b, 0x00}, > +}; > + > +/* > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > + * driver. > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > + */ > +static const struct imx219_reg mode_3280x2464_regs[] = { > {0x0164, 0x00}, > {0x0165, 0x00}, > {0x0166, 0x0c}, > @@ -174,53 +212,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > {0x016d, 0xd0}, > {0x016e, 0x09}, > {0x016f, 0xa0}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x00}, > + {0x0174, 0x00}, /* No-Binning */ > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x0c}, > {0x0625, 0xd0}, > {0x0626, 0x09}, > {0x0627, 0xa0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > }; > > static const struct imx219_reg mode_1920_1080_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > {0x0164, 0x02}, > {0x0165, 0xa8}, > {0x0166, 0x0a}, > @@ -233,49 +233,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > {0x016d, 0x80}, > {0x016e, 0x04}, > {0x016f, 0x38}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x00}, > + {0x0174, 0x00}, /* No-Binning */ > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x07}, > {0x0625, 0x80}, > {0x0626, 0x04}, > {0x0627, 0x38}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > }; > > static const struct imx219_reg mode_1640_1232_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x0c}, > - {0x30eb, 0x05}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > {0x0164, 0x00}, > {0x0165, 0x00}, > {0x0166, 0x0c}, > @@ -288,53 +254,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > {0x016d, 0x68}, > {0x016e, 0x04}, > {0x016f, 0xd0}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x01}, > + {0x0174, 0x01}, /* x2-Binning */ > {0x0175, 0x01}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > {0x0627, 0xd0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > }; > > static const struct imx219_reg mode_640_480_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > - {0x0114, 0x01}, > - {0x0128, 0x00}, > - {0x012a, 0x18}, > - {0x012b, 0x00}, > - {0x0162, 0x0d}, > - {0x0163, 0x78}, > {0x0164, 0x03}, > {0x0165, 0xe8}, > {0x0166, 0x08}, > @@ -347,35 +275,12 @@ static const struct imx219_reg mode_640_480_regs[] = { > {0x016d, 0x80}, > {0x016e, 0x01}, > {0x016f, 0xe0}, > - {0x0170, 0x01}, > - {0x0171, 0x01}, > - {0x0174, 0x03}, > + {0x0174, 0x03}, /* x2-analog binning */ > {0x0175, 0x03}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > {0x0627, 0xd0}, > - {0x455e, 0x00}, > - {0x471e, 0x4b}, > - {0x4767, 0x0f}, > - {0x4750, 0x14}, > - {0x4540, 0x00}, > - {0x47b4, 0x14}, > - {0x4713, 0x30}, > - {0x478b, 0x10}, > - {0x478f, 0x10}, > - {0x4793, 0x10}, > - {0x4797, 0x0e}, > - {0x479b, 0x0e}, > }; > > static const struct imx219_reg raw8_framefmt_regs[] = { > @@ -1041,6 +946,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > if (ret < 0) > return ret; > > + /* Send all registers that are common to all modes */ > + ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs)); > + if (ret) { > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > + goto err_rpm_put; > + } > + > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > -- > 2.34.1 >
On Wed, Jul 27, 2022 at 9:30 AM Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Adam > > Sorry for the delay in reviewing. > > On Sat, 9 Jul 2022 at 14:51, 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. > > This paragraph isn't relevant any more. Oops. I forgot to review the commit message. I'll fix on V4. > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > V3: Keep the helper function doing the link and lane parsing to > > keep th probe function small. > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index faa5dab3c2ec..bb4125e7e113 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -42,10 +42,16 @@ > > /* External clock frequency is 24.0M */ > > #define IMX219_XCLK_FREQ 24000000 > > > > -/* Pixel rate is fixed at 182.4M for all the modes */ > > +/* Pixel rate is fixed for all the modes */ > > #define IMX219_PIXEL_RATE 182400000 > > +#define IMX219_PIXEL_RATE_4LANE 280800000 > > > > #define IMX219_DEFAULT_LINK_FREQ 456000000 > > +#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 > > + > > +#define IMX219_REG_CSI_LANE_MODE 0x0114 > > +#define IMX219_CSI_2_LANE_MODE 0x01 > > +#define IMX219_CSI_4_LANE_MODE 0x03 > > > > /* V_TIMING internal */ > > #define IMX219_REG_VTS 0x0160 > > @@ -299,6 +305,10 @@ static const s64 imx219_link_freq_menu[] = { > > IMX219_DEFAULT_LINK_FREQ, > > }; > > > > +static const s64 imx219_link_freq_4lane_menu[] = { > > + IMX219_DEFAULT_LINK_FREQ_4LANE, > > +}; > > + > > static const char * const imx219_test_pattern_menu[] = { > > "Disabled", > > "Color Bars", > > @@ -474,6 +484,9 @@ struct imx219 { > > > > /* Streaming on/off */ > > bool streaming; > > + > > + /* Two or Four lanes */ > > + u8 lanes; > > }; > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > > @@ -936,6 +949,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > > return -EINVAL; > > } > > > > +static int imx219_configure_lanes(struct imx219 *imx219) > > +{ > > + return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, > > + IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ? > > + IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE); > > +}; > > + > > static int imx219_start_streaming(struct imx219 *imx219) > > { > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > @@ -953,6 +973,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > > goto err_rpm_put; > > } > > > > + /* Configure two or four Lane mode */ > > + ret = imx219_configure_lanes(imx219); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to configure lanes\n", __func__); > > + goto err_rpm_put; > > + } > > + > > /* Apply default values of current mode */ > > reg_list = &imx219->mode->reg_list; > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > @@ -1184,6 +1211,11 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { > > .open = imx219_open, > > }; > > > > +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > > +{ > > + return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > > +} > > + > > /* Initialize control handlers */ > > static int imx219_init_controls(struct imx219 *imx219) > > { > > @@ -1205,15 +1237,16 @@ static int imx219_init_controls(struct imx219 *imx219) > > /* By default, PIXEL_RATE is read only */ > > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > > V4L2_CID_PIXEL_RATE, > > - IMX219_PIXEL_RATE, > > - IMX219_PIXEL_RATE, 1, > > - IMX219_PIXEL_RATE); > > + imx219_get_pixel_rate(imx219), > > + imx219_get_pixel_rate(imx219), 1, > > + imx219_get_pixel_rate(imx219)); > > > > imx219->link_freq = > > v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, > > V4L2_CID_LINK_FREQ, > > ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, > > - imx219_link_freq_menu); > > + (imx219->lanes == 2) ? imx219_link_freq_menu : > > + imx219_link_freq_4lane_menu); > > if (imx219->link_freq) > > imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > @@ -1308,7 +1341,7 @@ static void imx219_free_controls(struct imx219 *imx219) > > mutex_destroy(&imx219->mutex); > > } > > > > -static int imx219_check_hwcfg(struct device *dev) > > +static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219) > > { > > struct fwnode_handle *endpoint; > > struct v4l2_fwnode_endpoint ep_cfg = { > > @@ -1328,10 +1361,12 @@ static int imx219_check_hwcfg(struct device *dev) > > } > > > > /* Check the number of MIPI CSI2 data lanes */ > > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) { > > - dev_err(dev, "only 2 data lanes are currently supported\n"); > > + if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 && > > + ep_cfg.bus.mipi_csi2.num_data_lanes != 4) { > > + dev_err(dev, "only 2 or 4 data lanes are currently supported\n"); > > goto error_out; > > } > > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes; > > > > /* Check the link frequency set in device tree */ > > if (!ep_cfg.nr_of_link_frequencies) { > > @@ -1339,8 +1374,8 @@ static int imx219_check_hwcfg(struct device *dev) > > goto error_out; > > } > > > > - if (ep_cfg.nr_of_link_frequencies != 1 || > > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) { > > + if (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ? > > + IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE)) { > > You've lost the check of ep_cfg.nr_of_link_frequencies != 1 in this update. oops. I'll fix in V4. > > Otherwise it looks good to me. > Thanks for the review and feedback. adam > Dave > > > dev_err(dev, "Link frequency not supported: %lld\n", > > ep_cfg.link_frequencies[0]); > > goto error_out; > > @@ -1368,7 +1403,7 @@ static int imx219_probe(struct i2c_client *client) > > v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops); > > > > /* Check the hardware configuration in device tree */ > > - if (imx219_check_hwcfg(dev)) > > + if (imx219_check_hwcfg(dev, imx219)) > > return -EINVAL; > > > > /* Get system clock (xclk) */ > > -- > > 2.34.1 > >
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index e10af3f74b38..faa5dab3c2ec 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -145,23 +145,61 @@ struct imx219_mode { struct imx219_reg_list reg_list; }; -/* - * Register sets lifted off the i2C interface from the Raspberry Pi firmware - * driver. - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. - */ -static const struct imx219_reg mode_3280x2464_regs[] = { - {0x0100, 0x00}, +static const struct imx219_reg imx219_common_regs[] = { + {0x0100, 0x00}, /* Mode Select */ + + /* To Access Addresses 3000-5fff, send the following commands */ {0x30eb, 0x0c}, {0x30eb, 0x05}, {0x300a, 0xff}, {0x300b, 0xff}, {0x30eb, 0x05}, {0x30eb, 0x09}, - {0x0114, 0x01}, - {0x0128, 0x00}, - {0x012a, 0x18}, + + /* PLL Clock Table */ + {0x0301, 0x05}, /* VTPXCK_DIV */ + {0x0303, 0x01}, /* VTSYSCK_DIV */ + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ + {0x0306, 0x00}, /* PLL_VT_MPY */ + {0x0307, 0x39}, + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ + {0x030c, 0x00}, /* PLL_OP_MPY */ + {0x030d, 0x72}, + + /* Undocumented registers */ + {0x455e, 0x00}, + {0x471e, 0x4b}, + {0x4767, 0x0f}, + {0x4750, 0x14}, + {0x4540, 0x00}, + {0x47b4, 0x14}, + {0x4713, 0x30}, + {0x478b, 0x10}, + {0x478f, 0x10}, + {0x4793, 0x10}, + {0x4797, 0x0e}, + {0x479b, 0x0e}, + + /* Frame Bank Register Group "A" */ + {0x0162, 0x0d}, /* Line_Length_A */ + {0x0163, 0x78}, + {0x0170, 0x01}, /* X_ODD_INC_A */ + {0x0171, 0x01}, /* Y_ODD_INC_A */ + + /* Output setup registers */ + {0x0114, 0x01}, /* CSI 2-Lane Mode */ + {0x0128, 0x00}, /* DPHY Auto Mode */ + {0x012a, 0x18}, /* EXCK_Freq */ {0x012b, 0x00}, +}; + +/* + * Register sets lifted off the i2C interface from the Raspberry Pi firmware + * driver. + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. + */ +static const struct imx219_reg mode_3280x2464_regs[] = { {0x0164, 0x00}, {0x0165, 0x00}, {0x0166, 0x0c}, @@ -174,53 +212,15 @@ static const struct imx219_reg mode_3280x2464_regs[] = { {0x016d, 0xd0}, {0x016e, 0x09}, {0x016f, 0xa0}, - {0x0170, 0x01}, - {0x0171, 0x01}, - {0x0174, 0x00}, + {0x0174, 0x00}, /* No-Binning */ {0x0175, 0x00}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x0c}, {0x0625, 0xd0}, {0x0626, 0x09}, {0x0627, 0xa0}, - {0x455e, 0x00}, - {0x471e, 0x4b}, - {0x4767, 0x0f}, - {0x4750, 0x14}, - {0x4540, 0x00}, - {0x47b4, 0x14}, - {0x4713, 0x30}, - {0x478b, 0x10}, - {0x478f, 0x10}, - {0x4793, 0x10}, - {0x4797, 0x0e}, - {0x479b, 0x0e}, - {0x0162, 0x0d}, - {0x0163, 0x78}, }; static const struct imx219_reg mode_1920_1080_regs[] = { - {0x0100, 0x00}, - {0x30eb, 0x05}, - {0x30eb, 0x0c}, - {0x300a, 0xff}, - {0x300b, 0xff}, - {0x30eb, 0x05}, - {0x30eb, 0x09}, - {0x0114, 0x01}, - {0x0128, 0x00}, - {0x012a, 0x18}, - {0x012b, 0x00}, - {0x0162, 0x0d}, - {0x0163, 0x78}, {0x0164, 0x02}, {0x0165, 0xa8}, {0x0166, 0x0a}, @@ -233,49 +233,15 @@ static const struct imx219_reg mode_1920_1080_regs[] = { {0x016d, 0x80}, {0x016e, 0x04}, {0x016f, 0x38}, - {0x0170, 0x01}, - {0x0171, 0x01}, - {0x0174, 0x00}, + {0x0174, 0x00}, /* No-Binning */ {0x0175, 0x00}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x07}, {0x0625, 0x80}, {0x0626, 0x04}, {0x0627, 0x38}, - {0x455e, 0x00}, - {0x471e, 0x4b}, - {0x4767, 0x0f}, - {0x4750, 0x14}, - {0x4540, 0x00}, - {0x47b4, 0x14}, - {0x4713, 0x30}, - {0x478b, 0x10}, - {0x478f, 0x10}, - {0x4793, 0x10}, - {0x4797, 0x0e}, - {0x479b, 0x0e}, }; static const struct imx219_reg mode_1640_1232_regs[] = { - {0x0100, 0x00}, - {0x30eb, 0x0c}, - {0x30eb, 0x05}, - {0x300a, 0xff}, - {0x300b, 0xff}, - {0x30eb, 0x05}, - {0x30eb, 0x09}, - {0x0114, 0x01}, - {0x0128, 0x00}, - {0x012a, 0x18}, - {0x012b, 0x00}, {0x0164, 0x00}, {0x0165, 0x00}, {0x0166, 0x0c}, @@ -288,53 +254,15 @@ static const struct imx219_reg mode_1640_1232_regs[] = { {0x016d, 0x68}, {0x016e, 0x04}, {0x016f, 0xd0}, - {0x0170, 0x01}, - {0x0171, 0x01}, - {0x0174, 0x01}, + {0x0174, 0x01}, /* x2-Binning */ {0x0175, 0x01}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x06}, {0x0625, 0x68}, {0x0626, 0x04}, {0x0627, 0xd0}, - {0x455e, 0x00}, - {0x471e, 0x4b}, - {0x4767, 0x0f}, - {0x4750, 0x14}, - {0x4540, 0x00}, - {0x47b4, 0x14}, - {0x4713, 0x30}, - {0x478b, 0x10}, - {0x478f, 0x10}, - {0x4793, 0x10}, - {0x4797, 0x0e}, - {0x479b, 0x0e}, - {0x0162, 0x0d}, - {0x0163, 0x78}, }; static const struct imx219_reg mode_640_480_regs[] = { - {0x0100, 0x00}, - {0x30eb, 0x05}, - {0x30eb, 0x0c}, - {0x300a, 0xff}, - {0x300b, 0xff}, - {0x30eb, 0x05}, - {0x30eb, 0x09}, - {0x0114, 0x01}, - {0x0128, 0x00}, - {0x012a, 0x18}, - {0x012b, 0x00}, - {0x0162, 0x0d}, - {0x0163, 0x78}, {0x0164, 0x03}, {0x0165, 0xe8}, {0x0166, 0x08}, @@ -347,35 +275,12 @@ static const struct imx219_reg mode_640_480_regs[] = { {0x016d, 0x80}, {0x016e, 0x01}, {0x016f, 0xe0}, - {0x0170, 0x01}, - {0x0171, 0x01}, - {0x0174, 0x03}, + {0x0174, 0x03}, /* x2-analog binning */ {0x0175, 0x03}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x06}, {0x0625, 0x68}, {0x0626, 0x04}, {0x0627, 0xd0}, - {0x455e, 0x00}, - {0x471e, 0x4b}, - {0x4767, 0x0f}, - {0x4750, 0x14}, - {0x4540, 0x00}, - {0x47b4, 0x14}, - {0x4713, 0x30}, - {0x478b, 0x10}, - {0x478f, 0x10}, - {0x4793, 0x10}, - {0x4797, 0x0e}, - {0x479b, 0x0e}, }; static const struct imx219_reg raw8_framefmt_regs[] = { @@ -1041,6 +946,13 @@ static int imx219_start_streaming(struct imx219 *imx219) if (ret < 0) return ret; + /* Send all registers that are common to all modes */ + ret = imx219_write_regs(imx219, imx219_common_regs, ARRAY_SIZE(imx219_common_regs)); + if (ret) { + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); + goto err_rpm_put; + } + /* Apply default values of current mode */ reg_list = &imx219->mode->reg_list; ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
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> --- V3: Change some of the comments to better reflect their description Add a few more entries to the imx219_common_regs table.