[PATCH RESEND v3 0/2] drm: add DisplayPort connector

[PATCH RESEND v3 0/2] drm: add DisplayPort connector

[Tomi Valkeinen]

Hi,

This series adds the DT bindings and a driver for DisplayPort connector.

This is a resend of
https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg326794.html
with no changes (rebased on top of latest drm-misc).

 Tomi

Tomi Valkeinen (2):
  dt-bindings: dp-connector: add binding for DisplayPort connector
  drm/bridge: display-connector: add DP support

 .../display/connector/dp-connector.yaml       | 55 +++++++++++++++++++
 drivers/gpu/drm/bridge/display-connector.c    | 46 +++++++++++++++-
 2 files changed, 99 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/connector/dp-connector.yaml

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RESEND v3 1/2] dt-bindings: dp-connector: add binding for DisplayPort connector

[Tomi Valkeinen]

Add binding for DisplayPort connector. A few notes:

* Similar to hdmi-connector, it has hpd-gpios as an optional property,
  as the HPD could also be handled by, e.g., the DP bridge.

* dp-pwr-supply, which provides 3.3V on DP_PWR pin, is optional, as it
  is not strictly required: standard DP cables do not even have the pin
  connected.

* Connector type. Full size and mini connectors are identical except for
  the connector size and form, so I believe there is no functional need
  for this property. But similar to 'label' property, it might be used
  to present information about the connector to the userspace.

* No eDP. There's really no "eDP connector", as it's always a custom
  made connection between the DP and the DP panel, although the eDP spec
  does offer a few suggested pin setups. So possibly there is no need for
  edp-connector binding, but even if there is, I don't want to guess what
  it could look like, and could it be part of the dp-connector binding.

* No DP++. I'm not familiar with DP++. DP++ might need an i2c bus added
  to the bindings.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Rob Herring <robh+dt@kernel.org>
---
 .../display/connector/dp-connector.yaml       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/connector/dp-connector.yaml

diff --git a/Documentation/devicetree/bindings/display/connector/dp-connector.yaml b/Documentation/devicetree/bindings/display/connector/dp-connector.yaml
new file mode 100644
index 000000000000..b5fc3e52899e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/connector/dp-connector.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/connector/dp-connector.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DisplayPort Connector
+
+maintainers:
+  - Tomi Valkeinen <tomi.valkeinen@ti.com>
+
+properties:
+  compatible:
+    const: dp-connector
+
+  label: true
+
+  type:
+    enum:
+      - full-size
+      - mini
+
+  hpd-gpios:
+    description: A GPIO line connected to HPD
+    maxItems: 1
+
+  dp-pwr-supply:
+    description: Power supply for the DP_PWR pin
+    maxItems: 1
+
+  port:
+    description: Connection to controller providing DP signals
+
+required:
+  - compatible
+  - type
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    connector {
+        compatible = "dp-connector";
+        label = "dp0";
+        type = "full-size";
+
+        port {
+            dp_connector_in: endpoint {
+                remote-endpoint = <&dp_out>;
+            };
+        };
+    };
+
+...
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RESEND v3 2/2] drm/bridge: display-connector: add DP support

[Tomi Valkeinen]

Add DP support to display-connector driver. The driver will support HPD
via a GPIO and DP PWR.

DP PWR will be enabled at probe, which is not optimal, but I'm not sure
what would be a good place to enable and disable DP PWR. Perhaps
attach/detach, but I don't know if enabling HW is something that attach
is supposed to do.

In any case, I don't think there's much difference in power consumption
between the version in this patch and enabling the regulator later: if
the driver probes, supposedly it will attach very soon afterwards, and
we need to enable the DP PWR as soon as possible.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/display-connector.c | 46 +++++++++++++++++++++-
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index 4d278573cdb9..04362feccd75 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 
 #include <drm/drm_bridge.h>
 #include <drm/drm_edid.h>
@@ -20,6 +21,8 @@ struct display_connector {
 
 	struct gpio_desc	*hpd_gpio;
 	int			hpd_irq;
+
+	struct regulator	*dp_pwr;
 };
 
 static inline struct display_connector *
