[RESEND PATCH v5 0/3] drm/atmel-hlcdc: bus-width override support

[RESEND PATCH v5 0/3] drm/atmel-hlcdc: bus-width override support

[Peter Rosin]

Hi!

This is perhaps not a true resend in that these three patches were
originally in a larger series [1], and the series have been rebased
to v4.18-rc6. However, I did ask that these three patches should be
considered separately. The other patches in the original series have
been adopted and reworked by Russell King [2].

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.
I of course need my old patches or the new ones from Russell to
actually use the HDMI encoder with the SAMA5D3, but that is not
directly related to this (shortened) series.

Cheers,
Peter

[1] https://lkml.org/lkml/2018/5/23/273
[2] https://www.spinics.net/lists/arm-kernel/msg669238.html

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

 .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++
 .../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   | 67 ++++++++++++++++++---
 5 files changed, 144 insertions(+), 28 deletions(-)

-- 
2.11.0

[RESEND PATCH v5 1/3] 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

[RESEND PATCH v5 2/3] 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>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
index 82f2acb3d374..9de434a8f523 100644
--- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
+++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
@@ -15,6 +15,14 @@ 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-type: must be <0>.
+ - 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@f0030000 {
@@ -50,3 +58,21 @@ 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@0 {
+				hlcdc_panel_output: endpoint@0 {
+					bus-type = <0>;
+					bus-width = <16>;
+				};
+			};
+		};
+	};
-- 
2.11.0

[RESEND PATCH v5 3/3] 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>
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 | 67 ++++++++++++++++++++---
 3 files changed, 111 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 8db51fb131db..db4d6aaaef93 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -27,33 +27,86 @@
 
 #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(struct device_node *ep)
+{
+	u32 bus_width = 0;
+
+	of_property_read_u32(ep, "bus-width", &bus_width);
+
+	switch (bus_width) {
+	case 0:
+		return 0;
+	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, int endpoint)
 {
-	struct drm_encoder *encoder;
+	struct atmel_hlcdc_rgb_output *output;
+	struct device_node *ep;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	int ret;
 
+	ep = of_graph_get_endpoint_by_regs(dev->dev->of_node, 0, endpoint);
+	if (!ep)
+		return -ENODEV;
+
 	ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
 					  &panel, &bridge);
 	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 -EINVAL;
 
-	ret = drm_encoder_init(dev, encoder,
+	output->bus_fmt = atmel_hlcdc_of_bus_fmt(ep);
+	if (output->bus_fmt < 0) {
+		dev_err(dev->dev, "invalid bus width\n");
+		return -EINVAL;
+	}
+
+	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);
@@ -62,7 +115,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 	}
 
 	if (bridge) {
-		ret = drm_bridge_attach(encoder, bridge, NULL);
+		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
 		if (!ret)
 			return 0;
 
@@ -70,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 			drm_panel_bridge_remove(bridge);
 	}
 
-	drm_encoder_cleanup(encoder);
+	drm_encoder_cleanup(&output->encoder);
 
 	return ret;
 }
-- 
2.11.0

Re: [RESEND PATCH v5 0/3] drm/atmel-hlcdc: bus-width override support

[Boris Brezillon]

On Fri,  3 Aug 2018 09:23:05 +0200
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> This is perhaps not a true resend in that these three patches were
> originally in a larger series [1], and the series have been rebased
> to v4.18-rc6. However, I did ask that these three patches should be
> considered separately. The other patches in the original series have
> been adopted and reworked by Russell King [2].

Oops, completely forgot to apply those 3 patches. If there's no
objection I'll queue them to drm-misc-next.

Regards,

Boris

> 
> 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.
> I of course need my old patches or the new ones from Russell to
> actually use the HDMI encoder with the SAMA5D3, but that is not
> directly related to this (shortened) series.
> 
> Cheers,
> Peter
> 
> [1] https://lkml.org/lkml/2018/5/23/273
> [2] https://www.spinics.net/lists/arm-kernel/msg669238.html
> 
> Peter Rosin (3):
>   dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
>   dt-bindings: display: atmel: optional video-interface of endpoints
>   drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes
> 
>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++
>  .../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   | 67 ++++++++++++++++++---
>  5 files changed, 144 insertions(+), 28 deletions(-)
> 

Re: [RESEND PATCH v5 2/3] dt-bindings: display: atmel: optional video-interface of endpoints

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]

Hi Peter!

On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> 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>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> index 82f2acb3d374..9de434a8f523 100644
> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> @@ -15,6 +15,14 @@ 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-type: must be <0>.

Is there any value in specifying this, if it has a fixed value to
"autodetect"? I understand it's optional, so if nobody else objects,
feels free to keep it there.


