Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK

From: Adam Ford
Date: Mon Apr 25 2022 - 13:29:08 EST

On Mon, Apr 25, 2022 at 11:13 AM Dave Stevenson
<dave.stevenson@xxxxxxxxxxxxxxx> wrote:
>
> Hi again
>
> On Mon, 25 Apr 2022 at 16:58, Dave Stevenson
> <dave.stevenson@xxxxxxxxxxxxxxx> 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@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);
>
> 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
> > >