[PATCH v5 0/2] usb: typec: qcom-pmic-typec: enable DP support

[PATCH v5 0/2] usb: typec: qcom-pmic-typec: enable DP support

[Dmitry Baryshkov]

To enable DisplayPort on the platforms supported by qcom-pmic-typec
driver, we need to register a corresponding drm_bridge for this device
and also be able to send the OOB hotplug event to the corresponding DRM
connector. All this is implemented by [1], but there is no direct
dependency on that patchset.

[1] https://patchwork.freedesktop.org/series/120393/

Changes since v4:
- Put of_node field access under CONFIG_OF (LKP)
- Changed ifdef CONFIG_DRM to IS_ENABLED(CONFIG_DRM) (Guenter)

Changes since v3:
- Fixed changelog for v1 (Guenter)
- After discussion on IRC, change connector type to
  DRM_MODE_CONNECTOR_DisplayPort to follow i915 and amdgpu example.
  The fact that this is a DP wrapped in the USB connector will be
  handled separately via the subconnector property (Simon Ser, Janne
  Grunau)

Changes since v2:
- Reworded commit message for the first patch to explicitly mention that
  the "displayport" OF property was rejected (Bjorn)
- Removed several #ifdefs from the qcom-pmic-typec patch (Bryan, Konrad,
  Greg K-H)

Changes since v1:
- Properly handle CONFIG_DRM dependency. Disallow building
  qcom-pmic-typec into the kernel if DRM is built as module (Bryan).

Dmitry Baryshkov (2):
  usb: typec: altmodes/displayport: add support for embedded DP cases
  usb: typec: qcom-pmic-typec: register drm_bridge

 drivers/usb/typec/altmodes/displayport.c      |  5 ++-
 drivers/usb/typec/tcpm/Kconfig                |  1 +
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 37 +++++++++++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH v5 1/2] usb: typec: altmodes/displayport: add support for embedded DP cases

[Dmitry Baryshkov]

In the embedded cases, the DisplayPort connector is handled by the TCPM
itself. It was proposed to add the "displayport" OF property to the DT
bindings, but it  was rejected in favour of properly describing the
electrical signal path using of_graph.

Fallback to the controller fwnode for HPD notifications to
support such usecases without requiring additional DT properties.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/altmodes/displayport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index bc4af130940d..a4cf1045b535 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -594,7 +594,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
 	alt->ops = &dp_altmode_ops;
 
 	fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */
-	dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0);
+	if (fwnode_property_present(fwnode, "displayport"))
+		dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0);
+	else
+		dp->connector_fwnode = fwnode_handle_get(fwnode); /* embedded DP */
 	if (IS_ERR(dp->connector_fwnode))
 		dp->connector_fwnode = NULL;
 
-- 
2.39.2

Re: [PATCH v5 1/2] usb: typec: altmodes/displayport: add support for embedded DP cases

[Heikki Krogerus]

On Thu, Aug 17, 2023 at 06:08:23PM +0300, Dmitry Baryshkov wrote:
> In the embedded cases, the DisplayPort connector is handled by the TCPM
> itself. It was proposed to add the "displayport" OF property to the DT
> bindings, but it  was rejected in favour of properly describing the
> electrical signal path using of_graph.
> 
> Fallback to the controller fwnode for HPD notifications to
> support such usecases without requiring additional DT properties.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/altmodes/displayport.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index bc4af130940d..a4cf1045b535 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -594,7 +594,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
>  	alt->ops = &dp_altmode_ops;
>  
>  	fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */
> -	dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0);
> +	if (fwnode_property_present(fwnode, "displayport"))
> +		dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0);
> +	else
> +		dp->connector_fwnode = fwnode_handle_get(fwnode); /* embedded DP */
>  	if (IS_ERR(dp->connector_fwnode))
>  		dp->connector_fwnode = NULL;
>  
> -- 
> 2.39.2

-- 
heikki

[PATCH v5 2/2] usb: typec: qcom-pmic-typec: register drm_bridge

[Dmitry Baryshkov]