> + - 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@f0030000 {
> @@ -50,3 +58,21 @@ Example:
>  			#pwm-cells = <3>;
>  		};
>  	};
> +

Two blank lines here.

> +
> +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@0 {

The node has a unit address specified, you're missing a reg = <0>
property (no big deal, it's an example, but the other one has it)

> +				hlcdc_panel_output: endpoint@0 {

Missing reg here too.

Minors apart:

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

> +					bus-type = <0>;
> +					bus-width = <16>;
> +				};
> +			};
> +		};
> +	};
> --
> 2.11.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH v5 3/3] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 8096 bytes --]

Hi Peter,

On Fri, Aug 03, 2018 at 09:23:08AM +0200, Peter Rosin wrote:
> 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>
> 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 | 67 ++++++++++++++++++++---
>  3 files changed, 111 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 8db51fb131db..db4d6aaaef93 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -27,33 +27,86 @@
>
>  #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)

'static' missing?

> +{
> +	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(struct device_node *ep)
> +{
> +	u32 bus_width = 0;
> +
> +	of_property_read_u32(ep, "bus-width", &bus_width);
> +
> +	switch (bus_width) {
> +	case 0:
> +		return 0;
> +	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, int endpoint)
>  {
> -	struct drm_encoder *encoder;
> +	struct atmel_hlcdc_rgb_output *output;
> +	struct device_node *ep;
>  	struct drm_panel *panel;
>  	struct drm_bridge *bridge;
>  	int ret;
>
> +	ep = of_graph_get_endpoint_by_regs(dev->dev->of_node, 0, endpoint);
> +	if (!ep)
> +		return -ENODEV;
> +
>  	ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
>  					  &panel, &bridge);
>  	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 -EINVAL;
>
> -	ret = drm_encoder_init(dev, encoder,
> +	output->bus_fmt = atmel_hlcdc_of_bus_fmt(ep);
> +	if (output->bus_fmt < 0) {
> +		dev_err(dev->dev, "invalid bus width\n");

Nit: Bindings do not specify 0 as an accepted value.

Feel free to add my
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
with those two minors addressed.

Thanks
   j
> +		return -EINVAL;
> +	}
> +
> +	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);
> @@ -62,7 +115,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>  	}
>
>  	if (bridge) {
> -		ret = drm_bridge_attach(encoder, bridge, NULL);
> +		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
>  		if (!ret)
>  			return 0;
>
> @@ -70,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>  			drm_panel_bridge_remove(bridge);
>  	}
>
> -	drm_encoder_cleanup(encoder);
> +	drm_encoder_cleanup(&output->encoder);
>
>  	return ret;
>  }
> --
> 2.11.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH v5 3/3] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

[Peter Rosin]

On 2018-08-03 10:17, jacopo mondi wrote:
> Hi Peter,
> 
> On Fri, Aug 03, 2018 at 09:23:08AM +0200, Peter Rosin wrote:
>> 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>
>> 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 | 67 ++++++++++++++++++++---
>>  3 files changed, 111 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 8db51fb131db..db4d6aaaef93 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
>> @@ -27,33 +27,86 @@
>>
>>  #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)
> 
> 'static' missing?

No, it's also used from atmel_hlcdc_crtc.c

>> +{
>> +	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(struct device_node *ep)
>> +{
>> +	u32 bus_width = 0;
>> +
>> +	of_property_read_u32(ep, "bus-width", &bus_width);
>> +
>> +	switch (bus_width) {
>> +	case 0:
>> +		return 0;
>> +	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, int endpoint)
>>  {
>> -	struct drm_encoder *encoder;
>> +	struct atmel_hlcdc_rgb_output *output;
>> +	struct device_node *ep;
>>  	struct drm_panel *panel;
>>  	struct drm_bridge *bridge;
>>  	int ret;
>>
>> +	ep = of_graph_get_endpoint_by_regs(dev->dev->of_node, 0, endpoint);
>> +	if (!ep)
>> +		return -ENODEV;
>> +
>>  	ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
>>  					  &panel, &bridge);
>>  	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 -EINVAL;
>>
>> -	ret = drm_encoder_init(dev, encoder,
>> +	output->bus_fmt = atmel_hlcdc_of_bus_fmt(ep);
>> +	if (output->bus_fmt < 0) {
>> +		dev_err(dev->dev, "invalid bus width\n");
> 
> Nit: Bindings do not specify 0 as an accepted value.

I'll add some error checks in atmel_hlcdc_of_bus_fmt...

Cheers,
Peter

> Feel free to add my
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> with those two minors addressed.
> 
> Thanks
>    j
>> +		return -EINVAL;
>> +	}
>> +
>> +	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);
>> @@ -62,7 +115,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>>  	}
>>
>>  	if (bridge) {
>> -		ret = drm_bridge_attach(encoder, bridge, NULL);
>> +		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
>>  		if (!ret)
>>  			return 0;
>>
>> @@ -70,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
>>  			drm_panel_bridge_remove(bridge);
>>  	}
>>
>> -	drm_encoder_cleanup(encoder);
>> +	drm_encoder_cleanup(&output->encoder);
>>
>>  	return ret;
>>  }
>> --
>> 2.11.0
>>

