[PATCH v8 0/4] drm/atmel-hlcdc: bus-width override support

[PATCH v8 0/4] drm/atmel-hlcdc: bus-width override support

[Peter Rosin]

Hi!

The background for these patches is that our PCB interface between
the SAMA5D3 and the ds90c185 lvds encoder is only using 16 bits, and
this has to be described somewhere, or the atmel-hlcdc driver have no
chance of selecting the correct output mode. Since we have similar
problems with a tda19988 HDMI encoder I added patches to override
the atmel-hlcdc output format via DT properties compatible with the
media video-interface binding and things start to play together.

Cheers,
Peter

Changes since v7  https://lkml.org/lkml/2018/8/4/288
- The ep device_node was leaked in v7 patch 3/3, so add patch 3/4
  which simplifies fixing this in patch 4/4 (and adds flexibility)
  and adjust patch 4/4 to the changes done in the new 3/4.
- return -ENOMEM on allocation failure in patch 4/4

Changes since v6  https://lkml.org/lkml/2018/8/3/333
- zap bus-type from the binding in patch 2/3

Changes since (the shortened) v5  https://lkml.org/lkml/2018/8/3/182
- add reg properties (and #*-cells) to the example in patch 2/3
- prohibit bus-width 0 in the device-tree in patch 3/3
- added reviewed-by from Jacopo to patch 2/3 and 3/3

Peter Rosin (4):
  dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
  dt-bindings: display: atmel: optional video-interface of endpoints
  drm/atmel-hlcdc: iterate over all output endpoints
  drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

 .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 28 +++++++
 .../bindings/display/bridge/lvds-transmitter.txt   |  8 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     | 70 +++++++++++-----
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h       |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   | 98 ++++++++++++++++++----
 5 files changed, 170 insertions(+), 35 deletions(-)

-- 
2.11.0

[PATCH v8 1/4] dt-bindings: display: bridge: lvds-transmitter: add ti, ds90c185

[Peter Rosin]

Start list of actual chips compatible with "lvds-encoder".

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/bridge/lvds-transmitter.txt       | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index fd39ad34c383..50220190c203 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,7 +22,13 @@ among others.
 
 Required properties:
 
-- compatible: Must be "lvds-encoder"
+- compatible: Must be one or more of the following
+  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
+  - "lvds-encoder" for a generic LVDS encoder device
+
+  When compatible with the generic version, nodes must list the
+  device-specific version corresponding to the device first
+  followed by the generic version.
 
 Required nodes:
 
-- 
2.11.0

[PATCH v8 2/4] dt-bindings: display: atmel: optional video-interface of endpoints

[Peter Rosin]

With bus-type/bus-width properties in the endpoint nodes, the video-
interface of the connection can be specified for cases where the
heuristic fails to select the correct output mode. This can happen
e.g. if not all RGB pins are routed on the PCB; the driver has no
way of knowing this, and needs to be told explicitly.

This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
index 82f2acb3d374..d29e1e425518 100644
--- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
+++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
@@ -15,6 +15,13 @@ Required children nodes:
  to external devices using the OF graph reprensentation (see ../graph.txt).
  At least one port node is required.
 
+Optional properties in grandchild nodes:
+ Any endpoint grandchild node may specify a desired video interface
+ according to ../../media/video-interfaces.txt, specifically
+ - bus-width: recognized values are <12>, <16>, <18> and <24>, and
+   override any output mode selection heuristic, forcing "rgb444",
+   "rgb565", "rgb666" and "rgb888" respectively.
+
 Example:
 
 	hlcdc: hlcdc at f0030000 {
@@ -50,3 +57,24 @@ Example:
 			#pwm-cells = <3>;
 		};
 	};
+
+Example 2: With a video interface override to force rgb565; as above
+but with these changes/additions:
+
+	&hlcdc {
+		hlcdc-display-controller {
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
+
+			port at 0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+
+				hlcdc_panel_output: endpoint at 0 {
+					reg = <0>;
+					bus-width = <16>;
+				};
+			};
+		};
+	};
-- 
2.11.0

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

[Peter Rosin]

This enables more flexible devicetrees. You can e.g. have two output
nodes where one is not enabled, without the ordering affecting things.

Prior to this patch the active node had to have endpoint id zero.

Signed-off-by: Peter Rosin <peda@axentia.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,
 					  &panel, &bridge);
 	if (ret)
 		return ret;
