Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 wrote:
Hi Marek,
On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut 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 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
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
Hi Marek,
Small correction in the previous comment.
On Sat, Mar 12, 2022 at 2:05 AM Jagan Teki wrote:
>
> Hi Marek,
>
> On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut 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
> > > 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
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
Hi Marek,
On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut 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
> > 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
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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
> > > > > > > > > > > > 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
>
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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
> > > > > > > > > > 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
> >
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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,
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
signature.asc
Description: PGP signature
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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
> > > > > > > > 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 ?
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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.
--
Regards,
Laurent Pinchart
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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 ...
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
signature.asc
Description: PGP signature
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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.
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
signature.asc
Description: PGP signature
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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.
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 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
signature.asc
Description: PGP signature
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
Hi Marek,
On Tue, Mar 8, 2022 at 4:00 PM Marek Vasut wrote:
>
> On 3/8/22 11:07, Jagan Teki wrote:
> > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut 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.
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
On 3/8/22 11:07, Jagan Teki wrote:
On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut 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.
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut 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.
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
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 ?
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
Hi Marek,
On Fri, Mar 4, 2022 at 5:55 AM Marek Vasut 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
> Cc: Jagan Teki
> Cc: Maxime Ripard
> Cc: Robert Foss
> Cc: Sam Ravnborg
> Cc: Thomas Zimmermann
> 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.
