Hi Marek,
On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut <marex@xxxxxxx> wrote:
>
> On 3/11/22 17:29, Maxime Ripard wrote:
> > On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote:
> >> On 3/10/22 15:18, Maxime Ripard wrote:
> >>> On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
> >>>> On 3/10/22 11:53, Maxime Ripard wrote:
> >>>>> On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> >>>>>> On 3/8/22 17:21, Maxime Ripard wrote:
> >>>>>>> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> >>>>>>>> On 3/8/22 14:49, Maxime Ripard wrote:
> >>>>>>>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> >>>>>>>>>> On 3/8/22 13:51, Maxime Ripard wrote:
> >>>>>>>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> >>>>>>>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
> >>>>>>>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@xxxxxxx> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [...]
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> >>>>>>>>>>>>>>>> static int chipone_parse_dt(struct chipone *icn)
> >>>>>>>>>>>>>>>> {
> >>>>>>>>>>>>>>>> struct device *dev = icn->dev;
> >>>>>>>>>>>>>>>> + struct device_node *endpoint;
> >>>>>>>>>>>>>>>> struct drm_panel *panel;
> >>>>>>>>>>>>>>>> + int dsi_lanes;
> >>>>>>>>>>>>>>>> int ret;
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> >>>>>>>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> >>>>>>>>>>>>>>>> return PTR_ERR(icn->enable_gpio);
> >>>>>>>>>>>>>>>> }
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> >>>>>>>>>>>>>>>> + dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> >>>>>>>>>>>>>>>> + icn->host_node = of_graph_get_remote_port_parent(endpoint);
> >>>>>>>>>>>>>>>> + of_node_put(endpoint);
> >>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>> + if (!icn->host_node)
> >>>>>>>>>>>>>>>> + return -ENODEV;
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
> >>>>>>>>>>>>>>> Display pipeline in R16 [1].
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
> >>>>>>>>>>>>>>> done it for drm_of_find_bridge.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
> >>>>>>>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> >>>>>>>>>>>>>> required, see:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> What is "non-ports-based OF graph" ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
> >>>>>>>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
> >>>>>>>>>>>>> has to fix it to make port@0 optional)
> >>>>>>>>>>>>
> >>>>>>>>>>>> The current upstream DT binding document says:
> >>>>>>>>>>>>
> >>>>>>>>>>>> required:
> >>>>>>>>>>>> - port@0
> >>>>>>>>>>>> - port@1
> >>>>>>>>>>>>
> >>>>>>>>>>>> So port@0 is mandatory.
> >>>>>>>>>>>
> >>>>>>>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> >>>>>>>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
> >>>>>>>>>>> graph in the first place: the bridge node will be a child node of the
> >>>>>>>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> >>>>>>>>>>> MIPI-DSI controller).
> >>>>>>>>>>>
> >>>>>>>>>>> I believe port@0 should be made optional (or downright removed if
> >>>>>>>>>>> MIPI-DCS in the only control bus).
> >>>>>>>>>>
> >>>>>>>>>> That's out of scope of this series anyway, so Jagan can implement patches
> >>>>>>>>>> for that mode if needed.
> >>>>>>>>>
> >>>>>>>>> Not really? You can't count on the port@0 being there generally
> >>>>>>>>> speaking, so you can't count on data-lanes being there either, which
> >>>>>>>>> exactly what you're doing in this patch.
> >>>>>>>>
> >>>>>>>> I can because the upstream DT bindings currently say port@0 must be present,
> >>>>>>>> see above. If that requirement should be relaxed, sure, but that's a
> >>>>>>>> separate series.
> >>>>>>>
> >>>>>>> And another upstream DT bindings say that you don't need them at all.
> >>>>>>
> >>>>>> Which "another upstream DT bindings" do you refer to ?
> >>>>>
> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>>>
> >>>>>>> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> >>>>>>> binding is far more relevant than a single bridge driver.
> >>>>>>
> >>>>>> That seems to be the wrong way around, how can generic subsystem-wide
> >>>>>> binding take precedence over specific driver binding ?
> >>>>>
> >>>>> This is the binding of the bus. You're part of that bus. You're a child
> >>>>> node of that bus, but nothing ever mandates that your parent node uses
> >>>>> the same convention. And some don't. And since your bridge can be
> >>>>> connected to pretty much any DSI controller, you have to use the lowest
> >>>>> common denominator, not make up some new constraints that not all
> >>>>> controller will be able to comply with.
> >>>>
> >>>> It seems to me the ICN6211 DT bindings currently further constraint the
> >>>> generic DSI bus bindings, and that seems OK to me.
> >>>>
> >>>> Let me reiterate this again -- if someone wants to relax the requirements
> >>>> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
> >>>> a separate patchset AND that needs DT bindings update. Furthermore, there
> >>>> are no users of this ICN6211 bridge in upstream DTs, so there is currently
> >>>> no bridge which would operate without OF graph using this driver.
> >>>
> >>> And let me reiterate this again: something that used to work for a user
> >>> doesn't anymore when your series is applied. This is a textbook
> >>> regression. I suggested a way forward, that you don't like for some
> >>> reason, fine. But pushing through a regression is just not acceptable.
> >>
> >> How can this be a regression if this mode of operation could not have ever
> >> been supported with valid upstream DT bindings in the first place ?
> >>
> >> Should we now require that kernel drivers somehow magically support all
> >> kinds of random broken DT bindings in addition to ones which pass YAML DT
> >> validation ?
> >
> > The thing is, as I told you multiple times already, it was broken from
> > the bridge standpoint, but not from the controller's. If it had been
> > correct for the bridge, it wouldn't have been for the controller. So,
> > same story.
> >
> > The only difference is that it wouldn't affect you, but I don't see how
> > it's relevant.
>
> I'm sorry, I do not understand this answer.
>
> >>>>>>> So figuring it out is very much a prerequisite to that series,
> >>>>>>> especially since those patches effectively make the OF-graph mandatory
> >>>>>>> in some situations, while it was purely aesthetics before.
> >>>>>>
> >>>>>> The OF-graph is mandatory per the DT bindings of this driver. If you
> >>>>>> implement invalid DT which does not contain port@0, it will fail DT
> >>>>>> validation.
> >>>>>>
> >>>>>> If this requirement should be relaxed, sure, it can and I don't think it
> >>>>>> would be hard to do, but I don't see why that should be part of this series,
> >>>>>> which follows the upstream DT binding document for this driver.
> >>>>>>
> >>>>>> If I cannot trust the driver DT bindings to indicate what is and is not
> >>>>>> mandatory, what other document can I trust then ...
> >>>>>
> >>>>> Oh, come on. Doing that, you also require OF-Graph support for the DSI
> >>>>> controller you attach to, and you can't require that. This is very
> >>>>> different from just requiring a property that doesn't have any impact on
> >>>>> any other device, and you know that very well.
> >>>>
> >>>> Currently the ICN6211 DT bindings DO require that kind of bridge.
> >>>
> >>> And while this wasn't enforced before, you make it a hard requirement
> >>> with this series. This is what changed, and what caused this whole
> >>> discussion.
> >>
> >> The current DT bindings already make it a hard requirement, so no, nothing
> >> changed here.
> >>
> >> Unless what you are trying to ask for is support for broken DT bindings
> >> which do not pass YAML DT validation by this driver, but that is very
> >> dangerous, because then the question is, how far should we support such
> >> broken bindings. What kind of broken is still OK and what kind of broken is
> >> no longer OK ?
> >
> > If it ever worked in a mainline release, it must always work. See:
> > https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html
>
> > As far as I'm concerned, it's the sole criteria. So to answer your
> > question, if it was broken but worked at some point, yes, we need to
> > keep supporting it. If it never worked, no, we don't.
>
> There are no users of this driver in any mainline release.
>
> DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this
> driver worked with some broken downstream DT without port@0, then that
> downstream depended on undefined behavior which I cannot fathom how it
> can be considered part of kernel ABI. That downstream should fix its DT
> instead.
Yes, agreed that ICN6211 DT bindings say port@0 is mandatory. However,
marking port@0 (after fixing DT binding) with non-I2C-ICN6211 is still
valid and we can see many examples of adding a bridge as a child to
the DSI host (without port@0) and it is not broken DT binding at all
(at least when it comes to DSI). You are correct and port@0 is
mandatory for I2C-ICN6211.
Since the driver and this series trying to support the non-I2C and I2C
based ICN6211 bridge. I think it is valid to make changes according to
this series - IMHO.
Thanks,
Jagan.