@@ -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);
+
+		if (endpoint.port)
+			continue;
+
+		ret = atmel_hlcdc_attach_endpoint(dev, &endpoint);
+		if (ret == -ENODEV)
+			continue;
+		if (ret) {
+			of_node_put(node);
+			break;
+		}
+		count++;
+	}
 
 	/* At least one device was successfully attached.*/
-	if (ret == -ENODEV && endpoint)
+	if (ret == -ENODEV && count)
 		return 0;
 
 	return ret;
-- 
2.11.0

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

[jacopo mondi]

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.
> 
> Prior to this patch the active node had to have endpoint id zero.
> 
> Signed-off-by: Peter Rosin <peda@axentia.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.

Apart from that small nit:
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

>  					  &panel, &bridge);
>  	if (ret)
>  		return ret;
> @@ -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);
> +
> +		if (endpoint.port)
> +			continue;
> +
> +		ret = atmel_hlcdc_attach_endpoint(dev, &endpoint);
> +		if (ret == -ENODEV)
> +			continue;
> +		if (ret) {
> +			of_node_put(node);
> +			break;
> +		}
> +		count++;
> +	}
>  
>  	/* At least one device was successfully attached.*/
> -	if (ret == -ENODEV && endpoint)
> +	if (ret == -ENODEV && count)
>  		return 0;
>  
>  	return ret;
> -- 
> 2.11.0
> 

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

[Peter Rosin]

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.
>>
>> Prior to this patch the active node had to have endpoint id zero.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.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...

Cheers,
Peter


> Apart from that small nit:
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Thanks
>   j
> 
>>  					  &panel, &bridge);
>>  	if (ret)
>>  		return ret;
>> @@ -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);
>> +
>> +		if (endpoint.port)
>> +			continue;
>> +
>> +		ret = atmel_hlcdc_attach_endpoint(dev, &endpoint);
>> +		if (ret == -ENODEV)
>> +			continue;
>> +		if (ret) {
>> +			of_node_put(node);
>> +			break;
>> +		}
>> +		count++;
>> +	}
>>  
>>  	/* At least one device was successfully attached.*/
>> -	if (ret == -ENODEV && endpoint)
>> +	if (ret == -ENODEV && count)
>>  		return 0;
>>  
>>  	return ret;
>> -- 
>> 2.11.0
>>

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

[Rob Herring]

On Mon, Aug 13, 2018 at 8:25 AM Peter Rosin <peda@axentia.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.

> >>
> >> Prior to this patch the active node had to have endpoint id zero.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.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.

> >> @@ -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.

> >> +
> >> +            if (endpoint.port)
> >> +                    continue;
> >> +
> >> +            ret = atmel_hlcdc_attach_endpoint(dev, &endpoint);
> >> +            if (ret == -ENODEV)
> >> +                    continue;
> >> +            if (ret) {
> >> +                    of_node_put(node);
> >> +                    break;
> >> +            }
> >> +            count++;
> >> +    }
> >>
> >>      /* At least one device was successfully attached.*/
> >> -    if (ret == -ENODEV && endpoint)
> >> +    if (ret == -ENODEV && count)
> >>              return 0;
> >>
> >>      return ret;
> >> --
> >> 2.11.0
> >>
>

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

[Peter Rosin]

On 2018-08-13 22:52, Rob Herring wrote:
> On Mon, Aug 13, 2018 at 8:25 AM Peter Rosin <peda@axentia.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 at 0 {
			hlcdc_lvds: endpoint at 0 {
				reg = <0>;
				remote-endpoint = <&lvds_encoder_input>;
			};

			hlcdc_hdmi: endpoint at 1 {
				reg = <1>;
				remote-endpoint = <&hdmi_encoder_input>;
			};
		};
	};

	lvds_encoder: lvds-encoder {
		status = "disabled";

		...

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port at 0 {
				reg = <0>;

				lvds_encoder_input: endpoint {
					remote-endpoint = <&hlcdc_lvds>;
				};
			};

			port at 1 {
				reg = <1>;

				lvds_encoder_output: endpoint {
				};
			};
		};
	};
};

