[PATCH v2 0/4] usb: typec: hd3ss3220: enhance driver with port type, power opmode, and role preference settings

[PATCH v2 0/4] usb: typec: hd3ss3220: enhance driver with port type, power opmode, and role preference settings

[Oliver Facklam]

The TI HD3SS3220 Type-C controller supports configuring the port type,
the advertised power operation mode, and its role preference
through its I2C register map.

This patch series adds support for configuring these registers
based on existing fwnode properties and typec_operations.

Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
---
Changes in v2:
- Better support optional fwnode properties being absent by setting
  defaults in struct typec_capability and checking return code of
  typec_get_fw_cap()
- Link to v1: https://lore.kernel.org/r/20241107-usb-typec-controller-enhancements-v1-0-3886c1acced2@zuehlke.com

---
Oliver Facklam (4):
      usb: typec: hd3ss3220: configure advertised power opmode based on fwnode property
      usb: typec: hd3ss3220: use typec_get_fw_cap() to fill typec_cap
      usb: typec: hd3ss3220: support configuring port type
      usb: typec: hd3ss3220: support configuring role preference based on fwnode property and typec_operation

 drivers/usb/typec/hd3ss3220.c | 172 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 148 insertions(+), 24 deletions(-)
---
base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230
change-id: 20241104-usb-typec-controller-enhancements-6871249429f0

Best regards,
-- 
Oliver Facklam <oliver.facklam@zuehlke.com>

[PATCH v2 1/4] usb: typec: hd3ss3220: configure advertised power opmode based on fwnode property

[Oliver Facklam]

The TI HD3SS3220 Type-C controller supports configuring its advertised
power operation mode over I2C using the CURRENT_MODE_ADVERTISE field
of the Connection Status Register.

Configure this power mode based on the existing (optional) property
"typec-power-opmode" of /schemas/connector/usb-connector.yaml

Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
---
 drivers/usb/typec/hd3ss3220.c | 53 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index fb1242e82ffdc64a9a3330f50155bb8f0fe45685..56f74bf70895ca701083bde44a5bbe0b691551e1 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -16,10 +16,17 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
+#define HD3SS3220_REG_CN_STAT		0x08
 #define HD3SS3220_REG_CN_STAT_CTRL	0x09
 #define HD3SS3220_REG_GEN_CTRL		0x0A
 #define HD3SS3220_REG_DEV_REV		0xA0
 
+/* Register HD3SS3220_REG_CN_STAT */
+#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_MASK		(BIT(7) | BIT(6))
+#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_DEFAULT	0x00
+#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_MID		BIT(6)
+#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_HIGH		BIT(7)
+
 /* Register HD3SS3220_REG_CN_STAT_CTRL*/
 #define HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK	(BIT(7) | BIT(6))
 #define HD3SS3220_REG_CN_STAT_CTRL_AS_DFP		BIT(6)
@@ -43,6 +50,31 @@ struct hd3ss3220 {
 	bool poll;
 };
 
+static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opmode)
+{
+	int current_mode;
+
+	switch (power_opmode) {
+	case TYPEC_PWR_MODE_USB:
+		current_mode = HD3SS3220_REG_CN_STAT_CURRENT_MODE_DEFAULT;
+		break;
+	case TYPEC_PWR_MODE_1_5A:
+		current_mode = HD3SS3220_REG_CN_STAT_CURRENT_MODE_MID;
+		break;
+	case TYPEC_PWR_MODE_3_0A:
+		current_mode = HD3SS3220_REG_CN_STAT_CURRENT_MODE_HIGH;
+		break;
+	case TYPEC_PWR_MODE_PD: /* Power delivery not supported */
+	default:
+		dev_err(hd3ss3220->dev, "bad power operation mode: %d\n", power_opmode);
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT,
+				  HD3SS3220_REG_CN_STAT_CURRENT_MODE_MASK,
+				  current_mode);
+}
+
 static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
 {
 	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
@@ -162,6 +194,23 @@ static irqreturn_t hd3ss3220_irq_handler(int irq, void *data)
 	return hd3ss3220_irq(hd3ss3220);
 }
 
+static int hd3ss3220_configure_power_opmode(struct hd3ss3220 *hd3ss3220,
+					    struct fwnode_handle *connector)
+{
+	/*
+	 * Supported power operation mode can be configured through device tree
+	 */
+	const char *cap_str;
+	int ret, power_opmode;
+
+	ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str);
+	if (ret)
+		return 0;
+
+	power_opmode = typec_find_pwr_opmode(cap_str);
+	return hd3ss3220_set_power_opmode(hd3ss3220, power_opmode);
+}
+
 static const struct regmap_config config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -223,6 +272,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
 		goto err_put_role;
 	}
 
+	ret = hd3ss3220_configure_power_opmode(hd3ss3220, connector);
+	if (ret < 0)
+		goto err_unreg_port;
+
 	hd3ss3220_set_role(hd3ss3220);
 	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL, &data);
 	if (ret < 0)

-- 
2.34.1

Re: [PATCH v2 1/4] usb: typec: hd3ss3220: configure advertised power opmode based on fwnode property

[Heikki Krogerus]

On Thu, Nov 14, 2024 at 09:02:06AM +0100, Oliver Facklam wrote:
> The TI HD3SS3220 Type-C controller supports configuring its advertised
> power operation mode over I2C using the CURRENT_MODE_ADVERTISE field
> of the Connection Status Register.
> 
> Configure this power mode based on the existing (optional) property
> "typec-power-opmode" of /schemas/connector/usb-connector.yaml
> 
> Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>

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

> ---
>  drivers/usb/typec/hd3ss3220.c | 53 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index fb1242e82ffdc64a9a3330f50155bb8f0fe45685..56f74bf70895ca701083bde44a5bbe0b691551e1 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -16,10 +16,17 @@
>  #include <linux/delay.h>
>  #include <linux/workqueue.h>
>  
> +#define HD3SS3220_REG_CN_STAT		0x08
>  #define HD3SS3220_REG_CN_STAT_CTRL	0x09
>  #define HD3SS3220_REG_GEN_CTRL		0x0A
>  #define HD3SS3220_REG_DEV_REV		0xA0
>  
> +/* Register HD3SS3220_REG_CN_STAT */
> +#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_MASK		(BIT(7) | BIT(6))
> +#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_DEFAULT	0x00
> +#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_MID		BIT(6)
> +#define HD3SS3220_REG_CN_STAT_CURRENT_MODE_HIGH		BIT(7)
> +
>  /* Register HD3SS3220_REG_CN_STAT_CTRL*/
>  #define HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK	(BIT(7) | BIT(6))
>  #define HD3SS3220_REG_CN_STAT_CTRL_AS_DFP		BIT(6)
> @@ -43,6 +50,31 @@ struct hd3ss3220 {
>  	bool poll;
>  };
>  
> +static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opmode)
> +{
> +	int current_mode;
> +
> +	switch (power_opmode) {
> +	case TYPEC_PWR_MODE_USB:
> +		current_mode = HD3SS3220_REG_CN_STAT_CURRENT_MODE_DEFAULT;
> +		break;
> +	case TYPEC_PWR_MODE_1_5A:
> +		current_mode = HD3SS3220_REG_CN_STAT_CURRENT_MODE_MID;
> +		break;
> +	case TYPEC_PWR_MODE_3_0A:
> +		current_mode = HD3SS3220_REG_CN_STAT_CURRENT_MODE_HIGH;
> +		break;
> +	case TYPEC_PWR_MODE_PD: /* Power delivery not supported */
> +	default:
> +		dev_err(hd3ss3220->dev, "bad power operation mode: %d\n", power_opmode);
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT,
> +				  HD3SS3220_REG_CN_STAT_CURRENT_MODE_MASK,
> +				  current_mode);
> +}
> +
>  static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
>  {
>  	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> @@ -162,6 +194,23 @@ static irqreturn_t hd3ss3220_irq_handler(int irq, void *data)
>  	return hd3ss3220_irq(hd3ss3220);
>  }
>  
> +static int hd3ss3220_configure_power_opmode(struct hd3ss3220 *hd3ss3220,
> +					    struct fwnode_handle *connector)
> +{
> +	/*
> +	 * Supported power operation mode can be configured through device tree
> +	 */
> +	const char *cap_str;
> +	int ret, power_opmode;
> +
> +	ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str);
> +	if (ret)
> +		return 0;
> +
> +	power_opmode = typec_find_pwr_opmode(cap_str);
> +	return hd3ss3220_set_power_opmode(hd3ss3220, power_opmode);
> +}
> +
>  static const struct regmap_config config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -223,6 +272,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  		goto err_put_role;
>  	}
>  
> +	ret = hd3ss3220_configure_power_opmode(hd3ss3220, connector);
> +	if (ret < 0)
> +		goto err_unreg_port;
> +
>  	hd3ss3220_set_role(hd3ss3220);
>  	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL, &data);
>  	if (ret < 0)
> 
> -- 
> 2.34.1

-- 
heikki

[PATCH v2 2/4] usb: typec: hd3ss3220: use typec_get_fw_cap() to fill typec_cap

[Oliver Facklam]

The type, data, and prefer_role properties were previously hard-coded
when creating the struct typec_capability.

Use typec_get_fw_cap() to populate these fields based on the
respective fwnode properties, if present.

Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
---
 drivers/usb/typec/hd3ss3220.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 56f74bf70895ca701083bde44a5bbe0b691551e1..e581272bb47de95dee8363a5491f543354fcbbf8 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -264,7 +264,14 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	typec_cap.type = TYPEC_PORT_DRP;
 	typec_cap.data = TYPEC_PORT_DRD;
 	typec_cap.ops = &hd3ss3220_ops;
-	typec_cap.fwnode = connector;
+
+	/*
+	 * Try to get properties from connector,
+	 * but continue with defaults anyway if they are not found
+	 */
+	ret = typec_get_fw_cap(&typec_cap, connector);
+	if (ret != 0 && ret != -EINVAL && ret != -ENXIO)
+		goto err_put_role;
 
 	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(hd3ss3220->port)) {

-- 
2.34.1

Re: [PATCH v2 2/4] usb: typec: hd3ss3220: use typec_get_fw_cap() to fill typec_cap

[Heikki Krogerus]

On Thu, Nov 14, 2024 at 09:02:07AM +0100, Oliver Facklam wrote:
> The type, data, and prefer_role properties were previously hard-coded
> when creating the struct typec_capability.
> 
> Use typec_get_fw_cap() to populate these fields based on the
> respective fwnode properties, if present.
> 
> Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>

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

> ---
>  drivers/usb/typec/hd3ss3220.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index 56f74bf70895ca701083bde44a5bbe0b691551e1..e581272bb47de95dee8363a5491f543354fcbbf8 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -264,7 +264,14 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  	typec_cap.type = TYPEC_PORT_DRP;
>  	typec_cap.data = TYPEC_PORT_DRD;
>  	typec_cap.ops = &hd3ss3220_ops;
> -	typec_cap.fwnode = connector;
> +
> +	/*
> +	 * Try to get properties from connector,
> +	 * but continue with defaults anyway if they are not found
> +	 */
> +	ret = typec_get_fw_cap(&typec_cap, connector);
> +	if (ret != 0 && ret != -EINVAL && ret != -ENXIO)
> +		goto err_put_role;
>  
>  	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
>  	if (IS_ERR(hd3ss3220->port)) {
> 
> -- 
> 2.34.1

-- 
heikki

[PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Oliver Facklam]

The TI HD3SS3220 Type-C controller supports configuring the port type
it will operate as through the MODE_SELECT field of the General
Control Register.

Configure the port type based on the fwnode property "power-role"
during probe, and through the port_type_set typec_operation.

The MODE_SELECT field can only be changed when the controller is in
unattached state, so follow the sequence recommended by the datasheet to:
1. disable termination on CC pins to disable the controller
2. change the mode
3. re-enable termination

This will effectively cause a connected device to disconnect
for the duration of the mode change.

Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
---
 drivers/usb/typec/hd3ss3220.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a01f311fb60b4284da 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -35,10 +35,16 @@
 #define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(4)
 
 /* Register HD3SS3220_REG_GEN_CTRL*/
+#define HD3SS3220_REG_GEN_CTRL_DISABLE_TERM		BIT(0)
 #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK		(BIT(2) | BIT(1))
 #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT	0x00
 #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK	BIT(1)
 #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC	(BIT(2) | BIT(1))
+#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK		(BIT(5) | BIT(4))
+#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DEFAULT	0x00
+#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP		BIT(5)
+#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP		BIT(4)
+#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP		(BIT(5) | BIT(4))
 
 struct hd3ss3220 {
 	struct device *dev;
@@ -75,6 +81,52 @@ static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opm
 				  current_mode);
 }
 
+static int hd3ss3220_set_port_type(struct hd3ss3220 *hd3ss3220, int type)
+{
+	int mode_select, err;
+
+	switch (type) {
+	case TYPEC_PORT_SRC:
+		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP;
+		break;
+	case TYPEC_PORT_SNK:
+		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP;
+		break;
+	case TYPEC_PORT_DRP:
+		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP;
+		break;
+	default:
+		dev_err(hd3ss3220->dev, "bad port type: %d\n", type);
+		return -EINVAL;
+	}
+
+	/* Disable termination before changing MODE_SELECT as required by datasheet */
+	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
+				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM,
+				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM);
+	if (err < 0) {
+		dev_err(hd3ss3220->dev, "Failed to disable port for mode change: %d\n", err);
+		return err;
+	}
+
+	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
+				 HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK,
+				 mode_select);
+	if (err < 0) {
+		dev_err(hd3ss3220->dev, "Failed to change mode: %d\n", err);
+		regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
+				   HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0);
+		return err;
+	}
+
+	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
+				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0);
+	if (err < 0)
+		dev_err(hd3ss3220->dev, "Failed to re-enable port after mode change: %d\n", err);
+
+	return err;
+}
+
 static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
 {
 	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
@@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role)
 	return ret;
 }
 
