On Wed, Jan 12, 2022 at 6:37 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> On Wed, Jan 12, 2022 at 03:44:00PM +0530, Jagan Teki wrote:
> > On Wed, Jan 12, 2022 at 3:33 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
> > > > Some OF graphs don't require 'port' or 'ports' to represent the
> > > > downstream panel or bridge; instead it simply adds a child node
> > > > on a given parent node.
> > >
> > > All bindings using OF graph nodes require either port or ports.
> > >
> > > DSI Host however don't have to use the OF graph, and that's what you're
> > > talking about.
> >
> > Yes, right now I can see DSI but this change is generic to any OF graph.
>
> Not really? Generic to all users of drm_of_find_panel_or_bridge sure,
> but DSI is the only use case where we could have bridges or panels that
> would be child of the user.
>
> > > > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > > > node based on the OF graph port and endpoint and it fails to use
> > > > if the given node has a child panel or bridge.
> > > >
> > > > This patch add support to lookup that given node has child panel
> > > > or bridge however that child node is neither a 'port' nor a 'ports'
> > > > node.
> > > >
> > > > Example OF graph representation of DSI host, which has 'port'
> > > > but not has 'ports' and has child panel node.
> > > >
> > > > dsi {
> > > > compatible = "allwinner,sun6i-a31-mipi-dsi";
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > port {
> > > > dsi_in_tcon0: endpoint {
> > > > remote-endpoint = <tcon0_out_dsi>;
> > > > };
> > > >
> > > > panel@0 {
> > > > reg = <0>;
> > > > };
> > > > };
> > > >
> > > > Example OF graph representation of DSI host, which has 'ports'
> > > > but not has 'port' and has child panel node.
> > > >
> > > > dsi {
> > > > compatible = "samsung,exynos5433-mipi-dsi";
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > ports {
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > port@0 {
> > > > reg = <0>;
> > > >
> > > > dsi_to_mic: endpoint {
> > > > remote-endpoint = <&mic_to_dsi>;
> > > > };
> > > > };
> > > > };
> > > >
> > > > panel@0 {
> > > > reg = <0>;
> > > > };
> > > > };
> > >
> > > I can't see how that one makes sense. The endpoint seems to have a
> > > single output, yet you also have a panel under it which is also an
> > > output? You should have at least the virtual channel of that endpoint
> > > somewhere to differentiate data between the panel and whatever is
> > > connected on the other side of that endpoint.
> >
> > Same that I understood so far (based on v2 change), However we have
> > exynos5433 has this pipeline and Andrzej mentioned it is valid
> > pipeline on other thread.
> >
> > May be Andrzej, can give more conclusive evidence for it.
> >
> > >
> > > > Example OF graph representation of DSI host, which has neither
> > > > a 'port' nor a 'ports' but has child panel node.
> > > >
> > > > dsi0 {
> > > > compatible = "ste,mcde-dsi";
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > panel@0 {
> > > > reg = <0>;
> > > > };
> > > > };
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > Changes for v3:
> > > > - updated based on other usecase where 'ports' used along with child
> > > > Changes for v2:
> > > > - drop of helper
> > > > https://patchwork.kernel.org/project/dri-devel/cover/20211207054747.461029-1-jagan@xxxxxxxxxxxxxxxxxxxx/
> > > > - support 'port' alone OF graph
> > > > - updated comments
> > > > - added simple code
> > > >
> > > > drivers/gpu/drm/drm_of.c | 18 ++++++++++++++++++
> > > > 1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > > index 59d368ea006b..aeddd39b8df6 100644
> > > > --- a/drivers/gpu/drm/drm_of.c
> > > > +++ b/drivers/gpu/drm/drm_of.c
> > > > @@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > > if (panel)
> > > > *panel = NULL;
> > > >
> > > > + /**
> > > > + * Some OF graphs don't require 'port' or 'ports' to represent the
> > > > + * downstream panel or bridge; instead it simply adds a child node
> > > > + * on a given parent node.
> > >
> > > All OF graphs require either port or ports. DSI hosts can just have a
> > > child node.
> >
> > As commented above, it can be DSI or any host which has child nodes
> > and are looking to find panel/bridge.
>
> DSI is the only case in this situation, but ok. It still has nothing to
> do with OF graph. All bindings using OF graph will require either port
> or ports, and the DSI binding doesn't mandate to use OF graph. So the
> comment above is misleading at best.
Okay. How about this updated comment?
/**
* All bindings using OF graph will require either 'port' or 'ports'
* to represent the downstream panel or bridge however the DSI binding
* doesn't mandate to use OF graph; instead it simply adds a panel or
* bridge in child node.
*
* Lookup that child node for a given parent however that child is
* neither a 'port' nor a 'ports' node.
*/
Thanks,
Jagan.
