RE: [PATCH v2 6/9] drm/bridge: Add ITE IT6263 LVDS to HDMI converter

[Biju Das]

Hi Liu Ying,

> -----Original Message-----
> From: Liu Ying <victor....@nxp.com>
> Sent: Monday, October 14, 2024 8:18 AM
> Subject: Re: [PATCH v2 6/9] drm/bridge: Add ITE IT6263 LVDS to HDMI converter
> 
> On 10/14/2024, Dmitry Baryshkov wrote:
> > On Sat, Oct 12, 2024 at 03:35:40PM +0800, Liu Ying wrote:
> >> Add basic HDMI video output support. Currently, only RGB888 output
> >> pixel format is supported.  At the LVDS input side, the driver
> >> supports single LVDS link and dual LVDS links with "jeida-24" LVDS
> >> mapping.
> >>
> >> Product link:
> >> https://www.ite.com.tw/en/product/cate1/IT6263
> >>
> >> Signed-off-by: Liu Ying <victor....@nxp.com>
> >> ---
> >> v2:
> >> * Add AVI inforframe support.  (Maxime)
> >> * Add DRM_MODE_CONNECTOR_HDMIA.  (Biju)
> >> * Rename it6263_reset() to it6263_hw_reset().  (Biju)
> >> * Check number of LVDS link data lanes.  (Biju)
> >>
> >>  drivers/gpu/drm/bridge/Kconfig      |   8 +
> >>  drivers/gpu/drm/bridge/Makefile     |   1 +
> >>  drivers/gpu/drm/bridge/ite-it6263.c | 919
> >> ++++++++++++++++++++++++++++
> >>  3 files changed, 928 insertions(+)
> >>  create mode 100644 drivers/gpu/drm/bridge/ite-it6263.c
> >>
> >
> > [...]
> >
> >> +static int it6263_parse_dt(struct it6263 *it) {
> >> +  struct device *dev = it->dev;
> >> +  struct device_node *port0, *port1;
> >> +  int ret;
> >> +
> >> +  ret = of_property_read_u8(dev->of_node, "ite,lvds-link-num-data-lanes",
> >> +                            &it->lvds_link_num_dlanes);
> >> +  if (ret) {
> >> +          dev_err(dev, "failed to get LVDS link number of data lanes: 
> >> %d\n",
> >> +                  ret);
> >> +          return ret;
> >> +  }
> >> +
> >> +  it->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0);
> >> +  if (IS_ERR(it->next_bridge))
> >> +          return dev_err_probe(dev, PTR_ERR(it->next_bridge),
> >> +                               "failed to get next bridge\n");
> >> +
> >> +  port0 = of_graph_get_port_by_id(dev->of_node, 0);
> >> +  port1 = of_graph_get_port_by_id(dev->of_node, 1);
> >> +  if (port0 && port1) {
> >> +          if (of_property_read_bool(port0, "dual-lvds-even-pixels") &&
> >> +              of_property_read_bool(port1, "dual-lvds-odd-pixels")) {
> >> +                  it->lvds_dual_link = true;
> >> +                  it->lvds_link12_swap = true;
> >> +          } else if (of_property_read_bool(port0, "dual-lvds-odd-pixels") 
> >> &&
> >> +                     of_property_read_bool(port1, 
> >> "dual-lvds-even-pixels")) {
> >> +                  it->lvds_dual_link = true;
> >> +          }
> >> +
> >> +          if (!it->lvds_dual_link) {
> >> +                  dev_err(dev,
> >> +                          "failed to get LVDS dual link pixel order\n");
> >> +                  ret = -EINVAL;
> >> +          }
> >
> > Please use drm_of_lvds_get_dual_link_pixel_order(), it validates that
> > the DT definition is sound: one port for odd pixels, one port for even
> > pixels.
> 
> It cannot be used, because it get the pixel order for the LVDS source not 
> sink. IT6263 is the LVDS
> sink.
> 
>  * drm_of_lvds_get_dual_link_pixel_order - Get LVDS dual-link pixel order
>  * @port1: First DT port node of the Dual-link LVDS source
>  * @port2: Second DT port node of the Dual-link LVDS source


Can't you use drm_of_lvds_get_dual_link_pixel_order() from host for the same 
use case. See [1]?

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/renesas/rcar-du/rcar_lvds.c?h=next-20241011

lvds_bridge0: lvds-to-hdmi-bridge@4c {
                compatible = "ite,it6263";
                reg = <0x4c>;
                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        port@0 {
                                reg = <0>;
                                dual-lvds-odd-pixels;
                                lvds0_in0: endpoint@0 {
                                        remote-endpoint = <&lvds0_out>;
                                };
                        };

                        port@1 {
                                reg = <1>;
                                dual-lvds-even-pixels;
                                lvds0_in1: endpoint@1 {
                                        remote-endpoint = <&lvds1_out>;
                                };
                        };

                        port@2 {
                                reg = <2>;
                                it6263_out: endpoint@2 {
                                        remote-endpoint = 
<&lvds_to_hdmi_con_out>;
                                };
                        };
                };
        };
};

Cheers,
Biju


> 
> >
> >> +  } else if (port1) {
> >> +          ret = -EINVAL;
> >> +          dev_err(dev, "single input LVDS port1 is not supported\n");
> >> +  } else if (!port0) {
> >> +          ret = -EINVAL;
> >> +          dev_err(dev, "no input LVDS port\n");
> >> +  }
> >> +
> >> +  of_node_put(port0);
> >> +  of_node_put(port1);
> >> +
> >> +  return ret;
> >> +}
> >> +
> >
> 
> --
> Regards,
> Liu Ying