Re: [PATCH RESEND 1/4] media: i2c: imx214: Calculate link bit rate from clock frequency

[Ricardo Ribalda Delgado]

On Sat, Mar 8, 2025 at 10:48 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@xxxxxxxxxx> wrote:
>
> From: André Apitzsch <git@xxxxxxxxxxx>
>
> Replace the magic link bit rate number (4800) by its calculation based
> on the used parameters and the provided external clock frequency.
>
> The link bit rate is output bitrate multiplied by the number lanes. The
> output bitrate is the OPPXCK clock frequency multiplied by the number of
> bits per pixel. The OPPXCK clock frequency is OPCK multiplied by the
> OPPXCK clock division ratio. And OPCK is the external clock frequency
> multiplied by the PreDivider setting and the PLL multiple setting.
>
> Signed-off-by: André Apitzsch <git@xxxxxxxxxxx>
Acked-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
>  drivers/media/i2c/imx214.c | 51 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 6c3f6f3c8b1f7724110dc55fead0f8a168edf35b..14a4c5094799014da38ab1beec401f0d923c2152 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -84,6 +84,7 @@
>  #define IMX214_CSI_DATA_FORMAT_RAW10   0x0A0A
>  #define IMX214_CSI_DATA_FORMAT_COMP6   0x0A06
>  #define IMX214_CSI_DATA_FORMAT_COMP8   0x0A08
> +#define IMX214_BITS_PER_PIXEL_MASK     0xFF
>
>  #define IMX214_REG_CSI_LANE_MODE       CCI_REG8(0x0114)
>  #define IMX214_CSI_2_LANE_MODE         1
> @@ -104,11 +105,23 @@
>
>  /* PLL settings */
>  #define IMX214_REG_VTPXCK_DIV          CCI_REG8(0x0301)
> +#define IMX214_DEFAULT_VTPXCK_DIV      5
> +
>  #define IMX214_REG_VTSYCK_DIV          CCI_REG8(0x0303)
> +#define IMX214_DEFAULT_VTSYCK_DIV      2
> +
>  #define IMX214_REG_PREPLLCK_VT_DIV     CCI_REG8(0x0305)
> +#define IMX214_DEFAULT_PREPLLCK_VT_DIV 3
> +
>  #define IMX214_REG_PLL_VT_MPY          CCI_REG16(0x0306)
> +#define IMX214_DEFAULT_PLL_VT_MPY      150
> +
>  #define IMX214_REG_OPPXCK_DIV          CCI_REG8(0x0309)
> +#define IMX214_DEFAULT_OPPXCK_DIV      10
> +
>  #define IMX214_REG_OPSYCK_DIV          CCI_REG8(0x030b)
> +#define IMX214_DEFAULT_OPSYCK_DIV      1
> +
>  #define IMX214_REG_PLL_MULT_DRIV       CCI_REG8(0x0310)
>  #define IMX214_PLL_SINGLE              0
>  #define IMX214_PLL_DUAL                        1
> @@ -204,6 +217,14 @@
>  #define IMX214_PIXEL_ARRAY_WIDTH       4208U
>  #define IMX214_PIXEL_ARRAY_HEIGHT      3120U
>
> +/* Link bit rate for a given input clock frequency in single PLL mode */
> +#define IMX214_LINK_BIT_RATE(clk_freq) \
> +       (((clk_freq) / 1000000) / IMX214_DEFAULT_PREPLLCK_VT_DIV \
> +       * IMX214_DEFAULT_PLL_VT_MPY \
> +       / (IMX214_DEFAULT_OPSYCK_DIV * IMX214_DEFAULT_OPPXCK_DIV) \
> +       * (IMX214_CSI_DATA_FORMAT_RAW10 & IMX214_BITS_PER_PIXEL_MASK) \
> +       * (IMX214_CSI_4_LANE_MODE + 1))
I am always very scared of these macros.... Integer over/underflows
are very easy to miss
If we support a small number of frequencies I would rather see a
function with a switch and a comment explaining where the number comes
from.