The current approach to handling DP on bridge-enabled platforms requires
a chain of DP bridges up to the USB-C connector. Register a last DRM
bridge for such chain.

Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/tcpm/Kconfig                |  1 +
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 37 +++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
index 5d393f520fc2..0b2993fef564 100644
--- a/drivers/usb/typec/tcpm/Kconfig
+++ b/drivers/usb/typec/tcpm/Kconfig
@@ -79,6 +79,7 @@ config TYPEC_WCOVE
 config TYPEC_QCOM_PMIC
 	tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver"
 	depends on ARCH_QCOM || COMPILE_TEST
+	depends on DRM || DRM=n
 	help
 	  A Type-C port and Power Delivery driver which aggregates two
 	  discrete pieces of silicon in the PM8150b PMIC block: the
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
index af44ee4e6e86..581199d37b49 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
@@ -17,6 +17,9 @@
 #include <linux/usb/role.h>
 #include <linux/usb/tcpm.h>
 #include <linux/usb/typec_mux.h>
+
+#include <drm/drm_bridge.h>
+
 #include "qcom_pmic_typec_pdphy.h"
 #include "qcom_pmic_typec_port.h"
 
@@ -33,6 +36,7 @@ struct pmic_typec {
 	struct pmic_typec_port	*pmic_typec_port;
 	bool			vbus_enabled;
 	struct mutex		lock;		/* VBUS state serialization */
+	struct drm_bridge	bridge;
 };
 
 #define tcpc_to_tcpm(_tcpc_) container_of(_tcpc_, struct pmic_typec, tcpc)