+static int hd3ss3220_port_type_set(struct typec_port *port, enum typec_port_type type)
+{
+	struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
+
+	return hd3ss3220_set_port_type(hd3ss3220, type);
+}
+
 static const struct typec_operations hd3ss3220_ops = {
-	.dr_set = hd3ss3220_dr_set
+	.dr_set = hd3ss3220_dr_set,
+	.port_type_set = hd3ss3220_port_type_set,
 };
 
 static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
@@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	if (ret != 0 && ret != -EINVAL && ret != -ENXIO)
 		goto err_put_role;
 
+	ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type);
+	if (ret < 0)
+		goto err_put_role;
+
 	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(hd3ss3220->port)) {
 		ret = PTR_ERR(hd3ss3220->port);

-- 
2.34.1

Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Heikki Krogerus]

Hi Oliver,

I'm sorry, I noticed a problem with this...

On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> The TI HD3SS3220 Type-C controller supports configuring the port type
> it will operate as through the MODE_SELECT field of the General
> Control Register.
> 
> Configure the port type based on the fwnode property "power-role"
> during probe, and through the port_type_set typec_operation.
> 
> The MODE_SELECT field can only be changed when the controller is in
> unattached state, so follow the sequence recommended by the datasheet to:
> 1. disable termination on CC pins to disable the controller
> 2. change the mode
> 3. re-enable termination
> 
> This will effectively cause a connected device to disconnect
> for the duration of the mode change.

Changing the type of the port is really problematic, and IMO we should
actually never support that.

Consider for example, if your port is sink only, then the platform
almost certainly can't drive the VBUS. This patch would still allow
the port to be changed to source port.

Sorry for not realising this in v1.

I think what you want here is just a power role swap. Currently power
role swap is only supported when USB PD is supported in the class
code, but since the USB Type-C specification quite clearly states that
power role and data role swap can be optionally supported even when
USB PD is not supported (section 2.3.3) we need to fix that:

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 58f40156de56..ee81909565a4 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device *dev,
                return -EOPNOTSUPP;
        }

-       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
-               dev_dbg(dev, "partner unable to swap power role\n");
-               return -EIO;
-       }
-
        ret = sysfs_match_string(typec_roles, buf);
        if (ret < 0)
                return ret;


After that it should be possible to do power role swap also in this
driver when the port is DRP capable.

> Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> ---
>  drivers/usb/typec/hd3ss3220.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a01f311fb60b4284da 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -35,10 +35,16 @@
>  #define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(4)
>  
>  /* Register HD3SS3220_REG_GEN_CTRL*/
> +#define HD3SS3220_REG_GEN_CTRL_DISABLE_TERM		BIT(0)
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK		(BIT(2) | BIT(1))
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT	0x00
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK	BIT(1)
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC	(BIT(2) | BIT(1))
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK		(BIT(5) | BIT(4))
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DEFAULT	0x00
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP		BIT(5)
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP		BIT(4)
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP		(BIT(5) | BIT(4))
>  
>  struct hd3ss3220 {
>  	struct device *dev;
> @@ -75,6 +81,52 @@ static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opm
>  				  current_mode);
>  }
>  
> +static int hd3ss3220_set_port_type(struct hd3ss3220 *hd3ss3220, int type)
> +{
> +	int mode_select, err;
> +
> +	switch (type) {
> +	case TYPEC_PORT_SRC:
> +		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP;
> +		break;
> +	case TYPEC_PORT_SNK:
> +		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP;
> +		break;
> +	case TYPEC_PORT_DRP:
> +		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP;
> +		break;
> +	default:
> +		dev_err(hd3ss3220->dev, "bad port type: %d\n", type);
> +		return -EINVAL;
> +	}
> +
> +	/* Disable termination before changing MODE_SELECT as required by datasheet */
> +	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM,
> +				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM);
> +	if (err < 0) {
> +		dev_err(hd3ss3220->dev, "Failed to disable port for mode change: %d\n", err);
> +		return err;
> +	}
> +
> +	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				 HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK,
> +				 mode_select);
> +	if (err < 0) {
> +		dev_err(hd3ss3220->dev, "Failed to change mode: %d\n", err);
> +		regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				   HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0);
> +		return err;
> +	}
> +
> +	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0);
> +	if (err < 0)
> +		dev_err(hd3ss3220->dev, "Failed to re-enable port after mode change: %d\n", err);
> +
> +	return err;
> +}
> +
>  static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
>  {
>  	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role)
>  	return ret;
>  }
>  
> +static int hd3ss3220_port_type_set(struct typec_port *port, enum typec_port_type type)
> +{
> +	struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> +
> +	return hd3ss3220_set_port_type(hd3ss3220, type);
> +}

This wrapper seems completely useless. You only need one function here
for the callback.

>  static const struct typec_operations hd3ss3220_ops = {
> -	.dr_set = hd3ss3220_dr_set
> +	.dr_set = hd3ss3220_dr_set,
> +	.port_type_set = hd3ss3220_port_type_set,
>  };

So here I think you should implement the pr_set callback instead.

Let me kwno wh

>  static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
> @@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  	if (ret != 0 && ret != -EINVAL && ret != -ENXIO)
>  		goto err_put_role;
>  
> +	ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type);
> +	if (ret < 0)
> +		goto err_put_role;
> +
>  	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
>  	if (IS_ERR(hd3ss3220->port)) {
>  		ret = PTR_ERR(hd3ss3220->port);
> 

thanks,

-- 
heikki

RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Facklam, Olivér]

Hello Heikki,

Thanks for reviewing.

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Monday, November 18, 2024 12:50 PM
> To: Facklam, Olivér <oliver.facklam@zuehlke.com>
> Cc: Biju Das <biju.das@bp.renesas.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; von Heyl, Benedict
> <benedict.vonheyl@zuehlke.com>; Först, Mathis
> <mathis.foerst@zuehlke.com>; Glettig, Michael
> <Michael.Glettig@zuehlke.com>
> Subject: Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port
> type
> 
> Hi Oliver,
> 
> I'm sorry, I noticed a problem with this...
> 
> On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > The TI HD3SS3220 Type-C controller supports configuring the port type
> > it will operate as through the MODE_SELECT field of the General
> > Control Register.
> >
> > Configure the port type based on the fwnode property "power-role"
> > during probe, and through the port_type_set typec_operation.
> >
> > The MODE_SELECT field can only be changed when the controller is in
> > unattached state, so follow the sequence recommended by the datasheet
> to:
> > 1. disable termination on CC pins to disable the controller
> > 2. change the mode
> > 3. re-enable termination
> >
> > This will effectively cause a connected device to disconnect
> > for the duration of the mode change.
> 
> Changing the type of the port is really problematic, and IMO we should
> actually never support that.

Could you clarify why you think it is problematic?

> 
> Consider for example, if your port is sink only, then the platform
> almost certainly can't drive the VBUS. This patch would still allow
> the port to be changed to source port.

In my testing, it appeared to me that when registering a type-c port with
"typec_cap.type = TYPEC_PORT_SNK" (for example), then the type-c class
disables the port_type_store functionality:
	if (port->cap->type != TYPEC_PORT_DRP ||
	    !port->ops || !port->ops->port_type_set) {
		dev_dbg(dev, "changing port type not supported\n");
		return -EOPNOTSUPP;
	}

So to my understanding, a platform which cannot drive VBUS should simply
set the fwnode `power-role="sink"`. Since patch 2/4 correctly parses this property,
wouldn't that solve this case?

> 
> Sorry for not realising this in v1.
> 
> I think what you want here is just a power role swap. Currently power
> role swap is only supported when USB PD is supported in the class
> code, but since the USB Type-C specification quite clearly states that
> power role and data role swap can be optionally supported even when
> USB PD is not supported (section 2.3.3) we need to fix that:

My interpretation of section 2.3.3 is that the 2 mechanisms allowing 
power role swap are:
- USB PD (after initial connection)
- "as part of the initial connection process": to me this is simply referring to the
	Try.SRC / Try.SNK mechanism, for which we already have 
	the "try_role" callback.

Maybe I'm misunderstanding what the intentions are behind each of the 
typec_operations, so if you could clarify that (or give some pointer), that would
be appreciated. My understanding:
- "try_role": set Try.SRC / Try.SNK / no preference for a dual-role port for initial connection
- "pr_set" / "dr_set" / "vconn_set": swap power and data role resp.
	after the initial connection using USB-PD.
- "port_type_set": configure what port type to operate as, i.e. which initial connection
	state machine from the USB-C standard to apply for the next connection
Please correct me if any of these are incorrect.

> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 58f40156de56..ee81909565a4 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device
> *dev,
>                 return -EOPNOTSUPP;
>         }
> 
> -       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> -               dev_dbg(dev, "partner unable to swap power role\n");
> -               return -EIO;
> -       }
> -
>         ret = sysfs	_match_string(typec_roles, buf);
>         if (ret < 0)
>                 return ret;
> 
> 
> After that it should be possible to do power role swap also in this
> driver when the port is DRP capable.
> 
> > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > ---
> >  drivers/usb/typec/hd3ss3220.c | 66
> ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/hd3ss3220.c
> b/drivers/usb/typec/hd3ss3220.c
> > index
> e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> 01f311fb60b4284da 100644
> > --- a/drivers/usb/typec/hd3ss3220.c
> > +++ b/drivers/usb/typec/hd3ss3220.c
[...]
> > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port
> *port, enum typec_data_role role)
> >       return ret;
> >  }
> >
> > +static int hd3ss3220_port_type_set(struct typec_port *port, enum
> typec_port_type type)
> > +{
> > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > +
> > +     return hd3ss3220_set_port_type(hd3ss3220, type);
> > +}
> 
> This wrapper seems completely useless. You only need one function here
> for the callback.

The wrapper is to extract the struct hd3ss3220 from the typec_port.
The underlying hd3ss3220_set_port_type I am also using during probe
to configure initial port type.

One point worth mentioning here is that if the MODE_SELECT register
is not configured, the chip will operate according to a default which is 
chosen by an external pin (sorry if this was not detailed enough in commit msg)
From the datasheet:
-------------------
| PORT | 4 | I | Tri-level input pin to indicate port mode. The state of this pin is sampled when HD3SS3220's
		ENn_CC is asserted low, and VDD5 is active. This pin is also sampled following a
		I2C_SOFT_RESET.
		H - DFP (Pull-up to VDD5 if DFP mode is desired)
		NC - DRP (Leave unconnected if DRP mode is desired)
		L - UFP (Pull-down or tie to GND if UFP mode is desired)

In our use case, it was not desirable to leave this default based on wiring,
and it makes more sense to me to allow the configuration to come from
the fwnode property. Hence the port type setting in probe().