&i2c2 {
	...
	hdmi_encoder: hdmi-encoder at 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 at 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.

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@axentia.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.

Cheers,
Peter

>>>> +
>>>> +            if (endpoint.port)
>>>> +                    continue;
>>>> +
>>>> +            ret = atmel_hlcdc_attach_endpoint(dev, &endpoint);
>>>> +            if (ret == -ENODEV)
>>>> +                    continue;
>>>> +            if (ret) {
>>>> +                    of_node_put(node);
>>>> +                    break;
>>>> +            }
>>>> +            count++;
>>>> +    }
>>>>
>>>>      /* At least one device was successfully attached.*/
>>>> -    if (ret == -ENODEV && endpoint)
>>>> +    if (ret == -ENODEV && count)
>>>>              return 0;
>>>>
>>>>      return ret;
>>>> --
>>>> 2.11.0
>>>>
>>

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

[Rob Herring]

On Tue, Aug 14, 2018 at 12:36 AM Peter Rosin <peda@axentia.se> wrote:
>
> On 2018-08-13 22:52, Rob Herring wrote:
> > On Mon, Aug 13, 2018 at 8:25 AM Peter Rosin <peda@axentia.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 at 0 {
>                         hlcdc_lvds: endpoint at 0 {
>                                 reg = <0>;
>                                 remote-endpoint = <&lvds_encoder_input>;
>                         };
>
>                         hlcdc_hdmi: endpoint at 1 {
>                                 reg = <1>;
>                                 remote-endpoint = <&hdmi_encoder_input>;
>                         };
>                 };
>         };
>
>         lvds_encoder: lvds-encoder {
>                 status = "disabled";
>
>                 ...
>
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>
>                         port at 0 {
>                                 reg = <0>;
>
>                                 lvds_encoder_input: endpoint {
>                                         remote-endpoint = <&hlcdc_lvds>;
>                                 };
>                         };
>
>                         port at 1 {
>                                 reg = <1>;
>
>                                 lvds_encoder_output: endpoint {
>                                 };
>                         };
>                 };
>         };
> };
>
> &i2c2 {
>         ...
>         hdmi_encoder: hdmi-encoder at 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 at 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.

> 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@axentia.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.

Rob

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

[Peter Rosin]

On 2018-08-14 16:33, Rob Herring wrote:
> On Tue, Aug 14, 2018 at 12:36 AM Peter Rosin <peda@axentia.se> wrote:
>>
>> On 2018-08-13 22:52, Rob Herring wrote:
>>> On Mon, Aug 13, 2018 at 8:25 AM Peter Rosin <peda@axentia.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 at 0 {
>>                         hlcdc_lvds: endpoint at 0 {
>>                                 reg = <0>;
>>                                 remote-endpoint = <&lvds_encoder_input>;
>>                         };
>>
>>                         hlcdc_hdmi: endpoint at 1 {
>>                                 reg = <1>;
>>                                 remote-endpoint = <&hdmi_encoder_input>;
>>                         };
>>                 };
>>         };
>>
>>         lvds_encoder: lvds-encoder {
>>                 status = "disabled";
>>
>>                 ...
>>
>>                 ports {
>>                         #address-cells = <1>;
>>                         #size-cells = <0>;
>>
>>                         port at 0 {
>>                                 reg = <0>;
>>
>>                                 lvds_encoder_input: endpoint {
>>                                         remote-endpoint = <&hlcdc_lvds>;
>>                                 };
>>                         };
>>
>>                         port at 1 {
>>                                 reg = <1>;
>>
>>                                 lvds_encoder_output: endpoint {
>>                                 };
>>                         };
>>                 };
>>         };
>> };
>>
>> &i2c2 {
>>         ...
>>         hdmi_encoder: hdmi-encoder at 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 at 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@axentia.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

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

[Boris Brezillon]

On Tue, 14 Aug 2018 17:05:29 +0200
Peter Rosin <peda@axentia.se> wrote:

> >>>>>> @@ -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...

I agree, and actually, I think this is Rob who suggested to do what we
do here :-) (iterate from 0 to X, and stop as soon as
-ENODEV is returned).

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

[Rob Herring]

On Fri, Aug 24, 2018 at 2:47 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Tue, 14 Aug 2018 17:05:29 +0200
> Peter Rosin <peda@axentia.se> wrote:
>
> > >>>>>> @@ -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...
>
> I agree, and actually, I think this is Rob who suggested to do what we
> do here :-) (iterate from 0 to X, and stop as soon as
> -ENODEV is returned).

By suggested, you mean implemented because that is what it currently
does. My suggestion now is to do this:

for (endpoint = 0; endpoint < SOME_REASONABLE_MAX_NUMBER_OF_ENDPOINTS;
endpoint++) {
  ret = atmel_hlcdc_attach_endpoint(dev, endpoint);
  if (ret == -ENODEV)
    continue;
  // handle other errors?
}

