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

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

[Oliver Facklam via B4 Relay]

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>
---
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 | 171 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 144 insertions(+), 27 deletions(-)
---
base-commit: afb92ad8733ef0a2843cc229e4d96aead80bc429
change-id: 20241104-usb-typec-controller-enhancements-6871249429f0

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

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

[Oliver Facklam via B4 Relay]

From: Oliver Facklam <oliver.facklam@zuehlke.com>

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

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

[Oliver Facklam via B4 Relay]

From: Oliver Facklam <oliver.facklam@zuehlke.com>

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.

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

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 56f74bf70895ca701083bde44a5bbe0b691551e1..e6e4b1871b5d805f8c367131509f4e6ec0d2b5f0 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -259,12 +259,12 @@ static int hd3ss3220_probe(struct i2c_client *client)
 		goto err_put_fwnode;
 	}
 
-	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+	ret = typec_get_fw_cap(&typec_cap, connector);
+	if (ret)
+		goto err_put_role;
+
 	typec_cap.driver_data = hd3ss3220;
-	typec_cap.type = TYPEC_PORT_DRP;
-	typec_cap.data = TYPEC_PORT_DRD;
 	typec_cap.ops = &hd3ss3220_ops;
-	typec_cap.fwnode = connector;
 
 	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(hd3ss3220->port)) {

-- 
2.34.1

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

[Oliver Facklam via B4 Relay]

From: Oliver Facklam <oliver.facklam@zuehlke.com>

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 e6e4b1871b5d805f8c367131509f4e6ec0d2b5f0..357c4e9491a2e3047046db91858ac98ab484ef62 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)
@@ -263,6 +323,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	if (ret)
 		goto err_put_role;
 
+	ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type);
+	if (ret < 0)
+		goto err_put_role;
+
 	typec_cap.driver_data = hd3ss3220;
 	typec_cap.ops = &hd3ss3220_ops;
 

-- 
2.34.1

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

[Oliver Facklam via B4 Relay]

From: Oliver Facklam <oliver.facklam@zuehlke.com>

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 357c4e9491a2e3047046db91858ac98ab484ef62..c6922989a3cd04fd878e37888856ac1b9c5135ec 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) {
@@ -323,6 +319,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
 	if (ret)
 		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

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

[Heikki Krogerus]

Hi Oliver,

On Thu, Nov 07, 2024 at 12:43:28PM +0100, Oliver Facklam via B4 Relay wrote:
> From: Oliver Facklam <oliver.facklam@zuehlke.com>
> 
> 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.
> 
> Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> ---
>  drivers/usb/typec/hd3ss3220.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index 56f74bf70895ca701083bde44a5bbe0b691551e1..e6e4b1871b5d805f8c367131509f4e6ec0d2b5f0 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -259,12 +259,12 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  		goto err_put_fwnode;
>  	}
>  
> -	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> +	ret = typec_get_fw_cap(&typec_cap, connector);
> +	if (ret)
> +		goto err_put_role;

You are not leaving any fallback here. Are you sure all the existing
systems supply those properties?

There is another problem. At least "data-role" property is optional,
but that will in practice make it a requirement.

I think it would be safer to use the values from the device properties
only if they are available. Otherwise simply use default values.

>  	typec_cap.driver_data = hd3ss3220;
> -	typec_cap.type = TYPEC_PORT_DRP;
> -	typec_cap.data = TYPEC_PORT_DRD;
>  	typec_cap.ops = &hd3ss3220_ops;
> -	typec_cap.fwnode = connector;
>  
>  	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
>  	if (IS_ERR(hd3ss3220->port)) {

thanks,

-- 
heikki

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

[Facklam, Olivér]

Hello Heikki,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Thursday, November 7, 2024 3:21 PM
> To: Facklam, Olivér <oliver.facklam@zuehlke.com>
> Cc: 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 2/4] usb: typec: hd3ss3220: use typec_get_fw_cap() to fill
> typec_cap
> 
> Hi Oliver,
> 
> On Thu, Nov 07, 2024 at 12:43:28PM +0100, Oliver Facklam via B4 Relay
> wrote:
> > From: Oliver Facklam <oliver.facklam@zuehlke.com>
> >
> > 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.
> >
> > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > ---
> >  drivers/usb/typec/hd3ss3220.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/typec/hd3ss3220.c
> > b/drivers/usb/typec/hd3ss3220.c index
> >
> 56f74bf70895ca701083bde44a5bbe0b691551e1..e6e4b1871b5d805f8c3671
> 31509f
> > 4e6ec0d2b5f0 100644
> > --- a/drivers/usb/typec/hd3ss3220.c
> > +++ b/drivers/usb/typec/hd3ss3220.c
> > @@ -259,12 +259,12 @@ static int hd3ss3220_probe(struct i2c_client
> *client)
> >  		goto err_put_fwnode;
> >  	}
> >
> > -	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > +	ret = typec_get_fw_cap(&typec_cap, connector);
> > +	if (ret)
> > +		goto err_put_role;
> 
> You are not leaving any fallback here. Are you sure all the existing systems
> supply those properties?
> 
> There is another problem. At least "data-role" property is optional, but that
> will in practice make it a requirement.

I missed that typec_get_fw_cap() was not setting a default if the property is absent.

> 
> I think it would be safer to use the values from the device properties only if
> they are available. Otherwise simply use default values.

I'll add back the previous values as defaults before calling typec_get_fw_cap().
That will make data-role and try-power-role optional again, as intended.

However, "power-role" seems to be required by the function. Is this expected?
Or should I write my own fwnode parsing logic?

