| Message ID | 20220412135534.2796158-4-aford173@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] media: i2c: imx219: Split common registers from mode tables | expand |
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 >
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 > >
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 > > >
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 > > > >
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) {
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(-)