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

[PATCH v3 0/3] 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 v3:
- Drop PATCH 2/4 from v2 (using typec_get_fw_cap() for capability
  parsing)
- Implement parsing manually to have better control over
  each property being present / absent.
- If the "power-role"/"try-power-role" property is absent, we don't
  set the corresponding register during probe anymore, but let the
  chip use its default.
- Link to v2: https://lore.kernel.org/r/20241114-usb-typec-controller-enhancements-v2-0-362376856aea@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 (3):
      usb: typec: hd3ss3220: configure advertised power opmode based on fwnode property
      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 | 207 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 184 insertions(+), 23 deletions(-)
---
base-commit: 237d4e0f41130a5ff0e1c7dc1cb41ee2fe21cd2a
change-id: 20241104-usb-typec-controller-enhancements-6871249429f0

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

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

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
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

[PATCH v3 2/3] 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, if present. If the property is absent, leave the
operation mode at the default, which is defined by the PORT pin
of the chip.
Support configuring the port type through the port_type_set
typec_operation as well.

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 | 88 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 56f74bf70895ca701083bde44a5bbe0b691551e1..e2059b925ab15733ff097e940751759ed51e0ab3 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)
@@ -211,6 +271,28 @@ static int hd3ss3220_configure_power_opmode(struct hd3ss3220 *hd3ss3220,
 	return hd3ss3220_set_power_opmode(hd3ss3220, power_opmode);
 }
 
+static int hd3ss3220_configure_port_type(struct hd3ss3220 *hd3ss3220,
+					 struct fwnode_handle *connector,
+					 struct typec_capability *cap)
+{
+	/*
+	 * Port type can be configured through device tree
+	 */
+	const char *cap_str;
+	int ret;
+
+	ret = fwnode_property_read_string(connector, "power-role", &cap_str);
+	if (ret)
+		return 0;
+
+	ret = typec_find_port_power_role(cap_str);
+	if (ret < 0)
+		return ret;
+
+	cap->type = ret;
+	return hd3ss3220_set_port_type(hd3ss3220, cap->type);
+}
+
 static const struct regmap_config config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -266,6 +348,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	typec_cap.ops = &hd3ss3220_ops;
 	typec_cap.fwnode = connector;
 
+	ret = hd3ss3220_configure_port_type(hd3ss3220, connector, &typec_cap);
+	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 v3 2/3] usb: typec: hd3ss3220: support configuring port type

[Heikki Krogerus]

On Wed, Dec 11, 2024 at 05:32:46PM +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, if present. If the property is absent, leave the
> operation mode at the default, which is defined by the PORT pin
> of the chip.
> Support configuring the port type through the port_type_set
> typec_operation as well.
> 
> 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>

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

> ---
>  drivers/usb/typec/hd3ss3220.c | 88 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index 56f74bf70895ca701083bde44a5bbe0b691551e1..e2059b925ab15733ff097e940751759ed51e0ab3 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)
> @@ -211,6 +271,28 @@ static int hd3ss3220_configure_power_opmode(struct hd3ss3220 *hd3ss3220,
>  	return hd3ss3220_set_power_opmode(hd3ss3220, power_opmode);
>  }
>  
> +static int hd3ss3220_configure_port_type(struct hd3ss3220 *hd3ss3220,
> +					 struct fwnode_handle *connector,
> +					 struct typec_capability *cap)
> +{
> +	/*
> +	 * Port type can be configured through device tree
> +	 */
> +	const char *cap_str;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(connector, "power-role", &cap_str);
> +	if (ret)
> +		return 0;
> +
> +	ret = typec_find_port_power_role(cap_str);
> +	if (ret < 0)
> +		return ret;
> +
> +	cap->type = ret;
> +	return hd3ss3220_set_port_type(hd3ss3220, cap->type);
> +}
> +
>  static const struct regmap_config config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -266,6 +348,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  	typec_cap.ops = &hd3ss3220_ops;
>  	typec_cap.fwnode = connector;
>  
> +	ret = hd3ss3220_configure_port_type(hd3ss3220, connector, &typec_cap);
> +	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

