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

[Marek Vasut]

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 at 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 at 0 OF graph link ? Both port at 0 and port at 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 at 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 at 0 optional)
>>>>>>
>>>>>> The current upstream DT binding document says:
>>>>>>
>>>>>>        required:
>>>>>>          - port at 0
>>>>>>          - port at 1
>>>>>>
>>>>>> So port at 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 at 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 at 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 at 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 at 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 ...