From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752228AbdLAM7N (ORCPT ); Fri, 1 Dec 2017 07:59:13 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:52086 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751874AbdLAM7M (ORCPT ); Fri, 1 Dec 2017 07:59:12 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 4099E6025C Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=architt@codeaurora.org Subject: Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi To: Nickey Yang , robh+dt@kernel.org, heiko@sntech.de, mark.rutland@arm.com, airlied@linux.ie Cc: hl@rock-chips.com, zyw@rock-chips.comg, briannorris@chromium.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, xbl@rock-chips.com References: <1508903463-7254-1-git-send-email-nickey.yang@rock-chips.com> <1508903463-7254-5-git-send-email-nickey.yang@rock-chips.com> <816a1d2d-d213-709d-f357-63e7c5248ad3@codeaurora.org> <2dab7233-139f-b148-2f5e-40a32b3dc797@rock-chips.com> From: Archit Taneja Message-ID: <0dcf03d3-eda6-1b90-10ed-1d14f07824ab@codeaurora.org> Date: Fri, 1 Dec 2017 18:29:04 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <2dab7233-139f-b148-2f5e-40a32b3dc797@rock-chips.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/30/2017 11:02 PM, Nickey Yang wrote: > Hi Archit, > > > On 2017年10月26日 12:53, Archit Taneja wrote: >> >> >> On 10/25/2017 09:21 AM, Nickey Yang wrote: >>> Configure dsi slave channel when driving a panel >>> which needs 2 DSI links. >>> >>> Signed-off-by: Nickey Yang >>> --- >>> .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++ >>>   1 file changed, 2 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt >>> b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt >>> index 6bb59ab..a2bea22 100644 >>> --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt >>> +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt >>> @@ -19,6 +19,8 @@ Optional properties: >>>   - power-domains: a phandle to mipi dsi power domain node. >>>   - resets: list of phandle + reset specifier pairs, as described in [3]. >>>   - reset-names: string reset name, must be "apb". >>> +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave >>> +channel when driving a panel which needs 2 DSI links. >> The example below is how dual DSI bindings could look like. Let me know what >> you think of it. >> >> If both DSI outputs drive the same device (i.e, point to the same panel DT >> node), then I think it's reasonable enough to assume that the DSIs are >> operating in a 'dual-channel' mode. That being said, we still need DT to >> describe which of the DSIs generates the clock for both the channels. This >> is done with the 'clock-master' DT binding. >> >> Thanks, >> Archit >> >> mipi_dsi: mipi@ff960000 { >>     ... >>     ... >> >>     clock-master;    /* implies that this DSI instance drivers the clock >>              * for both the DSIs. >>              */ >> >>     ports { >>         mipi_in: port { >>             ... >>             ... >>         }; >> >>         /* add extra output ports for both DSIs */ >>         mipi_out: port { >>             mipi_panel_out: endpoint { >>                 remote-endpoint = <&panel_in_channel0>; >>             }; >>         }; >>     }; >> >>     panel { >>         ... >>         ... >>         /* >>          * panel node can describe its input ports, if both the DSIs output >>          * ports are connected to the same device (i.e, the same DSI panel), >>          * we can assume that the DSIs need to operate in dual DSI mode >>          */ >>         ports { >>             ... >>             port@0 { >>                 panel_in_channel0: endpoint { >>                     remote-endpoint = <&mipi_panel_out>; >>                 }; >>             }; >> >>             port@1 { >>                 panel_in_channel1: endpoint { >>                     remote-endpoint = <&mipi1_panel_out>; >>                 }; >> >>             }; >>         }; >>     }; >> }; >> >> >> mipi_dsi1: mipi@ff968000 { >>     ... >>     ... >> >>     ports { >>         mipi1_in: port { >>             ... >>             ... >>         }; >> >>         mipi1_out: port { >>             mipi1_panel_out: endpoint { >>                 remote-endpoint = <&panel_in_channel1>; >>             }; >>         }; >>     }; >> }; >> > I try to follow as you suggested,use > > mipi_dsi: mipi@ff960000 { >     ... >     ... >     clock-master;    /* implies that this DSI instance drivers the clock >              * for both the DSIs. >              */ >     ports { >         mipi_in: port { >             ... >             ... >         }; >         /* add extra output ports for both DSIs */ >         mipi_out: port { >             mipi_panel_out: endpoint { >                 remote-endpoint = <&panel_in_channel0>; >             }; >         }; >     }; >     panel { >         ... >         ... >         /* >          * panel node can describe its input ports, if both the DSIs output >          * ports are connected to the same device (i.e, the same DSI panel), >          * we can assume that the DSIs need to operate in dual DSI mode >          */ >         ports { >             ... >             port@0 { >                 panel_in_channel0: endpoint { >                     remote-endpoint = <&mipi_panel_out>; >                 }; >             }; >             port@1 { >                 panel_in_channel1: endpoint { >                     remote-endpoint = <&mipi1_panel_out>; >                 }; > >             }; >         }; >     }; > }; > > mipi_dsi1: mipi@ff968000 { >     ... >     ... >     ports { >         mipi1_in: port { >             ... >             ... >         }; >         mipi1_out: port { >             mipi1_panel_out: endpoint { >                 remote-endpoint = <&panel_in_channel1>; >             }; >         }; >     }; > } > > But it seems we can not use of_drm_find_panel(like below) > > /* >         port = of_graph_get_port_by_id(dev->of_node, 1); >         if (port) { >                 endpoint = of_get_child_by_name(port, "endpoint"); >                 of_node_put(port); >                 if (!endpoint) { >                         dev_err(dev, "no output endpoint found\n"); >                         return -EINVAL; >                 } >                 panel_node = of_graph_get_remote_port_parent(endpoint); >                 of_node_put(endpoint); >                 if (!panel_node) { >                         dev_err(dev, "no output node found\n"); >                         return -EINVAL; >                 } >                 panel = of_drm_find_panel(panel_node); >                 of_node_put(panel_node); >                 if (!panel) >                         return -EPROBE_DEFER; >         } > */ > to get DSI1 outputs,because of_drm_find_panel need compare > > if (panel->dev->of_node == np) > > in dsi_panel driver innolux->base.dev = &innolux->link->dev; > dsi->dev Yes, we should only have 1 drm_panel in the global panel list. Shouldn't it be possible to modify the dsi driver such that dsi1 doesn't care whether it has a drm_panel for it or not, if we are in dual dsi mode? I imagine a sequence like this: 1. dsi0 probes, parses the of-graph, finds the panel and saves its device node. 2. dsi1 probes, parses the of-graph, find the panel's device node - dsi1 checks if it is the same as the panel attached to dsi0. - If so, it just takes the drm_panel pointer from dsi0. - If not, it tries a of_drm_find_panel() on the panel's device node. A dual DSI panel driver would also be a bit different. It will be a mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using the of-graph helpers, we would get the device node of dsi1 using of_find_mipi_dsi_host_by_node(), and create another DSI device using mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on both the dsi devices. > > struct innolux_panel { >         struct drm_panel base; >         struct mipi_dsi_device *link; > }; > It means one panel can only be found in his dsi node,(like dsi0 above). > > I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe > (drivers/gpu/drm/tergra/dsi.c) method. This method will add a new binding similar to "nvidia,ganged-mode", which is something we don't want to do. Archit > > > Thanks, > Nickey > >>>     [1] Documentation/devicetree/bindings/clock/clock-bindings.txt >>>   [2] Documentation/devicetree/bindings/media/video-interfaces.txt >>> >> > > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project