-- 
heikki

[PATCH v3 3/3] 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 | 72 ++++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index e2059b925ab15733ff097e940751759ed51e0ab3..3ecc688dda82a371929e204a490a68c8e9d81fe9 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:
@@ -293,6 +291,28 @@ static int hd3ss3220_configure_port_type(struct hd3ss3220 *hd3ss3220,
 	return hd3ss3220_set_port_type(hd3ss3220, cap->type);
 }
 
+static int hd3ss3220_configure_source_pref(struct hd3ss3220 *hd3ss3220,
+					   struct fwnode_handle *connector,
+					   struct typec_capability *cap)
+{
+	/*
+	 * Preferred role can be configured through device tree
+	 */
+	const char *cap_str;
+	int ret;
+
+	ret = fwnode_property_read_string(connector, "try-power-role", &cap_str);
+	if (ret)
+		return 0;
+
+	ret = typec_find_power_role(cap_str);
+	if (ret < 0)
+		return ret;
+
+	cap->prefer_role = ret;
+	return hd3ss3220_set_source_pref(hd3ss3220, cap->prefer_role);
+}
+
 static const struct regmap_config config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -319,8 +339,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) {
@@ -348,6 +366,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	typec_cap.ops = &hd3ss3220_ops;
 	typec_cap.fwnode = connector;
 
+	ret = hd3ss3220_configure_source_pref(hd3ss3220, connector, &typec_cap);
+	if (ret < 0)
+		goto err_put_role;
+
 	ret = hd3ss3220_configure_port_type(hd3ss3220, connector, &typec_cap);
 	if (ret < 0)
 		goto err_put_role;

-- 
2.34.1

Re: [PATCH v3 3/3] usb: typec: hd3ss3220: support configuring role preference based on fwnode property and typec_operation

[Heikki Krogerus]

On Wed, Dec 11, 2024 at 05:32:47PM +0100, Oliver Facklam wrote:
> 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>

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

> ---
>  drivers/usb/typec/hd3ss3220.c | 72 ++++++++++++++++++++++++++++---------------
>  1 file changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index e2059b925ab15733ff097e940751759ed51e0ab3..3ecc688dda82a371929e204a490a68c8e9d81fe9 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:
> @@ -293,6 +291,28 @@ static int hd3ss3220_configure_port_type(struct hd3ss3220 *hd3ss3220,
>  	return hd3ss3220_set_port_type(hd3ss3220, cap->type);
>  }
>  
> +static int hd3ss3220_configure_source_pref(struct hd3ss3220 *hd3ss3220,
> +					   struct fwnode_handle *connector,
> +					   struct typec_capability *cap)
> +{
> +	/*
> +	 * Preferred role can be configured through device tree
> +	 */
> +	const char *cap_str;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(connector, "try-power-role", &cap_str);
> +	if (ret)
> +		return 0;
> +
> +	ret = typec_find_power_role(cap_str);
> +	if (ret < 0)
> +		return ret;
> +
> +	cap->prefer_role = ret;
> +	return hd3ss3220_set_source_pref(hd3ss3220, cap->prefer_role);
> +}
> +
>  static const struct regmap_config config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -319,8 +339,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) {
> @@ -348,6 +366,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  	typec_cap.ops = &hd3ss3220_ops;
>  	typec_cap.fwnode = connector;
>  
> +	ret = hd3ss3220_configure_source_pref(hd3ss3220, connector, &typec_cap);
> +	if (ret < 0)
> +		goto err_put_role;
> +
>  	ret = hd3ss3220_configure_port_type(hd3ss3220, connector, &typec_cap);
>  	if (ret < 0)
>  		goto err_put_role;
> 
> -- 
> 2.34.1

-- 
heikki