Re: [RESEND PATCH v5 2/3] dt-bindings: display: atmel: optional video-interface of endpoints

[Peter Rosin]

On 2018-08-03 10:11, jacopo mondi wrote:
> Hi Peter!
> 
> On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
>> 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>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>> index 82f2acb3d374..9de434a8f523 100644
>> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>> @@ -15,6 +15,14 @@ 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-type: must be <0>.
> 
> Is there any value in specifying this, if it has a fixed value to
> "autodetect"? I understand it's optional, so if nobody else objects,
> feels free to keep it there.

That's just how media/video-interfaces.txt works.

bus-type 0 means that other properties describe the bus type. In this
case bus-width is specified, so that means a parallel bus. But bus-width
has no meaning (or may not have) if bus-type is non-zero. But checking
that bus-type for zero in the code seemed like overkill to me since the
driver already knows that it is a parallel bus...

TL;DR I'd like to keep it.

> 
>> + - 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@f0030000 {
>> @@ -50,3 +58,21 @@ Example:
>>  			#pwm-cells = <3>;
>>  		};
>>  	};
>> +
> 
> Two blank lines here.
> 
>> +
>> +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@0 {
> 
> The node has a unit address specified, you're missing a reg = <0>
> property (no big deal, it's an example, but the other one has it)
> 
>> +				hlcdc_panel_output: endpoint@0 {
> 
> Missing reg here too.

I'll fix those (I think they appeared for the original example after I
wrote the patch).

Cheers,
Peter

> Minors apart:
> 
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Thanks
>    j
> 
>> +					bus-type = <0>;
>> +					bus-width = <16>;
>> +				};
>> +			};
>> +		};
>> +	};
>> --
>> 2.11.0
>>

Re: [RESEND PATCH v5 2/3] dt-bindings: display: atmel: optional video-interface of endpoints

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4109 bytes --]

On Fri, Aug 03, 2018 at 10:40:02AM +0200, Peter Rosin wrote:
> On 2018-08-03 10:11, jacopo mondi wrote:
> > Hi Peter!
> >
> > On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> >> 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>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
> >>  1 file changed, 26 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> index 82f2acb3d374..9de434a8f523 100644
> >> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> @@ -15,6 +15,14 @@ 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-type: must be <0>.
> >
> > Is there any value in specifying this, if it has a fixed value to
> > "autodetect"? I understand it's optional, so if nobody else objects,
> > feels free to keep it there.
>
> That's just how media/video-interfaces.txt works.
>
> bus-type 0 means that other properties describe the bus type. In this
> case bus-width is specified, so that means a parallel bus. But bus-width
> has no meaning (or may not have) if bus-type is non-zero. But checking
> that bus-type for zero in the code seemed like overkill to me since the
> driver already knows that it is a parallel bus...
>

Yeah, I felt like pointing that out since you're not cheking for its value,
and that property is only used by v4l2-fwnode to handle some
not-that-used-anymore bus as CCP2 is.

> TL;DR I'd like to keep it.
>

Fine with me then.

> >
> >> + - 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@f0030000 {
> >> @@ -50,3 +58,21 @@ Example:
> >>  			#pwm-cells = <3>;
> >>  		};
> >>  	};
> >> +
> >
> > Two blank lines here.
> >
> >> +
> >> +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@0 {
> >
> > The node has a unit address specified, you're missing a reg = <0>
> > property (no big deal, it's an example, but the other one has it)
> >
> >> +				hlcdc_panel_output: endpoint@0 {
> >
> > Missing reg here too.
>
> I'll fix those (I think they appeared for the original example after I
> wrote the patch).
>

Ok, then please consider also describing the port@0 node cell sizes too
since it has a child endpoint node.

Cheers
  j

> Cheers,
> Peter
>
> > Minors apart:
> >
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > Thanks
> >    j
> >
> >> +					bus-type = <0>;
> >> +					bus-width = <16>;
> >> +				};
> >> +			};
> >> +		};
> >> +	};
> >> --
> >> 2.11.0
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH v5 2/3] dt-bindings: display: atmel: optional video-interface of endpoints

