Re: [PATCH v8 3/4] drm/atmel-hlcdc: iterate over all output endpoints

From: Peter Rosin
Date: Tue Aug 14 2018 - 11:05:49 EST

On 2018-08-14 16:33, Rob Herring wrote:
> On Tue, Aug 14, 2018 at 12:36 AM Peter Rosin <peda@xxxxxxxxxx> wrote:
>>
>> On 2018-08-13 22:52, Rob Herring wrote:
>>> On Mon, Aug 13, 2018 at 8:25 AM Peter Rosin <peda@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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