Re: [PATCH v3 12/12] media: i2c: imx214: Fix link frequency

[Ricardo Ribalda Delgado <ribalda@kernel.org>]

Acked-by: Ricardo Ribalda <ribalda@chromium.org>

nit:  media: i2c: imx214: Fix link frequency validation



On Sat, Dec 7, 2024 at 9:49 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@kernel.org> wrote:
>
> From: André Apitzsch <git@apitzsch.eu>
>
> The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
> IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10),
> which works out as 384MPix/s. (The 8 is 4 lanes and DDR.)
>
> Parsing the PLL registers with the defined 24MHz input. We're in single
> PLL mode, so MIPI frequency is directly linked to pixel rate.  VTCK ends
> up being 1200MHz, and VTPXCK and OPPXCK both are 120MHz.  Section 5.3
> "Frame rate calculation formula" says "Pixel rate
> [pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which basically
> agrees with my number above.
>
> 3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK * reg
> 0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a link
> frequency of 600MHz due to DDR.
> That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR.
>
> Keep the previous link frequency for backward compatibility.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/media/i2c/imx214.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 1330f13207beec0960c384681bf0b49e99fe860f..910ad03cda23345d3d10d13cd30f007954534e80 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -31,7 +31,9 @@
>  #define IMX214_REG_FAST_STANDBY_CTRL   CCI_REG8(0x0106)
>
>  #define IMX214_DEFAULT_CLK_FREQ        24000000
> -#define IMX214_DEFAULT_LINK_FREQ 480000000
> +#define IMX214_DEFAULT_LINK_FREQ       600000000
> +/* Keep wrong link frequency for backward compatibility */
> +#define IMX214_DEFAULT_LINK_FREQ_LEGACY        480000000
>  #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
>  #define IMX214_FPS 30
>
> @@ -1216,18 +1218,22 @@ static int imx214_parse_fwnode(struct device *dev)
>                 goto done;
>         }
>

Now that I think about this... We only support
buf_cfg.no_of_link_frequencies ==1.
Maybe you can add a check before the loop
if (bus_cfg.nr_of_link_frequencies != 1)
                       dev_warn(dev, "Only one link-frequency
supported, please review your DT. Continuing anyway\n ")


> -       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> +       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
>                 if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
>                         break;
> -
> -       if (i == bus_cfg.nr_of_link_frequencies) {
> -               dev_err_probe(dev, -EINVAL,
> -                             "link-frequencies %d not supported, Please review your DT\n",
> -                             IMX214_DEFAULT_LINK_FREQ);
> -               ret = -EINVAL;
> -               goto done;
> +               if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> +                       dev_warn(dev,
> +                                "link-frequencies %d not supported, please review your DT. Continuing anyway\n",
> +                                IMX214_DEFAULT_LINK_FREQ);
> +                       break;
> +               }
>         }
>
> +       if (i == bus_cfg.nr_of_link_frequencies)
> +               ret = dev_err_probe(dev, -EINVAL,
> +                                   "link-frequencies %d not supported, please review your DT\n",
> +                                   IMX214_DEFAULT_LINK_FREQ);
> +
>  done:
>         v4l2_fwnode_endpoint_free(&bus_cfg);
>         fwnode_handle_put(endpoint);
>
> --
> 2.47.1
>
>
>