> 
> >  static const struct typec_operations hd3ss3220_ops = {
> > -     .dr_set = hd3ss3220_dr_set
> > +     .dr_set = hd3ss3220_dr_set,
> > +     .port_type_set = hd3ss3220_port_type_set,
> >  };
> 
> So here I think you should implement the pr_set callback instead.

I can do that, but based on the MODE_SELECT register description, 
it seems to me that this setting is fundamentally changing the operation
mode of the chip, i.e. the state machine that is being run for initial connection.
So there would have to be a way of "resetting" it to be a dual-role port again,
which the "pr_set" callback doesn't seem to have?
	This register can be written to set the HD3SS3220 mode
	operation. The ADDR pin must be set to I2C mode. If the default
	is maintained, HD3SS3220 shall operate according to the PORT
	pin levels and modes. The MODE_SELECT can only be
	changed when in the unattached state.
	00 - DRP mode (start from unattached.SNK) (default)
	01 - UFP mode (unattached.SNK)
	10 - DFP mode (unattached.SRC)
	11 - DRP mode (start from unattached.SNK)

> 
> Let me kwno wh
> 
> >  static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
> > @@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client
> *client)
> >       if (ret != 0 && ret != -EINVAL && ret != -ENXIO)
> >               goto err_put_role;
> >
> > +     ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type);
> > +     if (ret < 0)
> > +             goto err_put_role;
> > +
> >       hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
> >       if (IS_ERR(hd3ss3220->port)) {
> >               ret = PTR_ERR(hd3ss3220->port);
> >
> 
> thanks,
> 
> --
> heikki

Thanks!
Oliver

Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Heikki Krogerus]

Hi Olivér,

Sorry to keep you waiting.

On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> Hello Heikki,
> 
> Thanks for reviewing.
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Monday, November 18, 2024 12:50 PM
> > To: Facklam, Olivér <oliver.facklam@zuehlke.com>
> > Cc: Biju Das <biju.das@bp.renesas.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org; von Heyl, Benedict
> > <benedict.vonheyl@zuehlke.com>; Först, Mathis
> > <mathis.foerst@zuehlke.com>; Glettig, Michael
> > <Michael.Glettig@zuehlke.com>
> > Subject: Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port
> > type
> > 
> > Hi Oliver,
> > 
> > I'm sorry, I noticed a problem with this...
> > 
> > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > The TI HD3SS3220 Type-C controller supports configuring the port type
> > > it will operate as through the MODE_SELECT field of the General
> > > Control Register.
> > >
> > > Configure the port type based on the fwnode property "power-role"
> > > during probe, and through the port_type_set typec_operation.
> > >
> > > The MODE_SELECT field can only be changed when the controller is in
> > > unattached state, so follow the sequence recommended by the datasheet
> > to:
> > > 1. disable termination on CC pins to disable the controller
> > > 2. change the mode
> > > 3. re-enable termination
> > >
> > > This will effectively cause a connected device to disconnect
> > > for the duration of the mode change.
> > 
> > Changing the type of the port is really problematic, and IMO we should
> > actually never support that.
> 
> Could you clarify why you think it is problematic?

It's not completely clear to me what it's meant for. If it was just
for fixing the type of the port to be sink, source or DRP before
connections, it would make sense, but since it can be use even when
there is an actice connection (there is nothing preventing that), it
can in practice be used to swap the role.

And in some cases in the past where this attribute file was proposed
to be used with some other drivers, the actual goal really ended up
being to be able to just swap the role with an existing connection
instead of being able to fix the type of the port. The commit message
made it sound like that could be the goal in this case as well, but
maybe I misunderstood.

Even in cases where it's clear that the intention is to just fix the
role before connections, why would user space needs to control that is
still not completely clear, at least not to me.

> > Consider for example, if your port is sink only, then the platform
> > almost certainly can't drive the VBUS. This patch would still allow
> > the port to be changed to source port.
> 
> In my testing, it appeared to me that when registering a type-c port with
> "typec_cap.type = TYPEC_PORT_SNK" (for example), then the type-c class
> disables the port_type_store functionality:
> 	if (port->cap->type != TYPEC_PORT_DRP ||
> 	    !port->ops || !port->ops->port_type_set) {
> 		dev_dbg(dev, "changing port type not supported\n");
> 		return -EOPNOTSUPP;
> 	}
> 
> So to my understanding, a platform which cannot drive VBUS should simply
> set the fwnode `power-role="sink"`. Since patch 2/4 correctly parses this property,
> wouldn't that solve this case?

True. I stand corrected.

> > Sorry for not realising this in v1.
> > 
> > I think what you want here is just a power role swap. Currently power
> > role swap is only supported when USB PD is supported in the class
> > code, but since the USB Type-C specification quite clearly states that
> > power role and data role swap can be optionally supported even when
> > USB PD is not supported (section 2.3.3) we need to fix that:
> 
> My interpretation of section 2.3.3 is that the 2 mechanisms allowing 
> power role swap are:
> - USB PD (after initial connection)
> - "as part of the initial connection process": to me this is simply referring to the
> 	Try.SRC / Try.SNK mechanism, for which we already have 
> 	the "try_role" callback.
> 
> Maybe I'm misunderstanding what the intentions are behind each of the 
> typec_operations, so if you could clarify that (or give some pointer), that would
> be appreciated. My understanding:
> - "try_role": set Try.SRC / Try.SNK / no preference for a dual-role port for initial connection
> - "pr_set" / "dr_set" / "vconn_set": swap power and data role resp.
> 	after the initial connection using USB-PD.
> - "port_type_set": configure what port type to operate as, i.e. which initial connection
> 	state machine from the USB-C standard to apply for the next connection
> Please correct me if any of these are incorrect.

I don't know what's the intention with the port_type attribute file
unfortunately.

> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 58f40156de56..ee81909565a4 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device
> > *dev,
> >                 return -EOPNOTSUPP;
> >         }
> > 
> > -       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> > -               dev_dbg(dev, "partner unable to swap power role\n");
> > -               return -EIO;
> > -       }
> > -
> >         ret = sysfs	_match_string(typec_roles, buf);
> >         if (ret < 0)
> >                 return ret;
> > 
> > 
> > After that it should be possible to do power role swap also in this
> > driver when the port is DRP capable.
> > 
> > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > ---
> > >  drivers/usb/typec/hd3ss3220.c | 66
> > ++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > b/drivers/usb/typec/hd3ss3220.c
> > > index
> > e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > 01f311fb60b4284da 100644
> > > --- a/drivers/usb/typec/hd3ss3220.c
> > > +++ b/drivers/usb/typec/hd3ss3220.c
> [...]
> > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port
> > *port, enum typec_data_role role)
> > >       return ret;
> > >  }
> > >
> > > +static int hd3ss3220_port_type_set(struct typec_port *port, enum
> > typec_port_type type)
> > > +{
> > > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > +
> > > +     return hd3ss3220_set_port_type(hd3ss3220, type);
> > > +}
> > 
> > This wrapper seems completely useless. You only need one function here
> > for the callback.
> 
> The wrapper is to extract the struct hd3ss3220 from the typec_port.
> The underlying hd3ss3220_set_port_type I am also using during probe
> to configure initial port type.

Ah, I missed that. Sorry about that.

> One point worth mentioning here is that if the MODE_SELECT register
> is not configured, the chip will operate according to a default which is 
> chosen by an external pin (sorry if this was not detailed enough in commit msg)
> >From the datasheet:
> -------------------
> | PORT | 4 | I | Tri-level input pin to indicate port mode. The state of this pin is sampled when HD3SS3220's
> 		ENn_CC is asserted low, and VDD5 is active. This pin is also sampled following a
> 		I2C_SOFT_RESET.
> 		H - DFP (Pull-up to VDD5 if DFP mode is desired)
> 		NC - DRP (Leave unconnected if DRP mode is desired)
> 		L - UFP (Pull-down or tie to GND if UFP mode is desired)
> 
> In our use case, it was not desirable to leave this default based on wiring,
> and it makes more sense to me to allow the configuration to come from
> the fwnode property. Hence the port type setting in probe().

I get that, but that just means you want to fix the type during probe,
no? Why do you need to expose this to the user space?

> > >  static const struct typec_operations hd3ss3220_ops = {
> > > -     .dr_set = hd3ss3220_dr_set
> > > +     .dr_set = hd3ss3220_dr_set,
> > > +     .port_type_set = hd3ss3220_port_type_set,
> > >  };
> > 
> > So here I think you should implement the pr_set callback instead.
> 
> I can do that, but based on the MODE_SELECT register description, 
> it seems to me that this setting is fundamentally changing the operation
> mode of the chip, i.e. the state machine that is being run for initial connection.
> So there would have to be a way of "resetting" it to be a dual-role port again,
> which the "pr_set" callback doesn't seem to have?
> 	This register can be written to set the HD3SS3220 mode
> 	operation. The ADDR pin must be set to I2C mode. If the default
> 	is maintained, HD3SS3220 shall operate according to the PORT
> 	pin levels and modes. The MODE_SELECT can only be
> 	changed when in the unattached state.
> 	00 - DRP mode (start from unattached.SNK) (default)
> 	01 - UFP mode (unattached.SNK)
> 	10 - DFP mode (unattached.SRC)
> 	11 - DRP mode (start from unattached.SNK)

Okay, I see. This is not a case for pr_set.

I'm still confused about the use case here. It seems you are not
interested in role swapping after all, so why would you need this
functionality to be exposed to the user space?

I'm sorry if I've missed something.

About the port_type attribute file itself. I would feel more
comfortable with it if it was allowed to be written only when there is
nothing connected to the port. At the very least, I think it should be
documented better so what it's really meant for would be more clear to
everybody.

thanks,

-- 
heikki

RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Facklam, Olivér]

Hi Heikki,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Monday, November 25, 2024 11:28 AM
> 
> Hi Olivér,
> 
> Sorry to keep you waiting.
> 
> On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> > Hello Heikki,
> >
> > Thanks for reviewing.
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Monday, November 18, 2024 12:50 PM
> > >
> > > Hi Oliver,
> > >
> > > I'm sorry, I noticed a problem with this...
> > >
> > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > > The TI HD3SS3220 Type-C controller supports configuring the port
> > > > type it will operate as through the MODE_SELECT field of the
> > > > General Control Register.
> > > >
> > > > Configure the port type based on the fwnode property "power-role"
> > > > during probe, and through the port_type_set typec_operation.
> > > >
> > > > The MODE_SELECT field can only be changed when the controller is
> > > > in unattached state, so follow the sequence recommended by the
> > > > datasheet
> > > to:
> > > > 1. disable termination on CC pins to disable the controller 2.
> > > > change the mode 3. re-enable termination
> > > >
> > > > This will effectively cause a connected device to disconnect for
> > > > the duration of the mode change.
> > >
> > > Changing the type of the port is really problematic, and IMO we
> > > should actually never support that.
> >
> > Could you clarify why you think it is problematic?
> 
> It's not completely clear to me what it's meant for. If it was just for fixing the
> type of the port to be sink, source or DRP before connections, it would make
> sense, but since it can be use even when there is an actice connection (there
> is nothing preventing that), it can in practice be used to swap the role.
> 
> And in some cases in the past where this attribute file was proposed to be
> used with some other drivers, the actual goal really ended up being to be
> able to just swap the role with an existing connection instead of being able to
> fix the type of the port. The commit message made it sound like that could be
> the goal in this case as well, but maybe I misunderstood.
> 
> Even in cases where it's clear that the intention is to just fix the role before
> connections, why would user space needs to control that is still not
> completely clear, at least not to me.

The idea is to give the user the possibility to control/restrict how the port is
operating even if they have an actual dual-role capable port.

Let me clarify. In my use case, I have a DRP port, and in most cases I would
like to use it as such.
However, there are cases where this operating mode causes additional
difficulties -- for example when connecting to another dual-role port 
implementing the same role preference (e.g. 2 Try.SNK devices connected
together). Then the role outcome is random.
Since this chip doesn't support PD, there is no way to switch the role from
userspace.
When I know I'm going to be working with these types of connections, it
would be better if I can restrict the operation mode to "sink-only" (for example)
for that duration. Without needing to change my device tree.