[Laurent Pinchart]

Hi Peter,

(CC'ing Sakari Ailus)

On Friday, 3 August 2018 11:40:02 EEST Peter Rosin wrote:
> On 2018-08-03 10:11, jacopo mondi wrote:
> > On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> >> 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>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >> 
> >>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26
> >>  ++++++++++++++++++++++ 1 file changed, 26 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt index
> >> 82f2acb3d374..9de434a8f523 100644
> >> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> 
> >> @@ -15,6 +15,14 @@ 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-type: must be <0>.
> > 
> > Is there any value in specifying this, if it has a fixed value to
> > "autodetect"? I understand it's optional, so if nobody else objects,
> > feels free to keep it there.
> 
> That's just how media/video-interfaces.txt works.
> 
> bus-type 0 means that other properties describe the bus type. In this
> case bus-width is specified, so that means a parallel bus. But bus-width
> has no meaning (or may not have) if bus-type is non-zero. But checking
> that bus-type for zero in the code seemed like overkill to me since the
> driver already knows that it is a parallel bus...
> 
> TL;DR I'd like to keep it.

Sakari told me recently that he was planning to introduce explicit bus types 
for parallel buses (both external sync and BT.656). Sakari, do you want to 
comment on this ?

> >> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
> >> +   override any output mode selection heuristic, forcing "rgb444",
> >> +   "rgb565", "rgb666" and "rgb888" respectively.

[snip]

-- 
Regards,

Laurent Pinchart

Re: [RESEND PATCH v5 2/3] dt-bindings: display: atmel: optional video-interface of endpoints

[Peter Rosin]

On 2018-08-03 10:51, jacopo mondi wrote:
> On Fri, Aug 03, 2018 at 10:40:02AM +0200, Peter Rosin wrote:
>> On 2018-08-03 10:11, jacopo mondi wrote:
>>> Hi Peter!
>>>
>>> On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
>>>> 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>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>>>> index 82f2acb3d374..9de434a8f523 100644
>>>> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>>>> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>>>> @@ -15,6 +15,14 @@ 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-type: must be <0>.
>>>
>>> Is there any value in specifying this, if it has a fixed value to
>>> "autodetect"? I understand it's optional, so if nobody else objects,
>>> feels free to keep it there.
>>
>> That's just how media/video-interfaces.txt works.
>>
>> bus-type 0 means that other properties describe the bus type. In this
>> case bus-width is specified, so that means a parallel bus. But bus-width
>> has no meaning (or may not have) if bus-type is non-zero. But checking
>> that bus-type for zero in the code seemed like overkill to me since the
>> driver already knows that it is a parallel bus...
>>
> 
> Yeah, I felt like pointing that out since you're not cheking for its value,
> and that property is only used by v4l2-fwnode to handle some
> not-that-used-anymore bus as CCP2 is.
> 
>> TL;DR I'd like to keep it.
>>
> 
> Fine with me then.
> 
>>>
>>>> + - 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@f0030000 {
>>>> @@ -50,3 +58,21 @@ Example:
>>>>  			#pwm-cells = <3>;
>>>>  		};
>>>>  	};
>>>> +
>>>
>>> Two blank lines here.
>>>
>>>> +
>>>> +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@0 {
>>>
>>> The node has a unit address specified, you're missing a reg = <0>
>>> property (no big deal, it's an example, but the other one has it)
>>>
>>>> +				hlcdc_panel_output: endpoint@0 {
>>>
>>> Missing reg here too.
>>
>> I'll fix those (I think they appeared for the original example after I
>> wrote the patch).
>>
> 
> Ok, then please consider also describing the port@0 node cell sizes too
> since it has a child endpoint node.

Ok, I have now figured out why this was as it were, and I no longer agree
with adding the extra properties. The whole of example 2 is inside a
reference (using the &hlcdc notation) to the hlcdc node in example 1, and
therefore these "missing" properties are not missing. I think they are
just clutter that hides what is really needed/different between example 1
and 2, and apparently Boris and Rob agreed when they acked/reviewed. The
description of example 2 also clearly states that example 2 is changes
and additions on top of example 1. So, I plan to have this in the next
iteration:

	&hlcdc {
		hlcdc-display-controller {
			pinctrl-names = "default";
			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;

			port@0 {
				hlcdc_panel_output: endpoint@0 {
					bus-width = <16>;
				};
			};
		};
	};

Jacopo, please let me know if you want me to keep your review tag anyway...


>> Cheers,
>> Peter
>>
>>> Minors apart:

...because I interpret this to mean that I could add your tag if I made the
changes you suggested. Or did it mean that I could add your tag regardless
because the issues were minor?

Cheers,
Peter

>>>
>>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>
>>> Thanks
>>>    j
>>>
>>>> +					bus-type = <0>;
>>>> +					bus-width = <16>;
>>>> +				};
>>>> +			};
>>>> +		};
>>>> +	};
>>>> --
>>>> 2.11.0
>>>>
>>

