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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>
