Acked-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
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@xxxxxxxxxx> wrote:
>
> From: André Apitzsch <git@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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
>
>
>
