| Message ID | 20220304002508.75676-6-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: bridge: icn6211: Fix hard-coded panel settings and add I2C support | expand |
Hi Marek, On Fri, Mar 4, 2022 at 5:55 AM Marek Vasut <marex@denx.de> wrote: > > The driver currently hard-codes DSI lane count to two, however the chip > is capable of operating in 1..4 DSI lanes mode. Parse 'data-lanes' DT > property and program the result into DSI_CTRL register. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Maxime Ripard <maxime@cerno.tech> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > To: dri-devel@lists.freedesktop.org > --- > V2: Rebase on next-20220214 > V3: Default to 4 data lanes unless specified otherwise > --- > drivers/gpu/drm/bridge/chipone-icn6211.c | 45 +++++++++++++++++++++--- > 1 file changed, 41 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c > index 2ac8eb7e25f52..df8e75a068ad0 100644 > --- a/drivers/gpu/drm/bridge/chipone-icn6211.c > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c > @@ -136,10 +136,12 @@ struct chipone { > struct drm_bridge bridge; > struct drm_display_mode mode; > struct drm_bridge *panel_bridge; > + struct device_node *host_node; > struct gpio_desc *enable_gpio; > struct regulator *vdd1; > struct regulator *vdd2; > struct regulator *vdd3; > + int dsi_lanes; > }; > > static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge) > @@ -212,6 +214,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge, > /* dsi specific sequence */ > ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80); > ICN6211_DSI(icn, HFP_MIN, hfp & 0xff); > + > + /* DSI data lane count */ > + ICN6211_DSI(icn, DSI_CTRL, > + DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1)); > + > ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0); > ICN6211_DSI(icn, PLL_CTRL(12), 0xff); > > @@ -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/ Thanks, Jagan.
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 ?
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) Example OF-graph is on the commit message. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/drm_of.c?id=80253168dbfd256bca97cf7f13312863c5a7f2e5 Jagan.
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. So I don't think any change is needed to this patch ? > Example OF-graph is on the commit message. > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/drm_of.c?id=80253168dbfd256bca97cf7f13312863c5a7f2e5 It seems the current upstream DT bindings only support DSI hosts which do provide an endpoint, because port@0 is required per DT binding document. If you want to support the other options as listed in the aforementioned commit, then you would have to extend this driver and its bindings, but that is out of scope of this series.
Hi Marek, On Tue, Mar 8, 2022 at 4:00 PM Marek Vasut <marex@denx.de> 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. > > So I don't think any change is needed to this patch ? > > > Example OF-graph is on the commit message. > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/drm_of.c?id=80253168dbfd256bca97cf7f13312863c5a7f2e5 > > It seems the current upstream DT bindings only support DSI hosts which > do provide an endpoint, because port@0 is required per DT binding > document. If you want to support the other options as listed in the > aforementioned commit, then you would have to extend this driver and its > bindings, but that is out of scope of this series. Your comments seem to be valid, thanks. Jagan.
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). Maxime
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.
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
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@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. 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.
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@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. > > 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. Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI binding is far more relevant than a single bridge driver. 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. Maxime
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@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. >> >> 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 ? > 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 ? > 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 ...
Hi Maxime, On Tue, Mar 08, 2022 at 01:51:40PM +0100, 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). I think we should make ports mandatory in all cases actually. The DT parent-child hierarchy is meant to model control relations between devices, so a DSI device controlled through DCS should be a child of the DSI controller. No disagreement there. The OF graph is meant to model data connections. While a DSI device controlled through DCS will use the same DSI link for data transfer, the two concepts are different. We have taken shortcuts and decided to not use OF graph for some DSI devices (not necessarily as a well thought decision, it was sometimes just not considered). This has led to different issues that we're having to deal with today, making it more difficult to develop generic code. Going forward, I think new bindings should always use OF graph to model the data connection.
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@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. > > > > > > 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. > > 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. Anyway, you wanted my review. You have it. Do what you want with it. Maxime
On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote: > Hi Maxime, > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, 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). > > I think we should make ports mandatory in all cases actually. > > The DT parent-child hierarchy is meant to model control relations > between devices, so a DSI device controlled through DCS should be a > child of the DSI controller. No disagreement there. > > The OF graph is meant to model data connections. While a DSI device > controlled through DCS will use the same DSI link for data transfer, the > two concepts are different. We have taken shortcuts and decided to not > use OF graph for some DSI devices (not necessarily as a well thought > decision, it was sometimes just not considered). I disagree. Unless the data path is explicitly stated using the OF-graph or some other binding, it's inferred. We never asked ourselves where the data from an i2c chip, an ethernet controller or an v4l2 output device was coming from. It comes from the parent bus, because it's what makes sense. Making a requirement on the OF-Graph to model this would create a big inconsistency. > This has led to different issues that we're having to deal with today, > making it more difficult to develop generic code. Going forward, I > think new bindings should always use OF graph to model the data > connection. Either way, that discussion is irrelevant. Not all DSI controllers use OF-Graph, a bridge can be attached to any of them, so we can't require OF-Graph support in any bridge. Maxime
Hi Maxime, On Thu, Mar 10, 2022 at 11:57:38AM +0100, Maxime Ripard wrote: > On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote: > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, 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). > > > > I think we should make ports mandatory in all cases actually. > > > > The DT parent-child hierarchy is meant to model control relations > > between devices, so a DSI device controlled through DCS should be a > > child of the DSI controller. No disagreement there. > > > > The OF graph is meant to model data connections. While a DSI device > > controlled through DCS will use the same DSI link for data transfer, the > > two concepts are different. We have taken shortcuts and decided to not > > use OF graph for some DSI devices (not necessarily as a well thought > > decision, it was sometimes just not considered). > > I disagree. Unless the data path is explicitly stated using the OF-graph > or some other binding, it's inferred. It is today, and for video data, I think it's showing to be a problem :-) > We never asked ourselves where the > data from an i2c chip, an ethernet controller or an v4l2 output device > was coming from. It comes from the parent bus, because it's what makes > sense. Making a requirement on the OF-Graph to model this would create a > big inconsistency. I'm afraid I disagree, especially when it comes to data transfers from device to device. The device tree has never tried to model those until OF graph. > > This has led to different issues that we're having to deal with today, > > making it more difficult to develop generic code. Going forward, I > > think new bindings should always use OF graph to model the data > > connection. > > Either way, that discussion is irrelevant. Not all DSI controllers use > OF-Graph, a bridge can be attached to any of them, so we can't require > OF-Graph support in any bridge. Not in any bridge, but we could in new ones, and we could also require it in new DT for existing bridge to support new features.
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@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. >>>> >>>> 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. >>> 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. The new ad-hoc requirement to support operation with invalid DT bindings is something I don't agree with, sorry. But there is a way to cater for this new requirement, see above. It just won't be in this series, because I have no such bridge and I cannot test it in any way, so someone else has to do that. > Anyway, you wanted my review. You have it. Do what you want with it. I see.
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@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. > > > > > > > > > > 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
On Thu, Mar 10, 2022 at 01:16:57PM +0200, Laurent Pinchart wrote: > On Thu, Mar 10, 2022 at 11:57:38AM +0100, Maxime Ripard wrote: > > On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote: > > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, 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). > > > > > > I think we should make ports mandatory in all cases actually. > > > > > > The DT parent-child hierarchy is meant to model control relations > > > between devices, so a DSI device controlled through DCS should be a > > > child of the DSI controller. No disagreement there. > > > > > > The OF graph is meant to model data connections. While a DSI device > > > controlled through DCS will use the same DSI link for data transfer, the > > > two concepts are different. We have taken shortcuts and decided to not > > > use OF graph for some DSI devices (not necessarily as a well thought > > > decision, it was sometimes just not considered). > > > > I disagree. Unless the data path is explicitly stated using the OF-graph > > or some other binding, it's inferred. > > It is today, and for video data, I think it's showing to be a problem > :-) > > > We never asked ourselves where the > > data from an i2c chip, an ethernet controller or an v4l2 output device > > was coming from. It comes from the parent bus, because it's what makes > > sense. Making a requirement on the OF-Graph to model this would create a > > big inconsistency. > > I'm afraid I disagree, especially when it comes to data transfers from > device to device. The device tree has never tried to model those until > OF graph. Except the data transfer isn't happening between "siblings" devices like a CRTC and an HDMI controller that the OF-Graph describes perfectly fine. Here the transfer happens between a parent device and its child. So it's a bus to device transfer, just like any DMA transfer, or MDIO transfer, or SPI transfer, etc. > > > This has led to different issues that we're having to deal with today, > > > making it more difficult to develop generic code. Going forward, I > > > think new bindings should always use OF graph to model the data > > > connection. > > > > Either way, that discussion is irrelevant. Not all DSI controllers use > > OF-Graph, a bridge can be attached to any of them, so we can't require > > OF-Graph support in any bridge. > > Not in any bridge, but we could in new ones, and we could also require > it in new DT for existing bridge to support new features. Above disagreement aside, since not all controllers are using it, it would effectively prevent those new bridges from being used with !OF-Graph controllers. I don't see how that can be an option. Maxime
On 3/10/22 15:18, Maxime Ripard wrote: > 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@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. >>>>>> >>>>>> 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. How can this be a regression if this mode of operation could not have ever been supported with valid upstream DT bindings in the first place ? Should we now require that kernel drivers somehow magically support all kinds of random broken DT bindings in addition to ones which pass YAML DT validation ? >>>>> 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. The current DT bindings already make it a hard requirement, so no, nothing changed here. Unless what you are trying to ask for is support for broken DT bindings which do not pass YAML DT validation by this driver, but that is very dangerous, because then the question is, how far should we support such broken bindings. What kind of broken is still OK and what kind of broken is no longer OK ?
On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote: > On 3/10/22 15:18, Maxime Ripard wrote: > > 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@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. > > > > > > > > > > > > > > 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. > > How can this be a regression if this mode of operation could not have ever > been supported with valid upstream DT bindings in the first place ? > > Should we now require that kernel drivers somehow magically support all > kinds of random broken DT bindings in addition to ones which pass YAML DT > validation ? The thing is, as I told you multiple times already, it was broken from the bridge standpoint, but not from the controller's. If it had been correct for the bridge, it wouldn't have been for the controller. So, same story. The only difference is that it wouldn't affect you, but I don't see how it's relevant. > > > > > > 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. > > The current DT bindings already make it a hard requirement, so no, nothing > changed here. > > Unless what you are trying to ask for is support for broken DT bindings > which do not pass YAML DT validation by this driver, but that is very > dangerous, because then the question is, how far should we support such > broken bindings. What kind of broken is still OK and what kind of broken is > no longer OK ? If it ever worked in a mainline release, it must always work. See: https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html As far as I'm concerned, it's the sole criteria. So to answer your question, if it was broken but worked at some point, yes, we need to keep supporting it. If it never worked, no, we don't. Honestly, I don't get the push-back. To fix this properly would require: - to remove port@0 being mandatory - to move the data-lanes property to the bridge node itself. That's it. It takes 5 minutes, 30 minutes with the test and the commit log. We've spent more time arguing about it already. So if you want to continue debating on whether it's a regression or not, or whether DT ABI stability is a good thing or not, go ahead. But as far as I'm concerned, this isn't really up for debate. Maxime
On 3/11/22 17:29, Maxime Ripard wrote: > On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote: >> On 3/10/22 15:18, Maxime Ripard wrote: >>> 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@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. >>>>>>>> >>>>>>>> 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. >> >> How can this be a regression if this mode of operation could not have ever >> been supported with valid upstream DT bindings in the first place ? >> >> Should we now require that kernel drivers somehow magically support all >> kinds of random broken DT bindings in addition to ones which pass YAML DT >> validation ? > > The thing is, as I told you multiple times already, it was broken from > the bridge standpoint, but not from the controller's. If it had been > correct for the bridge, it wouldn't have been for the controller. So, > same story. > > The only difference is that it wouldn't affect you, but I don't see how > it's relevant. I'm sorry, I do not understand this answer. >>>>>>> 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. >> >> The current DT bindings already make it a hard requirement, so no, nothing >> changed here. >> >> Unless what you are trying to ask for is support for broken DT bindings >> which do not pass YAML DT validation by this driver, but that is very >> dangerous, because then the question is, how far should we support such >> broken bindings. What kind of broken is still OK and what kind of broken is >> no longer OK ? > > If it ever worked in a mainline release, it must always work. See: > https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html > As far as I'm concerned, it's the sole criteria. So to answer your > question, if it was broken but worked at some point, yes, we need to > keep supporting it. If it never worked, no, we don't. There are no users of this driver in any mainline release. DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this driver worked with some broken downstream DT without port@0, then that downstream depended on undefined behavior which I cannot fathom how it can be considered part of kernel ABI. That downstream should fix its DT instead. > Honestly, I don't get the push-back. Because what I am being asked to do here is implement some sort of undefined behavior backward compatibility. For behavior which has no users. Worse, for behavior which can only be triggered if your DT is broken and does not even pass the YAML DT validation. That kind of new requirement seems bonkers to me. > To fix this properly would require: > - to remove port@0 being mandatory > - to move the data-lanes property to the bridge node itself. That does not work, data-lanes is port property. > That's it. It takes 5 minutes, 30 minutes with the test and the commit > log. We've spent more time arguing about it already. How can I test this bug compatibility requirement ? I don't have a broken DT, my DT does pass the YAML DT validation, so should I explicitly break my DT to perform the test ? Are we going to start asking people to implement support for randomly broken DTs in their drivers during code review too ? > So if you want to continue debating on whether it's a regression or not, > or whether DT ABI stability is a good thing or not, go ahead. But as far > as I'm concerned, this isn't really up for debate. As far as I can tell, undefined behavior can hardly be part of stable ABI. [...]
Hi Marek, On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut <marex@denx.de> wrote: > > On 3/11/22 17:29, Maxime Ripard wrote: > > On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote: > >> On 3/10/22 15:18, Maxime Ripard wrote: > >>> 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@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. > >>>>>>>> > >>>>>>>> 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. > >> > >> How can this be a regression if this mode of operation could not have ever > >> been supported with valid upstream DT bindings in the first place ? > >> > >> Should we now require that kernel drivers somehow magically support all > >> kinds of random broken DT bindings in addition to ones which pass YAML DT > >> validation ? > > > > The thing is, as I told you multiple times already, it was broken from > > the bridge standpoint, but not from the controller's. If it had been > > correct for the bridge, it wouldn't have been for the controller. So, > > same story. > > > > The only difference is that it wouldn't affect you, but I don't see how > > it's relevant. > > I'm sorry, I do not understand this answer. > > >>>>>>> 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. > >> > >> The current DT bindings already make it a hard requirement, so no, nothing > >> changed here. > >> > >> Unless what you are trying to ask for is support for broken DT bindings > >> which do not pass YAML DT validation by this driver, but that is very > >> dangerous, because then the question is, how far should we support such > >> broken bindings. What kind of broken is still OK and what kind of broken is > >> no longer OK ? > > > > If it ever worked in a mainline release, it must always work. See: > > https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html > > > As far as I'm concerned, it's the sole criteria. So to answer your > > question, if it was broken but worked at some point, yes, we need to > > keep supporting it. If it never worked, no, we don't. > > There are no users of this driver in any mainline release. > > DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this > driver worked with some broken downstream DT without port@0, then that > downstream depended on undefined behavior which I cannot fathom how it > can be considered part of kernel ABI. That downstream should fix its DT > instead. Yes, agreed that ICN6211 DT bindings say port@0 is mandatory. However, marking port@0 (after fixing DT binding) with non-I2C-ICN6211 is still valid and we can see many examples of adding a bridge as a child to the DSI host (without port@0) and it is not broken DT binding at all (at least when it comes to DSI). You are correct and port@0 is mandatory for I2C-ICN6211. Since the driver and this series trying to support the non-I2C and I2C based ICN6211 bridge. I think it is valid to make changes according to this series - IMHO. Thanks, Jagan.
Hi Marek, Small correction in the previous comment. On Sat, Mar 12, 2022 at 2:05 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > Hi Marek, > > On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut <marex@denx.de> wrote: > > > > On 3/11/22 17:29, Maxime Ripard wrote: > > > On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote: > > >> On 3/10/22 15:18, Maxime Ripard wrote: > > >>> 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@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. > > >>>>>>>> > > >>>>>>>> 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. > > >> > > >> How can this be a regression if this mode of operation could not have ever > > >> been supported with valid upstream DT bindings in the first place ? > > >> > > >> Should we now require that kernel drivers somehow magically support all > > >> kinds of random broken DT bindings in addition to ones which pass YAML DT > > >> validation ? > > > > > > The thing is, as I told you multiple times already, it was broken from > > > the bridge standpoint, but not from the controller's. If it had been > > > correct for the bridge, it wouldn't have been for the controller. So, > > > same story. > > > > > > The only difference is that it wouldn't affect you, but I don't see how > > > it's relevant. > > > > I'm sorry, I do not understand this answer. > > > > >>>>>>> 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. > > >> > > >> The current DT bindings already make it a hard requirement, so no, nothing > > >> changed here. > > >> > > >> Unless what you are trying to ask for is support for broken DT bindings > > >> which do not pass YAML DT validation by this driver, but that is very > > >> dangerous, because then the question is, how far should we support such > > >> broken bindings. What kind of broken is still OK and what kind of broken is > > >> no longer OK ? > > > > > > If it ever worked in a mainline release, it must always work. See: > > > https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html > > > > > As far as I'm concerned, it's the sole criteria. So to answer your > > > question, if it was broken but worked at some point, yes, we need to > > > keep supporting it. If it never worked, no, we don't. > > > > There are no users of this driver in any mainline release. > > > > DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this > > driver worked with some broken downstream DT without port@0, then that > > downstream depended on undefined behavior which I cannot fathom how it > > can be considered part of kernel ABI. That downstream should fix its DT > > instead. > > Yes, agreed that ICN6211 DT bindings say port@0 is mandatory. However, > marking port@0 (after fixing DT binding) with non-I2C-ICN6211 is still s/port@0/port@0 as optional/
On 3/11/22 21:38, Jagan Teki wrote: > Hi Marek, > > Small correction in the previous comment. > > On Sat, Mar 12, 2022 at 2:05 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >> >> Hi Marek, >> >> On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut <marex@denx.de> wrote: >>> >>> On 3/11/22 17:29, Maxime Ripard wrote: >>>> On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote: >>>>> On 3/10/22 15:18, Maxime Ripard wrote: >>>>>> 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@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. >>>>>>>>>>> >>>>>>>>>>> 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. >>>>> >>>>> How can this be a regression if this mode of operation could not have ever >>>>> been supported with valid upstream DT bindings in the first place ? >>>>> >>>>> Should we now require that kernel drivers somehow magically support all >>>>> kinds of random broken DT bindings in addition to ones which pass YAML DT >>>>> validation ? >>>> >>>> The thing is, as I told you multiple times already, it was broken from >>>> the bridge standpoint, but not from the controller's. If it had been >>>> correct for the bridge, it wouldn't have been for the controller. So, >>>> same story. >>>> >>>> The only difference is that it wouldn't affect you, but I don't see how >>>> it's relevant. >>> >>> I'm sorry, I do not understand this answer. >>> >>>>>>>>>> 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. >>>>> >>>>> The current DT bindings already make it a hard requirement, so no, nothing >>>>> changed here. >>>>> >>>>> Unless what you are trying to ask for is support for broken DT bindings >>>>> which do not pass YAML DT validation by this driver, but that is very >>>>> dangerous, because then the question is, how far should we support such >>>>> broken bindings. What kind of broken is still OK and what kind of broken is >>>>> no longer OK ? >>>> >>>> If it ever worked in a mainline release, it must always work. See: >>>> https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html >>> >>>> As far as I'm concerned, it's the sole criteria. So to answer your >>>> question, if it was broken but worked at some point, yes, we need to >>>> keep supporting it. If it never worked, no, we don't. >>> >>> There are no users of this driver in any mainline release. >>> >>> DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this >>> driver worked with some broken downstream DT without port@0, then that >>> downstream depended on undefined behavior which I cannot fathom how it >>> can be considered part of kernel ABI. That downstream should fix its DT >>> instead. >> >> Yes, agreed that ICN6211 DT bindings say port@0 is mandatory. However, >> marking port@0 (after fixing DT binding) with non-I2C-ICN6211 is still > > s/port@0/port@0 as optional/ The V4 requires the port@0 only for I2C bus option now, that should at least retain the "compatibility".
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index 2ac8eb7e25f52..df8e75a068ad0 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -136,10 +136,12 @@ struct chipone { struct drm_bridge bridge; struct drm_display_mode mode; struct drm_bridge *panel_bridge; + struct device_node *host_node; struct gpio_desc *enable_gpio; struct regulator *vdd1; struct regulator *vdd2; struct regulator *vdd3; + int dsi_lanes; }; static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge) @@ -212,6 +214,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge, /* dsi specific sequence */ ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80); ICN6211_DSI(icn, HFP_MIN, hfp & 0xff); + + /* DSI data lane count */ + ICN6211_DSI(icn, DSI_CTRL, + DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1)); + ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0); ICN6211_DSI(icn, PLL_CTRL(12), 0xff); @@ -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; + + /* + * If the 'data-lanes' property does not exist in DT or is invalid, + * default to previously hard-coded behavior, which was 4 data lanes. + */ + if (dsi_lanes < 0) { + icn->dsi_lanes = 4; + } else if (dsi_lanes > 4) { + ret = -EINVAL; + goto err_data_lanes; + } else { + icn->dsi_lanes = dsi_lanes; + } + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); if (ret) - return ret; + goto err_data_lanes; icn->panel_bridge = devm_drm_panel_bridge_add(dev, panel); - if (IS_ERR(icn->panel_bridge)) - return PTR_ERR(icn->panel_bridge); + if (IS_ERR(icn->panel_bridge)) { + ret = PTR_ERR(icn->panel_bridge); + goto err_data_lanes; + } return 0; + +err_data_lanes: + of_node_put(icn->host_node); + return ret; } static int chipone_probe(struct mipi_dsi_device *dsi) @@ -384,7 +420,7 @@ static int chipone_probe(struct mipi_dsi_device *dsi) drm_bridge_add(&icn->bridge); - dsi->lanes = 4; + dsi->lanes = icn->dsi_lanes; dsi->format = MIPI_DSI_FMT_RGB888; dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE; @@ -403,6 +439,7 @@ static int chipone_remove(struct mipi_dsi_device *dsi) mipi_dsi_detach(dsi); drm_bridge_remove(&icn->bridge); + of_node_put(icn->host_node); return 0; }
The driver currently hard-codes DSI lane count to two, however the chip is capable of operating in 1..4 DSI lanes mode. Parse 'data-lanes' DT property and program the result into DSI_CTRL register. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Maxime Ripard <maxime@cerno.tech> Cc: Robert Foss <robert.foss@linaro.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> To: dri-devel@lists.freedesktop.org --- V2: Rebase on next-20220214 V3: Default to 4 data lanes unless specified otherwise --- drivers/gpu/drm/bridge/chipone-icn6211.c | 45 +++++++++++++++++++++--- 1 file changed, 41 insertions(+), 4 deletions(-)