On Mon, Dec 13, 2021 at 03:35:16PM +0200, Laurent Pinchart wrote:
> Hi Maxime,
>
> On Mon, Dec 13, 2021 at 02:09:36PM +0100, Maxime Ripard wrote:
> > On Mon, Dec 13, 2021 at 02:28:39PM +0200, Laurent Pinchart wrote:
> > > On Mon, Dec 13, 2021 at 05:46:13PM +0530, Jagan Teki wrote:
> > > > Some OF graphs don't require 'ports' to represent the
> > > > downstream panel or bridge; instead it simply adds a child
> > > > node on a given parent node.
> > > >
> > > > 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 cannot be a 'port'
> > > > alone or it cannot be a 'port' node too.
> > > >
> > > > Example OF graph representation of DSI host, which doesn't
> > > > have 'ports' and has child panel.
> > > >
> > > > 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 doesn't
> > > > have 'ports' and has child bridge.
> > > >
> > > > dsi {
> > > > compatible = "allwinner,sun6i-a31-mipi-dsi";
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > port {
> > > > dsi_in_tcon0: endpoint {
> > > > remote-endpoint = <tcon0_out_dsi>;
> > > > };
> > > >
> > > > bridge@0 {
> > > > reg = <0>;
> > > >
> > > > ports {
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > bridge_out: port@1 {
> > > > reg = <1>;
> > > >
> > > > bridge_out_panel: endpoint {
> > > > remote-endpoint = <&panel_out_bridge>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > Example OF graph representation of DSI host, which doesn't
> > > > have 'ports' or 'port' and has child panel.
> > > >
> > > > dsi0 {
> > > > compatible = "ste,mcde-dsi";
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > panel@0 {
> > > > reg = <0>;
> > > > };
> > > > };
> > > >
> > > > Example OF graph representation of LTDC host, which doesn't
> > > > have 'ports' or child panel/bridge and has 'port'.
> > > >
> > > > ltdc {
> > > > compatible = "st,stm32-ltdc";
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > port {
> > > > };
> > > > };
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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 | 23 +++++++++++++++++++++++
> > > > 1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > > index 59d368ea006b..7d018ff8bc83 100644
> > > > --- a/drivers/gpu/drm/drm_of.c
> > > > +++ b/drivers/gpu/drm/drm_of.c
> > > > @@ -249,6 +249,27 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > > if (panel)
> > > > *panel = NULL;
> > > >
> > > > + /**
> > > > + * Some OF graphs don't require 'ports' to represent the downstream
> > > > + * panel or bridge; instead it simply adds a child node on a given
> > > > + * parent node.
> > > > + *
> > > > + * Lookup that child node for a given parent however that child
> > > > + * cannot be a 'port' alone or it cannot be a 'port' node too.
> > > > + */
> > > > + if (!of_get_child_by_name(np, "ports")) {
> > > > + if (of_get_child_by_name(np, "port") && (of_get_child_count(np) == 1))
> > >
> > > This messes up reference counting of device_node.
> > >
> > > > + goto of_graph_get_remote;
> > > > +
> > > > + for_each_available_child_of_node(np, remote) {
> > > > + if (of_node_name_eq(remote, "port"))
> > > > + continue;
> > > > +
> > > > + goto of_find_panel_or_bridge;
> > > > + }
> > > > + }
> > >
> > > This really looks like a hack to me, I'm worried it may cause issues. It
> > > would be better, I think, to split the drm_of_find_panel_or_bridge()
> > > function in two, with the of_graph_get_remote_node() call moved to a
> > > wrapper function, calling an inner function that takes the remote
> > > device_node pointer. For the DSI use case, you could either look up the
> > > panel DT node in the display driver and call the inner function
> > > directly, or implement a DSI-specific wrapper.
> >
> > I disagree. The whole point of drm_of_find_panel_or_bridge was that it
> > was a helper for the encoder / upstream bridge to retrieve whatever is
> > there next. It's useful and removes boilerplate.
> >
> > We definitely want to have something just as convenient for DSI.
>
> That could ba a drm_of_find_dsi_panel_or_bridge() :-) My point is that
> I'd like to avoid making assumptions on node names in the lower layers.
>
> I also have a different use case for a drm_of_find_panel_or_bridge()
> function ta would take a device_node pointer, so moving the
> of_graph_get_remote_node() lookup out would be useful there. We could
> have (names to be bikeshedded)
>
> - __drm_of_find_panel_or_bridge() without of_graph_get_remote_node()
> - drm_of_find_panel_or_bridge() calling of_graph_get_remote_node() and
> __drm_of_find_panel_or_bridge()
> - drm_of_find_dsi_panel_or_bridge() getting the device_node pointer in a
> way specific to DSI devices and calling
> __drm_of_find_panel_or_bridge()
I don't really like the idea of a DSI helper either. Those node names
are reserved so I'm not sure we'll ever find a conflict, but can we base
our decision on remote-endpoint (for ports/endpoints) or reg (for DSI
devices)?
> Ideally, though, the case where we have no port node should die out
> slowly, even when DSI devices are children of the DSI controller, there
> should be ports modelling the data connection.
I'm not really in favor of that either, it looks like making the DT more
complex than it needs to be for no particular reason, but I guess it's a
very subjective matter :)
Maxime
Attachment:
signature.asc
Description: PGP signature