And SOME_REASONABLE_MAX_NUMBER_OF_ENDPOINTS is something you make up
based on how many connections any board will have and physics (I
suppose you could have trees worth of buffers, but really what is the
worst case in an actual board design?). I would think 4 would be
plenty. If not, double it to 8.

Rob

[PATCH v8 4/4] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

[Peter Rosin]

This beats the heuristic that the connector is involved in what format
should be output for cases where this fails.

E.g. if there is a bridge that changes format between the encoder and the
connector, or if some of the RGB pins between the lcd controller and the
encoder are not routed on the PCB.

This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 70 +++++++++++++++++-------
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h     |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 66 +++++++++++++++++++---
 3 files changed, 110 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index d73281095fac..c38a479ada98 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -226,6 +226,55 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
 #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
 #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
 
+static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
+{
+	struct drm_connector *connector = state->connector;
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_encoder *encoder;
+	unsigned int supported_fmts = 0;
+	int j;
+
+	encoder = state->best_encoder;
+	if (!encoder)
+		encoder = connector->encoder;
+
+	switch (atmel_hlcdc_encoder_get_bus_fmt(encoder)) {
+	case 0:
+		break;
+	case MEDIA_BUS_FMT_RGB444_1X12:
+		return ATMEL_HLCDC_RGB444_OUTPUT;
+	case MEDIA_BUS_FMT_RGB565_1X16:
+		return ATMEL_HLCDC_RGB565_OUTPUT;
+	case MEDIA_BUS_FMT_RGB666_1X18:
+		return ATMEL_HLCDC_RGB666_OUTPUT;
+	case MEDIA_BUS_FMT_RGB888_1X24:
+		return ATMEL_HLCDC_RGB888_OUTPUT;
+	default:
+		return -EINVAL;
+	}
+
+	for (j = 0; j < info->num_bus_formats; j++) {
+		switch (info->bus_formats[j]) {
+		case MEDIA_BUS_FMT_RGB444_1X12:
+			supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB565_1X16:
+			supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB666_1X18:
+			supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X24:
+			supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return supported_fmts;
+}
+
 static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 {
 	unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
@@ -238,31 +287,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 	crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
 
 	for_each_new_connector_in_state(state->state, connector, cstate, i) {
-		struct drm_display_info *info = &connector->display_info;
 		unsigned int supported_fmts = 0;
-		int j;
 
 		if (!cstate->crtc)
 			continue;
 
-		for (j = 0; j < info->num_bus_formats; j++) {
-			switch (info->bus_formats[j]) {
-			case MEDIA_BUS_FMT_RGB444_1X12:
-				supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB565_1X16:
-				supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB666_1X18:
-				supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB888_1X24:
-				supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
-				break;
-			default:
-				break;
-			}
-		}
+		supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
 
 		if (crtc->dc->desc->conflicting_output_formats)
 			output_fmts &= supported_fmts;
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index 60c937f42114..4cc1e03f0aee 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -441,5 +441,6 @@ void atmel_hlcdc_crtc_irq(struct drm_crtc *c);
 int atmel_hlcdc_crtc_create(struct drm_device *dev);
 
 int atmel_hlcdc_create_outputs(struct drm_device *dev);
+int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder);
 
 #endif /* DRM_ATMEL_HLCDC_H */
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index 16c1b2f54b42..10ed7c221a3c 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -27,14 +27,59 @@
 
 #include "atmel_hlcdc_dc.h"
 
+struct atmel_hlcdc_rgb_output {
+	struct drm_encoder encoder;
+	int bus_fmt;
+};
+
 static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
+static struct atmel_hlcdc_rgb_output *
+atmel_hlcdc_encoder_to_rgb_output(struct drm_encoder *encoder)
+{
+	return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
+}
+
+int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder)
+{
+	struct atmel_hlcdc_rgb_output *output;
+
+	output = atmel_hlcdc_encoder_to_rgb_output(encoder);
+
+	return output->bus_fmt;
+}
+
+static int atmel_hlcdc_of_bus_fmt(const struct device_node *ep)
+{
+	u32 bus_width;
+	int ret;
+
+	ret = of_property_read_u32(ep, "bus-width", &bus_width);
+	if (ret == -EINVAL)
+		return 0;
+	if (ret)
+		return ret;
+
+	switch (bus_width) {
+	case 12:
+		return MEDIA_BUS_FMT_RGB444_1X12;
+	case 16:
+		return MEDIA_BUS_FMT_RGB565_1X16;
+	case 18:
+		return MEDIA_BUS_FMT_RGB666_1X18;
+	case 24:
+		return MEDIA_BUS_FMT_RGB888_1X24;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int atmel_hlcdc_attach_endpoint(struct drm_device *dev,
 				       struct of_endpoint *endpoint)
 {
-	struct drm_encoder *encoder;
+	struct atmel_hlcdc_rgb_output *output;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	int ret;
@@ -45,17 +90,24 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
-	if (!encoder)
+	output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
+	if (!output)
+		return -ENOMEM;
+
+	output->bus_fmt = atmel_hlcdc_of_bus_fmt(endpoint->local_node);
+	if (output->bus_fmt < 0) {
+		dev_err(dev->dev, "endpoint %d: invalid bus width\n",
+			endpoint->id);
 		return -EINVAL;
+	}
 
-	ret = drm_encoder_init(dev, encoder,
+	ret = drm_encoder_init(dev, &output->encoder,
 			       &atmel_hlcdc_panel_encoder_funcs,
 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret)
 		return ret;
 
-	encoder->possible_crtcs = 0x1;
+	output->encoder.possible_crtcs = 0x1;
 
 	if (panel) {
 		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
@@ -64,7 +116,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev,
 	}
 
 	if (bridge) {
-		ret = drm_bridge_attach(encoder, bridge, NULL);
+		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
 		if (!ret)
 			return 0;
 
@@ -72,7 +124,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev,
 			drm_panel_bridge_remove(bridge);
 	}
 
-	drm_encoder_cleanup(encoder);
+	drm_encoder_cleanup(&output->encoder);
 
 	return ret;
 }
-- 
2.11.0

[PATCH v8 0/4] drm/atmel-hlcdc: bus-width override support

[Boris Brezillon]

Hi Peter,

On Fri, 10 Aug 2018 15:03:55 +0200
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> The background for these patches is that our PCB interface between
> the SAMA5D3 and the ds90c185 lvds encoder is only using 16 bits, and
> this has to be described somewhere, or the atmel-hlcdc driver have no
> chance of selecting the correct output mode. Since we have similar
> problems with a tda19988 HDMI encoder I added patches to override
> the atmel-hlcdc output format via DT properties compatible with the
> media video-interface binding and things start to play together.
> 
> Cheers,
> Peter
> 
> Changes since v7  https://lkml.org/lkml/2018/8/4/288
> - The ep device_node was leaked in v7 patch 3/3, so add patch 3/4
>   which simplifies fixing this in patch 4/4 (and adds flexibility)
>   and adjust patch 4/4 to the changes done in the new 3/4.
> - return -ENOMEM on allocation failure in patch 4/4

I stopped following the discussion at some point. Are there any open
issues or Ack you're waiting for?

[PATCH v8 0/4] drm/atmel-hlcdc: bus-width override support

[Peter Rosin]

On 2018-08-24 09:51, Boris Brezillon wrote:
> Hi Peter,
> 
> On Fri, 10 Aug 2018 15:03:55 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> Hi!
>>
>> The background for these patches is that our PCB interface between
>> the SAMA5D3 and the ds90c185 lvds encoder is only using 16 bits, and
>> this has to be described somewhere, or the atmel-hlcdc driver have no
>> chance of selecting the correct output mode. Since we have similar
>> problems with a tda19988 HDMI encoder I added patches to override
>> the atmel-hlcdc output format via DT properties compatible with the
>> media video-interface binding and things start to play together.
>>
>> Cheers,
>> Peter
>>
>> Changes since v7  https://lkml.org/lkml/2018/8/4/288
>> - The ep device_node was leaked in v7 patch 3/3, so add patch 3/4
>>   which simplifies fixing this in patch 4/4 (and adds flexibility)
>>   and adjust patch 4/4 to the changes done in the new 3/4.
>> - return -ENOMEM on allocation failure in patch 4/4
> 
> I stopped following the discussion at some point. Are there any open
> issues or Ack you're waiting for?

The only worry is the silence from Rob on that multiple-endpoint
discussion. Since I don't really care deeply, my plan was fix up
the of_node leak in v7 without using of_graph_parse_endpoint. I.e.
drop 3/4.

But I would have prefer Rob to add some final opinion instead of the
discussion petering out like it did.

Cheers,
Peter