Sure, the mechanism can be abused to switch the role on an active connection,
but that was not the primary idea here.
I would even argue that calling a port type change during an active
connection terminates that connection, and starts a new connection
from scratch with the new behavior.

> 
> > > Consider for example, if your port is sink only, then the platform
> > > almost certainly can't drive the VBUS. This patch would still allow
> > > the port to be changed to source port.
> >
> > In my testing, it appeared to me that when registering a type-c port
> > with "typec_cap.type = TYPEC_PORT_SNK" (for example), then the type-c
> > class disables the port_type_store functionality:
> > 	if (port->cap->type != TYPEC_PORT_DRP ||
> > 	    !port->ops || !port->ops->port_type_set) {
> > 		dev_dbg(dev, "changing port type not supported\n");
> > 		return -EOPNOTSUPP;
> > 	}
> >
> > So to my understanding, a platform which cannot drive VBUS should
> > simply set the fwnode `power-role="sink"`. Since patch 2/4 correctly
> > parses this property, wouldn't that solve this case?
> 
> True. I stand corrected.
> 
> > > Sorry for not realising this in v1.
> > >
> > > I think what you want here is just a power role swap. Currently
> > > power role swap is only supported when USB PD is supported in the
> > > class code, but since the USB Type-C specification quite clearly
> > > states that power role and data role swap can be optionally
> > > supported even when USB PD is not supported (section 2.3.3) we need to
> fix that:
> >
> > My interpretation of section 2.3.3 is that the 2 mechanisms allowing
> > power role swap are:
> > - USB PD (after initial connection)
> > - "as part of the initial connection process": to me this is simply referring to
> the
> > 	Try.SRC / Try.SNK mechanism, for which we already have
> > 	the "try_role" callback.
> >
> > Maybe I'm misunderstanding what the intentions are behind each of the
> > typec_operations, so if you could clarify that (or give some pointer),
> > that would be appreciated. My understanding:
> > - "try_role": set Try.SRC / Try.SNK / no preference for a dual-role
> > port for initial connection
> > - "pr_set" / "dr_set" / "vconn_set": swap power and data role resp.
> > 	after the initial connection using USB-PD.
> > - "port_type_set": configure what port type to operate as, i.e. which initial
> connection
> > 	state machine from the USB-C standard to apply for the next
> > connection Please correct me if any of these are incorrect.
> 
> I don't know what's the intention with the port_type attribute file
> unfortunately.
> 
> > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > index 58f40156de56..ee81909565a4 100644
> > > --- a/drivers/usb/typec/class.c
> > > +++ b/drivers/usb/typec/class.c
> > > @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device
> > > *dev,
> > >                 return -EOPNOTSUPP;
> > >         }
> > >
> > > -       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> > > -               dev_dbg(dev, "partner unable to swap power role\n");
> > > -               return -EIO;
> > > -       }
> > > -
> > >         ret = sysfs	_match_string(typec_roles, buf);
> > >         if (ret < 0)
> > >                 return ret;
> > >
> > >
> > > After that it should be possible to do power role swap also in this
> > > driver when the port is DRP capable.
> > >
> > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > > ---
> > > >  drivers/usb/typec/hd3ss3220.c | 66
> > > ++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > b/drivers/usb/typec/hd3ss3220.c
> > > > index
> > >
> e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > > 01f311fb60b4284da 100644
> > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > [...]
> > > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port
> > > *port, enum typec_data_role role)
> > > >       return ret;
> > > >  }
> > > >
> > > > +static int hd3ss3220_port_type_set(struct typec_port *port, enum
> > > typec_port_type type)
> > > > +{
> > > > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > > +
> > > > +     return hd3ss3220_set_port_type(hd3ss3220, type); }
> > >
> > > This wrapper seems completely useless. You only need one function
> > > here for the callback.
> >
> > The wrapper is to extract the struct hd3ss3220 from the typec_port.
> > The underlying hd3ss3220_set_port_type I am also using during probe to
> > configure initial port type.
> 
> Ah, I missed that. Sorry about that.
> 
> > One point worth mentioning here is that if the MODE_SELECT register is
> > not configured, the chip will operate according to a default which is
> > chosen by an external pin (sorry if this was not detailed enough in
> > commit msg)
> > >From the datasheet:
> > -------------------
> > | PORT | 4 | I | Tri-level input pin to indicate port mode. The state
> > | of this pin is sampled when HD3SS3220's
> > 		ENn_CC is asserted low, and VDD5 is active. This pin is also
> sampled following a
> > 		I2C_SOFT_RESET.
> > 		H - DFP (Pull-up to VDD5 if DFP mode is desired)
> > 		NC - DRP (Leave unconnected if DRP mode is desired)
> > 		L - UFP (Pull-down or tie to GND if UFP mode is desired)
> >
> > In our use case, it was not desirable to leave this default based on
> > wiring, and it makes more sense to me to allow the configuration to
> > come from the fwnode property. Hence the port type setting in probe().
> 
> I get that, but that just means you want to fix the type during probe, no?
> Why do you need to expose this to the user space?

I've been thinking a bit more about this "fixing the type during probe" feature.
My current implementation always fixes the type, even if no device tree entry
for "power-role" was found. Could that cause issues for people relying on the
configuration through the PORT pin?

I could consider a solution where if the property is absent, the type is not
reconfigured during the probe. Although then I would have to do manual parsing
of that DT property. With typec_get_fw_cap() from patch 2/4, I loose
the information about individual properties being present/absent.
Would be interested in hearing your thoughts.

> 
> > > >  static const struct typec_operations hd3ss3220_ops = {
> > > > -     .dr_set = hd3ss3220_dr_set
> > > > +     .dr_set = hd3ss3220_dr_set,
> > > > +     .port_type_set = hd3ss3220_port_type_set,
> > > >  };
> > >
> > > So here I think you should implement the pr_set callback instead.
> >
> > I can do that, but based on the MODE_SELECT register description, it
> > seems to me that this setting is fundamentally changing the operation
> > mode of the chip, i.e. the state machine that is being run for initial
> connection.
> > So there would have to be a way of "resetting" it to be a dual-role
> > port again, which the "pr_set" callback doesn't seem to have?
> > 	This register can be written to set the HD3SS3220 mode
> > 	operation. The ADDR pin must be set to I2C mode. If the default
> > 	is maintained, HD3SS3220 shall operate according to the PORT
> > 	pin levels and modes. The MODE_SELECT can only be
> > 	changed when in the unattached state.
> > 	00 - DRP mode (start from unattached.SNK) (default)
> > 	01 - UFP mode (unattached.SNK)
> > 	10 - DFP mode (unattached.SRC)
> > 	11 - DRP mode (start from unattached.SNK)
> 
> Okay, I see. This is not a case for pr_set.
> 
> I'm still confused about the use case here. It seems you are not interested in
> role swapping after all, so why would you need this functionality to be
> exposed to the user space?
> 
> I'm sorry if I've missed something.
> 
> About the port_type attribute file itself. I would feel more comfortable with it
> if it was allowed to be written only when there is nothing connected to the
> port. At the very least, I think it should be documented better so what it's
> really meant for would be more clear to everybody.

After researching some more about this operation, I came across the driver
for TUSB320 [1] which seems to have a very similar behavior (also from TI).
[1] - https://lore.kernel.org/all/20220730180500.152004-1-marex@denx.de/T/#ma7a322bc207414e4185c29d257ff30f5769f5d70

For one variant of the chip, the implementation relies on the CC disabling like
in this patch. A different variant tests the current connection status before
proceeding.

> 
> thanks,
> 
> --
> heikki

Thanks,
Oliver

RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Biju Das]

Hi Facklam, Olivér,

> -----Original Message-----
> From: Facklam, Olivér <oliver.facklam@zuehlke.com>
> Sent: 27 November 2024 08:03
> Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type
> 
> Hi Heikki,
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Monday, November 25, 2024 11:28 AM
> >
> > Hi Olivér,
> >
> > Sorry to keep you waiting.
> >
> > On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> > > Hello Heikki,
> > >
> > > Thanks for reviewing.
> > >
> > > > -----Original Message-----
> > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Sent: Monday, November 18, 2024 12:50 PM
> > > >
> > > > Hi Oliver,
> > > >
> > > > I'm sorry, I noticed a problem with this...
> > > >
> > > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > > > The TI HD3SS3220 Type-C controller supports configuring the port
> > > > > type it will operate as through the MODE_SELECT field of the
> > > > > General Control Register.
> > > > >
> > > > > Configure the port type based on the fwnode property "power-role"
> > > > > during probe, and through the port_type_set typec_operation.
> > > > >
> > > > > The MODE_SELECT field can only be changed when the controller is
> > > > > in unattached state, so follow the sequence recommended by the
> > > > > datasheet
> > > > to:
> > > > > 1. disable termination on CC pins to disable the controller 2.
> > > > > change the mode 3. re-enable termination
> > > > >
> > > > > This will effectively cause a connected device to disconnect for
> > > > > the duration of the mode change.
> > > >
> > > > Changing the type of the port is really problematic, and IMO we
> > > > should actually never support that.
> > >
> > > Could you clarify why you think it is problematic?
> >
> > It's not completely clear to me what it's meant for. If it was just
> > for fixing the type of the port to be sink, source or DRP before
> > connections, it would make sense, but since it can be use even when
> > there is an actice connection (there is nothing preventing that), it can in practice be used to swap
> the role.
> >
> > And in some cases in the past where this attribute file was proposed
> > to be used with some other drivers, the actual goal really ended up
> > being to be able to just swap the role with an existing connection
> > instead of being able to fix the type of the port. The commit message
> > made it sound like that could be the goal in this case as well, but maybe I misunderstood.
> >
> > Even in cases where it's clear that the intention is to just fix the
> > role before connections, why would user space needs to control that is
> > still not completely clear, at least not to me.
> 
> The idea is to give the user the possibility to control/restrict how the port is operating even if
> they have an actual dual-role capable port.
> 
> Let me clarify. In my use case, I have a DRP port, and in most cases I would like to use it as such.
> However, there are cases where this operating mode causes additional difficulties -- for example when
> connecting to another dual-role port implementing the same role preference (e.g. 2 Try.SNK devices
> connected together). Then the role outcome is random.
> Since this chip doesn't support PD, there is no way to switch the role from userspace.
> When I know I'm going to be working with these types of connections, it would be better if I can
> restrict the operation mode to "sink-only" (for example) for that duration. Without needing to change
> my device tree.
> 
> Sure, the mechanism can be abused to switch the role on an active connection, but that was not the
> primary idea here.
> I would even argue that calling a port type change during an active connection terminates that
> connection, and starts a new connection from scratch with the new behavior.
> 
> >
> > > > Consider for example, if your port is sink only, then the platform
> > > > almost certainly can't drive the VBUS. This patch would still
> > > > allow the port to be changed to source port.
> > >
> > > In my testing, it appeared to me that when registering a type-c port
> > > with "typec_cap.type = TYPEC_PORT_SNK" (for example), then the
> > > type-c class disables the port_type_store functionality:
> > > 	if (port->cap->type != TYPEC_PORT_DRP ||
> > > 	    !port->ops || !port->ops->port_type_set) {
> > > 		dev_dbg(dev, "changing port type not supported\n");
> > > 		return -EOPNOTSUPP;
> > > 	}
> > >
> > > So to my understanding, a platform which cannot drive VBUS should
> > > simply set the fwnode `power-role="sink"`. Since patch 2/4 correctly
> > > parses this property, wouldn't that solve this case?
> >
> > True. I stand corrected.
> >
> > > > Sorry for not realising this in v1.
> > > >
> > > > I think what you want here is just a power role swap. Currently
> > > > power role swap is only supported when USB PD is supported in the
> > > > class code, but since the USB Type-C specification quite clearly
> > > > states that power role and data role swap can be optionally
> > > > supported even when USB PD is not supported (section 2.3.3) we
> > > > need to
> > fix that:
> > >
> > > My interpretation of section 2.3.3 is that the 2 mechanisms allowing
> > > power role swap are:
> > > - USB PD (after initial connection)
> > > - "as part of the initial connection process": to me this is simply
> > > referring to
> > the
> > > 	Try.SRC / Try.SNK mechanism, for which we already have
> > > 	the "try_role" callback.
> > >
> > > Maybe I'm misunderstanding what the intentions are behind each of
> > > the typec_operations, so if you could clarify that (or give some
> > > pointer), that would be appreciated. My understanding:
> > > - "try_role": set Try.SRC / Try.SNK / no preference for a dual-role
> > > port for initial connection
> > > - "pr_set" / "dr_set" / "vconn_set": swap power and data role resp.
> > > 	after the initial connection using USB-PD.
> > > - "port_type_set": configure what port type to operate as, i.e.
> > > which initial
> > connection
> > > 	state machine from the USB-C standard to apply for the next
> > > connection Please correct me if any of these are incorrect.
> >
> > I don't know what's the intention with the port_type attribute file
> > unfortunately.
> >
> > > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > > index 58f40156de56..ee81909565a4 100644
> > > > --- a/drivers/usb/typec/class.c
> > > > +++ b/drivers/usb/typec/class.c
> > > > @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct
> > > > device *dev,
> > > >                 return -EOPNOTSUPP;
> > > >         }
> > > >
> > > > -       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> > > > -               dev_dbg(dev, "partner unable to swap power role\n");
> > > > -               return -EIO;
> > > > -       }
> > > > -
> > > >         ret = sysfs	_match_string(typec_roles, buf);
> > > >         if (ret < 0)
> > > >                 return ret;
> > > >
> > > >
> > > > After that it should be possible to do power role swap also in
> > > > this driver when the port is DRP capable.
> > > >
> > > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > > > ---
> > > > >  drivers/usb/typec/hd3ss3220.c | 66
> > > > ++++++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > > b/drivers/usb/typec/hd3ss3220.c
> > > > > index
> > > >
> > e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > > > 01f311fb60b4284da 100644
> > > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > > [...]
> > > > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct
> > > > > typec_port
> > > > *port, enum typec_data_role role)
> > > > >       return ret;
> > > > >  }
> > > > >
> > > > > +static int hd3ss3220_port_type_set(struct typec_port *port,
> > > > > +enum
> > > > typec_port_type type)
> > > > > +{
> > > > > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > > > +
> > > > > +     return hd3ss3220_set_port_type(hd3ss3220, type); }
> > > >
> > > > This wrapper seems completely useless. You only need one function
> > > > here for the callback.
> > >
> > > The wrapper is to extract the struct hd3ss3220 from the typec_port.
> > > The underlying hd3ss3220_set_port_type I am also using during probe
> > > to configure initial port type.
> >
> > Ah, I missed that. Sorry about that.
> >
> > > One point worth mentioning here is that if the MODE_SELECT register
> > > is not configured, the chip will operate according to a default
> > > which is chosen by an external pin (sorry if this was not detailed
> > > enough in commit msg)
> > > >From the datasheet:
> > > -------------------
> > > | PORT | 4 | I | Tri-level input pin to indicate port mode. The
> > > | state of this pin is sampled when HD3SS3220's
> > > 		ENn_CC is asserted low, and VDD5 is active. This pin is also
> > sampled following a
> > > 		I2C_SOFT_RESET.
> > > 		H - DFP (Pull-up to VDD5 if DFP mode is desired)
> > > 		NC - DRP (Leave unconnected if DRP mode is desired)
> > > 		L - UFP (Pull-down or tie to GND if UFP mode is desired)
> > >
> > > In our use case, it was not desirable to leave this default based on
> > > wiring, and it makes more sense to me to allow the configuration to
> > > come from the fwnode property. Hence the port type setting in probe().
> >
> > I get that, but that just means you want to fix the type during probe, no?
> > Why do you need to expose this to the user space?
> 
> I've been thinking a bit more about this "fixing the type during probe" feature.
> My current implementation always fixes the type, even if no device tree entry for "power-role" was
> found. Could that cause issues for people relying on the configuration through the PORT pin?
> 
> I could consider a solution where if the property is absent, the type is not reconfigured during the
> probe. Although then I would have to do manual parsing of that DT property. With typec_get_fw_cap()
> from patch 2/4, I loose the information about individual properties being present/absent.
> Would be interested in hearing your thoughts.
> 
> >
> > > > >  static const struct typec_operations hd3ss3220_ops = {
> > > > > -     .dr_set = hd3ss3220_dr_set
> > > > > +     .dr_set = hd3ss3220_dr_set,
> > > > > +     .port_type_set = hd3ss3220_port_type_set,
> > > > >  };
> > > >
> > > > So here I think you should implement the pr_set callback instead.
> > >
> > > I can do that, but based on the MODE_SELECT register description, it
> > > seems to me that this setting is fundamentally changing the
> > > operation mode of the chip, i.e. the state machine that is being run
> > > for initial
> > connection.
> > > So there would have to be a way of "resetting" it to be a dual-role
> > > port again, which the "pr_set" callback doesn't seem to have?
> > > 	This register can be written to set the HD3SS3220 mode
> > > 	operation. The ADDR pin must be set to I2C mode. If the default
> > > 	is maintained, HD3SS3220 shall operate according to the PORT
> > > 	pin levels and modes. The MODE_SELECT can only be
> > > 	changed when in the unattached state.
> > > 	00 - DRP mode (start from unattached.SNK) (default)
> > > 	01 - UFP mode (unattached.SNK)
> > > 	10 - DFP mode (unattached.SRC)
> > > 	11 - DRP mode (start from unattached.SNK)
> >
> > Okay, I see. This is not a case for pr_set.
> >
> > I'm still confused about the use case here. It seems you are not
> > interested in role swapping after all, so why would you need this
> > functionality to be exposed to the user space?
> >
> > I'm sorry if I've missed something.
> >
> > About the port_type attribute file itself. I would feel more
> > comfortable with it if it was allowed to be written only when there is
> > nothing connected to the port. At the very least, I think it should be
> > documented better so what it's really meant for would be more clear to everybody.
> 
> After researching some more about this operation, I came across the driver for TUSB320 [1] which seems
> to have a very similar behavior (also from TI).
> [1] - https://lore.kernel.org/all/20220730180500.152004-1-
> marex@denx.de/T/#ma7a322bc207414e4185c29d257ff30f5769f5d70
> 
> For one variant of the chip, the implementation relies on the CC disabling like in this patch. A
> different variant tests the current connection status before proceeding.
> 


Can you please provide your test logs?

Previously I tested 2 devices with
Switching roles host->device->host using 

echo device > /sys/class/typec/port0/data_role

and

echo host > /sys/class/typec/port0/data_role


Cheers,
Biju

RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Facklam, Olivér]

Hello Biju,

Thanks for your response and sorry for the delay.

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Wednesday, November 27, 2024 9:48 AM
> Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port
> type
> 
> Hi Facklam, Olivér,
> 
> > -----Original Message-----
> > From: Facklam, Olivér <oliver.facklam@zuehlke.com>
> > Sent: 27 November 2024 08:03
> > Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring
> > port type
> >
> > Hi Heikki,
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Monday, November 25, 2024 11:28 AM
> > >
> > > Hi Olivér,
> > >
> > > Sorry to keep you waiting.
> > >
> > > On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> > > > Hello Heikki,
> > > >
> > > > Thanks for reviewing.
> > > >
> > > > > -----Original Message-----
> > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > Sent: Monday, November 18, 2024 12:50 PM
> > > > >
> > > > > Hi Oliver,
> > > > >
> > > > > I'm sorry, I noticed a problem with this...
> > > > >
> > > > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > > > > The TI HD3SS3220 Type-C controller supports configuring the
> > > > > > port type it will operate as through the MODE_SELECT field of
> > > > > > the General Control Register.
> > > > > >
> > > > > > Configure the port type based on the fwnode property "power-role"
> > > > > > during probe, and through the port_type_set typec_operation.
> > > > > >
> > > > > > The MODE_SELECT field can only be changed when the controller
> > > > > > is in unattached state, so follow the sequence recommended by
> > > > > > the datasheet
> > > > > to:
> > > > > > 1. disable termination on CC pins to disable the controller 2.
> > > > > > change the mode 3. re-enable termination
> > > > > >
> > > > > > This will effectively cause a connected device to disconnect
> > > > > > for the duration of the mode change.
> > > > >
> > > > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > > > > ---
> > > > > >  drivers/usb/typec/hd3ss3220.c | 66
> > > > > ++++++++++++++++++++++++++++++++++++++++++-
> > > > > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > > > b/drivers/usb/typec/hd3ss3220.c
> > > > > > index
> > > > >
> > >
> e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > > > > 01f311fb60b4284da 100644
> > > > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > > > [...]
> > > > > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct
> > > > > > typec_port
> > > > > *port, enum typec_data_role role)
> > > > > >       return ret;
> > > > > >  }
> > > > > >
> > > > > > +static int hd3ss3220_port_type_set(struct typec_port *port,
> > > > > > +enum
> > > > > typec_port_type type)
> > > > > > +{
> > > > > > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > > > > +
> > > > > > +     return hd3ss3220_set_port_type(hd3ss3220, type); }
> > >
> > > > > >  static const struct typec_operations hd3ss3220_ops = {
> > > > > > -     .dr_set = hd3ss3220_dr_set
> > > > > > +     .dr_set = hd3ss3220_dr_set,
> > > > > > +     .port_type_set = hd3ss3220_port_type_set,
> > > > > >  };
> > > > >
> > > > > So here I think you should implement the pr_set callback instead.
> > > >
> > > > I can do that, but based on the MODE_SELECT register description,
> > > > it seems to me that this setting is fundamentally changing the
> > > > operation mode of the chip, i.e. the state machine that is being
> > > > run for initial
> > > connection.
> > > > So there would have to be a way of "resetting" it to be a
> > > > dual-role port again, which the "pr_set" callback doesn't seem to have?
> > > >   This register can be written to set the HD3SS3220 mode
> > > >   operation. The ADDR pin must be set to I2C mode. If the default
> > > >   is maintained, HD3SS3220 shall operate according to the PORT
> > > >   pin levels and modes. The MODE_SELECT can only be
> > > >   changed when in the unattached state.
> > > >   00 - DRP mode (start from unattached.SNK) (default)
> > > >   01 - UFP mode (unattached.SNK)
> > > >   10 - DFP mode (unattached.SRC)
> > > >   11 - DRP mode (start from unattached.SNK)
> > >
> > > Okay, I see. This is not a case for pr_set.
> > >
> > > I'm still confused about the use case here. It seems you are not
> > > interested in role swapping after all, so why would you need this
> > > functionality to be exposed to the user space?
> > >
> > > I'm sorry if I've missed something.
> > >
> > > About the port_type attribute file itself. I would feel more
> > > comfortable with it if it was allowed to be written only when there
> > > is nothing connected to the port. At the very least, I think it
> > > should be documented better so what it's really meant for would be more
> clear to everybody.
> >
> > After researching some more about this operation, I came across the
> > driver for TUSB320 [1] which seems to have a very similar behavior (also
> from TI).
> > [1] - https://lore.kernel.org/all/20220730180500.152004-1-
> > marex@denx.de/T/#ma7a322bc207414e4185c29d257ff30f5769f5d70
> >
> > For one variant of the chip, the implementation relies on the CC
> > disabling like in this patch. A different variant tests the current connection
> status before proceeding.
> >
> 
> 
> Can you please provide your test logs?

Adding them below.

> 
> Previously I tested 2 devices with
> Switching roles host->device->host using
> 
> echo device > /sys/class/typec/port0/data_role
> 
> and
> 
> echo host > /sys/class/typec/port0/data_role

Could you clarify what your test setup was?
Did you control both sides of the USB connection and execute these commands
on both sides?

> 
> 
> Cheers,
> Biju

Here are my test logs for the "port type switch" functionality.
The hd3ss3220 driver doesn't log much at all, I added one debug line
(NOT part of the patch series) for testing purposes:
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index c6922989a3cd..76fea657398a 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -199,6 +199,7 @@ static const struct typec_operations hd3ss3220_ops = {
 static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
 {
 	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
+	dev_info(hd3ss3220->dev, "Propagating role %s\n", usb_role_string(role_state));
 
 	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);

My test setup is as follows:
+-------------------------------------------+      +--------------------+
|        My device (Linux 6.12)             |      |                    |
| +-----------------+                       |      |    Remote device   |
| |                 +------I2C----+         |      |                    |
| |   i.MX8 M Plus  |    +--------v-------+ |      |   - phone          |
| |   with USB3 DRD |    |                | |      |   - USB-to-Ethernet|
| |   controller    +----+ TI HD3SS3220   +-+------+     dongle         |
| |                 |    |                | |      |   - computer       |
| +-----------------+    +----------------+ |      |                    |
+-------------------------------------------+      +--------------------+

========
Case 1: Device tree: power-role = "sink";
========
# cat /sys/class/typec/port0/port_type
[sink]
# echo "source" > /sys/class/typec/port0/port_type
zsh: permission denied: /sys/class/typec/port0/port_type
# echo "dual" > /sys/class/typec/port0/port_type
zsh: permission denied: /sys/class/typec/port0/port_type

-> plug in ethernet adapter --> nothing happens
-> unplug
-> plug in Samsung phone

[  147.581160] hd3ss3220 4-0047: Propagating role device
# cat /sys/class/typec/port0/data_role
host [device]

-> unplug

[  485.495874] hd3ss3220 4-0047: Propagating role none

========
Case 2: Device tree: power-role = "source";
========
# cat /sys/class/typec/port0/port_type
[source]
# echo "sink" > /sys/class/typec/port0/port_type
zsh: permission denied: /sys/class/typec/port0/port_type

->plug in ethernet adapter

[   94.590028] hd3ss3220 4-0047: Propagating role host
[   94.733892] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[   94.739427] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[   94.747464] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[   94.756900] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[   94.763060] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[   94.768565] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[   94.776246] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[   94.783382] hub 3-0:1.0: USB hub found
[   94.787178] hub 3-0:1.0: 1 port detected
[   94.791484] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[   94.800068] hub 4-0:1.0: USB hub found
[   94.803859] hub 4-0:1.0: 1 port detected
[   95.781709] usb 4-1: new SuperSpeed USB device number 2 using xhci-hcd
[   96.195802] usbcore: registered new interface driver cdc_ether
[   96.396140] cdc_ncm 4-1:2.0: MAC-Address: a0:ce:c8:9e:54:a4
[   96.401753] cdc_ncm 4-1:2.0: setting rx_max = 16384
[   96.419345] cdc_ncm 4-1:2.0: setting tx_max = 16384
[   96.434385] cdc_ncm 4-1:2.0 eth1: register 'cdc_ncm' at usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP), a0:ce:c8:9e:54:a4
[   96.444940] usbcore: registered new interface driver cdc_ncm

-> unplug

[  133.501500] hd3ss3220 4-0047: Propagating role none
[  133.506518] xhci-hcd xhci-hcd.1.auto: remove, state 1
[  133.511636] usb usb4: USB disconnect, device number 1
[  133.516732] usb 4-1: USB disconnect, device number 2
[  133.521934] cdc_ncm 4-1:2.0 eth1: unregister 'cdc_ncm' usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP)
[  133.602023] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  133.610348] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  133.616023] usb usb3: USB disconnect, device number 1
[  133.624475] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  133.733092] using host ethernet address: f8:dc:7a:a0:c0:ae
[  133.733104] using self ethernet address: 1A:22:33:44:55:66
[  133.739134] g_ncm gadget.0: HOST MAC f8:dc:7a:a0:c0:ae
[  133.749825] g_ncm gadget.0: MAC 1a:22:33:44:55:66
[  133.754587] g_ncm gadget.0: NCM Gadget
[  133.758354] g_ncm gadget.0: g_ncm ready

-> plug in phone --> phone is getting charged
-> unplug

==========
Case 3: Device tree: power-role = "dual";
==========
# cat /sys/class/typec/port0/port_type
[dual] source sink

-> plug in ethernet adapter

[  252.688228] hd3ss3220 4-0047: Propagating role host
[  252.823920] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  252.829468] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[  252.837506] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[  252.846943] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[  252.853098] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  252.858614] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[  252.866307] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[  252.873424] hub 3-0:1.0: USB hub found
[  252.877221] hub 3-0:1.0: 1 port detected
[  252.881532] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[  252.890122] hub 4-0:1.0: USB hub found
[  252.893911] hub 4-0:1.0: 1 port detected
[  253.872375] usb 4-1: new SuperSpeed USB device number 2 using xhci-hcd
[  254.282643] usbcore: registered new interface driver cdc_ether
[  254.483834] cdc_ncm 4-1:2.0: MAC-Address: a0:ce:c8:9e:54:a4
[  254.489447] cdc_ncm 4-1:2.0: setting rx_max = 16384
[  254.505900] cdc_ncm 4-1:2.0: setting tx_max = 16384
[  254.520769] cdc_ncm 4-1:2.0 eth1: register 'cdc_ncm' at usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP), a0:ce:c8:9e:54:a4
[  254.531322] usbcore: registered new interface driver cdc_ncm

-> unplug

[  273.270877] usb 4-1: USB disconnect, device number 2
[  273.276084] cdc_ncm 4-1:2.0 eth1: unregister 'cdc_ncm' usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP)
[  274.192136] hd3ss3220 4-0047: Propagating role none
[  274.197161] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  274.202251] usb usb4: USB disconnect, device number 1
[  274.207994] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  274.213739] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  274.218841] usb usb3: USB disconnect, device number 1
[  274.225032] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  274.335934] using host ethernet address: f8:dc:7a:a0:c0:ae
[  274.335947] using self ethernet address: 1A:22:33:44:55:66
[  274.341994] g_ncm gadget.0: HOST MAC f8:dc:7a:a0:c0:ae
[  274.352692] g_ncm gadget.0: MAC 1a:22:33:44:55:66
[  274.357455] g_ncm gadget.0: NCM Gadget
[  274.361225] g_ncm gadget.0: g_ncm ready

-> plug in Windows PC
[  322.321716] hd3ss3220 4-0047: Propagating role device
-> unplug
[  343.825254] hd3ss3220 4-0047: Propagating role none

-> plug in phone
[  454.410340] hd3ss3220 4-0047: Propagating role host
[  454.558026] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  454.563594] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[  454.571652] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[  454.581104] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[  454.587295] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  454.592836] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[  454.600546] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[  454.607623] hub 3-0:1.0: USB hub found
[  454.611475] hub 3-0:1.0: 1 port detected
[  454.615817] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[  454.624451] hub 4-0:1.0: USB hub found
[  454.628295] hub 4-0:1.0: 1 port detected
[  455.085270] usb 4-1: new SuperSpeed USB device number 2 using xhci-hcd
[  455.203088] cdc_acm 4-1:1.1: ttyACM0: USB ACM device
[  455.208249] usbcore: registered new interface driver cdc_acm
[  455.213947] cdc_acm: USB Abstract Control Model driver for USB modems and ISDN adapters

-> unplug
[  477.940537] usb 4-1: USB disconnect, device number 2
[  477.960408] hd3ss3220 4-0047: Propagating role none
[  477.965448] xhci-hcd xhci-hcd.1.auto: remove, state 1
[  477.970570] usb usb4: USB disconnect, device number 1
[  477.999753] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  478.005519] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  478.010605] usb usb3: USB disconnect, device number 1
[  478.016507] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  478.124154] using host ethernet address: f8:dc:7a:a0:c0:ae
[  478.124168] using self ethernet address: 1A:22:33:44:55:66
[  478.130222] g_ncm gadget.0: HOST MAC f8:dc:7a:a0:c0:ae
[  478.140901] g_ncm gadget.0: MAC 1a:22:33:44:55:66
[  478.145663] g_ncm gadget.0: NCM Gadget
[  478.149433] g_ncm gadget.0: g_ncm ready


# echo "source" > /sys/class/typec/port0/port_type
# cat /sys/class/typec/port0/port_type
# dual [source] sink
-> plug in ethernet dongle --> same as before
-> plug in phone --> same as before
-> plug in Windows PC
[  593.662330] hd3ss3220 4-0047: Propagating role host
[  593.794008] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  593.800172] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[  593.808248] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[  593.817695] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[  593.823871] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  593.829390] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[  593.837075] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[  593.844370] hub 3-0:1.0: USB hub found
[  593.848153] hub 3-0:1.0: 1 port detected
[  593.852335] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[  593.860805] hub 4-0:1.0: USB hub found
[  593.864586] hub 4-0:1.0: 1 port detected

# echo "sink" > /sys/class/typec/port0/port_type
# cat /sys/class/typec/port0/port_type
dual source [sink]

-> plug in ethernet dongle --> nothing happens
-> plug in laptop / phone
[  726.770383] hd3ss3220 4-0047: Propagating role device
-> unplug
[  729.842140] hd3ss3220 4-0047: Propagating role none

As you can see, setting the port type really changes the behavior of the port
w.r.t. the initial connection setup with the peer.
I hope this helps.

Best regards
Oliver

RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Biju Das]

Hi Facklam, Olivér,

> -----Original Message-----
> From: Facklam, Olivér <oliver.facklam@zuehlke.com>
> Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type
> 
> Hello Biju,
> 
> Thanks for your response and sorry for the delay.
> 
> > -----Original Message-----
> > From: Biju Das <biju.das.jz@bp.renesas.com>
> > Sent: Wednesday, November 27, 2024 9:48 AM
> > Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring
> > port type
> >
> > Hi Facklam, Olivér,
> >
> > > -----Original Message-----
> > > From: Facklam, Olivér <oliver.facklam@zuehlke.com>
> > > Sent: 27 November 2024 08:03
> > > Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support
> > > configuring port type
> > >
> > > Hi Heikki,
> > >
> > > > -----Original Message-----
> > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Sent: Monday, November 25, 2024 11:28 AM
> > > >
> > > > Hi Olivér,
> > > >
> > > > Sorry to keep you waiting.
> > > >
> > > > On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> > > > > Hello Heikki,
> > > > >
> > > > > Thanks for reviewing.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > > Sent: Monday, November 18, 2024 12:50 PM
> > > > > >
> > > > > > Hi Oliver,
> > > > > >
> > > > > > I'm sorry, I noticed a problem with this...
> > > > > >
> > > > > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > > > > > The TI HD3SS3220 Type-C controller supports configuring the
> > > > > > > port type it will operate as through the MODE_SELECT field
> > > > > > > of the General Control Register.
> > > > > > >
> > > > > > > Configure the port type based on the fwnode property "power-role"
> > > > > > > during probe, and through the port_type_set typec_operation.
> > > > > > >
> > > > > > > The MODE_SELECT field can only be changed when the
> > > > > > > controller is in unattached state, so follow the sequence
> > > > > > > recommended by the datasheet
> > > > > > to:
> > > > > > > 1. disable termination on CC pins to disable the controller 2.
> > > > > > > change the mode 3. re-enable termination
> > > > > > >
> > > > > > > This will effectively cause a connected device to disconnect
> > > > > > > for the duration of the mode change.
> > > > > >
> > > > > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > > > > > ---
> > > > > > >  drivers/usb/typec/hd3ss3220.c | 66
> > > > > > ++++++++++++++++++++++++++++++++++++++++++-
> > > > > > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > > > > b/drivers/usb/typec/hd3ss3220.c
> > > > > > > index
> > > > > >
> > > >
> > e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > > > > > 01f311fb60b4284da 100644
> > > > > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > > > > [...]
> > > > > > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct
> > > > > > > typec_port
> > > > > > *port, enum typec_data_role role)
> > > > > > >       return ret;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int hd3ss3220_port_type_set(struct typec_port *port,
> > > > > > > +enum
> > > > > > typec_port_type type)
> > > > > > > +{
> > > > > > > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > > > > > +
> > > > > > > +     return hd3ss3220_set_port_type(hd3ss3220, type); }
> > > >
> > > > > > >  static const struct typec_operations hd3ss3220_ops = {
> > > > > > > -     .dr_set = hd3ss3220_dr_set
> > > > > > > +     .dr_set = hd3ss3220_dr_set,
> > > > > > > +     .port_type_set = hd3ss3220_port_type_set,
> > > > > > >  };
> > > > > >
> > > > > > So here I think you should implement the pr_set callback instead.
> > > > >
> > > > > I can do that, but based on the MODE_SELECT register
> > > > > description, it seems to me that this setting is fundamentally
> > > > > changing the operation mode of the chip, i.e. the state machine
> > > > > that is being run for initial
> > > > connection.
> > > > > So there would have to be a way of "resetting" it to be a
> > > > > dual-role port again, which the "pr_set" callback doesn't seem to have?
> > > > >   This register can be written to set the HD3SS3220 mode
> > > > >   operation. The ADDR pin must be set to I2C mode. If the default
> > > > >   is maintained, HD3SS3220 shall operate according to the PORT
> > > > >   pin levels and modes. The MODE_SELECT can only be
> > > > >   changed when in the unattached state.
> > > > >   00 - DRP mode (start from unattached.SNK) (default)
> > > > >   01 - UFP mode (unattached.SNK)
> > > > >   10 - DFP mode (unattached.SRC)
> > > > >   11 - DRP mode (start from unattached.SNK)
> > > >
> > > > Okay, I see. This is not a case for pr_set.
> > > >
> > > > I'm still confused about the use case here. It seems you are not
> > > > interested in role swapping after all, so why would you need this
> > > > functionality to be exposed to the user space?
> > > >
> > > > I'm sorry if I've missed something.
> > > >
> > > > About the port_type attribute file itself. I would feel more
> > > > comfortable with it if it was allowed to be written only when
> > > > there is nothing connected to the port. At the very least, I think
> > > > it should be documented better so what it's really meant for would
> > > > be more
> > clear to everybody.
> > >
> > > After researching some more about this operation, I came across the
> > > driver for TUSB320 [1] which seems to have a very similar behavior
> > > (also
> > from TI).
> > > [1] - https://lore.kernel.org/all/20220730180500.152004-1-
> > > marex@denx.de/T/#ma7a322bc207414e4185c29d257ff30f5769f5d70
> > >
> > > For one variant of the chip, the implementation relies on the CC
> > > disabling like in this patch. A different variant tests the current
> > > connection
> > status before proceeding.
> > >
> >
> >
> > Can you please provide your test logs?
> 
> Adding them below.
> 
> >
> > Previously I tested 2 devices with
> > Switching roles host->device->host using
> >
> > echo device > /sys/class/typec/port0/data_role
> >
> > and
> >
> > echo host > /sys/class/typec/port0/data_role
> 
> Could you clarify what your test setup was?
> Did you control both sides of the USB connection and execute these commands on both sides?


Yes, Two Renesas RZ/G3E boards connected each other by usb type-c cable and execute these commands on both sides.

1)Host->device->Host  (First board)
2)Device->Host->device (Second board)


Cheers,
Biju

Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Heikki Krogerus]

Hi,

On Wed, Nov 27, 2024 at 08:02:55AM +0000, Facklam, Olivér wrote:
> Hi Heikki,
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Monday, November 25, 2024 11:28 AM
> > 
> > Hi Olivér,
> > 
> > Sorry to keep you waiting.
> > 
> > On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> > > Hello Heikki,
> > >
> > > Thanks for reviewing.
> > >
> > > > -----Original Message-----
> > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Sent: Monday, November 18, 2024 12:50 PM
> > > >
> > > > Hi Oliver,
> > > >
> > > > I'm sorry, I noticed a problem with this...
> > > >
> > > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > > > The TI HD3SS3220 Type-C controller supports configuring the port
> > > > > type it will operate as through the MODE_SELECT field of the
> > > > > General Control Register.
> > > > >
> > > > > Configure the port type based on the fwnode property "power-role"
> > > > > during probe, and through the port_type_set typec_operation.
> > > > >
> > > > > The MODE_SELECT field can only be changed when the controller is
> > > > > in unattached state, so follow the sequence recommended by the
> > > > > datasheet
> > > > to:
> > > > > 1. disable termination on CC pins to disable the controller 2.
> > > > > change the mode 3. re-enable termination
> > > > >
> > > > > This will effectively cause a connected device to disconnect for
> > > > > the duration of the mode change.
> > > >
> > > > Changing the type of the port is really problematic, and IMO we
> > > > should actually never support that.
> > >
> > > Could you clarify why you think it is problematic?
> > 
> > It's not completely clear to me what it's meant for. If it was just for fixing the
> > type of the port to be sink, source or DRP before connections, it would make
> > sense, but since it can be use even when there is an actice connection (there
> > is nothing preventing that), it can in practice be used to swap the role.
> > 
> > And in some cases in the past where this attribute file was proposed to be
> > used with some other drivers, the actual goal really ended up being to be
> > able to just swap the role with an existing connection instead of being able to
> > fix the type of the port. The commit message made it sound like that could be
> > the goal in this case as well, but maybe I misunderstood.
> > 
> > Even in cases where it's clear that the intention is to just fix the role before
> > connections, why would user space needs to control that is still not
> > completely clear, at least not to me.
> 
> The idea is to give the user the possibility to control/restrict how the port is
> operating even if they have an actual dual-role capable port.
> 
> Let me clarify. In my use case, I have a DRP port, and in most cases I would
> like to use it as such.
> However, there are cases where this operating mode causes additional
> difficulties -- for example when connecting to another dual-role port 
> implementing the same role preference (e.g. 2 Try.SNK devices connected
> together). Then the role outcome is random.
> Since this chip doesn't support PD, there is no way to switch the role from
> userspace.
> When I know I'm going to be working with these types of connections, it
> would be better if I can restrict the operation mode to "sink-only" (for example)
> for that duration. Without needing to change my device tree.
> 
> Sure, the mechanism can be abused to switch the role on an active connection,
> but that was not the primary idea here.
> I would even argue that calling a port type change during an active
> connection terminates that connection, and starts a new connection
> from scratch with the new behavior.

Okay, thanks for the explanation.

> > > > Consider for example, if your port is sink only, then the platform
> > > > almost certainly can't drive the VBUS. This patch would still allow
> > > > the port to be changed to source port.
> > >
> > > In my testing, it appeared to me that when registering a type-c port
> > > with "typec_cap.type = TYPEC_PORT_SNK" (for example), then the type-c
> > > class disables the port_type_store functionality:
> > > 	if (port->cap->type != TYPEC_PORT_DRP ||
> > > 	    !port->ops || !port->ops->port_type_set) {
> > > 		dev_dbg(dev, "changing port type not supported\n");
> > > 		return -EOPNOTSUPP;
> > > 	}
> > >
> > > So to my understanding, a platform which cannot drive VBUS should
> > > simply set the fwnode `power-role="sink"`. Since patch 2/4 correctly
> > > parses this property, wouldn't that solve this case?
> > 
> > True. I stand corrected.
> > 
> > > > Sorry for not realising this in v1.
> > > >
> > > > I think what you want here is just a power role swap. Currently
> > > > power role swap is only supported when USB PD is supported in the
> > > > class code, but since the USB Type-C specification quite clearly
> > > > states that power role and data role swap can be optionally
> > > > supported even when USB PD is not supported (section 2.3.3) we need to
> > fix that:
> > >
> > > My interpretation of section 2.3.3 is that the 2 mechanisms allowing
> > > power role swap are:
> > > - USB PD (after initial connection)
> > > - "as part of the initial connection process": to me this is simply referring to
> > the
> > > 	Try.SRC / Try.SNK mechanism, for which we already have
> > > 	the "try_role" callback.
> > >
> > > Maybe I'm misunderstanding what the intentions are behind each of the
> > > typec_operations, so if you could clarify that (or give some pointer),
> > > that would be appreciated. My understanding:
> > > - "try_role": set Try.SRC / Try.SNK / no preference for a dual-role
> > > port for initial connection
> > > - "pr_set" / "dr_set" / "vconn_set": swap power and data role resp.
> > > 	after the initial connection using USB-PD.
> > > - "port_type_set": configure what port type to operate as, i.e. which initial
> > connection
> > > 	state machine from the USB-C standard to apply for the next
> > > connection Please correct me if any of these are incorrect.
> > 
> > I don't know what's the intention with the port_type attribute file
> > unfortunately.
> > 
> > > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > > index 58f40156de56..ee81909565a4 100644
> > > > --- a/drivers/usb/typec/class.c
> > > > +++ b/drivers/usb/typec/class.c
> > > > @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device
> > > > *dev,
> > > >                 return -EOPNOTSUPP;
> > > >         }
> > > >
> > > > -       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> > > > -               dev_dbg(dev, "partner unable to swap power role\n");
> > > > -               return -EIO;
> > > > -       }
> > > > -
> > > >         ret = sysfs	_match_string(typec_roles, buf);
> > > >         if (ret < 0)
> > > >                 return ret;
> > > >
> > > >
> > > > After that it should be possible to do power role swap also in this
> > > > driver when the port is DRP capable.
> > > >
> > > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > > > ---
> > > > >  drivers/usb/typec/hd3ss3220.c | 66
> > > > ++++++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > > b/drivers/usb/typec/hd3ss3220.c
> > > > > index
> > > >
> > e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > > > 01f311fb60b4284da 100644
> > > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > > [...]
> > > > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port
> > > > *port, enum typec_data_role role)
> > > > >       return ret;
> > > > >  }
> > > > >
> > > > > +static int hd3ss3220_port_type_set(struct typec_port *port, enum
> > > > typec_port_type type)
> > > > > +{
> > > > > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > > > +
> > > > > +     return hd3ss3220_set_port_type(hd3ss3220, type); }
> > > >
> > > > This wrapper seems completely useless. You only need one function
> > > > here for the callback.
> > >
> > > The wrapper is to extract the struct hd3ss3220 from the typec_port.
> > > The underlying hd3ss3220_set_port_type I am also using during probe to
> > > configure initial port type.
> > 
> > Ah, I missed that. Sorry about that.
> > 
> > > One point worth mentioning here is that if the MODE_SELECT register is
> > > not configured, the chip will operate according to a default which is
> > > chosen by an external pin (sorry if this was not detailed enough in
> > > commit msg)
> > > >From the datasheet:
> > > -------------------
> > > | PORT | 4 | I | Tri-level input pin to indicate port mode. The state
> > > | of this pin is sampled when HD3SS3220's
> > > 		ENn_CC is asserted low, and VDD5 is active. This pin is also
> > sampled following a
> > > 		I2C_SOFT_RESET.
> > > 		H - DFP (Pull-up to VDD5 if DFP mode is desired)
> > > 		NC - DRP (Leave unconnected if DRP mode is desired)
> > > 		L - UFP (Pull-down or tie to GND if UFP mode is desired)
> > >
> > > In our use case, it was not desirable to leave this default based on
> > > wiring, and it makes more sense to me to allow the configuration to
> > > come from the fwnode property. Hence the port type setting in probe().
> > 
> > I get that, but that just means you want to fix the type during probe, no?
> > Why do you need to expose this to the user space?
> 
> I've been thinking a bit more about this "fixing the type during probe" feature.
> My current implementation always fixes the type, even if no device tree entry
> for "power-role" was found. Could that cause issues for people relying on the
> configuration through the PORT pin?
> 
> I could consider a solution where if the property is absent, the type is not
> reconfigured during the probe. Although then I would have to do manual parsing
> of that DT property. With typec_get_fw_cap() from patch 2/4, I loose
> the information about individual properties being present/absent.
> Would be interested in hearing your thoughts.

I don't think it's a problem to check does the property exist directly
in the driver. It sounds like that's what should be done in this case.

Br,

-- 
heikki

RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Facklam, Olivér]

Hello,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Thursday, November 28, 2024 2:15 PM
> Subject: Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port
> type
> 
> Hi,
> 
> On Wed, Nov 27, 2024 at 08:02:55AM +0000, Facklam, Olivér wrote:
> > Hi Heikki,
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Monday, November 25, 2024 11:28 AM
> > >
> > > Hi Olivér,
> > >
> > > Sorry to keep you waiting.
> > >
> > > On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> > > > Hello Heikki,
> > > >
> > > > Thanks for reviewing.
> > > >
> > > > > -----Original Message-----
> > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > Sent: Monday, November 18, 2024 12:50 PM
> > > > >
> > > > > Hi Oliver,
> > > > >
> > > > > I'm sorry, I noticed a problem with this...
> > > > >
> > > > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > > > > The TI HD3SS3220 Type-C controller supports configuring the
> > > > > > port type it will operate as through the MODE_SELECT field of
> > > > > > the General Control Register.
> > > > > >
> > > > > > Configure the port type based on the fwnode property "power-role"
> > > > > > during probe, and through the port_type_set typec_operation.
> > > > > >
> > > > > > The MODE_SELECT field can only be changed when the controller
> > > > > > is in unattached state, so follow the sequence recommended by
> > > > > > the datasheet
> > > > > to:
> > > > > > 1. disable termination on CC pins to disable the controller 2.
> > > > > > change the mode 3. re-enable termination
> > > > > >
> > > > > > This will effectively cause a connected device to disconnect
> > > > > > for the duration of the mode change.
> > > > >
> > > > > Changing the type of the port is really problematic, and IMO we
> > > > > should actually never support that.
> > > >
> > > > Could you clarify why you think it is problematic?
> > >
> > > It's not completely clear to me what it's meant for. If it was just
> > > for fixing the type of the port to be sink, source or DRP before
> > > connections, it would make sense, but since it can be use even when
> > > there is an actice connection (there is nothing preventing that), it can in
> practice be used to swap the role.
> > >
> > > And in some cases in the past where this attribute file was proposed
> > > to be used with some other drivers, the actual goal really ended up
> > > being to be able to just swap the role with an existing connection
> > > instead of being able to fix the type of the port. The commit
> > > message made it sound like that could be the goal in this case as well, but
> maybe I misunderstood.
> > >
> > > Even in cases where it's clear that the intention is to just fix the
> > > role before connections, why would user space needs to control that
> > > is still not completely clear, at least not to me.
> >
> > The idea is to give the user the possibility to control/restrict how
> > the port is operating even if they have an actual dual-role capable port.
> >
> > Let me clarify. In my use case, I have a DRP port, and in most cases I
> > would like to use it as such.
> > However, there are cases where this operating mode causes additional
> > difficulties -- for example when connecting to another dual-role port
> > implementing the same role preference (e.g. 2 Try.SNK devices
> > connected together). Then the role outcome is random.
> > Since this chip doesn't support PD, there is no way to switch the role
> > from userspace.
> > When I know I'm going to be working with these types of connections,
> > it would be better if I can restrict the operation mode to "sink-only"
> > (for example) for that duration. Without needing to change my device tree.
> >
> > Sure, the mechanism can be abused to switch the role on an active
> > connection, but that was not the primary idea here.
> > I would even argue that calling a port type change during an active
> > connection terminates that connection, and starts a new connection
> > from scratch with the new behavior.
> 
> Okay, thanks for the explanation.
> 
> > > > > Consider for example, if your port is sink only, then the
> > > > > platform almost certainly can't drive the VBUS. This patch would
> > > > > still allow the port to be changed to source port.
> > > >
> > > > In my testing, it appeared to me that when registering a type-c
> > > > port with "typec_cap.type = TYPEC_PORT_SNK" (for example), then
> > > > the type-c class disables the port_type_store functionality:
> > > > 	if (port->cap->type != TYPEC_PORT_DRP ||
> > > > 	    !port->ops || !port->ops->port_type_set) {
> > > > 		dev_dbg(dev, "changing port type not supported\n");
> > > > 		return -EOPNOTSUPP;
> > > > 	}
> > > >
> > > > So to my understanding, a platform which cannot drive VBUS should
> > > > simply set the fwnode `power-role="sink"`. Since patch 2/4
> > > > correctly parses this property, wouldn't that solve this case?
> > >
> > > True. I stand corrected.
> > >
> > > > > Sorry for not realising this in v1.
> > > > >
> > > > > I think what you want here is just a power role swap. Currently
> > > > > power role swap is only supported when USB PD is supported in
> > > > > the class code, but since the USB Type-C specification quite
> > > > > clearly states that power role and data role swap can be
> > > > > optionally supported even when USB PD is not supported (section
> > > > > 2.3.3) we need to
> > > fix that:
> > > >
> > > > My interpretation of section 2.3.3 is that the 2 mechanisms
> > > > allowing power role swap are:
> > > > - USB PD (after initial connection)
> > > > - "as part of the initial connection process": to me this is
> > > > simply referring to
> > > the
> > > > 	Try.SRC / Try.SNK mechanism, for which we already have
> > > > 	the "try_role" callback.
> > > >
> > > > Maybe I'm misunderstanding what the intentions are behind each of
> > > > the typec_operations, so if you could clarify that (or give some
> > > > pointer), that would be appreciated. My understanding:
> > > > - "try_role": set Try.SRC / Try.SNK / no preference for a
> > > > dual-role port for initial connection
> > > > - "pr_set" / "dr_set" / "vconn_set": swap power and data role resp.
> > > > 	after the initial connection using USB-PD.
> > > > - "port_type_set": configure what port type to operate as, i.e.
> > > > which initial
> > > connection
> > > > 	state machine from the USB-C standard to apply for the next
> > > > connection Please correct me if any of these are incorrect.
> > >
> > > I don't know what's the intention with the port_type attribute file
> > > unfortunately.
> > >
> > > > > diff --git a/drivers/usb/typec/class.c
> > > > > b/drivers/usb/typec/class.c index 58f40156de56..ee81909565a4
> > > > > 100644
> > > > > --- a/drivers/usb/typec/class.c
> > > > > +++ b/drivers/usb/typec/class.c
> > > > > @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct
> > > > > device *dev,
> > > > >                 return -EOPNOTSUPP;
> > > > >         }
> > > > >
> > > > > -       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> > > > > -               dev_dbg(dev, "partner unable to swap power role\n");
> > > > > -               return -EIO;
> > > > > -       }
> > > > > -
> > > > >         ret = sysfs	_match_string(typec_roles, buf);
> > > > >         if (ret < 0)
> > > > >                 return ret;
> > > > >
> > > > >
> > > > > After that it should be possible to do power role swap also in
> > > > > this driver when the port is DRP capable.
> > > > >
> > > > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > > > > ---
> > > > > >  drivers/usb/typec/hd3ss3220.c | 66
> > > > > ++++++++++++++++++++++++++++++++++++++++++-
> > > > > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > > > b/drivers/usb/typec/hd3ss3220.c
> > > > > > index
> > > > >
> > >
> e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > > > > 01f311fb60b4284da 100644
> > > > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > > > [...]
> > > > > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct
> > > > > > typec_port
> > > > > *port, enum typec_data_role role)
> > > > > >       return ret;
> > > > > >  }
> > > > > >
> > > > > > +static int hd3ss3220_port_type_set(struct typec_port *port,
> > > > > > +enum
> > > > > typec_port_type type)
> > > > > > +{
> > > > > > +     struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > > > > +
> > > > > > +     return hd3ss3220_set_port_type(hd3ss3220, type); }
> > > > >
> > > > > This wrapper seems completely useless. You only need one
> > > > > function here for the callback.
> > > >
> > > > The wrapper is to extract the struct hd3ss3220 from the typec_port.
> > > > The underlying hd3ss3220_set_port_type I am also using during
> > > > probe to configure initial port type.
> > >
> > > Ah, I missed that. Sorry about that.
> > >
> > > > One point worth mentioning here is that if the MODE_SELECT
> > > > register is not configured, the chip will operate according to a
> > > > default which is chosen by an external pin (sorry if this was not
> > > > detailed enough in commit msg)
> > > > >From the datasheet:
> > > > -------------------
> > > > | PORT | 4 | I | Tri-level input pin to indicate port mode. The
> > > > | state of this pin is sampled when HD3SS3220's
> > > > 		ENn_CC is asserted low, and VDD5 is active. This pin is also
> > > sampled following a
> > > > 		I2C_SOFT_RESET.
> > > > 		H - DFP (Pull-up to VDD5 if DFP mode is desired)
> > > > 		NC - DRP (Leave unconnected if DRP mode is desired)
> > > > 		L - UFP (Pull-down or tie to GND if UFP mode is desired)
> > > >
> > > > In our use case, it was not desirable to leave this default based
> > > > on wiring, and it makes more sense to me to allow the
> > > > configuration to come from the fwnode property. Hence the port type
> setting in probe().
> > >
> > > I get that, but that just means you want to fix the type during probe, no?
> > > Why do you need to expose this to the user space?
> >
> > I've been thinking a bit more about this "fixing the type during probe"
> feature.
> > My current implementation always fixes the type, even if no device
> > tree entry for "power-role" was found. Could that cause issues for
> > people relying on the configuration through the PORT pin?
> >
> > I could consider a solution where if the property is absent, the type
> > is not reconfigured during the probe. Although then I would have to do
> > manual parsing of that DT property. With typec_get_fw_cap() from patch
> > 2/4, I loose the information about individual properties being
> present/absent.
> > Would be interested in hearing your thoughts.
> 
> I don't think it's a problem to check does the property exist directly in the
> driver. It sounds like that's what should be done in this case.

Thanks for the input.
I'll shortly send a v3 where I drop patch 2/4, and instead have the parsing
of the relevant property in patch 3/4 and 4/4 respectively.

> 
> Br,
> 
> --
> heikki

Thanks
Oliver

[PATCH v2 4/4] usb: typec: hd3ss3220: support configuring role preference based on fwnode property and typec_operation

[Oliver Facklam]

The TI HD3SS3220 Type-C controller supports configuring
its role preference when operating as a dual-role port
through the SOURCE_PREF field of the General Control Register.

The previous driver behavior was to set the role preference
based on the dr_set typec_operation.
However, the controller does not support swapping the data role
on an active connection due to its lack of Power Delivery support.

Remove previous dr_set typec_operation, and support setting
the role preference based on the corresponding fwnode property,
as well as the try_role typec_operation.

Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
---
 drivers/usb/typec/hd3ss3220.c | 50 +++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index e3e9b1597e3b09b82f0726a01f311fb60b4284da..666bf15f88a2adb778f14efd982c0803424512bd 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -127,8 +127,25 @@ static int hd3ss3220_set_port_type(struct hd3ss3220 *hd3ss3220, int type)
 	return err;
 }
 
-static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
+static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int prefer_role)
 {
+	int src_pref;
+
+	switch (prefer_role) {
+	case TYPEC_NO_PREFERRED_ROLE:
+		src_pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT;
+		break;
+	case TYPEC_SINK:
+		src_pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
+		break;
+	case TYPEC_SOURCE:
+		src_pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
+		break;
+	default:
+		dev_err(hd3ss3220->dev, "bad role preference: %d\n", prefer_role);
+		return -EINVAL;
+	}
+
 	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
 				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
 				  src_pref);
@@ -160,27 +177,11 @@ static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 *hd3ss3220)
 	return attached_state;
 }
 
-static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role)
+static int hd3ss3220_try_role(struct typec_port *port, int role)
 {
 	struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
-	enum usb_role role_val;
-	int pref, ret = 0;
 
-	if (role == TYPEC_HOST) {
-		role_val = USB_ROLE_HOST;
-		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
-	} else {
-		role_val = USB_ROLE_DEVICE;
-		pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
-	}
-
-	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
-	usleep_range(10, 100);
-
-	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
-	typec_set_data_role(hd3ss3220->port, role);
-
-	return ret;
+	return hd3ss3220_set_source_pref(hd3ss3220, role);
 }
 
 static int hd3ss3220_port_type_set(struct typec_port *port, enum typec_port_type type)
@@ -191,7 +192,7 @@ static int hd3ss3220_port_type_set(struct typec_port *port, enum typec_port_type
 }
 
 static const struct typec_operations hd3ss3220_ops = {
-	.dr_set = hd3ss3220_dr_set,
+	.try_role = hd3ss3220_try_role,
 	.port_type_set = hd3ss3220_port_type_set,
 };
 
@@ -200,9 +201,6 @@ static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
 	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
 
 	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
-	if (role_state == USB_ROLE_NONE)
-		hd3ss3220_set_source_pref(hd3ss3220,
-				HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
 
 	switch (role_state) {
 	case USB_ROLE_HOST:
@@ -297,8 +295,6 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	if (IS_ERR(hd3ss3220->regmap))
 		return PTR_ERR(hd3ss3220->regmap);
 
-	hd3ss3220_set_source_pref(hd3ss3220,
-				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
 	/* For backward compatibility check the connector child node first */
 	connector = device_get_named_child_node(hd3ss3220->dev, "connector");
 	if (connector) {
@@ -333,6 +329,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	if (ret != 0 && ret != -EINVAL && ret != -ENXIO)
 		goto err_put_role;
 
+	ret = hd3ss3220_set_source_pref(hd3ss3220, typec_cap.prefer_role);
+	if (ret < 0)
+		goto err_put_role;
+
 	ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type);
 	if (ret < 0)
 		goto err_put_role;

-- 
2.34.1