On Mon, Apr 25, 2022 at 11:20 AM Dave Stevenson
<dave.stevenson@xxxxxxxxxxxxxxx> wrote:
>
> Hi Adam
>
> On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
> >
