Message-ID: <56016787-0487-1571-7c28-2767033cda23@axentia.se>
Date:   Tue, 14 Aug 2018 17:05:29 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     Mark Rutland <mark.rutland@....com>, devicetree@...r.kernel.org,
        jmondi <jacopo@...ndi.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        David Airlie <airlied@...ux.ie>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Boris Brezillon <boris.brezillon@...tlin.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Jyri Sarha <jsarha@...com>, Daniel Vetter <daniel@...ll.ch>,
        Russell King <linux@...linux.org.uk>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v8 3/4] drm/atmel-hlcdc: iterate over all output endpoints

On 2018-08-14 16:33, Rob Herring wrote:
> On Tue, Aug 14, 2018 at 12:36 AM Peter Rosin <peda@...ntia.se> wrote:
>>
>> On 2018-08-13 22:52, Rob Herring wrote:
>>> On Mon, Aug 13, 2018 at 8:25 AM Peter Rosin <peda@...ntia.se> wrote:
>>>>
>>>> On 2018-08-13 15:59, jacopo mondi wrote:
>>>>> Hi Peter,
>>>>>
>>>>> On Fri, Aug 10, 2018 at 03:03:58PM +0200, Peter Rosin wrote:
>>>>>> This enables more flexible devicetrees. You can e.g. have two output
>>>>>> nodes where one is not enabled, without the ordering affecting things.
>>>
>>> Your DTs are not supposed to be flexible. They should be well defined
>>> by the binding.
>>
>> I feel the need to explain the situation with more words...
>>
>> We have a board with this atmel-hlcdc wired to both an LVDS encoder
>> and an HDMI encoder. Depending on how we integrate this board, only
>> one of these paths make sense. Hence, I would like to do the following
>> in a .dtsi for that board:
>>
>> / {
>>         hlcdc-display-controller {
>>                 ...
>>                 port@0 {
>>                         hlcdc_lvds: endpoint@0 {
>>                                 reg = <0>;
>>                                 remote-endpoint = <&lvds_encoder_input>;
>>                         };
>>
>>                         hlcdc_hdmi: endpoint@1 {
>>                                 reg = <1>;
>>                                 remote-endpoint = <&hdmi_encoder_input>;
>>                         };
>>                 };
>>         };
>>
>>         lvds_encoder: lvds-encoder {
>>                 status = "disabled";
>>
>>                 ...
>>
>>                 ports {
>>                         #address-cells = <1>;
>>                         #size-cells = <0>;
>>
>>                         port@0 {
>>                                 reg = <0>;
>>
>>                                 lvds_encoder_input: endpoint {
>>                                         remote-endpoint = <&hlcdc_lvds>;
>>                                 };
>>                         };
>>
>>                         port@1 {
>>                                 reg = <1>;
>>
>>                                 lvds_encoder_output: endpoint {
>>                                 };
>>                         };
>>                 };
>>         };
>> };
>>
>> &i2c2 {
>>         ...
>>         hdmi_encoder: hdmi-encoder@70 {
>>                 status = "disabled";
>>
>>                 ...
>>
>>                 port {
>>                         hdmi_encoder_input: endpoint {
>>                                 remote-endpoint = <&hlcdc_hdmi>;
>>                         };
>>                 };
>>         };
>> };
>>
>>
>> And then, depending on what components are fitted and what LVDS panel has
>> been attached to the LVDS encoder output, this can be used as follows
>> in the .dts file for LVDS case:
>>
>> / {
>>         panel: panel {
>>                 compatible = "litemax,dlf2118", "panel-lvds";
>>
>>                 ...
>>
>>                 port {
>>                         panel_input: endpoint {
>>                                 remote-endpoint = <&lvds_encoder_output>;
>>                         };
>>                 };
>>         };
>> };
>>
>>
>> &lvds_encoder {
>>         status = "okay";
>> };
>>
>> &lvds_encoder_output {
>>         remote-endpoint = <&panel_input>;
>> };
>>
>>
>> For the HDMI case, it would be this in the .dts file:
>>
>> &hdmi_encoder {
>>         status = "okay";
>> };
>>
>>
>> The above seem so much better than just having the following in
>> the .dtsi file
>>
>> / {
>>         hlcdc-display-controller {
>>                 ...
>>                 port@0 {
>>                         hlcdc_lvds: endpoint {
>>                                 remote-endpoint = <&encoder_input>;
>>                         };
>>                 };
>>         };
>> };
>>
>> and pushing the lvds-encoder and hdmi-encoder nodes (slightly modified) down
>> to the relevant .dts files. Especially so since we have to this point
>> delivered several variants with different LVDS panels. The duplication
>> is ugly, and the number of different panels is expected to grow...
>>
>> But maybe I have misunderstood what endpoints are all about, but to me
>> the actual endpoint id is not that interesting and I see nothing in the
>> graph binding that suggests that endpoint id 0 has to be up and kicking
>> in order for endpoint id 1 to be examined at all.
> 
> I guess it depends if the numbering is significant. For a one to many
> fan out like this, not so much. For a muxed input, then it would be.