@@ -146,6 +150,35 @@ static int qcom_pmic_typec_init(struct tcpc_dev *tcpc)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_DRM)
+static int qcom_pmic_typec_attach(struct drm_bridge *bridge,
+				     enum drm_bridge_attach_flags flags)
+{
+	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
+}
+
+static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
+	.attach = qcom_pmic_typec_attach,
+};
+
+static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
+{
+	tcpm->bridge.funcs = &qcom_pmic_typec_bridge_funcs;
+#ifdef CONFIG_OF
+	tcpm->bridge.of_node = of_get_child_by_name(tcpm->dev->of_node, "connector");
+#endif
+	tcpm->bridge.ops = DRM_BRIDGE_OP_HPD;
+	tcpm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+
+	return devm_drm_bridge_add(tcpm->dev, &tcpm->bridge);
+}
+#else
+static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
+{
+	return 0;
+}
+#endif
+
 static int qcom_pmic_typec_probe(struct platform_device *pdev)
 {
 	struct pmic_typec *tcpm;
@@ -208,6 +241,10 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	mutex_init(&tcpm->lock);
 	platform_set_drvdata(pdev, tcpm);
 
+	ret = qcom_pmic_typec_init_drm(tcpm);
+	if (ret)
+		return ret;
+
 	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
 	if (!tcpm->tcpc.fwnode)
 		return -EINVAL;
-- 
2.39.2

Re: [PATCH v5 2/2] usb: typec: qcom-pmic-typec: register drm_bridge

[Heikki Krogerus]

On Thu, Aug 17, 2023 at 06:08:24PM +0300, Dmitry Baryshkov wrote:
> The current approach to handling DP on bridge-enabled platforms requires
> a chain of DP bridges up to the USB-C connector. Register a last DRM
> bridge for such chain.
> 
> Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/tcpm/Kconfig                |  1 +
>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 37 +++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> index 5d393f520fc2..0b2993fef564 100644
> --- a/drivers/usb/typec/tcpm/Kconfig
> +++ b/drivers/usb/typec/tcpm/Kconfig
> @@ -79,6 +79,7 @@ config TYPEC_WCOVE
>  config TYPEC_QCOM_PMIC
>  	tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on DRM || DRM=n
>  	help
>  	  A Type-C port and Power Delivery driver which aggregates two
>  	  discrete pieces of silicon in the PM8150b PMIC block: the

Would it be an option to put the below in separate c file that you
just compile based on CONFIG_DRM?

        obj-$(CONFIG_TYPEC_QCOM_PMIC)           += qcom_pmic_tcpm.o
        qcom_pmic_tcpm-y                        += qcom_pmic_typec.o \
                                                   qcom_pmic_typec_port.o \
                                                   qcom_pmic_typec_pdphy.o
        ifneq ($(CONFIG_DRM),)
               qcom_pmic_tcpm-y                 += qcom_pmic_bridge_func.o
        endif

Thouse ifdefs in c file just look a bit rough to me.

> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> index af44ee4e6e86..581199d37b49 100644
> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> @@ -17,6 +17,9 @@
>  #include <linux/usb/role.h>
>  #include <linux/usb/tcpm.h>
>  #include <linux/usb/typec_mux.h>
> +
> +#include <drm/drm_bridge.h>
> +
>  #include "qcom_pmic_typec_pdphy.h"
>  #include "qcom_pmic_typec_port.h"
>  
> @@ -33,6 +36,7 @@ struct pmic_typec {
>  	struct pmic_typec_port	*pmic_typec_port;
>  	bool			vbus_enabled;
>  	struct mutex		lock;		/* VBUS state serialization */
> +	struct drm_bridge	bridge;
>  };
>  
>  #define tcpc_to_tcpm(_tcpc_) container_of(_tcpc_, struct pmic_typec, tcpc)
> @@ -146,6 +150,35 @@ static int qcom_pmic_typec_init(struct tcpc_dev *tcpc)
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_DRM)
> +static int qcom_pmic_typec_attach(struct drm_bridge *bridge,
> +				     enum drm_bridge_attach_flags flags)
> +{
> +	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
> +}
> +
> +static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
> +	.attach = qcom_pmic_typec_attach,
> +};
> +
> +static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
> +{
> +	tcpm->bridge.funcs = &qcom_pmic_typec_bridge_funcs;
> +#ifdef CONFIG_OF
> +	tcpm->bridge.of_node = of_get_child_by_name(tcpm->dev->of_node, "connector");
> +#endif
> +	tcpm->bridge.ops = DRM_BRIDGE_OP_HPD;
> +	tcpm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> +
> +	return devm_drm_bridge_add(tcpm->dev, &tcpm->bridge);
> +}
> +#else
> +static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  {
>  	struct pmic_typec *tcpm;
> @@ -208,6 +241,10 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  	mutex_init(&tcpm->lock);
>  	platform_set_drvdata(pdev, tcpm);
>  
> +	ret = qcom_pmic_typec_init_drm(tcpm);
> +	if (ret)
> +		return ret;
> +
>  	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
>  	if (!tcpm->tcpc.fwnode)
>  		return -EINVAL;

thanks,

-- 
heikki

Re: [PATCH v5 2/2] usb: typec: qcom-pmic-typec: register drm_bridge

[Bryan O'Donoghue]

On 22/08/2023 14:54, Heikki Krogerus wrote:
> On Thu, Aug 17, 2023 at 06:08:24PM +0300, Dmitry Baryshkov wrote:
>> The current approach to handling DP on bridge-enabled platforms requires
>> a chain of DP bridges up to the USB-C connector. Register a last DRM
>> bridge for such chain.
>>
>> Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/usb/typec/tcpm/Kconfig                |  1 +
>>   drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 37 +++++++++++++++++++
>>   2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
>> index 5d393f520fc2..0b2993fef564 100644
>> --- a/drivers/usb/typec/tcpm/Kconfig
>> +++ b/drivers/usb/typec/tcpm/Kconfig
>> @@ -79,6 +79,7 @@ config TYPEC_WCOVE
>>   config TYPEC_QCOM_PMIC
>>   	tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>> +	depends on DRM || DRM=n
>>   	help
>>   	  A Type-C port and Power Delivery driver which aggregates two
>>   	  discrete pieces of silicon in the PM8150b PMIC block: the
> 
> Would it be an option to put the below in separate c file that you
> just compile based on CONFIG_DRM?
> 
>          obj-$(CONFIG_TYPEC_QCOM_PMIC)           += qcom_pmic_tcpm.o
>          qcom_pmic_tcpm-y                        += qcom_pmic_typec.o \
>                                                     qcom_pmic_typec_port.o \
>                                                     qcom_pmic_typec_pdphy.o
>          ifneq ($(CONFIG_DRM),)
>                 qcom_pmic_tcpm-y                 += qcom_pmic_bridge_func.o
>          endif
> 
> Thouse ifdefs in c file just look a bit rough to me.

+1 if that's possible.

Sounds nice.

---
bod