On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
> On 3/10/22 11:53, Maxime Ripard wrote:
> > On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> > > On 3/8/22 17:21, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 14:49, Maxime Ripard wrote:
> > > > > > 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@xxxxxxx> 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.
> > > > >
> > > > > I can because the upstream DT bindings currently say port@0 must be present,
> > > > > see above. If that requirement should be relaxed, sure, but that's a
> > > > > separate series.
> > > >
> > > > And another upstream DT bindings say that you don't need them at all.
> > >
> > > Which "another upstream DT bindings" do you refer to ?
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >
> > > > Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> > > > binding is far more relevant than a single bridge driver.
> > >
> > > That seems to be the wrong way around, how can generic subsystem-wide
> > > binding take precedence over specific driver binding ?
> >
> > This is the binding of the bus. You're part of that bus. You're a child
> > node of that bus, but nothing ever mandates that your parent node uses
> > the same convention. And some don't. And since your bridge can be
> > connected to pretty much any DSI controller, you have to use the lowest
> > common denominator, not make up some new constraints that not all
> > controller will be able to comply with.
>
> It seems to me the ICN6211 DT bindings currently further constraint the
> generic DSI bus bindings, and that seems OK to me.
>
> Let me reiterate this again -- if someone wants to relax the requirements
> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
> a separate patchset AND that needs DT bindings update. Furthermore, there
> are no users of this ICN6211 bridge in upstream DTs, so there is currently
> no bridge which would operate without OF graph using this driver.
And let me reiterate this again: something that used to work for a user
doesn't anymore when your series is applied. This is a textbook
regression. I suggested a way forward, that you don't like for some
reason, fine. But pushing through a regression is just not acceptable.
> > > > So figuring it out is very much a prerequisite to that series,
> > > > especially since those patches effectively make the OF-graph mandatory
> > > > in some situations, while it was purely aesthetics before.
> > >
> > > The OF-graph is mandatory per the DT bindings of this driver. If you
> > > implement invalid DT which does not contain port@0, it will fail DT
> > > validation.
> > >
> > > If this requirement should be relaxed, sure, it can and I don't think it
> > > would be hard to do, but I don't see why that should be part of this series,
> > > which follows the upstream DT binding document for this driver.
> > >
> > > If I cannot trust the driver DT bindings to indicate what is and is not
> > > mandatory, what other document can I trust then ...
> >
> > Oh, come on. Doing that, you also require OF-Graph support for the DSI
> > controller you attach to, and you can't require that. This is very
> > different from just requiring a property that doesn't have any impact on
> > any other device, and you know that very well.
>
> Currently the ICN6211 DT bindings DO require that kind of bridge.
And while this wasn't enforced before, you make it a hard requirement
with this series. This is what changed, and what caused this whole
discussion.
Maxime
Attachment:
signature.asc
Description: PGP signature