@@ -172,11 +175,12 @@ static int display_connector_probe(struct platform_device *pdev)
 	of_property_read_string(pdev->dev.of_node, "label", &label);
 
 	/*
-	 * Get the HPD GPIO for DVI and HDMI connectors. If the GPIO can provide
+	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
 	 * edge interrupts, register an interrupt handler.
 	 */
 	if (type == DRM_MODE_CONNECTOR_DVII ||
-	    type == DRM_MODE_CONNECTOR_HDMIA) {
+	    type == DRM_MODE_CONNECTOR_HDMIA ||
+	    type == DRM_MODE_CONNECTOR_DisplayPort) {
 		conn->hpd_gpio = devm_gpiod_get_optional(&pdev->dev, "hpd",
 							 GPIOD_IN);
 		if (IS_ERR(conn->hpd_gpio)) {
@@ -223,6 +227,38 @@ static int display_connector_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* Get the DP PWR for DP connector */
+	if (type == DRM_MODE_CONNECTOR_DisplayPort) {
+		int r;
+
+		conn->dp_pwr = devm_regulator_get_optional(&pdev->dev, "dp-pwr");
+
+		if (IS_ERR(conn->dp_pwr)) {
+			r = PTR_ERR(conn->dp_pwr);
+
+			switch (r) {
+			case -ENODEV:
+				conn->dp_pwr = NULL;
+				break;
+
+			case -EPROBE_DEFER:
+				return -EPROBE_DEFER;
+
+			default:
+				dev_err(&pdev->dev, "failed to get DP PWR regulator: %d\n", r);
+				return r;
+			}
+		}
+
+		if (conn->dp_pwr) {
+			r = regulator_enable(conn->dp_pwr);
+			if (r) {
+				dev_err(&pdev->dev, "failed to enable DP PWR regulator: %d\n", r);
+				return r;
+			}
+		}
+	}
+
 	conn->bridge.funcs = &display_connector_bridge_funcs;
 	conn->bridge.of_node = pdev->dev.of_node;
 
@@ -251,6 +287,9 @@ static int display_connector_remove(struct platform_device *pdev)
 {
 	struct display_connector *conn = platform_get_drvdata(pdev);
 
+	if (conn->dp_pwr)
+		regulator_disable(conn->dp_pwr);
+
 	drm_bridge_remove(&conn->bridge);
 
 	if (!IS_ERR(conn->bridge.ddc))
@@ -275,6 +314,9 @@ static const struct of_device_id display_connector_match[] = {
 	}, {
 		.compatible = "vga-connector",
 		.data = (void *)DRM_MODE_CONNECTOR_VGA,
+	}, {
+		.compatible = "dp-connector",
+		.data = (void *)DRM_MODE_CONNECTOR_DisplayPort,
 	},
 	{},
 };
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RESEND v3 1/2] dt-bindings: dp-connector: add binding for DisplayPort connector

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Nov 24, 2020 at 09:11:54AM +0200, Tomi Valkeinen wrote:
> Add binding for DisplayPort connector. A few notes:
> 
> * Similar to hdmi-connector, it has hpd-gpios as an optional property,
>   as the HPD could also be handled by, e.g., the DP bridge.
> 
> * dp-pwr-supply, which provides 3.3V on DP_PWR pin, is optional, as it
>   is not strictly required: standard DP cables do not even have the pin
>   connected.
> 
> * Connector type. Full size and mini connectors are identical except for
>   the connector size and form, so I believe there is no functional need
>   for this property. But similar to 'label' property, it might be used
>   to present information about the connector to the userspace.
> 
> * No eDP. There's really no "eDP connector", as it's always a custom
>   made connection between the DP and the DP panel, although the eDP spec
>   does offer a few suggested pin setups. So possibly there is no need for
>   edp-connector binding, but even if there is, I don't want to guess what
>   it could look like, and could it be part of the dp-connector binding.
> 
> * No DP++. I'm not familiar with DP++. DP++ might need an i2c bus added
>   to the bindings.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> ---
>  .../display/connector/dp-connector.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/connector/dp-connector.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/connector/dp-connector.yaml b/Documentation/devicetree/bindings/display/connector/dp-connector.yaml
> new file mode 100644
> index 000000000000..b5fc3e52899e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/connector/dp-connector.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/connector/dp-connector.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DisplayPort Connector
> +
> +maintainers:
> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
> +
> +properties:
> +  compatible:
> +    const: dp-connector
> +
> +  label: true
> +
> +  type:
> +    enum:
> +      - full-size
> +      - mini

I wonder if "full" would be better than "full-size" to match "mini". Up
to you.

> +
> +  hpd-gpios:
> +    description: A GPIO line connected to HPD
> +    maxItems: 1
> +
> +  dp-pwr-supply:
> +    description: Power supply for the DP_PWR pin
> +    maxItems: 1
> +
> +  port:
> +    description: Connection to controller providing DP signals

Now that the OF graph schema has landed, could you add the corresponding
$ref ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +required:
> +  - compatible
> +  - type
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    connector {
> +        compatible = "dp-connector";
> +        label = "dp0";
> +        type = "full-size";
> +
> +        port {
> +            dp_connector_in: endpoint {
> +                remote-endpoint = <&dp_out>;
> +            };
> +        };
> +    };
> +
> +...

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RESEND v3 2/2] drm/bridge: display-connector: add DP support

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Nov 24, 2020 at 09:11:55AM +0200, Tomi Valkeinen wrote:
> Add DP support to display-connector driver. The driver will support HPD
> via a GPIO and DP PWR.
> 
> DP PWR will be enabled at probe, which is not optimal, but I'm not sure
> what would be a good place to enable and disable DP PWR. Perhaps
> attach/detach, but I don't know if enabling HW is something that attach
> is supposed to do.
> 
> In any case, I don't think there's much difference in power consumption
> between the version in this patch and enabling the regulator later: if
> the driver probes, supposedly it will attach very soon afterwards, and
> we need to enable the DP PWR as soon as possible.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/display-connector.c | 46 +++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index 4d278573cdb9..04362feccd75 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_edid.h>
> @@ -20,6 +21,8 @@ struct display_connector {
>  
>  	struct gpio_desc	*hpd_gpio;
>  	int			hpd_irq;
> +
> +	struct regulator	*dp_pwr;
>  };
>  
>  static inline struct display_connector *
> @@ -172,11 +175,12 @@ static int display_connector_probe(struct platform_device *pdev)
>  	of_property_read_string(pdev->dev.of_node, "label", &label);
>  
>  	/*
> -	 * Get the HPD GPIO for DVI and HDMI connectors. If the GPIO can provide
> +	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
>  	 * edge interrupts, register an interrupt handler.
>  	 */
>  	if (type == DRM_MODE_CONNECTOR_DVII ||
> -	    type == DRM_MODE_CONNECTOR_HDMIA) {
> +	    type == DRM_MODE_CONNECTOR_HDMIA ||
> +	    type == DRM_MODE_CONNECTOR_DisplayPort) {
>  		conn->hpd_gpio = devm_gpiod_get_optional(&pdev->dev, "hpd",
>  							 GPIOD_IN);
>  		if (IS_ERR(conn->hpd_gpio)) {
> @@ -223,6 +227,38 @@ static int display_connector_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* Get the DP PWR for DP connector */

s/connector/connector./

> +	if (type == DRM_MODE_CONNECTOR_DisplayPort) {
> +		int r;

The driver uses "ret".

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +		conn->dp_pwr = devm_regulator_get_optional(&pdev->dev, "dp-pwr");
> +
> +		if (IS_ERR(conn->dp_pwr)) {
> +			r = PTR_ERR(conn->dp_pwr);
> +
> +			switch (r) {
> +			case -ENODEV:
> +				conn->dp_pwr = NULL;
> +				break;
> +
> +			case -EPROBE_DEFER:
> +				return -EPROBE_DEFER;
> +
> +			default:
> +				dev_err(&pdev->dev, "failed to get DP PWR regulator: %d\n", r);
> +				return r;
> +			}
> +		}
> +
> +		if (conn->dp_pwr) {
> +			r = regulator_enable(conn->dp_pwr);
> +			if (r) {
> +				dev_err(&pdev->dev, "failed to enable DP PWR regulator: %d\n", r);
> +				return r;
> +			}
> +		}
> +	}
> +
>  	conn->bridge.funcs = &display_connector_bridge_funcs;
>  	conn->bridge.of_node = pdev->dev.of_node;
>  
> @@ -251,6 +287,9 @@ static int display_connector_remove(struct platform_device *pdev)
>  {
>  	struct display_connector *conn = platform_get_drvdata(pdev);
>  
> +	if (conn->dp_pwr)
> +		regulator_disable(conn->dp_pwr);
> +
>  	drm_bridge_remove(&conn->bridge);
>  
>  	if (!IS_ERR(conn->bridge.ddc))
> @@ -275,6 +314,9 @@ static const struct of_device_id display_connector_match[] = {
>  	}, {
>  		.compatible = "vga-connector",
>  		.data = (void *)DRM_MODE_CONNECTOR_VGA,
> +	}, {
> +		.compatible = "dp-connector",
> +		.data = (void *)DRM_MODE_CONNECTOR_DisplayPort,
>  	},
>  	{},
>  };

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RESEND v3 1/2] dt-bindings: dp-connector: add binding for DisplayPort connector

[Tomi Valkeinen]

On 30/11/2020 12:10, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Nov 24, 2020 at 09:11:54AM +0200, Tomi Valkeinen wrote:
>> Add binding for DisplayPort connector. A few notes:
>>
>> * Similar to hdmi-connector, it has hpd-gpios as an optional property,
>>   as the HPD could also be handled by, e.g., the DP bridge.
>>
>> * dp-pwr-supply, which provides 3.3V on DP_PWR pin, is optional, as it
>>   is not strictly required: standard DP cables do not even have the pin
>>   connected.
>>
>> * Connector type. Full size and mini connectors are identical except for
>>   the connector size and form, so I believe there is no functional need
>>   for this property. But similar to 'label' property, it might be used
>>   to present information about the connector to the userspace.
>>
>> * No eDP. There's really no "eDP connector", as it's always a custom
>>   made connection between the DP and the DP panel, although the eDP spec
>>   does offer a few suggested pin setups. So possibly there is no need for
>>   edp-connector binding, but even if there is, I don't want to guess what
>>   it could look like, and could it be part of the dp-connector binding.
>>
>> * No DP++. I'm not familiar with DP++. DP++ might need an i2c bus added
>>   to the bindings.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> ---
>>  .../display/connector/dp-connector.yaml       | 55 +++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/connector/dp-connector.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/connector/dp-connector.yaml b/Documentation/devicetree/bindings/display/connector/dp-connector.yaml
>> new file mode 100644
>> index 000000000000..b5fc3e52899e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/connector/dp-connector.yaml
>> @@ -0,0 +1,55 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/connector/dp-connector.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: DisplayPort Connector
>> +
>> +maintainers:
>> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: dp-connector
>> +
>> +  label: true
>> +
>> +  type:
>> +    enum:
>> +      - full-size
>> +      - mini
> 
> I wonder if "full" would be better than "full-size" to match "mini". Up
> to you.

The DP spec (and e.g. wikipedia) uses "full-size" and "mini". Well, the spec mostly uses "mDP",
which is explained to mean "Mini DisplayPort". And full-size is used as "full-size DP".

E.g. "The plug on either end may be a full-size DP plug or an mDP plug."

We could use mDP here, but I think "mini" is more commonly used. And if we use mDP for mini, maybe
we should use DP for the full-size. But then it's maybe a bit confusing.

So I think "full-size" and "mini" match best to the spec and are still obvious for the human reader.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel