[PATCH 1/3] usb: typec: tcpm: Remove tcpc_config configuration mechanism

[PATCH 1/3] usb: typec: tcpm: Remove tcpc_config configuration mechanism

[Hans de Goede]

All configuration can and should be done through fwnodes instead of
through the tcpc_config struct and there are no existing users left of
struct tcpc_config, so lets remove it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 90 ++---------------------------------
 include/linux/usb/tcpm.h      | 41 ----------------
 2 files changed, 3 insertions(+), 128 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 5f61d9977a15..c5fa18759f8e 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -380,9 +380,6 @@ static enum tcpm_state tcpm_default_state(struct tcpm_port *port)
 			return SNK_UNATTACHED;
 		else if (port->try_role == TYPEC_SOURCE)
 			return SRC_UNATTACHED;
-		else if (port->tcpc->config &&
-			 port->tcpc->config->default_role == TYPEC_SINK)
-			return SNK_UNATTACHED;
 		/* Fall through to return SRC_UNATTACHED */
 	} else if (port->port_type == TYPEC_PORT_SNK) {
 		return SNK_UNATTACHED;
@@ -4131,7 +4128,7 @@ static int tcpm_try_role(const struct typec_capability *cap, int role)
 	mutex_lock(&port->lock);
 	if (tcpc->try_role)
 		ret = tcpc->try_role(tcpc, role);
-	if (!ret && (!tcpc->config || !tcpc->config->try_role_hw))
+	if (!ret)
 		port->try_role = role;
 	port->try_src_count = 0;
 	port->try_snk_count = 0;
@@ -4368,34 +4365,6 @@ void tcpm_tcpc_reset(struct tcpm_port *port)
 }
 EXPORT_SYMBOL_GPL(tcpm_tcpc_reset);
 
-static int tcpm_copy_pdos(u32 *dest_pdo, const u32 *src_pdo,
-			  unsigned int nr_pdo)
-{
-	unsigned int i;
-
-	if (nr_pdo > PDO_MAX_OBJECTS)
-		nr_pdo = PDO_MAX_OBJECTS;
-
-	for (i = 0; i < nr_pdo; i++)
-		dest_pdo[i] = src_pdo[i];
-
-	return nr_pdo;
-}
-
-static int tcpm_copy_vdos(u32 *dest_vdo, const u32 *src_vdo,
-			  unsigned int nr_vdo)
-{
-	unsigned int i;
-
-	if (nr_vdo > VDO_MAX_OBJECTS)
-		nr_vdo = VDO_MAX_OBJECTS;
-
-	for (i = 0; i < nr_vdo; i++)
-		dest_vdo[i] = src_vdo[i];
-
-	return nr_vdo;
-}
-
 static int tcpm_fw_get_caps(struct tcpm_port *port,
 			    struct fwnode_handle *fwnode)
 {
@@ -4698,35 +4667,10 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
 	return PTR_ERR_OR_ZERO(port->psy);
 }
 