Re: [RESEND PATCH v5 2/3] dt-bindings: display: atmel: optional video-interface of endpoints

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 5870 bytes --]

Hi Peter,

On Thu, Aug 16, 2018 at 02:52:41PM +0200, Peter Rosin wrote:
> On 2018-08-03 10:51, jacopo mondi wrote:
> > On Fri, Aug 03, 2018 at 10:40:02AM +0200, Peter Rosin wrote:
> >> On 2018-08-03 10:11, jacopo mondi wrote:
> >>> Hi Peter!
> >>>
> >>> On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> >>>> 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>
> >>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>> ---
> >>>>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
> >>>>  1 file changed, 26 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >>>> index 82f2acb3d374..9de434a8f523 100644
> >>>> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >>>> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >>>> @@ -15,6 +15,14 @@ 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-type: must be <0>.
> >>>
> >>> Is there any value in specifying this, if it has a fixed value to
> >>> "autodetect"? I understand it's optional, so if nobody else objects,
> >>> feels free to keep it there.
> >>
> >> That's just how media/video-interfaces.txt works.
> >>
> >> bus-type 0 means that other properties describe the bus type. In this
> >> case bus-width is specified, so that means a parallel bus. But bus-width
> >> has no meaning (or may not have) if bus-type is non-zero. But checking
> >> that bus-type for zero in the code seemed like overkill to me since the
> >> driver already knows that it is a parallel bus...
> >>
> >
> > Yeah, I felt like pointing that out since you're not cheking for its value,
> > and that property is only used by v4l2-fwnode to handle some
> > not-that-used-anymore bus as CCP2 is.
> >
> >> TL;DR I'd like to keep it.
> >>
> >
> > Fine with me then.
> >
> >>>
> >>>> + - 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@f0030000 {
> >>>> @@ -50,3 +58,21 @@ Example:
> >>>>  			#pwm-cells = <3>;
> >>>>  		};
> >>>>  	};
> >>>> +
> >>>
> >>> Two blank lines here.
> >>>
> >>>> +
> >>>> +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@0 {
> >>>
> >>> The node has a unit address specified, you're missing a reg = <0>
> >>> property (no big deal, it's an example, but the other one has it)
> >>>
> >>>> +				hlcdc_panel_output: endpoint@0 {
> >>>
> >>> Missing reg here too.
> >>
> >> I'll fix those (I think they appeared for the original example after I
> >> wrote the patch).
> >>
> >
> > Ok, then please consider also describing the port@0 node cell sizes too
> > since it has a child endpoint node.
>
> Ok, I have now figured out why this was as it were, and I no longer agree
> with adding the extra properties. The whole of example 2 is inside a
> reference (using the &hlcdc notation) to the hlcdc node in example 1, and
> therefore these "missing" properties are not missing. I think they are
> just clutter that hides what is really needed/different between example 1
> and 2, and apparently Boris and Rob agreed when they acked/reviewed. The
> description of example 2 also clearly states that example 2 is changes
> and additions on top of example 1. So, I plan to have this in the next
> iteration:
>
> 	&hlcdc {
> 		hlcdc-display-controller {
> 			pinctrl-names = "default";
> 			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
>
> 			port@0 {
> 				hlcdc_panel_output: endpoint@0 {
> 					bus-width = <16>;

Right, I thought that even when re-defining a node, reg properties
should be there, but yes, I had no reasons to think so :)

> 				};
> 			};
> 		};
> 	};
>
> Jacopo, please let me know if you want me to keep your review tag anyway...
>

Sure

>
> >> Cheers,
> >> Peter
> >>
> >>> Minors apart:
>
> ...because I interpret this to mean that I could add your tag if I made the
> changes you suggested. Or did it mean that I could add your tag regardless
> because the issues were minor?
>v

Sorry, I was not clear. That was a minor nit, you could have add my
tag anyhow.

Thanks
   j

> Cheers,
> Peter
>
> >>>
> >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>
>  >>> Thanks
> >>>    j
> >>>
> >>>> +					bus-type = <0>;
> >>>> +					bus-width = <16>;
> >>>> +				};
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> --
> >>>> 2.11.0
> >>>>
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]