[PATCH] video: bridge: add ITE IT66121 DPI-to-HDMI bridge driver

[Simon Glass]

Hi Shaurya,

On 2026-04-15T19:03:21, Shaurya Rane <ssrane_b23 at ee.vjti.ac.in> wrote:
> video: bridge: add ITE IT66121 DPI-to-HDMI bridge driver
>
> Add support for the ITE IT66121 DPI-to-HDMI bridge chip. The IT66121
> converts parallel RGB/DPI video input to HDMI output. It is found on
> boards like the BeaglePlay where the TI AM625 SoC's DPI output is
> routed to an HDMI connector through this chip.
>
> The driver is based on the IT66121 Programmer Guide and the Linux
> kernel driver drivers/gpu/drm/bridge/ite-it66121.c.
>
> Tested on BeaglePlay hardware with HDMI output via TI TIDSS.
>
> Signed-off-by: Shaurya Rane <ssrane_b23 at ee.vjti.ac.in>
>
> drivers/video/bridge/Kconfig   |  10 +
>  drivers/video/bridge/Makefile  |   1 +
>  drivers/video/bridge/it66121.c | 535 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 546 insertions(+)

General note: please use lower-case hex consistently.

> diff --git a/drivers/video/bridge/it66121.c b/drivers/video/bridge/it66121.c
> @@ -0,0 +1,535 @@
> +#include <dm.h>
> +#include <i2c.h>
> +#include <edid.h>
> +#include <log.h>

Does this need linux/bitops.h for the BIT() macro?

> +static int it66121_setup_afe(struct udevice *dev, unsigned long pclk_khz)
> +{
> +     int ret;
> +
> +     ret = it66121_reg_write(dev, IT66121_AFE_DRV_REG, IT66121_AFE_DRV_RST);
> +     if (ret)
> +             return ret;
> +
> +     if (pclk_khz > 80000) {
> +             ret = it66121_reg_set(dev, IT66121_AFE_XP_REG, 0x90, 0x80);
> +             if (ret)
> +                     return ret;
> +             ret = it66121_reg_set(dev, IT66121_AFE_IP_REG, 0x89, 0x80);
> +             if (ret)
> +                     return ret;
> +             ret = it66121_reg_set(dev, IT66121_AFE_XP_EC1, 0x10, 0x00);

Please can you add named defines for these bit fields (0x90, 0x89,
0x80, 0x10, 0x09, etc.) so it is clear what each AFE setting does. A
brief comment referencing the programming guide section would also
help.

> +     /* Preferred DTD pixel clock: bytes 54-55, units of 10 kHz. */
> +     pclk_khz = ((unsigned long)priv->edid[55] << 8 |
> +                  priv->edid[54]) * 10;

Please can you use EDID_DETAILED_TIMING_PIXEL_CLOCK() from edid.h (or
edid_get_timing()) rather than open-coding the EDID parsing.

> +static int it66121_attach(struct udevice *dev)
> +{
> +     struct it66121_priv *priv = dev_get_priv(dev);
> +     unsigned long pclk_khz;
> +     u8 status;
> +     int ret;
> +
> +     ret = video_bridge_set_active(dev, true);
> +     if (ret) {
> +             debug("IT66121: set_active failed: %d\n", ret);
> +             return ret;
> +     }

Just to check: video_bridge_set_active() returns 0 if the GPIOs are
not specified in DT. If BeaglePlay does not define reset-gpios, this
returns 0 but does not reset the chip. Is this intentional?

> +     if (pclk_khz > 0) {
> +             debug("IT66121: configuring video for %lu kHz pclk\n",
> +                   pclk_khz);
> +             ret = it66121_setup_video(dev, pclk_khz);
> +             if (ret)
> +                     debug("IT66121: setup_video failed: %d\n", ret);
> +     }
> +
> +     return 0;

If it66121_setup_video() fails you log it but return 0. Should this
return ret instead? If the failure is intentionally non-fatal, please
add a comment.

Regards,
Simon