> +
>  static const char * const imx214_supply_name[] = {
>         "vdda",
>         "vddd",
> @@ -299,15 +320,16 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
>         { IMX214_REG_DIG_CROP_WIDTH, 4096 },
>         { IMX214_REG_DIG_CROP_HEIGHT, 2304 },
>
> -       { IMX214_REG_VTPXCK_DIV, 5 },
> -       { IMX214_REG_VTSYCK_DIV, 2 },
> -       { IMX214_REG_PREPLLCK_VT_DIV, 3 },
> -       { IMX214_REG_PLL_VT_MPY, 150 },
> -       { IMX214_REG_OPPXCK_DIV, 10 },
> -       { IMX214_REG_OPSYCK_DIV, 1 },
> +       { IMX214_REG_VTPXCK_DIV, IMX214_DEFAULT_VTPXCK_DIV },
> +       { IMX214_REG_VTSYCK_DIV, IMX214_DEFAULT_VTSYCK_DIV },
> +       { IMX214_REG_PREPLLCK_VT_DIV, IMX214_DEFAULT_PREPLLCK_VT_DIV },
> +       { IMX214_REG_PLL_VT_MPY, IMX214_DEFAULT_PLL_VT_MPY },
> +       { IMX214_REG_OPPXCK_DIV, IMX214_DEFAULT_OPPXCK_DIV },
> +       { IMX214_REG_OPSYCK_DIV, IMX214_DEFAULT_OPSYCK_DIV },
>         { IMX214_REG_PLL_MULT_DRIV, IMX214_PLL_SINGLE },
>
> -       { IMX214_REG_REQ_LINK_BIT_RATE, IMX214_LINK_BIT_RATE_MBPS(4800) },
> +       { IMX214_REG_REQ_LINK_BIT_RATE,
> +               IMX214_LINK_BIT_RATE_MBPS(IMX214_LINK_BIT_RATE(IMX214_DEFAULT_CLK_FREQ)) },
>
>         { CCI_REG8(0x3A03), 0x09 },
>         { CCI_REG8(0x3A04), 0x50 },
> @@ -362,15 +384,16 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
>         { IMX214_REG_DIG_CROP_WIDTH, 1920 },
>         { IMX214_REG_DIG_CROP_HEIGHT, 1080 },
>
> -       { IMX214_REG_VTPXCK_DIV, 5 },
> -       { IMX214_REG_VTSYCK_DIV, 2 },
> -       { IMX214_REG_PREPLLCK_VT_DIV, 3 },
> -       { IMX214_REG_PLL_VT_MPY, 150 },
> -       { IMX214_REG_OPPXCK_DIV, 10 },
> -       { IMX214_REG_OPSYCK_DIV, 1 },
> +       { IMX214_REG_VTPXCK_DIV, IMX214_DEFAULT_VTPXCK_DIV },
> +       { IMX214_REG_VTSYCK_DIV, IMX214_DEFAULT_VTSYCK_DIV },
> +       { IMX214_REG_PREPLLCK_VT_DIV, IMX214_DEFAULT_PREPLLCK_VT_DIV },
> +       { IMX214_REG_PLL_VT_MPY, IMX214_DEFAULT_PLL_VT_MPY },
> +       { IMX214_REG_OPPXCK_DIV, IMX214_DEFAULT_OPPXCK_DIV },
> +       { IMX214_REG_OPSYCK_DIV, IMX214_DEFAULT_OPSYCK_DIV },
>         { IMX214_REG_PLL_MULT_DRIV, IMX214_PLL_SINGLE },
>
> -       { IMX214_REG_REQ_LINK_BIT_RATE, IMX214_LINK_BIT_RATE_MBPS(4800) },
> +       { IMX214_REG_REQ_LINK_BIT_RATE,
> +               IMX214_LINK_BIT_RATE_MBPS(IMX214_LINK_BIT_RATE(IMX214_DEFAULT_CLK_FREQ)) },
>
>         { CCI_REG8(0x3A03), 0x04 },
>         { CCI_REG8(0x3A04), 0xF8 },
>
> --
> 2.48.1
>
>