On Mon, Oct 14, 2024 at 04:28:29PM +0800, Liu Ying wrote: > On 10/14/2024, Dmitry Baryshkov wrote: > > On Mon, Oct 14, 2024 at 03:18:15PM +0800, Liu Ying wrote: > >> 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.liu@xxxxxxx> > >>>> --- > >>>> 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. > > > > Then you need a similar function for the sink side. Add it to the > > drm_of.c > > How about getting remote LVDS source ports first and use > drm_of_lvds_get_dual_link_pixel_order() like the snippet below? > This way, no need to add a similar function or modify > drm_of_lvds_get_dual_link_pixel_order() implementation. > > If you don't like this, can you please suggest a similar function > name or maybe an additional parameter(with type and name) for > drm_of_lvds_get_dual_link_pixel_order()? > > ---8<--- > port0_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); > if (port0_ep) { > remote_port0 = of_graph_get_remote_port(port0_ep); > of_node_put(port0_ep); > } > port1_ep = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); > if (port1_ep) { > remote_port1 = of_graph_get_remote_port(port1_ep); > of_node_put(port1_ep); > } I think getting remotes just to get remotes back is a little bit clumsy. Adding drm_of_lvds_get_dual_link_pixel_order_sink() looks like a claner solution. > > if (remote_port0 && remote_port1) { > order = drm_of_lvds_get_dual_link_pixel_order(remote_port0, > remote_port1); > if (order < 0) { > dev_err(dev, > "failed to get dual link pixel order: %d\n", > order); > ret = order; > } else if (order == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) { > it->lvds_dual_link = true; > } else if (order == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { > it->lvds_dual_link = true; > it->lvds_link12_swap = true; > } it->lvds_dual_link = true; order = drm_of_lvds_get_dual_link_pixel_order_sink(port0_ep, port1_ep); if (order < 0) { dev_err(...); ret = order; } else if (order == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { it->lvds_link12_swap = true; } > } else if (remote_port1) { > ret = -EINVAL; > dev_err(dev, "single input LVDS port1 is not supported\n"); > } else if (!remote_port0) { > ret = -EINVAL; > dev_err(dev, "no input LVDS port\n"); > } > > of_node_put(remote_port0); > of_node_put(remote_port1); > ---8<--- > > > > >> > >> * 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 > >> > >>> > >>>> + } 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 > >> > > > > -- > Regards, > Liu Ying > -- With best wishes Dmitry