> 
> >  	typec_cap.driver_data = hd3ss3220;
> > -	typec_cap.type = TYPEC_PORT_DRP;
> > -	typec_cap.data = TYPEC_PORT_DRD;
> >  	typec_cap.ops = &hd3ss3220_ops;
> > -	typec_cap.fwnode = connector;
> >
> >  	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
> >  	if (IS_ERR(hd3ss3220->port)) {
> 
> thanks,
> 
> --
> heikki

Best regards,
Oliver

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

[Heikki Krogerus]

On Thu, Nov 07, 2024 at 03:24:44PM +0000, Facklam, Olivér wrote:
> Hello Heikki,
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Thursday, November 7, 2024 3:21 PM
> > To: Facklam, Olivér <oliver.facklam@zuehlke.com>
> > Cc: 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 2/4] usb: typec: hd3ss3220: use typec_get_fw_cap() to fill
> > typec_cap
> > 
> > Hi Oliver,
> > 
> > On Thu, Nov 07, 2024 at 12:43:28PM +0100, Oliver Facklam via B4 Relay
> > wrote:
> > > From: Oliver Facklam <oliver.facklam@zuehlke.com>
> > >
> > > 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.
> > >
> > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > ---
> > >  drivers/usb/typec/hd3ss3220.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > b/drivers/usb/typec/hd3ss3220.c index
> > >
> > 56f74bf70895ca701083bde44a5bbe0b691551e1..e6e4b1871b5d805f8c3671
> > 31509f
> > > 4e6ec0d2b5f0 100644
> > > --- a/drivers/usb/typec/hd3ss3220.c
> > > +++ b/drivers/usb/typec/hd3ss3220.c
> > > @@ -259,12 +259,12 @@ static int hd3ss3220_probe(struct i2c_client
> > *client)
> > >  		goto err_put_fwnode;
> > >  	}
> > >
> > > -	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > > +	ret = typec_get_fw_cap(&typec_cap, connector);
> > > +	if (ret)
> > > +		goto err_put_role;
> > 
> > You are not leaving any fallback here. Are you sure all the existing systems
> > supply those properties?
> > 
> > There is another problem. At least "data-role" property is optional, but that
> > will in practice make it a requirement.
> 
> I missed that typec_get_fw_cap() was not setting a default if the property is absent.
> 
> > 
> > I think it would be safer to use the values from the device properties only if
> > they are available. Otherwise simply use default values.
> 
> I'll add back the previous values as defaults before calling typec_get_fw_cap().
> That will make data-role and try-power-role optional again, as intended.
> 
> However, "power-role" seems to be required by the function. Is this expected?

Yes.

> Or should I write my own fwnode parsing logic?

No, don't do that.

You can check the return value. If it's -EINVAL or -ENXIO, the
properties are not there, and you can safely use the defaults.
Otherwise, consider the failure as critical.

thanks,

-- 
heikki

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

[Facklam, Olivér]

Hello Heikki,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Friday, November 8, 2024 9:08 AM
> To: Facklam, Olivér <oliver.facklam@zuehlke.com>
> Cc: 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 2/4] usb: typec: hd3ss3220: use typec_get_fw_cap() to fill
> typec_cap
> 
> On Thu, Nov 07, 2024 at 03:24:44PM +0000, Facklam, Olivér wrote:
> > Hello Heikki,
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Thursday, November 7, 2024 3:21 PM
> > > To: Facklam, Olivér <oliver.facklam@zuehlke.com>
> > > Cc: 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 2/4] usb: typec: hd3ss3220: use
> > > typec_get_fw_cap() to fill typec_cap
> > >
> > > Hi Oliver,
> > >
> > > On Thu, Nov 07, 2024 at 12:43:28PM +0100, Oliver Facklam via B4
> > > Relay
> > > wrote:
> > > > From: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > >
> > > > 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.
> > > >
> > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> > > > ---
> > > >  drivers/usb/typec/hd3ss3220.c | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > > b/drivers/usb/typec/hd3ss3220.c index
> > > >
> > >
> 56f74bf70895ca701083bde44a5bbe0b691551e1..e6e4b1871b5d805f8c3671
> > > 31509f
> > > > 4e6ec0d2b5f0 100644
> > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > > > @@ -259,12 +259,12 @@ static int hd3ss3220_probe(struct i2c_client
> > > *client)
> > > >  		goto err_put_fwnode;
> > > >  	}
> > > >
> > > > -	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > > > +	ret = typec_get_fw_cap(&typec_cap, connector);
> > > > +	if (ret)
> > > > +		goto err_put_role;
> > >
> > > You are not leaving any fallback here. Are you sure all the existing
> > > systems supply those properties?
> > >
> > > There is another problem. At least "data-role" property is optional,
> > > but that will in practice make it a requirement.
> >
> > I missed that typec_get_fw_cap() was not setting a default if the property is
> absent.
> >
> > >
> > > I think it would be safer to use the values from the device
> > > properties only if they are available. Otherwise simply use default values.
> >
> > I'll add back the previous values as defaults before calling
> typec_get_fw_cap().
> > That will make data-role and try-power-role optional again, as intended.
> >
> > However, "power-role" seems to be required by the function. Is this
> expected?
> 
> Yes.
> 
> > Or should I write my own fwnode parsing logic?
> 
> No, don't do that.
> 
> You can check the return value. If it's -EINVAL or -ENXIO, the properties are
> not there, and you can safely use the defaults.
> Otherwise, consider the failure as critical.

Thanks for the hint, will do.

> 
> thanks,
> 
> --
> heikki