* [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane
@ 2022-04-12 13:55 Adam Ford
2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw)
To: linux-media
Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford,
laurent.pinchart, Adam Ford, Dave Stevenson,
Mauro Carvalho Chehab, linux-kernel
The driver currently only supports a 2-lane camera, a fixed external
clock (XCLK) at 24MHz, a fixed Pixel Rate of 182.4MHz, and a fixed
link rate of 456MHz. There are a bunch of hard-codec values in a
table of operating modes which expect the above to be true.
According to the datasheet, the driver is capable of operating in
either 4-lane with a pixel rate of 280.8MHz and Linux frequency
of 702MHz or 2-lane configured as stated above. The XCLK can be
anywhere from 6MHz - 27MHz instead of being fixed at 24MHz.
Split up the hard-coded values into smaller helper functions that
dynamically set the registers of the camera based on the XCLK and
desired number of lanes.
This series was tested on a Beacon RZ/G2M streaming video at 640x480
to an LCD with fbdevsink
media-ctl --links "'rcar_csi2 feaa0000.csi2':1->'VIN0 output':0[1]" -d /dev/media1
media-ctl --set-v4l2 "'imx219 2-0010':0[fmt:SRGGB8_1X8/640x480 field:none]" -d /dev/media1
yavta -w '0x009f0905 2048' /dev/v4l-subdev12
gst-launch-1.0 v4l2src device=/dev/video7 ! video/x-bayer,width=640,height=480,format=rggb ! queue ! bayer2rgb ! fbdevsink
Due to hardware limitations, the XCLK is still 24MHz, so anyone
willing to test this series with a different XCLK would be appreciated.
Due to the video format, streaming video at larger resolution was
not feasible, however individual frames captured at 1920x1080 were
successful.
Adam Ford (4):
media: i2c: imx219: Split common registers from mode tables
media: i2c: imx219: Support four-lane operation
media: i2c: imx219: Enable variable XCLK
media: i2c: imx219: Create DPHY helper function
drivers/media/i2c/imx219.c | 340 +++++++++++++++++++++++--------------
1 file changed, 213 insertions(+), 127 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables 2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford @ 2022-04-12 13:55 ` Adam Ford 2022-04-25 16:20 ` Dave Stevenson 2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford ` (3 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw) To: linux-media Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford, laurent.pinchart, Adam Ford, Dave Stevenson, Mauro Carvalho Chehab, linux-kernel There are four modes, and each mode has a table of registers. Some of the registers are common to all modes, so create new tables for these common registers to reduce duplicate code. Signed-off-by: Adam Ford <aford173@gmail.com> --- drivers/media/i2c/imx219.c | 103 ++++++++++++++----------------------- 1 file changed, 39 insertions(+), 64 deletions(-) diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index e10af3f74b38..b7cc36b16547 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -145,19 +145,36 @@ struct imx219_mode { struct imx219_reg_list reg_list; }; -/* - * Register sets lifted off the i2C interface from the Raspberry Pi firmware - * driver. - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. - */ -static const struct imx219_reg mode_3280x2464_regs[] = { - {0x0100, 0x00}, +/* To Access Addresses 3000-5fff, send the following commands */ +static const struct imx219_reg mfg_specific_reg[] = { + {0x0100, 0x00}, /* Mode Select */ {0x30eb, 0x0c}, {0x30eb, 0x05}, {0x300a, 0xff}, {0x300b, 0xff}, {0x30eb, 0x05}, {0x30eb, 0x09}, +}; + +static const struct imx219_reg pll_clk_table[] = { + + {0x0301, 0x05}, /* VTPXCK_DIV */ + {0x0303, 0x01}, /* VTSYSCK_DIV */ + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ + {0x0306, 0x00}, /* PLL_VT_MPY */ + {0x0307, 0x39}, + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ + {0x030c, 0x00}, /* PLL_OP_MPY */ + {0x030d, 0x72}, +}; + +/* + * Register sets lifted off the i2C interface from the Raspberry Pi firmware + * driver. + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. + */ +static const struct imx219_reg mode_3280x2464_regs[] = { {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, @@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { {0x0171, 0x01}, {0x0174, 0x00}, {0x0175, 0x00}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x0c}, {0x0625, 0xd0}, {0x0626, 0x09}, @@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { }; static const struct imx219_reg mode_1920_1080_regs[] = { - {0x0100, 0x00}, - {0x30eb, 0x05}, - {0x30eb, 0x0c}, - {0x300a, 0xff}, - {0x300b, 0xff}, - {0x30eb, 0x05}, - {0x30eb, 0x09}, {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, @@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { {0x0171, 0x01}, {0x0174, 0x00}, {0x0175, 0x00}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x07}, {0x0625, 0x80}, {0x0626, 0x04}, @@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { }; static const struct imx219_reg mode_1640_1232_regs[] = { - {0x0100, 0x00}, - {0x30eb, 0x0c}, - {0x30eb, 0x05}, - {0x300a, 0xff}, - {0x300b, 0xff}, - {0x30eb, 0x05}, - {0x30eb, 0x09}, {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, @@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { {0x0171, 0x01}, {0x0174, 0x01}, {0x0175, 0x01}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x06}, {0x0625, 0x68}, {0x0626, 0x04}, @@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { }; static const struct imx219_reg mode_640_480_regs[] = { - {0x0100, 0x00}, - {0x30eb, 0x05}, - {0x30eb, 0x0c}, - {0x300a, 0xff}, - {0x300b, 0xff}, - {0x30eb, 0x05}, - {0x30eb, 0x09}, {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, @@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = { {0x0171, 0x01}, {0x0174, 0x03}, {0x0175, 0x03}, - {0x0301, 0x05}, - {0x0303, 0x01}, - {0x0304, 0x03}, - {0x0305, 0x03}, - {0x0306, 0x00}, - {0x0307, 0x39}, - {0x030b, 0x01}, - {0x030c, 0x00}, - {0x030d, 0x72}, {0x0624, 0x06}, {0x0625, 0x68}, {0x0626, 0x04}, @@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219) if (ret < 0) return ret; + /* Send the Manufacturing Header common to all modes */ + ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg)); + if (ret) { + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); + goto err_rpm_put; + } + /* Apply default values of current mode */ reg_list = &imx219->mode->reg_list; ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); @@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219) goto err_rpm_put; } + /* Configure the PLL clocks */ + ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table)); + if (ret) { + dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__); + goto err_rpm_put; + } + + /* Apply customized values from user */ ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler); if (ret) -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables 2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford @ 2022-04-25 16:20 ` Dave Stevenson 2022-04-25 16:50 ` Adam Ford 0 siblings, 1 reply; 15+ messages in thread From: Dave Stevenson @ 2022-04-25 16:20 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML Hi Adam On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > There are four modes, and each mode has a table of registers. > Some of the registers are common to all modes, so create new > tables for these common registers to reduce duplicate code. > > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > drivers/media/i2c/imx219.c | 103 ++++++++++++++----------------------- > 1 file changed, 39 insertions(+), 64 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index e10af3f74b38..b7cc36b16547 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -145,19 +145,36 @@ struct imx219_mode { > struct imx219_reg_list reg_list; > }; > > -/* > - * Register sets lifted off the i2C interface from the Raspberry Pi firmware > - * driver. > - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > - */ > -static const struct imx219_reg mode_3280x2464_regs[] = { > - {0x0100, 0x00}, > +/* To Access Addresses 3000-5fff, send the following commands */ > +static const struct imx219_reg mfg_specific_reg[] = { > + {0x0100, 0x00}, /* Mode Select */ > {0x30eb, 0x0c}, > {0x30eb, 0x05}, > {0x300a, 0xff}, > {0x300b, 0xff}, > {0x30eb, 0x05}, > {0x30eb, 0x09}, > +}; > + > +static const struct imx219_reg pll_clk_table[] = { > + > + {0x0301, 0x05}, /* VTPXCK_DIV */ > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > + {0x0306, 0x00}, /* PLL_VT_MPY */ > + {0x0307, 0x39}, > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > + {0x030c, 0x00}, /* PLL_OP_MPY */ > + {0x030d, 0x72}, > +}; (I've come back to this patch last as my first reading was happy with it) Is there a good reason for making these two tables instead of one with comments as to what the registers are doing? As per my comment on patch 4, one table of registers setting these, the DPHY register, and registers {0x455e, 0x00}, {0x471e, 0x4b}, {0x4767, 0x0f}, {0x4750, 0x14}, {0x4540, 0x00}, {0x47b4, 0x14}, {0x4713, 0x30}, {0x478b, 0x10}, {0x478f, 0x10}, {0x4793, 0x10}, {0x4797, 0x0e}, {0x479b, 0x0e}, {0x0162, 0x0d}, {0x0163, 0x78}, would remove the duplication, reduce the code size, and be slightly more readable. Dave > +/* > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > + * driver. > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > + */ > +static const struct imx219_reg mode_3280x2464_regs[] = { > {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > @@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > {0x0171, 0x01}, > {0x0174, 0x00}, > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x0c}, > {0x0625, 0xd0}, > {0x0626, 0x09}, > @@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > }; > > static const struct imx219_reg mode_1920_1080_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > @@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > {0x0171, 0x01}, > {0x0174, 0x00}, > {0x0175, 0x00}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x07}, > {0x0625, 0x80}, > {0x0626, 0x04}, > @@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > }; > > static const struct imx219_reg mode_1640_1232_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x0c}, > - {0x30eb, 0x05}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > @@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > {0x0171, 0x01}, > {0x0174, 0x01}, > {0x0175, 0x01}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > @@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > }; > > static const struct imx219_reg mode_640_480_regs[] = { > - {0x0100, 0x00}, > - {0x30eb, 0x05}, > - {0x30eb, 0x0c}, > - {0x300a, 0xff}, > - {0x300b, 0xff}, > - {0x30eb, 0x05}, > - {0x30eb, 0x09}, > {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > @@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = { > {0x0171, 0x01}, > {0x0174, 0x03}, > {0x0175, 0x03}, > - {0x0301, 0x05}, > - {0x0303, 0x01}, > - {0x0304, 0x03}, > - {0x0305, 0x03}, > - {0x0306, 0x00}, > - {0x0307, 0x39}, > - {0x030b, 0x01}, > - {0x030c, 0x00}, > - {0x030d, 0x72}, > {0x0624, 0x06}, > {0x0625, 0x68}, > {0x0626, 0x04}, > @@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > if (ret < 0) > return ret; > > + /* Send the Manufacturing Header common to all modes */ > + ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg)); > + if (ret) { > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > + goto err_rpm_put; > + } > + > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > @@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219) > goto err_rpm_put; > } > > + /* Configure the PLL clocks */ > + ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table)); > + if (ret) { > + dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__); > + goto err_rpm_put; > + } > + > + > /* Apply customized values from user */ > ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler); > if (ret) > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables 2022-04-25 16:20 ` Dave Stevenson @ 2022-04-25 16:50 ` Adam Ford 2022-04-25 17:17 ` Dave Stevenson 0 siblings, 1 reply; 15+ messages in thread From: Adam Ford @ 2022-04-25 16:50 UTC (permalink / raw) To: Dave Stevenson Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML On Mon, Apr 25, 2022 at 11:20 AM Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Adam > > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > > > There are four modes, and each mode has a table of registers. > > Some of the registers are common to all modes, so create new > > tables for these common registers to reduce duplicate code. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > drivers/media/i2c/imx219.c | 103 ++++++++++++++----------------------- > > 1 file changed, 39 insertions(+), 64 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index e10af3f74b38..b7cc36b16547 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -145,19 +145,36 @@ struct imx219_mode { > > struct imx219_reg_list reg_list; > > }; > > > > -/* > > - * Register sets lifted off the i2C interface from the Raspberry Pi firmware > > - * driver. > > - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > > - */ > > -static const struct imx219_reg mode_3280x2464_regs[] = { > > - {0x0100, 0x00}, > > +/* To Access Addresses 3000-5fff, send the following commands */ > > +static const struct imx219_reg mfg_specific_reg[] = { > > + {0x0100, 0x00}, /* Mode Select */ > > {0x30eb, 0x0c}, > > {0x30eb, 0x05}, > > {0x300a, 0xff}, > > {0x300b, 0xff}, > > {0x30eb, 0x05}, > > {0x30eb, 0x09}, > > +}; > > + > > +static const struct imx219_reg pll_clk_table[] = { > > + > > + {0x0301, 0x05}, /* VTPXCK_DIV */ > > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > + {0x0306, 0x00}, /* PLL_VT_MPY */ > > + {0x0307, 0x39}, > > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > > + {0x030c, 0x00}, /* PLL_OP_MPY */ > > + {0x030d, 0x72}, > > +}; > > (I've come back to this patch last as my first reading was happy with it) > Is there a good reason for making these two tables instead of one with > comments as to what the registers are doing? The pll_clk tables were written after the resolution settings before I split them. I was concerned about having all the common tables in one place, because registers would be set in a different order than they were originally. I wasn't sure if the pll clock tables needed to be set after the resolution or not. It seemed possible to me that it wouldn't necessarily know how to set the clocks without knowing the desired resolution. I can certainly merge them together and run some tests to see if there are regressions. If there are none, I can keep them in a common block. I can certainly add comments to indicate what's being done. I had thought about it. > > As per my comment on patch 4, one table of registers setting these, > the DPHY register, and registers > {0x455e, 0x00}, > {0x471e, 0x4b}, > {0x4767, 0x0f}, > {0x4750, 0x14}, > {0x4540, 0x00}, > {0x47b4, 0x14}, > {0x4713, 0x30}, > {0x478b, 0x10}, > {0x478f, 0x10}, > {0x4793, 0x10}, > {0x4797, 0x0e}, > {0x479b, 0x0e}, > {0x0162, 0x0d}, > {0x0163, 0x78}, > would remove the duplication, reduce the code size, and be slightly > more readable. > > Dave > > > +/* > > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > > + * driver. > > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > > + */ > > +static const struct imx219_reg mode_3280x2464_regs[] = { > > {0x0114, 0x01}, > > {0x0128, 0x00}, > > {0x012a, 0x18}, > > @@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > {0x0171, 0x01}, > > {0x0174, 0x00}, > > {0x0175, 0x00}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x0c}, > > {0x0625, 0xd0}, > > {0x0626, 0x09}, > > @@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > }; > > > > static const struct imx219_reg mode_1920_1080_regs[] = { > > - {0x0100, 0x00}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x0c}, > > - {0x300a, 0xff}, > > - {0x300b, 0xff}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x09}, > > {0x0114, 0x01}, > > {0x0128, 0x00}, > > {0x012a, 0x18}, > > @@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > {0x0171, 0x01}, > > {0x0174, 0x00}, > > {0x0175, 0x00}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x07}, > > {0x0625, 0x80}, > > {0x0626, 0x04}, > > @@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > }; > > > > static const struct imx219_reg mode_1640_1232_regs[] = { > > - {0x0100, 0x00}, > > - {0x30eb, 0x0c}, > > - {0x30eb, 0x05}, > > - {0x300a, 0xff}, > > - {0x300b, 0xff}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x09}, > > {0x0114, 0x01}, > > {0x0128, 0x00}, > > {0x012a, 0x18}, > > @@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > {0x0171, 0x01}, > > {0x0174, 0x01}, > > {0x0175, 0x01}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x06}, > > {0x0625, 0x68}, > > {0x0626, 0x04}, > > @@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > }; > > > > static const struct imx219_reg mode_640_480_regs[] = { > > - {0x0100, 0x00}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x0c}, > > - {0x300a, 0xff}, > > - {0x300b, 0xff}, > > - {0x30eb, 0x05}, > > - {0x30eb, 0x09}, > > {0x0114, 0x01}, > > {0x0128, 0x00}, > > {0x012a, 0x18}, > > @@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = { > > {0x0171, 0x01}, > > {0x0174, 0x03}, > > {0x0175, 0x03}, > > - {0x0301, 0x05}, > > - {0x0303, 0x01}, > > - {0x0304, 0x03}, > > - {0x0305, 0x03}, > > - {0x0306, 0x00}, > > - {0x0307, 0x39}, > > - {0x030b, 0x01}, > > - {0x030c, 0x00}, > > - {0x030d, 0x72}, > > {0x0624, 0x06}, > > {0x0625, 0x68}, > > {0x0626, 0x04}, > > @@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > > if (ret < 0) > > return ret; > > > > + /* Send the Manufacturing Header common to all modes */ > > + ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg)); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > > + goto err_rpm_put; > > + } > > + > > /* Apply default values of current mode */ > > reg_list = &imx219->mode->reg_list; > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > @@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219) > > goto err_rpm_put; > > } > > > > + /* Configure the PLL clocks */ > > + ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table)); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__); > > + goto err_rpm_put; > > + } > > + > > + > > /* Apply customized values from user */ > > ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler); > > if (ret) > > -- > > 2.34.1 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables 2022-04-25 16:50 ` Adam Ford @ 2022-04-25 17:17 ` Dave Stevenson 0 siblings, 0 replies; 15+ messages in thread From: Dave Stevenson @ 2022-04-25 17:17 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML Hi Adam On Mon, 25 Apr 2022 at 17:51, Adam Ford <aford173@gmail.com> wrote: > > On Mon, Apr 25, 2022 at 11:20 AM Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Adam > > > > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > > > > > There are four modes, and each mode has a table of registers. > > > Some of the registers are common to all modes, so create new > > > tables for these common registers to reduce duplicate code. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > --- > > > drivers/media/i2c/imx219.c | 103 ++++++++++++++----------------------- > > > 1 file changed, 39 insertions(+), 64 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > > index e10af3f74b38..b7cc36b16547 100644 > > > --- a/drivers/media/i2c/imx219.c > > > +++ b/drivers/media/i2c/imx219.c > > > @@ -145,19 +145,36 @@ struct imx219_mode { > > > struct imx219_reg_list reg_list; > > > }; > > > > > > -/* > > > - * Register sets lifted off the i2C interface from the Raspberry Pi firmware > > > - * driver. > > > - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > > > - */ > > > -static const struct imx219_reg mode_3280x2464_regs[] = { > > > - {0x0100, 0x00}, > > > +/* To Access Addresses 3000-5fff, send the following commands */ > > > +static const struct imx219_reg mfg_specific_reg[] = { > > > + {0x0100, 0x00}, /* Mode Select */ > > > {0x30eb, 0x0c}, > > > {0x30eb, 0x05}, > > > {0x300a, 0xff}, > > > {0x300b, 0xff}, > > > {0x30eb, 0x05}, > > > {0x30eb, 0x09}, > > > +}; > > > + > > > +static const struct imx219_reg pll_clk_table[] = { > > > + > > > + {0x0301, 0x05}, /* VTPXCK_DIV */ > > > + {0x0303, 0x01}, /* VTSYSCK_DIV */ > > > + {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > > + {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > > + {0x0306, 0x00}, /* PLL_VT_MPY */ > > > + {0x0307, 0x39}, > > > + {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > > > + {0x030c, 0x00}, /* PLL_OP_MPY */ > > > + {0x030d, 0x72}, > > > +}; > > > > (I've come back to this patch last as my first reading was happy with it) > > Is there a good reason for making these two tables instead of one with > > comments as to what the registers are doing? > > The pll_clk tables were written after the resolution settings before I > split them. I was concerned about having all the common tables in one > place, because registers would be set in a different order than they > were originally. I wasn't sure if the pll clock tables needed to be > set after the resolution or not. It seemed possible to me that it > wouldn't necessarily know how to set the clocks without knowing the > desired resolution. I can certainly merge them together and run some > tests to see if there are regressions. If there are none, I can keep > them in a common block. > > I can certainly add comments to indicate what's being done. I had > thought about it. More often PLL settings are set before the resolution configuration. The initialisation sequence for imx219 was provided by Sony, but I note that the datasheet that Table 37 "Initialization sequence with XCLR" lists: - (1) to (3): Refer power up sequence timing diagram - (4) Set PLL parameters. Basic setting. Set Readout mode. Set MIPI parameters - (5) Start streaming with 0x0100 (mode_select = 1) So I really wouldn't be concerned about setting the PLL registers before the resolution. The PLL configuration is mode independent as the pixel rate and link frequency are effectively fixed. Dave > > > > As per my comment on patch 4, one table of registers setting these, > > the DPHY register, and registers > > {0x455e, 0x00}, > > {0x471e, 0x4b}, > > {0x4767, 0x0f}, > > {0x4750, 0x14}, > > {0x4540, 0x00}, > > {0x47b4, 0x14}, > > {0x4713, 0x30}, > > {0x478b, 0x10}, > > {0x478f, 0x10}, > > {0x4793, 0x10}, > > {0x4797, 0x0e}, > > {0x479b, 0x0e}, > > {0x0162, 0x0d}, > > {0x0163, 0x78}, > > would remove the duplication, reduce the code size, and be slightly > > more readable. > > > > Dave > > > > > +/* > > > + * Register sets lifted off the i2C interface from the Raspberry Pi firmware > > > + * driver. > > > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > > > + */ > > > +static const struct imx219_reg mode_3280x2464_regs[] = { > > > {0x0114, 0x01}, > > > {0x0128, 0x00}, > > > {0x012a, 0x18}, > > > @@ -178,15 +195,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > > {0x0171, 0x01}, > > > {0x0174, 0x00}, > > > {0x0175, 0x00}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x0c}, > > > {0x0625, 0xd0}, > > > {0x0626, 0x09}, > > > @@ -208,13 +216,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > > }; > > > > > > static const struct imx219_reg mode_1920_1080_regs[] = { > > > - {0x0100, 0x00}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x0c}, > > > - {0x300a, 0xff}, > > > - {0x300b, 0xff}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x09}, > > > {0x0114, 0x01}, > > > {0x0128, 0x00}, > > > {0x012a, 0x18}, > > > @@ -237,15 +238,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > > {0x0171, 0x01}, > > > {0x0174, 0x00}, > > > {0x0175, 0x00}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x07}, > > > {0x0625, 0x80}, > > > {0x0626, 0x04}, > > > @@ -265,13 +257,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > > }; > > > > > > static const struct imx219_reg mode_1640_1232_regs[] = { > > > - {0x0100, 0x00}, > > > - {0x30eb, 0x0c}, > > > - {0x30eb, 0x05}, > > > - {0x300a, 0xff}, > > > - {0x300b, 0xff}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x09}, > > > {0x0114, 0x01}, > > > {0x0128, 0x00}, > > > {0x012a, 0x18}, > > > @@ -292,15 +277,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > > {0x0171, 0x01}, > > > {0x0174, 0x01}, > > > {0x0175, 0x01}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x06}, > > > {0x0625, 0x68}, > > > {0x0626, 0x04}, > > > @@ -322,13 +298,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > > }; > > > > > > static const struct imx219_reg mode_640_480_regs[] = { > > > - {0x0100, 0x00}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x0c}, > > > - {0x300a, 0xff}, > > > - {0x300b, 0xff}, > > > - {0x30eb, 0x05}, > > > - {0x30eb, 0x09}, > > > {0x0114, 0x01}, > > > {0x0128, 0x00}, > > > {0x012a, 0x18}, > > > @@ -351,15 +320,6 @@ static const struct imx219_reg mode_640_480_regs[] = { > > > {0x0171, 0x01}, > > > {0x0174, 0x03}, > > > {0x0175, 0x03}, > > > - {0x0301, 0x05}, > > > - {0x0303, 0x01}, > > > - {0x0304, 0x03}, > > > - {0x0305, 0x03}, > > > - {0x0306, 0x00}, > > > - {0x0307, 0x39}, > > > - {0x030b, 0x01}, > > > - {0x030c, 0x00}, > > > - {0x030d, 0x72}, > > > {0x0624, 0x06}, > > > {0x0625, 0x68}, > > > {0x0626, 0x04}, > > > @@ -1041,6 +1001,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > > > if (ret < 0) > > > return ret; > > > > > > + /* Send the Manufacturing Header common to all modes */ > > > + ret = imx219_write_regs(imx219, mfg_specific_reg, ARRAY_SIZE(mfg_specific_reg)); > > > + if (ret) { > > > + dev_err(&client->dev, "%s failed to send mfg header\n", __func__); > > > + goto err_rpm_put; > > > + } > > > + > > > /* Apply default values of current mode */ > > > reg_list = &imx219->mode->reg_list; > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > > @@ -1056,6 +1023,14 @@ static int imx219_start_streaming(struct imx219 *imx219) > > > goto err_rpm_put; > > > } > > > > > > + /* Configure the PLL clocks */ > > > + ret = imx219_write_regs(imx219, pll_clk_table, ARRAY_SIZE(pll_clk_table)); > > > + if (ret) { > > > + dev_err(&client->dev, "%s failed to sent PLL clocks\n", __func__); > > > + goto err_rpm_put; > > > + } > > > + > > > + > > > /* Apply customized values from user */ > > > ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler); > > > if (ret) > > > -- > > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] media: i2c: imx219: Support four-lane operation 2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford 2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford @ 2022-04-12 13:55 ` Adam Ford 2022-04-25 15:39 ` Dave Stevenson 2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford ` (2 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw) To: linux-media Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford, laurent.pinchart, Adam Ford, Dave Stevenson, Mauro Carvalho Chehab, linux-kernel The imx219 camera is capable of either two-lane or four-lane operation. When operating in four-lane, both the pixel rate and link frequency change. Regardless of the mode, however, both frequencies remain fixed. Helper functions are needed to read and set pixel and link frequencies which also reduces the number of fixed registers in the table of modes. Since the link frequency and number of lanes is extracted from the endpoint, move the endpoint handling into the probe function and out of the imx219_check_hwcfg. This simplifies the imx219_check_hwcfg just a bit, and places all the parameters into the imx219 structure. It then allows imx219_check_hwcfg to simply validate the settings. Signed-off-by: Adam Ford <aford173@gmail.com> --- drivers/media/i2c/imx219.c | 144 ++++++++++++++++++++++++------------- 1 file changed, 96 insertions(+), 48 deletions(-) diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index b7cc36b16547..d13ce5c1ece6 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -42,10 +42,16 @@ /* External clock frequency is 24.0M */ #define IMX219_XCLK_FREQ 24000000 -/* Pixel rate is fixed at 182.4M for all the modes */ +/* Pixel rate is fixed for all the modes */ #define IMX219_PIXEL_RATE 182400000 +#define IMX219_PIXEL_RATE_4LANE 280800000 #define IMX219_DEFAULT_LINK_FREQ 456000000 +#define IMX219_DEFAULT_LINK_FREQ_4LANE 702000000 + +#define IMX219_REG_CSI_LANE_MODE 0x0114 +#define IMX219_CSI_2_LANE_MODE 0x01 +#define IMX219_CSI_4_LANE_MODE 0x03 /* V_TIMING internal */ #define IMX219_REG_VTS 0x0160 @@ -175,7 +181,6 @@ static const struct imx219_reg pll_clk_table[] = { * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. */ static const struct imx219_reg mode_3280x2464_regs[] = { - {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, {0x012b, 0x00}, @@ -216,7 +221,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { }; static const struct imx219_reg mode_1920_1080_regs[] = { - {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, {0x012b, 0x00}, @@ -257,7 +261,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { }; static const struct imx219_reg mode_1640_1232_regs[] = { - {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, {0x012b, 0x00}, @@ -298,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { }; static const struct imx219_reg mode_640_480_regs[] = { - {0x0114, 0x01}, {0x0128, 0x00}, {0x012a, 0x18}, {0x012b, 0x00}, @@ -352,6 +354,7 @@ static const struct imx219_reg raw10_framefmt_regs[] = { static const s64 imx219_link_freq_menu[] = { IMX219_DEFAULT_LINK_FREQ, + IMX219_DEFAULT_LINK_FREQ_4LANE, }; static const char * const imx219_test_pattern_menu[] = { @@ -529,6 +532,13 @@ struct imx219 { /* Streaming on/off */ bool streaming; + + /* Two or Four lanes */ + u8 lanes; + + /* Link Frequency info */ + unsigned int nr_of_link_frequencies; + u64 link_frequency; }; static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) @@ -991,6 +1001,20 @@ static int imx219_get_selection(struct v4l2_subdev *sd, return -EINVAL; } +static int imx219_configure_lanes(struct imx219 *imx219) +{ + int ret; + + /* imx219->lanes has already been validated to be either 2 or 4 */ + if (imx219->lanes == 2) + ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, + IMX219_REG_VALUE_08BIT, IMX219_CSI_2_LANE_MODE); + else + ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, + IMX219_REG_VALUE_08BIT, IMX219_CSI_4_LANE_MODE); + return ret; +}; + static int imx219_start_streaming(struct imx219 *imx219) { struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); @@ -1008,6 +1032,13 @@ static int imx219_start_streaming(struct imx219 *imx219) goto err_rpm_put; } + /* Configure two or four Lane mode */ + ret = imx219_configure_lanes(imx219); + if (ret) { + dev_err(&client->dev, "%s failed to configure lanes\n", __func__); + goto err_rpm_put; + } + /* Apply default values of current mode */ reg_list = &imx219->mode->reg_list; ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); @@ -1247,6 +1278,14 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { .open = imx219_open, }; +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) +{ + if (imx219->lanes == 2) + return IMX219_PIXEL_RATE; + else + return IMX219_PIXEL_RATE_4LANE; +} + /* Initialize control handlers */ static int imx219_init_controls(struct imx219 *imx219) { @@ -1268,14 +1307,15 @@ static int imx219_init_controls(struct imx219 *imx219) /* By default, PIXEL_RATE is read only */ imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_PIXEL_RATE, - IMX219_PIXEL_RATE, - IMX219_PIXEL_RATE, 1, - IMX219_PIXEL_RATE); + imx219_get_pixel_rate(imx219), + imx219_get_pixel_rate(imx219), 1, + imx219_get_pixel_rate(imx219)); imx219->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_LINK_FREQ, - ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, + ARRAY_SIZE(imx219_link_freq_menu) - 1, + (imx219->lanes == 4), imx219_link_freq_menu); if (imx219->link_freq) imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; @@ -1371,67 +1411,75 @@ static void imx219_free_controls(struct imx219 *imx219) mutex_destroy(&imx219->mutex); } -static int imx219_check_hwcfg(struct device *dev) +static int imx219_check_hwcfg(struct imx219 *imx219) { - struct fwnode_handle *endpoint; + struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); + + /* Check the number of MIPI CSI2 data lanes */ + if (imx219->lanes != 2 && imx219->lanes != 4) { + dev_err(&client->dev, "only 2 or 4 data lanes are currently supported\n"); + return -EINVAL; + } + + if (imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ && + imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ_4LANE) { + dev_err(&client->dev, "Link frequency not supported: %lld\n", + imx219->link_frequency); + return -EINVAL; + } + + return 0; +} + +static int imx219_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct imx219 *imx219; struct v4l2_fwnode_endpoint ep_cfg = { .bus_type = V4L2_MBUS_CSI2_DPHY }; - int ret = -EINVAL; + struct fwnode_handle *endpoint; + int ret = 0; + + imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL); + if (!imx219) + return -ENOMEM; + v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops); + + /* Fetch the endpoint to extract lanes and link-frequency */ endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); if (!endpoint) { dev_err(dev, "endpoint node not found\n"); - return -EINVAL; + ret = -EINVAL; + goto fwnode_cleanup; } if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) { dev_err(dev, "could not parse endpoint\n"); - goto error_out; + ret = -EINVAL; + goto fwnode_cleanup; } - /* Check the number of MIPI CSI2 data lanes */ - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) { - dev_err(dev, "only 2 data lanes are currently supported\n"); - goto error_out; - } + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes; + imx219->nr_of_link_frequencies = ep_cfg.nr_of_link_frequencies; - /* Check the link frequency set in device tree */ - if (!ep_cfg.nr_of_link_frequencies) { + if (imx219->nr_of_link_frequencies != 1) { dev_err(dev, "link-frequency property not found in DT\n"); - goto error_out; - } - - if (ep_cfg.nr_of_link_frequencies != 1 || - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) { - dev_err(dev, "Link frequency not supported: %lld\n", - ep_cfg.link_frequencies[0]); - goto error_out; + ret = -EINVAL; + goto fwnode_cleanup; } + imx219->link_frequency = ep_cfg.link_frequencies[0]; - ret = 0; - -error_out: + /* Cleanup endpoint and handle any errors from above */ +fwnode_cleanup: v4l2_fwnode_endpoint_free(&ep_cfg); fwnode_handle_put(endpoint); - - return ret; -} - -static int imx219_probe(struct i2c_client *client) -{ - struct device *dev = &client->dev; - struct imx219 *imx219; - int ret; - - imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL); - if (!imx219) - return -ENOMEM; - - v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops); + if (ret) + return ret; /* Check the hardware configuration in device tree */ - if (imx219_check_hwcfg(dev)) + if (imx219_check_hwcfg(imx219)) return -EINVAL; /* Get system clock (xclk) */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] media: i2c: imx219: Support four-lane operation 2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford @ 2022-04-25 15:39 ` Dave Stevenson 0 siblings, 0 replies; 15+ messages in thread From: Dave Stevenson @ 2022-04-25 15:39 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML Hi Adam Sorry for the delay in reviewing this. On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > The imx219 camera is capable of either two-lane or four-lane > operation. When operating in four-lane, both the pixel rate and > link frequency change. Regardless of the mode, however, both > frequencies remain fixed. > > Helper functions are needed to read and set pixel and link frequencies > which also reduces the number of fixed registers in the table of modes. > > Since the link frequency and number of lanes is extracted from the > endpoint, move the endpoint handling into the probe function and > out of the imx219_check_hwcfg. This simplifies the imx219_check_hwcfg > just a bit, and places all the parameters into the imx219 structure. > It then allows imx219_check_hwcfg to simply validate the settings. > > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > drivers/media/i2c/imx219.c | 144 ++++++++++++++++++++++++------------- > 1 file changed, 96 insertions(+), 48 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index b7cc36b16547..d13ce5c1ece6 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -42,10 +42,16 @@ > /* External clock frequency is 24.0M */ > #define IMX219_XCLK_FREQ 24000000 > > -/* Pixel rate is fixed at 182.4M for all the modes */ > +/* Pixel rate is fixed for all the modes */ > #define IMX219_PIXEL_RATE 182400000 > +#define IMX219_PIXEL_RATE_4LANE 280800000 > > #define IMX219_DEFAULT_LINK_FREQ 456000000 > +#define IMX219_DEFAULT_LINK_FREQ_4LANE 702000000 My datasheet table 9-2 lists pll1_vt_freq as being 702MHz, and Speed/Lane (Ch) is 726Mbps, so a link frequency of 363MHz. Total output rate is 2.904Gbps, which is the 726Mbps * 4 lanes. > + > +#define IMX219_REG_CSI_LANE_MODE 0x0114 > +#define IMX219_CSI_2_LANE_MODE 0x01 > +#define IMX219_CSI_4_LANE_MODE 0x03 > > /* V_TIMING internal */ > #define IMX219_REG_VTS 0x0160 > @@ -175,7 +181,6 @@ static const struct imx219_reg pll_clk_table[] = { > * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > */ > static const struct imx219_reg mode_3280x2464_regs[] = { > - {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > {0x012b, 0x00}, > @@ -216,7 +221,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > }; > > static const struct imx219_reg mode_1920_1080_regs[] = { > - {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > {0x012b, 0x00}, > @@ -257,7 +261,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > }; > > static const struct imx219_reg mode_1640_1232_regs[] = { > - {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > {0x012b, 0x00}, > @@ -298,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > }; > > static const struct imx219_reg mode_640_480_regs[] = { > - {0x0114, 0x01}, > {0x0128, 0x00}, > {0x012a, 0x18}, > {0x012b, 0x00}, > @@ -352,6 +354,7 @@ static const struct imx219_reg raw10_framefmt_regs[] = { > > static const s64 imx219_link_freq_menu[] = { > IMX219_DEFAULT_LINK_FREQ, > + IMX219_DEFAULT_LINK_FREQ_4LANE, This one always feels weird to me. There is only one link frequency supported at a time defined by the number of lanes. OK it's a read only control, but having 2 link_freq_menu arrays of 1 element each, and choosing the appropriate one seems more logical to me. > }; > > static const char * const imx219_test_pattern_menu[] = { > @@ -529,6 +532,13 @@ struct imx219 { > > /* Streaming on/off */ > bool streaming; > + > + /* Two or Four lanes */ > + u8 lanes; > + > + /* Link Frequency info */ > + unsigned int nr_of_link_frequencies; > + u64 link_frequency; > }; > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > @@ -991,6 +1001,20 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > return -EINVAL; > } > > +static int imx219_configure_lanes(struct imx219 *imx219) > +{ > + int ret; > + > + /* imx219->lanes has already been validated to be either 2 or 4 */ > + if (imx219->lanes == 2) > + ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, > + IMX219_REG_VALUE_08BIT, IMX219_CSI_2_LANE_MODE); > + else > + ret = imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, > + IMX219_REG_VALUE_08BIT, IMX219_CSI_4_LANE_MODE); > + return ret; Could be condensed to the one liner (with appropriate wrapping): return imx219_write_reg(imx219, IMX219_REG_CSI_LANE_MODE, IMX219_REG_VALUE_08BIT, (imx219->lanes == 2) ? IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE); > +}; > + > static int imx219_start_streaming(struct imx219 *imx219) > { > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > @@ -1008,6 +1032,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > goto err_rpm_put; > } > > + /* Configure two or four Lane mode */ > + ret = imx219_configure_lanes(imx219); > + if (ret) { > + dev_err(&client->dev, "%s failed to configure lanes\n", __func__); > + goto err_rpm_put; > + } > + > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > @@ -1247,6 +1278,14 @@ static const struct v4l2_subdev_internal_ops imx219_internal_ops = { > .open = imx219_open, > }; > > +static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > +{ > + if (imx219->lanes == 2) > + return IMX219_PIXEL_RATE; > + else > + return IMX219_PIXEL_RATE_4LANE; Again: return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; ? > +} > + > /* Initialize control handlers */ > static int imx219_init_controls(struct imx219 *imx219) > { > @@ -1268,14 +1307,15 @@ static int imx219_init_controls(struct imx219 *imx219) > /* By default, PIXEL_RATE is read only */ > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_PIXEL_RATE, > - IMX219_PIXEL_RATE, > - IMX219_PIXEL_RATE, 1, > - IMX219_PIXEL_RATE); > + imx219_get_pixel_rate(imx219), > + imx219_get_pixel_rate(imx219), 1, > + imx219_get_pixel_rate(imx219)); > > imx219->link_freq = > v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_LINK_FREQ, > - ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, > + ARRAY_SIZE(imx219_link_freq_menu) - 1, > + (imx219->lanes == 4), > imx219_link_freq_menu); > if (imx219->link_freq) > imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > @@ -1371,67 +1411,75 @@ static void imx219_free_controls(struct imx219 *imx219) > mutex_destroy(&imx219->mutex); > } > > -static int imx219_check_hwcfg(struct device *dev) > +static int imx219_check_hwcfg(struct imx219 *imx219) > { > - struct fwnode_handle *endpoint; > + struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > + > + /* Check the number of MIPI CSI2 data lanes */ > + if (imx219->lanes != 2 && imx219->lanes != 4) { > + dev_err(&client->dev, "only 2 or 4 data lanes are currently supported\n"); > + return -EINVAL; > + } > + > + if (imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ && > + imx219->link_frequency != IMX219_DEFAULT_LINK_FREQ_4LANE) { This permits choosing 4 lane operation with a link frequency of IMX219_DEFAULT_LINK_FREQ, or 2 lane operation with a link frequency of IMX219_DEFAULT_LINK_FREQ_4LANE. > + dev_err(&client->dev, "Link frequency not supported: %lld\n", > + imx219->link_frequency); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int imx219_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct imx219 *imx219; > struct v4l2_fwnode_endpoint ep_cfg = { > .bus_type = V4L2_MBUS_CSI2_DPHY > }; > - int ret = -EINVAL; > + struct fwnode_handle *endpoint; > + int ret = 0; > + > + imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL); > + if (!imx219) > + return -ENOMEM; > > + v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops); > + > + /* Fetch the endpoint to extract lanes and link-frequency */ > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > if (!endpoint) { > dev_err(dev, "endpoint node not found\n"); > - return -EINVAL; > + ret = -EINVAL; > + goto fwnode_cleanup; > } > > if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) { > dev_err(dev, "could not parse endpoint\n"); > - goto error_out; > + ret = -EINVAL; > + goto fwnode_cleanup; > } > > - /* Check the number of MIPI CSI2 data lanes */ > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) { > - dev_err(dev, "only 2 data lanes are currently supported\n"); > - goto error_out; > - } > + imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes; > + imx219->nr_of_link_frequencies = ep_cfg.nr_of_link_frequencies; > > - /* Check the link frequency set in device tree */ > - if (!ep_cfg.nr_of_link_frequencies) { > + if (imx219->nr_of_link_frequencies != 1) { > dev_err(dev, "link-frequency property not found in DT\n"); > - goto error_out; > - } > - > - if (ep_cfg.nr_of_link_frequencies != 1 || > - ep_cfg.link_frequencies[0] != IMX219_DEFAULT_LINK_FREQ) { > - dev_err(dev, "Link frequency not supported: %lld\n", > - ep_cfg.link_frequencies[0]); > - goto error_out; > + ret = -EINVAL; > + goto fwnode_cleanup; > } > + imx219->link_frequency = ep_cfg.link_frequencies[0]; imx219->link_frequency is only used within imx219_check_hwcfg, which is called from imx219_probe. Pass it as a parameter instead of storing it in the state structure? Cheers. Dave > > - ret = 0; > - > -error_out: > + /* Cleanup endpoint and handle any errors from above */ > +fwnode_cleanup: > v4l2_fwnode_endpoint_free(&ep_cfg); > fwnode_handle_put(endpoint); > - > - return ret; > -} > - > -static int imx219_probe(struct i2c_client *client) > -{ > - struct device *dev = &client->dev; > - struct imx219 *imx219; > - int ret; > - > - imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL); > - if (!imx219) > - return -ENOMEM; > - > - v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops); > + if (ret) > + return ret; > > /* Check the hardware configuration in device tree */ > - if (imx219_check_hwcfg(dev)) > + if (imx219_check_hwcfg(imx219)) > return -EINVAL; > > /* Get system clock (xclk) */ > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] media: i2c: imx219: Enable variable XCLK 2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford 2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford 2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford @ 2022-04-12 13:55 ` Adam Ford 2022-04-25 15:58 ` Dave Stevenson 2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford 2022-04-25 12:08 ` [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford 4 siblings, 1 reply; 15+ messages in thread From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw) To: linux-media Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford, laurent.pinchart, Adam Ford, Dave Stevenson, Mauro Carvalho Chehab, linux-kernel The datasheet shows the external clock can be anything from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and PREPLLCK_OP_DIV need to change based on the clock, so create helper functions to set these registers based on the rate of xclk. Move the validation of the clock rate into imx219_check_hwcfg which means delaying the call to it until after xclk has been determined. Signed-off-by: Adam Ford <aford173@gmail.com> --- drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index d13ce5c1ece6..08e7d0e72430 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -39,8 +39,12 @@ #define IMX219_REG_CHIP_ID 0x0000 #define IMX219_CHIP_ID 0x0219 -/* External clock frequency is 24.0M */ -#define IMX219_XCLK_FREQ 24000000 +/* Default external clock frequency is 24.0M */ +#define IMX219_XCLK_MIN_FREQ 6000000 +#define IMX219_XCLK_MAX_FREQ 27000000 +#define IMX219_REG_EXCK 0x012a +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304 +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305 /* Pixel rate is fixed for all the modes */ #define IMX219_PIXEL_RATE 182400000 @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_table[] = { {0x0301, 0x05}, /* VTPXCK_DIV */ {0x0303, 0x01}, /* VTSYSCK_DIV */ - {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ - {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ {0x0306, 0x00}, /* PLL_VT_MPY */ {0x0307, 0x39}, {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = { */ static const struct imx219_reg mode_3280x2464_regs[] = { {0x0128, 0x00}, - {0x012a, 0x18}, {0x012b, 0x00}, {0x0164, 0x00}, {0x0165, 0x00}, @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { static const struct imx219_reg mode_1920_1080_regs[] = { {0x0128, 0x00}, - {0x012a, 0x18}, {0x012b, 0x00}, {0x0162, 0x0d}, {0x0163, 0x78}, @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { static const struct imx219_reg mode_1640_1232_regs[] = { {0x0128, 0x00}, - {0x012a, 0x18}, {0x012b, 0x00}, {0x0164, 0x00}, {0x0165, 0x00}, @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { static const struct imx219_reg mode_640_480_regs[] = { {0x0128, 0x00}, - {0x012a, 0x18}, {0x012b, 0x00}, {0x0162, 0x0d}, {0x0163, 0x78}, @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219) return ret; }; +static int imx219_set_exck_freq(struct imx219 *imx219) +{ + int ret; + /* The imx219 registers need MHz not Hz */ + u8 clk = (u8) (imx219->xclk_freq/1000000U); + + /* Set the clock frequency in MHz */ + ret = imx219_write_reg(imx219, IMX219_REG_EXCK, + IMX219_REG_VALUE_08BIT, clk); + + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */ + switch (clk) { + case 6 ... 11: + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, + IMX219_REG_VALUE_08BIT, 0x01); + if (ret) + return ret; + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, + IMX219_REG_VALUE_08BIT, 0x01); + return ret; + case 12 ... 23: + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, + IMX219_REG_VALUE_08BIT, 0x02); + if (ret) + return ret; + + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, + IMX219_REG_VALUE_08BIT, 0x02); + + return ret; + case 24 ... 27: + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, + IMX219_REG_VALUE_08BIT, 0x03); + if (ret) + return ret; + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, + IMX219_REG_VALUE_08BIT, 0x03); + return ret; + default: + /* Should never get here */ + return -EINVAL; + } +} + static int imx219_start_streaming(struct imx219 *imx219) { struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219) goto err_rpm_put; } + /* Configure clock based on reference clock frequency */ + imx219_set_exck_freq(imx219); + /* Apply default values of current mode */ reg_list = &imx219->mode->reg_list; ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219) return -EINVAL; } + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) || + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) { + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n", + imx219->xclk_freq); + return -EINVAL; + } + return 0; } @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client) if (ret) return ret; - /* Check the hardware configuration in device tree */ - if (imx219_check_hwcfg(imx219)) - return -EINVAL; - /* Get system clock (xclk) */ imx219->xclk = devm_clk_get(dev, NULL); if (IS_ERR(imx219->xclk)) { @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client) } imx219->xclk_freq = clk_get_rate(imx219->xclk); - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { - dev_err(dev, "xclk frequency not supported: %d Hz\n", - imx219->xclk_freq); + + /* Check the hardware configuration in device tree */ + if (imx219_check_hwcfg(imx219)) return -EINVAL; - } ret = imx219_get_regulators(imx219); if (ret) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK 2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford @ 2022-04-25 15:58 ` Dave Stevenson 2022-04-25 16:13 ` Dave Stevenson 0 siblings, 1 reply; 15+ messages in thread From: Dave Stevenson @ 2022-04-25 15:58 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML Hi Adam I have no way of testing with an alternate XCLK value, so I'm working based on the datasheet only. On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > The datasheet shows the external clock can be anything > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and > PREPLLCK_OP_DIV need to change based on the clock, so > create helper functions to set these registers based on > the rate of xclk. > > Move the validation of the clock rate into imx219_check_hwcfg > which means delaying the call to it until after xclk has been > determined. > > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++-------- > 1 file changed, 63 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index d13ce5c1ece6..08e7d0e72430 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -39,8 +39,12 @@ > #define IMX219_REG_CHIP_ID 0x0000 > #define IMX219_CHIP_ID 0x0219 > > -/* External clock frequency is 24.0M */ > -#define IMX219_XCLK_FREQ 24000000 > +/* Default external clock frequency is 24.0M */ > +#define IMX219_XCLK_MIN_FREQ 6000000 > +#define IMX219_XCLK_MAX_FREQ 27000000 > +#define IMX219_REG_EXCK 0x012a > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304 > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305 > > /* Pixel rate is fixed for all the modes */ > #define IMX219_PIXEL_RATE 182400000 > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_table[] = { > > {0x0301, 0x05}, /* VTPXCK_DIV */ > {0x0303, 0x01}, /* VTSYSCK_DIV */ > - {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > - {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > {0x0306, 0x00}, /* PLL_VT_MPY */ > {0x0307, 0x39}, > {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = { > */ > static const struct imx219_reg mode_3280x2464_regs[] = { > {0x0128, 0x00}, > - {0x012a, 0x18}, > {0x012b, 0x00}, > {0x0164, 0x00}, > {0x0165, 0x00}, > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > static const struct imx219_reg mode_1920_1080_regs[] = { > {0x0128, 0x00}, > - {0x012a, 0x18}, > {0x012b, 0x00}, > {0x0162, 0x0d}, > {0x0163, 0x78}, > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > static const struct imx219_reg mode_1640_1232_regs[] = { > {0x0128, 0x00}, > - {0x012a, 0x18}, > {0x012b, 0x00}, > {0x0164, 0x00}, > {0x0165, 0x00}, > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > static const struct imx219_reg mode_640_480_regs[] = { > {0x0128, 0x00}, > - {0x012a, 0x18}, > {0x012b, 0x00}, > {0x0162, 0x0d}, > {0x0163, 0x78}, > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219) > return ret; > }; > > +static int imx219_set_exck_freq(struct imx219 *imx219) > +{ > + int ret; > + /* The imx219 registers need MHz not Hz */ > + u8 clk = (u8) (imx219->xclk_freq/1000000U); > + > + /* Set the clock frequency in MHz */ > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK, > + IMX219_REG_VALUE_08BIT, clk); > + > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */ > + switch (clk) { > + case 6 ... 11: > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > + IMX219_REG_VALUE_08BIT, 0x01); > + if (ret) > + return ret; > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > + IMX219_REG_VALUE_08BIT, 0x01); > + return ret; > + case 12 ... 23: > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > + IMX219_REG_VALUE_08BIT, 0x02); > + if (ret) > + return ret; > + > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > + IMX219_REG_VALUE_08BIT, 0x02); > + > + return ret; > + case 24 ... 27: > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > + IMX219_REG_VALUE_08BIT, 0x03); > + if (ret) > + return ret; > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > + IMX219_REG_VALUE_08BIT, 0x03); > + return ret; > + default: > + /* Should never get here */ > + return -EINVAL; > + } > +} > + > static int imx219_start_streaming(struct imx219 *imx219) > { > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219) > goto err_rpm_put; > } > > + /* Configure clock based on reference clock frequency */ > + imx219_set_exck_freq(imx219); You're not checking the return value from this function, so any I2C failures will be ignored. > + > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219) > return -EINVAL; > } > > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) || > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) { > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n", imx219->xclk_freq is unsigned, so %u > + imx219->xclk_freq); > + return -EINVAL; > + } > + > return 0; > } > > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client) > if (ret) > return ret; > > - /* Check the hardware configuration in device tree */ > - if (imx219_check_hwcfg(imx219)) > - return -EINVAL; > - > /* Get system clock (xclk) */ > imx219->xclk = devm_clk_get(dev, NULL); > if (IS_ERR(imx219->xclk)) { > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client) > } > > imx219->xclk_freq = clk_get_rate(imx219->xclk); My bug admittedly, but clk_get_rate returns an unsigned long, but imx219->xclk_freq is u32. Ideally imx219->xclk_freq should be unsigned long to match, and the dev_err I commented on earlier should be %lu. Cheers. Dave > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { > - dev_err(dev, "xclk frequency not supported: %d Hz\n", > - imx219->xclk_freq); > + > + /* Check the hardware configuration in device tree */ > + if (imx219_check_hwcfg(imx219)) > return -EINVAL; > - } > > ret = imx219_get_regulators(imx219); > if (ret) { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK 2022-04-25 15:58 ` Dave Stevenson @ 2022-04-25 16:13 ` Dave Stevenson 2022-04-25 17:28 ` Adam Ford 0 siblings, 1 reply; 15+ messages in thread From: Dave Stevenson @ 2022-04-25 16:13 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML Hi again On Mon, 25 Apr 2022 at 16:58, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Adam > > I have no way of testing with an alternate XCLK value, so I'm working > based on the datasheet only. > > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > > > The datasheet shows the external clock can be anything > > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and > > PREPLLCK_OP_DIV need to change based on the clock, so > > create helper functions to set these registers based on > > the rate of xclk. > > > > Move the validation of the clock rate into imx219_check_hwcfg > > which means delaying the call to it until after xclk has been > > determined. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++-------- > > 1 file changed, 63 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index d13ce5c1ece6..08e7d0e72430 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -39,8 +39,12 @@ > > #define IMX219_REG_CHIP_ID 0x0000 > > #define IMX219_CHIP_ID 0x0219 > > > > -/* External clock frequency is 24.0M */ > > -#define IMX219_XCLK_FREQ 24000000 > > +/* Default external clock frequency is 24.0M */ > > +#define IMX219_XCLK_MIN_FREQ 6000000 > > +#define IMX219_XCLK_MAX_FREQ 27000000 > > +#define IMX219_REG_EXCK 0x012a > > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304 > > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305 > > > > /* Pixel rate is fixed for all the modes */ > > #define IMX219_PIXEL_RATE 182400000 > > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_table[] = { > > > > {0x0301, 0x05}, /* VTPXCK_DIV */ > > {0x0303, 0x01}, /* VTSYSCK_DIV */ > > - {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > - {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > {0x0306, 0x00}, /* PLL_VT_MPY */ > > {0x0307, 0x39}, > > {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = { > > */ > > static const struct imx219_reg mode_3280x2464_regs[] = { > > {0x0128, 0x00}, > > - {0x012a, 0x18}, > > {0x012b, 0x00}, > > {0x0164, 0x00}, > > {0x0165, 0x00}, > > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > > > static const struct imx219_reg mode_1920_1080_regs[] = { > > {0x0128, 0x00}, > > - {0x012a, 0x18}, > > {0x012b, 0x00}, > > {0x0162, 0x0d}, > > {0x0163, 0x78}, > > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > > > static const struct imx219_reg mode_1640_1232_regs[] = { > > {0x0128, 0x00}, > > - {0x012a, 0x18}, > > {0x012b, 0x00}, > > {0x0164, 0x00}, > > {0x0165, 0x00}, > > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > > > static const struct imx219_reg mode_640_480_regs[] = { > > {0x0128, 0x00}, > > - {0x012a, 0x18}, > > {0x012b, 0x00}, > > {0x0162, 0x0d}, > > {0x0163, 0x78}, > > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219) > > return ret; > > }; > > > > +static int imx219_set_exck_freq(struct imx219 *imx219) > > +{ > > + int ret; > > + /* The imx219 registers need MHz not Hz */ > > + u8 clk = (u8) (imx219->xclk_freq/1000000U); > > + > > + /* Set the clock frequency in MHz */ > > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK, > > + IMX219_REG_VALUE_08BIT, clk); In reviewing your other patch I noticed that the EXCK register is actually a 16 bit value. The integer part is in 0x012a, and the fractional part (1/256) in 0x012b, which is currently initialised from the mode tables. Your division discards the fractional part totally, so if the configured frequency was 19.2MHz, then it would be programmed incorrectly. The value for register 0x012b needs to be computed and set here. > > + > > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */ > > + switch (clk) { > > + case 6 ... 11: > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > + IMX219_REG_VALUE_08BIT, 0x01); > > + if (ret) > > + return ret; > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > + IMX219_REG_VALUE_08BIT, 0x01); > > + return ret; > > + case 12 ... 23: > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > + IMX219_REG_VALUE_08BIT, 0x02); > > + if (ret) > > + return ret; > > + > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > + IMX219_REG_VALUE_08BIT, 0x02); > > + > > + return ret; > > + case 24 ... 27: > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > + IMX219_REG_VALUE_08BIT, 0x03); > > + if (ret) > > + return ret; > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > + IMX219_REG_VALUE_08BIT, 0x03); > > + return ret; > > + default: > > + /* Should never get here */ > > + return -EINVAL; > > + } > > +} > > + > > static int imx219_start_streaming(struct imx219 *imx219) > > { > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219) > > goto err_rpm_put; > > } > > > > + /* Configure clock based on reference clock frequency */ > > + imx219_set_exck_freq(imx219); > > You're not checking the return value from this function, so any I2C > failures will be ignored. > > > + > > /* Apply default values of current mode */ > > reg_list = &imx219->mode->reg_list; > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219) > > return -EINVAL; > > } > > > > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) || > > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) { > > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n", > > imx219->xclk_freq is unsigned, so %u > > > + imx219->xclk_freq); > > + return -EINVAL; > > + } > > + > > return 0; > > } > > > > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client) > > if (ret) > > return ret; > > > > - /* Check the hardware configuration in device tree */ > > - if (imx219_check_hwcfg(imx219)) > > - return -EINVAL; > > - > > /* Get system clock (xclk) */ > > imx219->xclk = devm_clk_get(dev, NULL); > > if (IS_ERR(imx219->xclk)) { > > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client) > > } > > > > imx219->xclk_freq = clk_get_rate(imx219->xclk); > > My bug admittedly, but clk_get_rate returns an unsigned long, but > imx219->xclk_freq is u32. > Ideally imx219->xclk_freq should be unsigned long to match, and the > dev_err I commented on earlier should be %lu. > > Cheers. > Dave > > > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { > > - dev_err(dev, "xclk frequency not supported: %d Hz\n", > > - imx219->xclk_freq); > > + > > + /* Check the hardware configuration in device tree */ > > + if (imx219_check_hwcfg(imx219)) > > return -EINVAL; > > - } > > > > ret = imx219_get_regulators(imx219); > > if (ret) { > > -- > > 2.34.1 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK 2022-04-25 16:13 ` Dave Stevenson @ 2022-04-25 17:28 ` Adam Ford 2022-04-25 17:57 ` Dave Stevenson 0 siblings, 1 reply; 15+ messages in thread From: Adam Ford @ 2022-04-25 17:28 UTC (permalink / raw) To: Dave Stevenson Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML On Mon, Apr 25, 2022 at 11:13 AM Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi again > > On Mon, 25 Apr 2022 at 16:58, Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Adam > > > > I have no way of testing with an alternate XCLK value, so I'm working > > based on the datasheet only. > > > > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > > > > > The datasheet shows the external clock can be anything > > > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and > > > PREPLLCK_OP_DIV need to change based on the clock, so > > > create helper functions to set these registers based on > > > the rate of xclk. > > > > > > Move the validation of the clock rate into imx219_check_hwcfg > > > which means delaying the call to it until after xclk has been > > > determined. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > --- > > > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++-------- > > > 1 file changed, 63 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > > index d13ce5c1ece6..08e7d0e72430 100644 > > > --- a/drivers/media/i2c/imx219.c > > > +++ b/drivers/media/i2c/imx219.c > > > @@ -39,8 +39,12 @@ > > > #define IMX219_REG_CHIP_ID 0x0000 > > > #define IMX219_CHIP_ID 0x0219 > > > > > > -/* External clock frequency is 24.0M */ > > > -#define IMX219_XCLK_FREQ 24000000 > > > +/* Default external clock frequency is 24.0M */ > > > +#define IMX219_XCLK_MIN_FREQ 6000000 > > > +#define IMX219_XCLK_MAX_FREQ 27000000 > > > +#define IMX219_REG_EXCK 0x012a > > > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304 > > > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305 > > > > > > /* Pixel rate is fixed for all the modes */ > > > #define IMX219_PIXEL_RATE 182400000 > > > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_table[] = { > > > > > > {0x0301, 0x05}, /* VTPXCK_DIV */ > > > {0x0303, 0x01}, /* VTSYSCK_DIV */ > > > - {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > > - {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > > {0x0306, 0x00}, /* PLL_VT_MPY */ > > > {0x0307, 0x39}, > > > {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > > > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = { > > > */ > > > static const struct imx219_reg mode_3280x2464_regs[] = { > > > {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > {0x012b, 0x00}, > > > {0x0164, 0x00}, > > > {0x0165, 0x00}, > > > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > > > > > static const struct imx219_reg mode_1920_1080_regs[] = { > > > {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > {0x012b, 0x00}, > > > {0x0162, 0x0d}, > > > {0x0163, 0x78}, > > > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > > > > > static const struct imx219_reg mode_1640_1232_regs[] = { > > > {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > {0x012b, 0x00}, > > > {0x0164, 0x00}, > > > {0x0165, 0x00}, > > > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > > > > > static const struct imx219_reg mode_640_480_regs[] = { > > > {0x0128, 0x00}, > > > - {0x012a, 0x18}, > > > {0x012b, 0x00}, > > > {0x0162, 0x0d}, > > > {0x0163, 0x78}, > > > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219) > > > return ret; > > > }; > > > > > > +static int imx219_set_exck_freq(struct imx219 *imx219) > > > +{ > > > + int ret; > > > + /* The imx219 registers need MHz not Hz */ > > > + u8 clk = (u8) (imx219->xclk_freq/1000000U); > > > + > > > + /* Set the clock frequency in MHz */ > > > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK, > > > + IMX219_REG_VALUE_08BIT, clk); > > In reviewing your other patch I noticed that the EXCK register is > actually a 16 bit value. The integer part is in 0x012a, and the > fractional part (1/256) in 0x012b, which is currently initialised from > the mode tables. > Your division discards the fractional part totally, so if the > configured frequency was 19.2MHz, then it would be programmed > incorrectly. > > The value for register 0x012b needs to be computed and set here. That makes sense. I am understanding your comment about register 0x12b, would the example frequency of 19.2MHz translate to 51 for register 12b? Part of me thinks I should drop this patch because I have no real way to test it, and I don't like writing 'theoretical' code. adam > > > > + > > > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */ > > > + switch (clk) { > > > + case 6 ... 11: > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > > + IMX219_REG_VALUE_08BIT, 0x01); > > > + if (ret) > > > + return ret; > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > > + IMX219_REG_VALUE_08BIT, 0x01); > > > + return ret; > > > + case 12 ... 23: > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > > + IMX219_REG_VALUE_08BIT, 0x02); > > > + if (ret) > > > + return ret; > > > + > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > > + IMX219_REG_VALUE_08BIT, 0x02); > > > + > > > + return ret; > > > + case 24 ... 27: > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > > + IMX219_REG_VALUE_08BIT, 0x03); > > > + if (ret) > > > + return ret; > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > > + IMX219_REG_VALUE_08BIT, 0x03); > > > + return ret; > > > + default: > > > + /* Should never get here */ > > > + return -EINVAL; > > > + } > > > +} > > > + > > > static int imx219_start_streaming(struct imx219 *imx219) > > > { > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219) > > > goto err_rpm_put; > > > } > > > > > > + /* Configure clock based on reference clock frequency */ > > > + imx219_set_exck_freq(imx219); > > > > You're not checking the return value from this function, so any I2C > > failures will be ignored. > > > > > + > > > /* Apply default values of current mode */ > > > reg_list = &imx219->mode->reg_list; > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219) > > > return -EINVAL; > > > } > > > > > > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) || > > > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) { > > > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n", > > > > imx219->xclk_freq is unsigned, so %u > > > > > + imx219->xclk_freq); > > > + return -EINVAL; > > > + } > > > + > > > return 0; > > > } > > > > > > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client) > > > if (ret) > > > return ret; > > > > > > - /* Check the hardware configuration in device tree */ > > > - if (imx219_check_hwcfg(imx219)) > > > - return -EINVAL; > > > - > > > /* Get system clock (xclk) */ > > > imx219->xclk = devm_clk_get(dev, NULL); > > > if (IS_ERR(imx219->xclk)) { > > > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client) > > > } > > > > > > imx219->xclk_freq = clk_get_rate(imx219->xclk); > > > > My bug admittedly, but clk_get_rate returns an unsigned long, but > > imx219->xclk_freq is u32. > > Ideally imx219->xclk_freq should be unsigned long to match, and the > > dev_err I commented on earlier should be %lu. > > > > Cheers. > > Dave > > > > > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { > > > - dev_err(dev, "xclk frequency not supported: %d Hz\n", > > > - imx219->xclk_freq); > > > + > > > + /* Check the hardware configuration in device tree */ > > > + if (imx219_check_hwcfg(imx219)) > > > return -EINVAL; > > > - } > > > > > > ret = imx219_get_regulators(imx219); > > > if (ret) { > > > -- > > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK 2022-04-25 17:28 ` Adam Ford @ 2022-04-25 17:57 ` Dave Stevenson 0 siblings, 0 replies; 15+ messages in thread From: Dave Stevenson @ 2022-04-25 17:57 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, Adam Ford-BE, Laurent Pinchart, Mauro Carvalho Chehab, LKML On Mon, 25 Apr 2022 at 18:28, Adam Ford <aford173@gmail.com> wrote: > > On Mon, Apr 25, 2022 at 11:13 AM Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi again > > > > On Mon, 25 Apr 2022 at 16:58, Dave Stevenson > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > Hi Adam > > > > > > I have no way of testing with an alternate XCLK value, so I'm working > > > based on the datasheet only. > > > > > > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > > > > > > > The datasheet shows the external clock can be anything > > > > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and > > > > PREPLLCK_OP_DIV need to change based on the clock, so > > > > create helper functions to set these registers based on > > > > the rate of xclk. > > > > > > > > Move the validation of the clock rate into imx219_check_hwcfg > > > > which means delaying the call to it until after xclk has been > > > > determined. > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > --- > > > > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++-------- > > > > 1 file changed, 63 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > > > index d13ce5c1ece6..08e7d0e72430 100644 > > > > --- a/drivers/media/i2c/imx219.c > > > > +++ b/drivers/media/i2c/imx219.c > > > > @@ -39,8 +39,12 @@ > > > > #define IMX219_REG_CHIP_ID 0x0000 > > > > #define IMX219_CHIP_ID 0x0219 > > > > > > > > -/* External clock frequency is 24.0M */ > > > > -#define IMX219_XCLK_FREQ 24000000 > > > > +/* Default external clock frequency is 24.0M */ > > > > +#define IMX219_XCLK_MIN_FREQ 6000000 > > > > +#define IMX219_XCLK_MAX_FREQ 27000000 > > > > +#define IMX219_REG_EXCK 0x012a > > > > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304 > > > > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305 > > > > > > > > /* Pixel rate is fixed for all the modes */ > > > > #define IMX219_PIXEL_RATE 182400000 > > > > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_table[] = { > > > > > > > > {0x0301, 0x05}, /* VTPXCK_DIV */ > > > > {0x0303, 0x01}, /* VTSYSCK_DIV */ > > > > - {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > > > - {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > > > {0x0306, 0x00}, /* PLL_VT_MPY */ > > > > {0x0307, 0x39}, > > > > {0x030b, 0x01}, /* OP_SYS_CLK_DIV */ > > > > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = { > > > > */ > > > > static const struct imx219_reg mode_3280x2464_regs[] = { > > > > {0x0128, 0x00}, > > > > - {0x012a, 0x18}, > > > > {0x012b, 0x00}, > > > > {0x0164, 0x00}, > > > > {0x0165, 0x00}, > > > > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > > > > > > > > static const struct imx219_reg mode_1920_1080_regs[] = { > > > > {0x0128, 0x00}, > > > > - {0x012a, 0x18}, > > > > {0x012b, 0x00}, > > > > {0x0162, 0x0d}, > > > > {0x0163, 0x78}, > > > > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > > > > > > > > static const struct imx219_reg mode_1640_1232_regs[] = { > > > > {0x0128, 0x00}, > > > > - {0x012a, 0x18}, > > > > {0x012b, 0x00}, > > > > {0x0164, 0x00}, > > > > {0x0165, 0x00}, > > > > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > > > > > > > > static const struct imx219_reg mode_640_480_regs[] = { > > > > {0x0128, 0x00}, > > > > - {0x012a, 0x18}, > > > > {0x012b, 0x00}, > > > > {0x0162, 0x0d}, > > > > {0x0163, 0x78}, > > > > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219) > > > > return ret; > > > > }; > > > > > > > > +static int imx219_set_exck_freq(struct imx219 *imx219) > > > > +{ > > > > + int ret; > > > > + /* The imx219 registers need MHz not Hz */ > > > > + u8 clk = (u8) (imx219->xclk_freq/1000000U); > > > > + > > > > + /* Set the clock frequency in MHz */ > > > > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK, > > > > + IMX219_REG_VALUE_08BIT, clk); > > > > In reviewing your other patch I noticed that the EXCK register is > > actually a 16 bit value. The integer part is in 0x012a, and the > > fractional part (1/256) in 0x012b, which is currently initialised from > > the mode tables. > > Your division discards the fractional part totally, so if the > > configured frequency was 19.2MHz, then it would be programmed > > incorrectly. > > > > The value for register 0x012b needs to be computed and set here. > > That makes sense. > I am understanding your comment about register 0x12b, would the > example frequency of 19.2MHz translate to 51 for register 12b? Yes, AIUI 19.2MHz would be 0x13 0x33 (decimal 19 and 51 / 256). The datasheet lists the default as being a 7.6MHz clock with register values 0x07 0x99 (7 and 153/256, or 7.5976MHz) > Part of me thinks I should drop this patch because I have no real way > to test it, and I don't like writing 'theoretical' code. I have some reservations over it for the same reasons. If you haven't got an actual use case for it, then drop it for now. Dave > adam > > > > > > + > > > > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */ > > > > + switch (clk) { > > > > + case 6 ... 11: > > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > > > + IMX219_REG_VALUE_08BIT, 0x01); > > > > + if (ret) > > > > + return ret; > > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > > > + IMX219_REG_VALUE_08BIT, 0x01); > > > > + return ret; > > > > + case 12 ... 23: > > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > > > + IMX219_REG_VALUE_08BIT, 0x02); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > > > + IMX219_REG_VALUE_08BIT, 0x02); > > > > + > > > > + return ret; > > > > + case 24 ... 27: > > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV, > > > > + IMX219_REG_VALUE_08BIT, 0x03); > > > > + if (ret) > > > > + return ret; > > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV, > > > > + IMX219_REG_VALUE_08BIT, 0x03); > > > > + return ret; > > > > + default: > > > > + /* Should never get here */ > > > > + return -EINVAL; > > > > + } > > > > +} > > > > + > > > > static int imx219_start_streaming(struct imx219 *imx219) > > > > { > > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > > > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219) > > > > goto err_rpm_put; > > > > } > > > > > > > > + /* Configure clock based on reference clock frequency */ > > > > + imx219_set_exck_freq(imx219); > > > > > > You're not checking the return value from this function, so any I2C > > > failures will be ignored. > > > > > > > + > > > > /* Apply default values of current mode */ > > > > reg_list = &imx219->mode->reg_list; > > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs); > > > > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219) > > > > return -EINVAL; > > > > } > > > > > > > > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) || > > > > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) { > > > > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n", > > > > > > imx219->xclk_freq is unsigned, so %u > > > > > > > + imx219->xclk_freq); > > > > + return -EINVAL; > > > > + } > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client) > > > > if (ret) > > > > return ret; > > > > > > > > - /* Check the hardware configuration in device tree */ > > > > - if (imx219_check_hwcfg(imx219)) > > > > - return -EINVAL; > > > > - > > > > /* Get system clock (xclk) */ > > > > imx219->xclk = devm_clk_get(dev, NULL); > > > > if (IS_ERR(imx219->xclk)) { > > > > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client) > > > > } > > > > > > > > imx219->xclk_freq = clk_get_rate(imx219->xclk); > > > > > > My bug admittedly, but clk_get_rate returns an unsigned long, but > > > imx219->xclk_freq is u32. > > > Ideally imx219->xclk_freq should be unsigned long to match, and the > > > dev_err I commented on earlier should be %lu. > > > > > > Cheers. > > > Dave > > > > > > > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { > > > > - dev_err(dev, "xclk frequency not supported: %d Hz\n", > > > > - imx219->xclk_freq); > > > > + > > > > + /* Check the hardware configuration in device tree */ > > > > + if (imx219_check_hwcfg(imx219)) > > > > return -EINVAL; > > > > - } > > > > > > > > ret = imx219_get_regulators(imx219); > > > > if (ret) { > > > > -- > > > > 2.34.1 > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] media: i2c: imx219: Create DPHY helper function 2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford ` (2 preceding siblings ...) 2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford @ 2022-04-12 13:55 ` Adam Ford 2022-04-25 16:07 ` Dave Stevenson 2022-04-25 12:08 ` [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford 4 siblings, 1 reply; 15+ messages in thread From: Adam Ford @ 2022-04-12 13:55 UTC (permalink / raw) To: linux-media Cc: prabhakar.mahadev-lad.rj, tharvey, cstevens, aford, laurent.pinchart, Adam Ford, Dave Stevenson, Mauro Carvalho Chehab, linux-kernel In the table of modes, each mode sets the DPHY to auto. Create a helper function which does the same thing while removing the entry for auto DPHY from ever mode entry. Signed-off-by: Adam Ford <aford173@gmail.com> --- drivers/media/i2c/imx219.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index 08e7d0e72430..bb0bc1b8d91c 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -35,6 +35,10 @@ #define IMX219_MODE_STANDBY 0x00 #define IMX219_MODE_STREAMING 0x01 + +#define IMX219_REG_DPHY_CTRL 0x0128 +#define IMX219_DPHY_AUTO 0 + /* Chip ID */ #define IMX219_REG_CHIP_ID 0x0000 #define IMX219_CHIP_ID 0x0219 @@ -183,7 +187,6 @@ static const struct imx219_reg pll_clk_table[] = { * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. */ static const struct imx219_reg mode_3280x2464_regs[] = { - {0x0128, 0x00}, {0x012b, 0x00}, {0x0164, 0x00}, {0x0165, 0x00}, @@ -222,7 +225,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { }; static const struct imx219_reg mode_1920_1080_regs[] = { - {0x0128, 0x00}, {0x012b, 0x00}, {0x0162, 0x0d}, {0x0163, 0x78}, @@ -261,7 +263,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { }; static const struct imx219_reg mode_1640_1232_regs[] = { - {0x0128, 0x00}, {0x012b, 0x00}, {0x0164, 0x00}, {0x0165, 0x00}, @@ -300,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { }; static const struct imx219_reg mode_640_480_regs[] = { - {0x0128, 0x00}, {0x012b, 0x00}, {0x0162, 0x0d}, {0x0163, 0x78}, @@ -999,6 +999,15 @@ static int imx219_get_selection(struct v4l2_subdev *sd, return -EINVAL; } +static int imx219_enable_dphy(struct imx219 *imx219, u8 mode) +{ + int ret; + + ret = imx219_write_reg(imx219, IMX219_REG_DPHY_CTRL, + IMX219_REG_VALUE_08BIT, mode); + return ret; +}; + static int imx219_configure_lanes(struct imx219 *imx219) { int ret; @@ -1081,6 +1090,13 @@ static int imx219_start_streaming(struct imx219 *imx219) goto err_rpm_put; } + /* Setup DPHY */ + ret = imx219_enable_dphy(imx219, IMX219_DPHY_AUTO); + if (ret) { + dev_err(&client->dev, "%s failed to configure dphy\n", __func__); + goto err_rpm_put; + } + /* Configure clock based on reference clock frequency */ imx219_set_exck_freq(imx219); -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] media: i2c: imx219: Create DPHY helper function 2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford @ 2022-04-25 16:07 ` Dave Stevenson 0 siblings, 0 replies; 15+ messages in thread From: Dave Stevenson @ 2022-04-25 16:07 UTC (permalink / raw) To: Adam Ford Cc: Linux Media Mailing List, Lad Prabhakar, Tim Harvey, cstevens, aford, Laurent Pinchart, Mauro Carvalho Chehab, LKML Hi Adam On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@gmail.com> wrote: > > In the table of modes, each mode sets the DPHY to auto. > Create a helper function which does the same thing while > removing the entry for auto DPHY from ever mode entry. s/ever/every > > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > drivers/media/i2c/imx219.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 08e7d0e72430..bb0bc1b8d91c 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -35,6 +35,10 @@ > #define IMX219_MODE_STANDBY 0x00 > #define IMX219_MODE_STREAMING 0x01 > > + > +#define IMX219_REG_DPHY_CTRL 0x0128 > +#define IMX219_DPHY_AUTO 0 > + > /* Chip ID */ > #define IMX219_REG_CHIP_ID 0x0000 > #define IMX219_CHIP_ID 0x0219 > @@ -183,7 +187,6 @@ static const struct imx219_reg pll_clk_table[] = { > * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > */ > static const struct imx219_reg mode_3280x2464_regs[] = { > - {0x0128, 0x00}, > {0x012b, 0x00}, > {0x0164, 0x00}, > {0x0165, 0x00}, > @@ -222,7 +225,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > }; > > static const struct imx219_reg mode_1920_1080_regs[] = { > - {0x0128, 0x00}, > {0x012b, 0x00}, > {0x0162, 0x0d}, > {0x0163, 0x78}, > @@ -261,7 +263,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > }; > > static const struct imx219_reg mode_1640_1232_regs[] = { > - {0x0128, 0x00}, > {0x012b, 0x00}, > {0x0164, 0x00}, > {0x0165, 0x00}, > @@ -300,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > }; > > static const struct imx219_reg mode_640_480_regs[] = { > - {0x0128, 0x00}, > {0x012b, 0x00}, > {0x0162, 0x0d}, > {0x0163, 0x78}, > @@ -999,6 +999,15 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > return -EINVAL; > } > > +static int imx219_enable_dphy(struct imx219 *imx219, u8 mode) > +{ > + int ret; > + > + ret = imx219_write_reg(imx219, IMX219_REG_DPHY_CTRL, > + IMX219_REG_VALUE_08BIT, mode); Is there a specific reason to extract this one register, but leave the block {0x455e, 0x00}, {0x471e, 0x4b}, {0x4767, 0x0f}, {0x4750, 0x14}, {0x4540, 0x00}, {0x47b4, 0x14}, {0x4713, 0x30}, {0x478b, 0x10}, {0x478f, 0x10}, {0x4793, 0x10}, {0x4797, 0x0e}, {0x479b, 0x0e}, {0x0162, 0x0d}, {0x0163, 0x78}, that appear to also be common to all modes. Other drivers break that lot out into a global registers array that is always sent, rather than individual register writes. Having this one register extra write as a new function is actually likely to increase the size of the module overall, instead of reducing it. Dave > + return ret; > +}; > + > static int imx219_configure_lanes(struct imx219 *imx219) > { > int ret; > @@ -1081,6 +1090,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > goto err_rpm_put; > } > > + /* Setup DPHY */ > + ret = imx219_enable_dphy(imx219, IMX219_DPHY_AUTO); > + if (ret) { > + dev_err(&client->dev, "%s failed to configure dphy\n", __func__); > + goto err_rpm_put; > + } > + > /* Configure clock based on reference clock frequency */ > imx219_set_exck_freq(imx219); > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane 2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford ` (3 preceding siblings ...) 2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford @ 2022-04-25 12:08 ` Adam Ford 4 siblings, 0 replies; 15+ messages in thread From: Adam Ford @ 2022-04-25 12:08 UTC (permalink / raw) To: linux-media Cc: Prabhakar Mahadev Lad, Tim Harvey, cstevens, Adam Ford-BE, Laurent Pinchart, Dave Stevenson, Mauro Carvalho Chehab, Linux Kernel Mailing List On Tue, Apr 12, 2022 at 8:55 AM Adam Ford <aford173@gmail.com> wrote: > > The driver currently only supports a 2-lane camera, a fixed external > clock (XCLK) at 24MHz, a fixed Pixel Rate of 182.4MHz, and a fixed > link rate of 456MHz. There are a bunch of hard-codec values in a > table of operating modes which expect the above to be true. > > According to the datasheet, the driver is capable of operating in > either 4-lane with a pixel rate of 280.8MHz and Linux frequency > of 702MHz or 2-lane configured as stated above. The XCLK can be > anywhere from 6MHz - 27MHz instead of being fixed at 24MHz. > > Split up the hard-coded values into smaller helper functions that > dynamically set the registers of the camera based on the XCLK and > desired number of lanes. > > This series was tested on a Beacon RZ/G2M streaming video at 640x480 > to an LCD with fbdevsink > > media-ctl --links "'rcar_csi2 feaa0000.csi2':1->'VIN0 output':0[1]" -d /dev/media1 > media-ctl --set-v4l2 "'imx219 2-0010':0[fmt:SRGGB8_1X8/640x480 field:none]" -d /dev/media1 > yavta -w '0x009f0905 2048' /dev/v4l-subdev12 > gst-launch-1.0 v4l2src device=/dev/video7 ! video/x-bayer,width=640,height=480,format=rggb ! queue ! bayer2rgb ! fbdevsink > > Due to hardware limitations, the XCLK is still 24MHz, so anyone > willing to test this series with a different XCLK would be appreciated. > > Due to the video format, streaming video at larger resolution was > not feasible, however individual frames captured at 1920x1080 were > successful. > Any comments from anyone on this series? > Adam Ford (4): > media: i2c: imx219: Split common registers from mode tables > media: i2c: imx219: Support four-lane operation > media: i2c: imx219: Enable variable XCLK > media: i2c: imx219: Create DPHY helper function > > drivers/media/i2c/imx219.c | 340 +++++++++++++++++++++++-------------- > 1 file changed, 213 insertions(+), 127 deletions(-) > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-04-25 17:57 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-12 13:55 [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford 2022-04-12 13:55 ` [PATCH 1/4] media: i2c: imx219: Split common registers from mode tables Adam Ford 2022-04-25 16:20 ` Dave Stevenson 2022-04-25 16:50 ` Adam Ford 2022-04-25 17:17 ` Dave Stevenson 2022-04-12 13:55 ` [PATCH 2/4] media: i2c: imx219: Support four-lane operation Adam Ford 2022-04-25 15:39 ` Dave Stevenson 2022-04-12 13:55 ` [PATCH 3/4] media: i2c: imx219: Enable variable XCLK Adam Ford 2022-04-25 15:58 ` Dave Stevenson 2022-04-25 16:13 ` Dave Stevenson 2022-04-25 17:28 ` Adam Ford 2022-04-25 17:57 ` Dave Stevenson 2022-04-12 13:55 ` [PATCH 4/4] media: i2c: imx219: Create DPHY helper function Adam Ford 2022-04-25 16:07 ` Dave Stevenson 2022-04-25 12:08 ` [PATCH 0/4] media: i2c: imx219: Enable variable xclk and 4-lane Adam Ford
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.