Right, the endpoint numbering certainly *can* be important for some cases,
I can see that, and am not arguing against that part...

>> For ports, yes, you are right that the port id needs to be defined and
>> fixed for a specific function, so scratch that. It was just a "maybe"
>> question anyway...
>>
>>>>>>
>>>>>> Prior to this patch the active node had to have endpoint id zero.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <peda@...ntia.se>
>>>>>> ---
>>>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 32 ++++++++++++++++++------
>>>>>>  1 file changed, 25 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
>>>>>> index 8db51fb131db..16c1b2f54b42 100644
>>>>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
>>>>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
>>>>>> @@ -31,14 +31,16 @@ static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
>>>>>>      .destroy = drm_encoder_cleanup,
>>>>>>  };
>>>>>>
>>>>>> -static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>>>>>> +static int atmel_hlcdc_attach_endpoint(struct drm_device *dev,
>>>>>> +                                   struct of_endpoint *endpoint)
>>>>>>  {
>>>>>>      struct drm_encoder *encoder;
>>>>>>      struct drm_panel *panel;
>>>>>>      struct drm_bridge *bridge;
>>>>>>      int ret;
>>>>>>
>>>>>> -    ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
>>>>>> +    ret = drm_of_find_panel_or_bridge(dev->dev->of_node,
>>>>>> +                                      endpoint->port, endpoint->id,
>>>>>
>>>>> You are refusing endpoint->port != 0 in the caller, so that could be
>>>>> 0.
>>>>
>>>> Yes, it could. However, I intentionally did not write 0 here, so that
>>>> the logic related to "port has to be zero" was in one place and not
>>>> scattered about. I guess it's up to Boris?
>>>>
>>>> Maybe the port do not actually have to be zero at all? With the old
>>>> code, it was kind of understandable that the port number was fixed,
>>>> but for the code in my patch it does not matter at all AFAICT. There
>>>> is nothing in the binding docs (except for the example) that hints
>>>> that port has to be zero, so that's one thing in favor of just getting
>>>> rid of the port number checking altogether...
>>>
>>> The port numbering must be defined and fixed. If that is not clear in
>>> the binding, fix the binding.
>>
>> Ok, so the code in my patch does the right thing forcing the port
>> number to zero.
>>
>>>>>> @@ -77,13 +79,29 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>>>>>>
>>>>>>  int atmel_hlcdc_create_outputs(struct drm_device *dev)
>>>>>>  {
>>>>>> -    int endpoint, ret = 0;
>>>>>> -
>>>>>> -    for (endpoint = 0; !ret; endpoint++)
>>>>>> -            ret = atmel_hlcdc_attach_endpoint(dev, endpoint);
>>>>>> +    struct of_endpoint endpoint;
>>>>>> +    struct device_node *node = NULL;
>>>>>> +    int count = 0;
>>>>>> +    int ret = 0;
>>>>>> +
>>>>>> +    for_each_endpoint_of_node(dev->dev->of_node, node) {
>>>>>> +            of_graph_parse_endpoint(node, &endpoint);
>>>
>>> I'd really like to kill off of_graph_parse_endpoint, not add more
>>> users (check the git history on this code). You should know what are
>>> possible port and endpoint numbers, so iterate over those.
>>
>> So, add a comment to that effect in the docs of the of_graph_parse_endpoint
>> function.
>>
>> How can the hlcdc driver know the actual number of endpoints? It's a
>> one-way signal path out from that port, and it can easily be routed to
>> 1, 2, 3 or even more places. As shown above, forcing the endpoint id
>> to start at zero is a nuisance, and I don't see the point of it.
>>
>> But I welcome suggestions on how to arrange the above dtsi/dts fragments
>> in a world where the endpoint id absolutely has to start at zero.
> 
> Your dts file arrangement seems fine. Can't you just not exit the loop
> on -ENODEV? IOW, just iterate til you find an enabled endpoint.

That would regress cases where two (or more) endpoints are enabled
(which is currently supported). As I see it, the driver will have a
very hard time knowing when to stop iterating with any solution not
involving the equivalent of the functions for_each_endpoint_of_node
and of_graph_parse_endpoint. Open-coding of_graph_parse_endpoint just
to avoid it is a bit more than silly IMHO...

Cheers,
Peter

Powered by blists - more mailing lists