-static int tcpm_copy_caps(struct tcpm_port *port,
-			  const struct tcpc_config *tcfg)
-{
-	if (tcpm_validate_caps(port, tcfg->src_pdo, tcfg->nr_src_pdo) ||
-	    tcpm_validate_caps(port, tcfg->snk_pdo, tcfg->nr_snk_pdo))
-		return -EINVAL;
-
-	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcfg->src_pdo,
-					  tcfg->nr_src_pdo);
-	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcfg->snk_pdo,
-					  tcfg->nr_snk_pdo);
-
-	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcfg->snk_vdo,
-					  tcfg->nr_snk_vdo);
-
-	port->operating_snk_mw = tcfg->operating_snk_mw;
-
-	port->typec_caps.prefer_role = tcfg->default_role;
-	port->typec_caps.type = tcfg->type;
-	port->typec_caps.data = tcfg->data;
-	port->self_powered = tcfg->self_powered;
-
-	return 0;
-}
-
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 {
 	struct tcpm_port *port;
-	int i, err;
+	int err;
 
 	if (!dev || !tcpc ||
 	    !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc ||
@@ -4759,15 +4703,10 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	tcpm_debugfs_init(port);
 
 	err = tcpm_fw_get_caps(port, tcpc->fwnode);
-	if ((err < 0) && tcpc->config)
-		err = tcpm_copy_caps(port, tcpc->config);
 	if (err < 0)
 		goto out_destroy_wq;
 
-	if (!tcpc->config || !tcpc->config->try_role_hw)
-		port->try_role = port->typec_caps.prefer_role;
-	else
-		port->try_role = TYPEC_NO_PREFERRED_ROLE;
+	port->try_role = port->typec_caps.prefer_role;
 
 	port->typec_caps.fwnode = tcpc->fwnode;
 	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
@@ -4797,29 +4736,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 		goto out_role_sw_put;
 	}
 
-	if (tcpc->config && tcpc->config->alt_modes) {
-		const struct typec_altmode_desc *paltmode = tcpc->config->alt_modes;
-
-		i = 0;
-		while (paltmode->svid && i < ARRAY_SIZE(port->port_altmode)) {
-			struct typec_altmode *alt;
-
-			alt = typec_port_register_altmode(port->typec_port,
-							  paltmode);
-			if (IS_ERR(alt)) {
-				tcpm_log(port,
-					 "%s: failed to register port alternate mode 0x%x",
-					 dev_name(dev), paltmode->svid);
-				break;
-			}
-			typec_altmode_set_drvdata(alt, port);
-			alt->ops = &tcpm_altmode_ops;
-			port->port_altmode[i] = alt;
-			i++;
-			paltmode++;
-		}
-	}
-
 	mutex_lock(&port->lock);
 	tcpm_init(port);
 	mutex_unlock(&port->lock);
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index f516955a0cf4..e7979c01c351 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -46,45 +46,6 @@ enum tcpm_transmit_type {
 	TCPC_TX_BIST_MODE_2 = 7
 };
 
-/**
- * struct tcpc_config - Port configuration
- * @src_pdo:	PDO parameters sent to port partner as response to
- *		PD_CTRL_GET_SOURCE_CAP message
- * @nr_src_pdo:	Number of entries in @src_pdo
- * @snk_pdo:	PDO parameters sent to partner as response to
- *		PD_CTRL_GET_SINK_CAP message
- * @nr_snk_pdo:	Number of entries in @snk_pdo
- * @operating_snk_mw:
- *		Required operating sink power in mW
- * @type:	Port type (TYPEC_PORT_DFP, TYPEC_PORT_UFP, or
- *		TYPEC_PORT_DRP)
- * @default_role:
- *		Default port role (TYPEC_SINK or TYPEC_SOURCE).
- *		Set to TYPEC_NO_PREFERRED_ROLE if no default role.
- * @try_role_hw:True if try.{Src,Snk} is implemented in hardware
- * @alt_modes:	List of supported alternate modes
- */
-struct tcpc_config {
-	const u32 *src_pdo;
-	unsigned int nr_src_pdo;
-
-	const u32 *snk_pdo;
-	unsigned int nr_snk_pdo;
-
-	const u32 *snk_vdo;
-	unsigned int nr_snk_vdo;
-
-	unsigned int operating_snk_mw;
-
-	enum typec_port_type type;
-	enum typec_port_data data;
-	enum typec_role default_role;
-	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
-	bool self_powered;	/* port belongs to a self powered device */
-
-	const struct typec_altmode_desc *alt_modes;
-};
-
 /* Mux state attributes */
 #define TCPC_MUX_USB_ENABLED		BIT(0)	/* USB enabled */
 #define TCPC_MUX_DP_ENABLED		BIT(1)	/* DP enabled */
@@ -92,7 +53,6 @@ struct tcpc_config {
 
 /**
  * struct tcpc_dev - Port configuration and callback functions
- * @config:	Pointer to port configuration
  * @fwnode:	Pointer to port fwnode
  * @get_vbus:	Called to read current VBUS state
  * @get_current_limit:
@@ -121,7 +81,6 @@ struct tcpc_config {
  * @mux:	Pointer to multiplexer data
  */
 struct tcpc_dev {
-	const struct tcpc_config *config;
 	struct fwnode_handle *fwnode;
 
 	int (*init)(struct tcpc_dev *dev);
-- 
2.23.0

[PATCH 2/3] usb: typec: tcpm: Add support for configuring DP altmode through device-properties

[Hans de Goede]

Add support for configuring display-port altmode through device-properties.

We could try to add a generic mechanism for describing altmodes in
device-properties, but various altmodes will likely need altmode specific
configuration. E.g. the display-port altmode needs some way to describe
which set of DP pins on the GPU is connected to the USB Type-C connector.

As such it is better to have a separate set of altmode specific properties
per altmode and this commit adds a property for basic display-port altmode
support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../bindings/connector/usb-connector.txt      |  3 ++
 drivers/usb/typec/tcpm/tcpm.c                 | 33 +++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
index d357987181ee..7bae3cc9c76a 100644
--- a/Documentation/devicetree/bindings/connector/usb-connector.txt
+++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
@@ -38,6 +38,9 @@ Optional properties for usb-c-connector:
   or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
 - data-role: should be one of "host", "device", "dual"(DRD) if typec
   connector supports USB data.
+- displayport-vdo: The presenence of this property indicates that the
+  usb-connector supports displayport-altmode (svid 0xff01), the value of
+  this property is an u32 with the vdo value for the displayport-altmode,
 
 Required properties for usb-c-connector with power delivery support:
 - source-pdos: An array of u32 with each entry providing supported power
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index c5fa18759f8e..2e3096657e96 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -28,6 +28,7 @@
 #include <linux/usb/role.h>
 #include <linux/usb/tcpm.h>
 #include <linux/usb/typec_altmode.h>
+#include <linux/usb/typec_dp.h>
 #include <linux/workqueue.h>
 
 #define FOREACH_STATE(S)			\
@@ -281,6 +282,7 @@ struct tcpm_port {
 	unsigned int nr_snk_pdo;
 	u32 snk_vdo[VDO_MAX_OBJECTS];
 	unsigned int nr_snk_vdo;
+	u32 displayport_vdo;
 
 	unsigned int operating_snk_mw;
 	bool update_sink_caps;
@@ -4433,6 +4435,9 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 					    port->nr_snk_pdo))
 		return -EINVAL;
 
+	fwnode_property_read_u32(fwnode, "displayport-vdo",
+				 &port->displayport_vdo);
+
 	if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
 		return -EINVAL;
 	port->operating_snk_mw = mw / 1000;
@@ -4667,6 +4672,28 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
 	return PTR_ERR_OR_ZERO(port->psy);
 }
 
+static int tcpm_register_port_altmodes(struct tcpm_port *port)
+{
+	struct typec_altmode_desc desc;
+	struct typec_altmode *alt;
+	int index = 0;
+
+	if (port->displayport_vdo) {
+		desc.svid = USB_TYPEC_DP_SID;
+		desc.mode = USB_TYPEC_DP_MODE;
+		desc.vdo  = port->displayport_vdo;
+		alt = typec_port_register_altmode(port->typec_port, &desc);
+		if (IS_ERR(alt))
+			return PTR_ERR(alt);
+		typec_altmode_set_drvdata(alt, port);
+		alt->ops = &tcpm_altmode_ops;
+		port->port_altmode[index] = alt;
+		index++;
+	}
+	/* Future support for further altmodes goes here */
+	return 0;
+}
+
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 {
 	struct tcpm_port *port;
@@ -4736,6 +4763,10 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 		goto out_role_sw_put;
 	}
 
+	err = tcpm_register_port_altmodes(port);
+	if (err)
+		goto out_unregister_port;
+
 	mutex_lock(&port->lock);
 	tcpm_init(port);
 	mutex_unlock(&port->lock);
@@ -4743,6 +4774,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	tcpm_log(port, "%s: registered", dev_name(dev));
 	return port;
 
+out_unregister_port:
+	typec_unregister_port(port->typec_port);
 out_role_sw_put:
 	usb_role_switch_put(port->role_sw);
 out_destroy_wq:
-- 
2.23.0

[PATCH 3/3] platform/x86/intel_cht_int33fe: Add displayport-vdo property to the connector node

[Hans de Goede]

Add a displayport-vdo property to the usb-connector fwnode, devices
which use this driver support display-port altmode through the
PI3USB30532 USB switch, this enabled support for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 1d5d877b9582..61f10b633678 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -129,6 +129,7 @@ static const struct property_entry usb_connector_props[] = {
 	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
 	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
 	PROPERTY_ENTRY_U32("op-sink-microwatt", 2500000),
+	PROPERTY_ENTRY_U32("displayport-vdo", 0x0c0086),
 	{ }
 };
 
-- 
2.23.0

Re: [PATCH 1/3] usb: typec: tcpm: Remove tcpc_config configuration mechanism

[Guenter Roeck]

On Fri, Oct 18, 2019 at 09:57:17PM +0200, Hans de Goede wrote:
> All configuration can and should be done through fwnodes instead of
> through the tcpc_config struct and there are no existing users left of
> struct tcpc_config, so lets remove it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 90 ++---------------------------------
>  include/linux/usb/tcpm.h      | 41 ----------------
>  2 files changed, 3 insertions(+), 128 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 5f61d9977a15..c5fa18759f8e 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -380,9 +380,6 @@ static enum tcpm_state tcpm_default_state(struct tcpm_port *port)
>  			return SNK_UNATTACHED;
>  		else if (port->try_role == TYPEC_SOURCE)
>  			return SRC_UNATTACHED;
> -		else if (port->tcpc->config &&
> -			 port->tcpc->config->default_role == TYPEC_SINK)
> -			return SNK_UNATTACHED;
>  		/* Fall through to return SRC_UNATTACHED */
>  	} else if (port->port_type == TYPEC_PORT_SNK) {
>  		return SNK_UNATTACHED;
> @@ -4131,7 +4128,7 @@ static int tcpm_try_role(const struct typec_capability *cap, int role)
>  	mutex_lock(&port->lock);
>  	if (tcpc->try_role)
>  		ret = tcpc->try_role(tcpc, role);
> -	if (!ret && (!tcpc->config || !tcpc->config->try_role_hw))
> +	if (!ret)
>  		port->try_role = role;
>  	port->try_src_count = 0;
>  	port->try_snk_count = 0;
> @@ -4368,34 +4365,6 @@ void tcpm_tcpc_reset(struct tcpm_port *port)
>  }
>  EXPORT_SYMBOL_GPL(tcpm_tcpc_reset);
>  
> -static int tcpm_copy_pdos(u32 *dest_pdo, const u32 *src_pdo,
> -			  unsigned int nr_pdo)
> -{
> -	unsigned int i;
> -
> -	if (nr_pdo > PDO_MAX_OBJECTS)
> -		nr_pdo = PDO_MAX_OBJECTS;
> -
> -	for (i = 0; i < nr_pdo; i++)
> -		dest_pdo[i] = src_pdo[i];
> -
> -	return nr_pdo;
> -}
> -
> -static int tcpm_copy_vdos(u32 *dest_vdo, const u32 *src_vdo,
> -			  unsigned int nr_vdo)
> -{
> -	unsigned int i;
> -
> -	if (nr_vdo > VDO_MAX_OBJECTS)
> -		nr_vdo = VDO_MAX_OBJECTS;
> -
> -	for (i = 0; i < nr_vdo; i++)
> -		dest_vdo[i] = src_vdo[i];
> -
> -	return nr_vdo;
> -}
> -
>  static int tcpm_fw_get_caps(struct tcpm_port *port,
>  			    struct fwnode_handle *fwnode)
>  {
> @@ -4698,35 +4667,10 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
>  	return PTR_ERR_OR_ZERO(port->psy);
>  }
>  
> -static int tcpm_copy_caps(struct tcpm_port *port,
> -			  const struct tcpc_config *tcfg)
> -{
> -	if (tcpm_validate_caps(port, tcfg->src_pdo, tcfg->nr_src_pdo) ||
> -	    tcpm_validate_caps(port, tcfg->snk_pdo, tcfg->nr_snk_pdo))
> -		return -EINVAL;
> -
> -	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcfg->src_pdo,
> -					  tcfg->nr_src_pdo);
> -	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcfg->snk_pdo,
> -					  tcfg->nr_snk_pdo);
> -
> -	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcfg->snk_vdo,
> -					  tcfg->nr_snk_vdo);
> -
> -	port->operating_snk_mw = tcfg->operating_snk_mw;
> -
> -	port->typec_caps.prefer_role = tcfg->default_role;
> -	port->typec_caps.type = tcfg->type;
> -	port->typec_caps.data = tcfg->data;
> -	port->self_powered = tcfg->self_powered;
> -
> -	return 0;
> -}
> -
>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  {
>  	struct tcpm_port *port;
> -	int i, err;
> +	int err;
>  
>  	if (!dev || !tcpc ||
>  	    !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc ||
> @@ -4759,15 +4703,10 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  	tcpm_debugfs_init(port);
>  
>  	err = tcpm_fw_get_caps(port, tcpc->fwnode);
> -	if ((err < 0) && tcpc->config)
> -		err = tcpm_copy_caps(port, tcpc->config);
>  	if (err < 0)
>  		goto out_destroy_wq;
>  
> -	if (!tcpc->config || !tcpc->config->try_role_hw)
> -		port->try_role = port->typec_caps.prefer_role;
> -	else
> -		port->try_role = TYPEC_NO_PREFERRED_ROLE;
> +	port->try_role = port->typec_caps.prefer_role;
>  
>  	port->typec_caps.fwnode = tcpc->fwnode;
>  	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
> @@ -4797,29 +4736,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  		goto out_role_sw_put;
>  	}
>  
> -	if (tcpc->config && tcpc->config->alt_modes) {
> -		const struct typec_altmode_desc *paltmode = tcpc->config->alt_modes;
> -
> -		i = 0;
> -		while (paltmode->svid && i < ARRAY_SIZE(port->port_altmode)) {
> -			struct typec_altmode *alt;
> -
> -			alt = typec_port_register_altmode(port->typec_port,
> -							  paltmode);
> -			if (IS_ERR(alt)) {
> -				tcpm_log(port,
> -					 "%s: failed to register port alternate mode 0x%x",
> -					 dev_name(dev), paltmode->svid);
> -				break;
> -			}
> -			typec_altmode_set_drvdata(alt, port);
> -			alt->ops = &tcpm_altmode_ops;
> -			port->port_altmode[i] = alt;
> -			i++;
> -			paltmode++;
> -		}
> -	}
> -
>  	mutex_lock(&port->lock);
>  	tcpm_init(port);
>  	mutex_unlock(&port->lock);
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index f516955a0cf4..e7979c01c351 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -46,45 +46,6 @@ enum tcpm_transmit_type {
>  	TCPC_TX_BIST_MODE_2 = 7
>  };
>  
> -/**
> - * struct tcpc_config - Port configuration
> - * @src_pdo:	PDO parameters sent to port partner as response to
> - *		PD_CTRL_GET_SOURCE_CAP message
> - * @nr_src_pdo:	Number of entries in @src_pdo
> - * @snk_pdo:	PDO parameters sent to partner as response to
> - *		PD_CTRL_GET_SINK_CAP message
> - * @nr_snk_pdo:	Number of entries in @snk_pdo
> - * @operating_snk_mw:
> - *		Required operating sink power in mW
> - * @type:	Port type (TYPEC_PORT_DFP, TYPEC_PORT_UFP, or
> - *		TYPEC_PORT_DRP)
> - * @default_role:
> - *		Default port role (TYPEC_SINK or TYPEC_SOURCE).
> - *		Set to TYPEC_NO_PREFERRED_ROLE if no default role.
> - * @try_role_hw:True if try.{Src,Snk} is implemented in hardware
> - * @alt_modes:	List of supported alternate modes
> - */
> -struct tcpc_config {
> -	const u32 *src_pdo;
> -	unsigned int nr_src_pdo;
> -
> -	const u32 *snk_pdo;
> -	unsigned int nr_snk_pdo;
> -
> -	const u32 *snk_vdo;
> -	unsigned int nr_snk_vdo;
> -
> -	unsigned int operating_snk_mw;
> -
> -	enum typec_port_type type;
> -	enum typec_port_data data;
> -	enum typec_role default_role;
> -	bool try_role_hw;	/* try.{src,snk} implemented in hardware */
> -	bool self_powered;	/* port belongs to a self powered device */
> -
> -	const struct typec_altmode_desc *alt_modes;
> -};
> -
>  /* Mux state attributes */
>  #define TCPC_MUX_USB_ENABLED		BIT(0)	/* USB enabled */
>  #define TCPC_MUX_DP_ENABLED		BIT(1)	/* DP enabled */
> @@ -92,7 +53,6 @@ struct tcpc_config {
>  
>  /**
>   * struct tcpc_dev - Port configuration and callback functions
> - * @config:	Pointer to port configuration
>   * @fwnode:	Pointer to port fwnode
>   * @get_vbus:	Called to read current VBUS state
>   * @get_current_limit:
> @@ -121,7 +81,6 @@ struct tcpc_config {
>   * @mux:	Pointer to multiplexer data
>   */
>  struct tcpc_dev {
> -	const struct tcpc_config *config;
>  	struct fwnode_handle *fwnode;
>  
>  	int (*init)(struct tcpc_dev *dev);
> -- 
> 2.23.0
> 

Re: [PATCH 2/3] usb: typec: tcpm: Add support for configuring DP altmode through device-properties

[Guenter Roeck]

On 10/18/19 12:57 PM, Hans de Goede wrote:
> Add support for configuring display-port altmode through device-properties.
> 
> We could try to add a generic mechanism for describing altmodes in
> device-properties, but various altmodes will likely need altmode specific
> configuration. E.g. the display-port altmode needs some way to describe
> which set of DP pins on the GPU is connected to the USB Type-C connector.
> 
> As such it is better to have a separate set of altmode specific properties
> per altmode and this commit adds a property for basic display-port altmode
> support.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   .../bindings/connector/usb-connector.txt      |  3 ++
>   drivers/usb/typec/tcpm/tcpm.c                 | 33 +++++++++++++++++++
>   2 files changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> index d357987181ee..7bae3cc9c76a 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -38,6 +38,9 @@ Optional properties for usb-c-connector:
>     or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
>   - data-role: should be one of "host", "device", "dual"(DRD) if typec
>     connector supports USB data.
> +- displayport-vdo: The presenence of this property indicates that the

 From a DT perspective, I wonder if the vdo properties should be listed
explicitly (capabilities, signaling, receptacle etc) or if it is ok to list
a single value. Either case, I wonder if the VDO should be explained in
more detail.

> +  usb-connector supports displayport-altmode (svid 0xff01), the value of
> +  this property is an u32 with the vdo value for the displayport-altmode,
>   

The added property will require approval by a DT maintainer.

>   Required properties for usb-c-connector with power delivery support:
>   - source-pdos: An array of u32 with each entry providing supported power
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index c5fa18759f8e..2e3096657e96 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -28,6 +28,7 @@
>   #include <linux/usb/role.h>
>   #include <linux/usb/tcpm.h>
>   #include <linux/usb/typec_altmode.h>
> +#include <linux/usb/typec_dp.h>
>   #include <linux/workqueue.h>
>   
>   #define FOREACH_STATE(S)			\
> @@ -281,6 +282,7 @@ struct tcpm_port {
>   	unsigned int nr_snk_pdo;
>   	u32 snk_vdo[VDO_MAX_OBJECTS];
>   	unsigned int nr_snk_vdo;
> +	u32 displayport_vdo;
>   
>   	unsigned int operating_snk_mw;
>   	bool update_sink_caps;
> @@ -4433,6 +4435,9 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>   					    port->nr_snk_pdo))
>   		return -EINVAL;
>   
> +	fwnode_property_read_u32(fwnode, "displayport-vdo",
> +				 &port->displayport_vdo);
> +
>   	if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
>   		return -EINVAL;
>   	port->operating_snk_mw = mw / 1000;
> @@ -4667,6 +4672,28 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
>   	return PTR_ERR_OR_ZERO(port->psy);
>   }
>   
> +static int tcpm_register_port_altmodes(struct tcpm_port *port)
> +{
> +	struct typec_altmode_desc desc;
> +	struct typec_altmode *alt;
> +	int index = 0;
> +
> +	if (port->displayport_vdo) {
> +		desc.svid = USB_TYPEC_DP_SID;
> +		desc.mode = USB_TYPEC_DP_MODE;
> +		desc.vdo  = port->displayport_vdo;
> +		alt = typec_port_register_altmode(port->typec_port, &desc);
> +		if (IS_ERR(alt))
> +			return PTR_ERR(alt);
> +		typec_altmode_set_drvdata(alt, port);
> +		alt->ops = &tcpm_altmode_ops;
> +		port->port_altmode[index] = alt;
> +		index++;
> +	}
> +	/* Future support for further altmodes goes here */
> +	return 0;
> +}
> +
>   struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>   {
>   	struct tcpm_port *port;
> @@ -4736,6 +4763,10 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>   		goto out_role_sw_put;
>   	}
>   
> +	err = tcpm_register_port_altmodes(port);
> +	if (err)
> +		goto out_unregister_port;
> +
>   	mutex_lock(&port->lock);
>   	tcpm_init(port);
>   	mutex_unlock(&port->lock);
> @@ -4743,6 +4774,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>   	tcpm_log(port, "%s: registered", dev_name(dev));
>   	return port;
>   
> +out_unregister_port:
> +	typec_unregister_port(port->typec_port);
>   out_role_sw_put:
>   	usb_role_switch_put(port->role_sw);
>   out_destroy_wq:
> 

Re: [PATCH 2/3] usb: typec: tcpm: Add support for configuring DP altmode through device-properties

[Heikki Krogerus]

Hi Hans,

On Fri, Oct 18, 2019 at 09:57:18PM +0200, Hans de Goede wrote:
> Add support for configuring display-port altmode through device-properties.
> 
> We could try to add a generic mechanism for describing altmodes in
> device-properties, but various altmodes will likely need altmode specific
> configuration. E.g. the display-port altmode needs some way to describe
> which set of DP pins on the GPU is connected to the USB Type-C connector.
> 
> As such it is better to have a separate set of altmode specific properties
> per altmode and this commit adds a property for basic display-port altmode
> support.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../bindings/connector/usb-connector.txt      |  3 ++
>  drivers/usb/typec/tcpm/tcpm.c                 | 33 +++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> index d357987181ee..7bae3cc9c76a 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -38,6 +38,9 @@ Optional properties for usb-c-connector:
>    or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
>  - data-role: should be one of "host", "device", "dual"(DRD) if typec
>    connector supports USB data.
> +- displayport-vdo: The presenence of this property indicates that the
> +  usb-connector supports displayport-altmode (svid 0xff01), the value of
> +  this property is an u32 with the vdo value for the displayport-altmode,

No, let's not take this approach.

Every alternate mode a connector supports will need to have its own
"sub-fwnode" under the connector fwnode. I thought we agreed this
earlier?

In any case, those sub-nodes will have default device properties named
"svid" and "vdo". If the alternate mode still needs some other
details, it can have other device properties that are specific to it,
but note that displayport alt mode does not need anything extra. The
"vdo" will already tells which pin configurations the connector
supports and that is all that the driver needs to know.

After we have the sub-nodes, it's not a big deal to walk through the
child-nodes the port has during port registration and register the
port alternate modes at the same time. That we can do in
typec_register_port(), so we do not need to do it in every driver
separately.


thanks,

-- 
heikki

Re: [PATCH 2/3] usb: typec: tcpm: Add support for configuring DP altmode through device-properties

[Hans de Goede]

Hi,

On 21-10-2019 08:55, Heikki Krogerus wrote:
> Hi Hans,
> 
> On Fri, Oct 18, 2019 at 09:57:18PM +0200, Hans de Goede wrote:
>> Add support for configuring display-port altmode through device-properties.
>>
>> We could try to add a generic mechanism for describing altmodes in
>> device-properties, but various altmodes will likely need altmode specific
>> configuration. E.g. the display-port altmode needs some way to describe
>> which set of DP pins on the GPU is connected to the USB Type-C connector.
>>
>> As such it is better to have a separate set of altmode specific properties
>> per altmode and this commit adds a property for basic display-port altmode
>> support.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   .../bindings/connector/usb-connector.txt      |  3 ++
>>   drivers/usb/typec/tcpm/tcpm.c                 | 33 +++++++++++++++++++
>>   2 files changed, 36 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>> index d357987181ee..7bae3cc9c76a 100644
>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>> @@ -38,6 +38,9 @@ Optional properties for usb-c-connector:
>>     or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
>>   - data-role: should be one of "host", "device", "dual"(DRD) if typec
>>     connector supports USB data.
>> +- displayport-vdo: The presenence of this property indicates that the
>> +  usb-connector supports displayport-altmode (svid 0xff01), the value of
>> +  this property is an u32 with the vdo value for the displayport-altmode,
> 
> No, let's not take this approach.
> 
> Every alternate mode a connector supports will need to have its own
> "sub-fwnode" under the connector fwnode. I thought we agreed this
> earlier?
> 
> In any case, those sub-nodes will have default device properties named
> "svid" and "vdo". If the alternate mode still needs some other
> details, it can have other device properties that are specific to it,
> but note that displayport alt mode does not need anything extra. The
> "vdo" will already tells which pin configurations the connector
> supports and that is all that the driver needs to know.
> 
> After we have the sub-nodes, it's not a big deal to walk through the
> child-nodes the port has during port registration and register the
> port alternate modes at the same time. That we can do in
> typec_register_port(), so we do not need to do it in every driver
> separately.

Yes we did agree to do the sub-fwnode thingie. But since this is a hobby
project I do not have a whole lot of time to work on this.

So when I started working on this, I though that the approach from this
patch-set would be more KISS and IMHO it works out well. But the sub-fwnode
approach is probably more future proof.

Anyways as said I do not have a whole lot of time to work on this,
if you want to go the sub-fwnode route, perhaps you can do a PoC
patch series for this? I would be happy to test this and if necessary
work it into something which works for the DP case.

Doing the port alternate modes registration from typec_register_port()
does sound like a good idea.

The first patch in this series is independent of this and IMHO it
would be good to get that upstream regardless of this alt-mode
registration stuff, so I will resend that as a standalone patch.

Regards,

Hans

Re: [PATCH 2/3] usb: typec: tcpm: Add support for configuring DP altmode through device-properties

[Heikki Krogerus]

On Thu, Nov 14, 2019 at 12:16:09PM +0100, Hans de Goede wrote:
> 
> Hi,
> 
> On 21-10-2019 08:55, Heikki Krogerus wrote:
> > Hi Hans,
> > 
> > On Fri, Oct 18, 2019 at 09:57:18PM +0200, Hans de Goede wrote:
> > > Add support for configuring display-port altmode through device-properties.
> > > 
> > > We could try to add a generic mechanism for describing altmodes in
> > > device-properties, but various altmodes will likely need altmode specific
> > > configuration. E.g. the display-port altmode needs some way to describe
> > > which set of DP pins on the GPU is connected to the USB Type-C connector.
> > > 
> > > As such it is better to have a separate set of altmode specific properties
> > > per altmode and this commit adds a property for basic display-port altmode
> > > support.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   .../bindings/connector/usb-connector.txt      |  3 ++
> > >   drivers/usb/typec/tcpm/tcpm.c                 | 33 +++++++++++++++++++
> > >   2 files changed, 36 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > index d357987181ee..7bae3cc9c76a 100644
> > > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > @@ -38,6 +38,9 @@ Optional properties for usb-c-connector:
> > >     or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > >   - data-role: should be one of "host", "device", "dual"(DRD) if typec
> > >     connector supports USB data.
> > > +- displayport-vdo: The presenence of this property indicates that the
> > > +  usb-connector supports displayport-altmode (svid 0xff01), the value of
> > > +  this property is an u32 with the vdo value for the displayport-altmode,
> > 
> > No, let's not take this approach.
> > 
> > Every alternate mode a connector supports will need to have its own
> > "sub-fwnode" under the connector fwnode. I thought we agreed this
> > earlier?
> > 
> > In any case, those sub-nodes will have default device properties named
> > "svid" and "vdo". If the alternate mode still needs some other
> > details, it can have other device properties that are specific to it,
> > but note that displayport alt mode does not need anything extra. The
> > "vdo" will already tells which pin configurations the connector
> > supports and that is all that the driver needs to know.
> > 
> > After we have the sub-nodes, it's not a big deal to walk through the
> > child-nodes the port has during port registration and register the
> > port alternate modes at the same time. That we can do in
> > typec_register_port(), so we do not need to do it in every driver
> > separately.
> 
> Yes we did agree to do the sub-fwnode thingie. But since this is a hobby
> project I do not have a whole lot of time to work on this.
> 
> So when I started working on this, I though that the approach from this
> patch-set would be more KISS and IMHO it works out well. But the sub-fwnode
> approach is probably more future proof.
> 
> Anyways as said I do not have a whole lot of time to work on this,
> if you want to go the sub-fwnode route, perhaps you can do a PoC
> patch series for this? I would be happy to test this and if necessary
> work it into something which works for the DP case.

Sure, I'll prepare something for that once I have some spare time.

> Doing the port alternate modes registration from typec_register_port()
> does sound like a good idea.
> 
> The first patch in this series is independent of this and IMHO it
> would be good to get that upstream regardless of this alt-mode
> registration stuff, so I will resend that as a standalone patch.

OK,

thanks,

-- 
heikki