Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3554 bytes --]

On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> On 3/8/22 13:51, Maxime Ripard wrote:
> > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > 
> > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > >     static int chipone_parse_dt(struct chipone *icn)
> > > > > > >     {
> > > > > > >            struct device *dev = icn->dev;
> > > > > > > +       struct device_node *endpoint;
> > > > > > >            struct drm_panel *panel;
> > > > > > > +       int dsi_lanes;
> > > > > > >            int ret;
> > > > > > > 
> > > > > > >            icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > >                    return PTR_ERR(icn->enable_gpio);
> > > > > > >            }
> > > > > > > 
> > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > +       of_node_put(endpoint);
> > > > > > > +
> > > > > > > +       if (!icn->host_node)
> > > > > > > +               return -ENODEV;
> > > > > > 
> > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > Display pipeline in R16 [1].
> > > > > > 
> > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > done it for drm_of_find_bridge.
> > > > > > 
> > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > 
> > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > required, see:
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > 
> > > > > What is "non-ports-based OF graph" ?
> > > > > 
> > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > 
> > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > has to fix it to make port@0 optional)
> > > 
> > > The current upstream DT binding document says:
> > > 
> > >      required:
> > >        - port@0
> > >        - port@1
> > > 
> > > So port@0 is mandatory.
> > 
> > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > correct. If the bridge supports DCS, there's no reason to use the OF
> > graph in the first place: the bridge node will be a child node of the
> > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > MIPI-DSI controller).
> > 
> > I believe port@0 should be made optional (or downright removed if
> > MIPI-DCS in the only control bus).
> 
> That's out of scope of this series anyway, so Jagan can implement patches
> for that mode if needed.

Not really? You can't count on the port@0 being there generally
speaking, so you can't count on data-lanes being there either, which
exactly what you're doing in this patch.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]