[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Some tcpc device-drivers need to explicitly be told to watch for connection
events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
being plugged into the Type-C port will not be noticed.

For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
watch for connection events. But for single-role ports we've so far been
falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
fusb302 this is not enough and no TCPM_CC_EVENT will be generated.

This commit adds a new start_srp_connection_detect callback to tcpc_dev
and when this is implemented calls this in place of start_drp_toggling
for SRP devices.

Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
 include/linux/usb/tcpm.h      | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index a2233d72ae7c..1af54af90b50 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
 			return true;
 	}
 
+	if (port->tcpc->start_srp_connection_detect &&
+	    port->port_type != TYPEC_PORT_DRP) {
+		tcpm_log_force(port, "Start SRP connection detection");
+		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
+		if (!ret)
+			return true;
+	}
+
 	return false;
 }
 
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 0c532ca3f079..bf2bbbf2e2b2 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -125,6 +125,10 @@ struct tcpc_config {
  *		Optional; if supported by hardware, called to start DRP
  *		toggling. DRP toggling is stopped automatically if
  *		a connection is established.
+ * @start_srp_connection_detect:
+ *		Optional; if supported by hardware, called to start connection
+ *		detection for single role ports. Connection detection is stopped
+ *		automatically if a connection is established.
  * @try_role:	Optional; called to set a preferred role
  * @pd_transmit:Called to transmit PD message
  * @mux:	Pointer to multiplexer data
@@ -149,6 +153,8 @@ struct tcpc_dev {
 			 enum typec_role role, enum typec_data_role data);
 	int (*start_drp_toggling)(struct tcpc_dev *dev,
 				  enum typec_cc_status cc);
+	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
+					   enum typec_cc_status cc);
 	int (*try_role)(struct tcpc_dev *dev, int role);
 	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
 			   const struct pd_message *msg);

[PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Some tcpc device-drivers need to explicitly be told to watch for connection
events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
being plugged into the Type-C port will not be noticed.

For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
watch for connection events. But for single-role ports we've so far been
falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
fusb302 this is not enough and no TCPM_CC_EVENT will be generated.

This commit adds a new start_srp_connection_detect callback to tcpc_dev
and when this is implemented calls this in place of start_drp_toggling
for SRP devices.

Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
 include/linux/usb/tcpm.h      | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index a2233d72ae7c..1af54af90b50 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
 			return true;
 	}
 
+	if (port->tcpc->start_srp_connection_detect &&
+	    port->port_type != TYPEC_PORT_DRP) {
+		tcpm_log_force(port, "Start SRP connection detection");
+		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
+		if (!ret)
+			return true;
+	}
+
 	return false;
 }
 
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 0c532ca3f079..bf2bbbf2e2b2 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -125,6 +125,10 @@ struct tcpc_config {
  *		Optional; if supported by hardware, called to start DRP
  *		toggling. DRP toggling is stopped automatically if
  *		a connection is established.
+ * @start_srp_connection_detect:
+ *		Optional; if supported by hardware, called to start connection
+ *		detection for single role ports. Connection detection is stopped
+ *		automatically if a connection is established.
  * @try_role:	Optional; called to set a preferred role
  * @pd_transmit:Called to transmit PD message
  * @mux:	Pointer to multiplexer data
@@ -149,6 +153,8 @@ struct tcpc_dev {
 			 enum typec_role role, enum typec_data_role data);
 	int (*start_drp_toggling)(struct tcpc_dev *dev,
 				  enum typec_cc_status cc);
+	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
+					   enum typec_cc_status cc);
 	int (*try_role)(struct tcpc_dev *dev, int role);
 	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
 			   const struct pd_message *msg);
-- 
2.21.0

[2/3] usb: typec: fusb302: Implement start_srp_connection_detect

[Hans de Goede]

When in single role port mode, we most start single-role toggling to
get an interrupt when a device / cable gets plugged into the port.

This commit implements the tcpc_dev start_srp_connection_detect callback
for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 6ea6199caafa..30413a45104f 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -876,6 +876,34 @@ static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
 	return ret;
 }
 
+static int tcpm_start_srp_connection_detect(struct tcpc_dev *dev,
+					    enum typec_cc_status cc)
+{
+	struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
+						 tcpc_dev);
+	int ret = 0;
+
+	mutex_lock(&chip->lock);
+	ret = fusb302_set_src_current(chip, cc_src_current[cc]);
+	if (ret < 0) {
+		fusb302_log(chip, "unable to set src current %s, ret=%d",
+			    typec_cc_status_name[cc], ret);
+		goto done;
+	}
+	ret = fusb302_set_toggling(chip, (cc == TYPEC_CC_RD) ?
+					 TOGGLING_MODE_SNK : TOGGLING_MODE_SRC);
+	if (ret < 0) {
+		fusb302_log(chip,
+			    "unable to start srp toggling, ret=%d", ret);
+		goto done;
+	}
+	fusb302_log(chip, "start srp toggling");
+done:
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
 static int tcpm_start_drp_toggling(struct tcpc_dev *dev,
 				   enum typec_cc_status cc)
 {
@@ -1095,6 +1123,8 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
 	fusb302_tcpc_dev->set_pd_rx = tcpm_set_pd_rx;
 	fusb302_tcpc_dev->set_roles = tcpm_set_roles;
 	fusb302_tcpc_dev->start_drp_toggling = tcpm_start_drp_toggling;
+	fusb302_tcpc_dev->start_srp_connection_detect =
+					tcpm_start_srp_connection_detect;
 	fusb302_tcpc_dev->pd_transmit = tcpm_pd_transmit;
 }
 

[PATCH 2/3] usb: typec: fusb302: Implement start_srp_connection_detect

[Hans de Goede]

When in single role port mode, we most start single-role toggling to
get an interrupt when a device / cable gets plugged into the port.

This commit implements the tcpc_dev start_srp_connection_detect callback
for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 6ea6199caafa..30413a45104f 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -876,6 +876,34 @@ static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
 	return ret;
 }
 
+static int tcpm_start_srp_connection_detect(struct tcpc_dev *dev,
+					    enum typec_cc_status cc)
+{
+	struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
+						 tcpc_dev);
+	int ret = 0;
+
+	mutex_lock(&chip->lock);
+	ret = fusb302_set_src_current(chip, cc_src_current[cc]);
+	if (ret < 0) {
+		fusb302_log(chip, "unable to set src current %s, ret=%d",
+			    typec_cc_status_name[cc], ret);
+		goto done;
+	}
+	ret = fusb302_set_toggling(chip, (cc == TYPEC_CC_RD) ?
+					 TOGGLING_MODE_SNK : TOGGLING_MODE_SRC);
+	if (ret < 0) {
+		fusb302_log(chip,
+			    "unable to start srp toggling, ret=%d", ret);
+		goto done;
+	}
+	fusb302_log(chip, "start srp toggling");
+done:
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
 static int tcpm_start_drp_toggling(struct tcpc_dev *dev,
 				   enum typec_cc_status cc)
 {
@@ -1095,6 +1123,8 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
 	fusb302_tcpc_dev->set_pd_rx = tcpm_set_pd_rx;
 	fusb302_tcpc_dev->set_roles = tcpm_set_roles;
 	fusb302_tcpc_dev->start_drp_toggling = tcpm_start_drp_toggling;
+	fusb302_tcpc_dev->start_srp_connection_detect =
+					tcpm_start_srp_connection_detect;
 	fusb302_tcpc_dev->pd_transmit = tcpm_pd_transmit;
 }
 
-- 
2.21.0

[3/3] usb: typec: fusb302: Revert "Resolve fixed power role contract setup"

[Hans de Goede]

Some tcpc device-drivers need to explicitly be told to watch for connection
events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
being plugged into the Type-C port will not be noticed.

For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
watch for connection events. But for single-role ports we've so far been
falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
fusb302 this is not enough and no TCPM_CC_EVENT will be generated.

Commit ea3b4d5523bc ("usb: typec: fusb302: Resolve fixed power role
contract setup") fixed SRPs not working because of this by making the
fusb302 driver start connection detection on every tcpm_set_cc() call.
It turns out this breaks src->snk power-role swapping because during the
swap we first set the Cc pins to Rp, calling set_cc, and then send a PS_RDY
message. But the fusb302 cannot send PD messages while its toggling engine
is active, so sending the PS_RDY message fails.

Struct tcpc_dev now has a new start_srp_connection_detect callback and
fusb302.c now implements this. This callback gets called when we the
fusb302 needs to start connection detection, fixing fusb302 SRPs not
seeing connected devices.

This allows us to revert the changes to fusb302's set_cc implementation,
making it once again purely setup the Cc-s and matching disconnect
detection, fixing src->snk power-role swapping no longer working.

Note that since the code was refactored in between, codewise this is not a
straight forward revert. Functionality wise this is a straight revert and
the original functionality is fully restored.

Fixes: ea3b4d5523bc ("usb: typec: fusb302: Resolve fixed power role ...")
Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 55 ++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 30413a45104f..96da41f82834 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -606,19 +606,16 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 			    FUSB_REG_SWITCHES0_CC2_PU_EN |
 			    FUSB_REG_SWITCHES0_CC1_PD_EN |
 			    FUSB_REG_SWITCHES0_CC2_PD_EN;
-	u8 switches0_data = 0x00;
+	u8 rd_mda, switches0_data = 0x00;
 	int ret = 0;
-	enum toggling_mode mode;
 
 	mutex_lock(&chip->lock);
 	switch (cc) {
 	case TYPEC_CC_OPEN:
-		mode = TOGGLING_MODE_OFF;
 		break;
 	case TYPEC_CC_RD:
 		switches0_data |= FUSB_REG_SWITCHES0_CC1_PD_EN |
 				  FUSB_REG_SWITCHES0_CC2_PD_EN;
-		mode = TOGGLING_MODE_SNK;
 		break;
 	case TYPEC_CC_RP_DEF:
 	case TYPEC_CC_RP_1_5:
@@ -626,7 +623,6 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 		switches0_data |= (chip->cc_polarity == TYPEC_POLARITY_CC1) ?
 				  FUSB_REG_SWITCHES0_CC1_PU_EN :
 				  FUSB_REG_SWITCHES0_CC2_PU_EN;
-		mode = TOGGLING_MODE_SRC;
 		break;
 	default:
 		fusb302_log(chip, "unsupported cc value %s",
@@ -637,6 +633,12 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 
 	fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
 
+	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
+	if (ret < 0) {
+		fusb302_log(chip, "cannot set toggling mode, ret=%d", ret);
+		goto done;
+	}
+
 	ret = fusb302_i2c_mask_write(chip, FUSB_REG_SWITCHES0,
 				     switches0_mask, switches0_data);
 	if (ret < 0) {
@@ -655,10 +657,45 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 		goto done;
 	}
 
-	ret = fusb302_set_toggling(chip, mode);
-	if (ret < 0)
-		fusb302_log(chip, "cannot set toggling mode, ret=%d", ret);
-
+	/* enable/disable interrupts, BC_LVL for SNK and COMP_CHNG for SRC */
+	switch (cc) {
+	case TYPEC_CC_RP_DEF:
+	case TYPEC_CC_RP_1_5:
+	case TYPEC_CC_RP_3_0:
+		rd_mda = rd_mda_value[cc_src_current[cc]];
+		ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
+		if (ret < 0) {
+			fusb302_log(chip,
+				    "cannot set SRC measure value, ret=%d",
+				    ret);
+			goto done;
+		}
+		ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK,
+					     FUSB_REG_MASK_BC_LVL |
+					     FUSB_REG_MASK_COMP_CHNG,
+					     FUSB_REG_MASK_COMP_CHNG);
+		if (ret < 0) {
+			fusb302_log(chip, "cannot set SRC interrupt, ret=%d",
+				    ret);
+			goto done;
+		}
+		chip->intr_comp_chng = true;
+		break;
+	case TYPEC_CC_RD:
+		ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK,
+					     FUSB_REG_MASK_BC_LVL |
+					     FUSB_REG_MASK_COMP_CHNG,
+					     FUSB_REG_MASK_BC_LVL);
+		if (ret < 0) {
+			fusb302_log(chip, "cannot set SRC interrupt, ret=%d",
+				    ret);
+			goto done;
+		}
+		chip->intr_bc_lvl = true;
+		break;
+	default:
+		break;
+	}
 done:
 	mutex_unlock(&chip->lock);
 

[PATCH 3/3] usb: typec: fusb302: Revert "Resolve fixed power role contract setup"

[Hans de Goede]

Some tcpc device-drivers need to explicitly be told to watch for connection
events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
being plugged into the Type-C port will not be noticed.

For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
watch for connection events. But for single-role ports we've so far been
falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
fusb302 this is not enough and no TCPM_CC_EVENT will be generated.

Commit ea3b4d5523bc ("usb: typec: fusb302: Resolve fixed power role
contract setup") fixed SRPs not working because of this by making the
fusb302 driver start connection detection on every tcpm_set_cc() call.
It turns out this breaks src->snk power-role swapping because during the
swap we first set the Cc pins to Rp, calling set_cc, and then send a PS_RDY
message. But the fusb302 cannot send PD messages while its toggling engine
is active, so sending the PS_RDY message fails.

Struct tcpc_dev now has a new start_srp_connection_detect callback and
fusb302.c now implements this. This callback gets called when we the
fusb302 needs to start connection detection, fixing fusb302 SRPs not
seeing connected devices.

This allows us to revert the changes to fusb302's set_cc implementation,
making it once again purely setup the Cc-s and matching disconnect
detection, fixing src->snk power-role swapping no longer working.

Note that since the code was refactored in between, codewise this is not a
straight forward revert. Functionality wise this is a straight revert and
the original functionality is fully restored.

Fixes: ea3b4d5523bc ("usb: typec: fusb302: Resolve fixed power role ...")
Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 55 ++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 30413a45104f..96da41f82834 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -606,19 +606,16 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 			    FUSB_REG_SWITCHES0_CC2_PU_EN |
 			    FUSB_REG_SWITCHES0_CC1_PD_EN |
 			    FUSB_REG_SWITCHES0_CC2_PD_EN;
-	u8 switches0_data = 0x00;
+	u8 rd_mda, switches0_data = 0x00;
 	int ret = 0;
-	enum toggling_mode mode;
 
 	mutex_lock(&chip->lock);
 	switch (cc) {
 	case TYPEC_CC_OPEN:
-		mode = TOGGLING_MODE_OFF;
 		break;
 	case TYPEC_CC_RD:
 		switches0_data |= FUSB_REG_SWITCHES0_CC1_PD_EN |
 				  FUSB_REG_SWITCHES0_CC2_PD_EN;
-		mode = TOGGLING_MODE_SNK;
 		break;
 	case TYPEC_CC_RP_DEF:
 	case TYPEC_CC_RP_1_5:
@@ -626,7 +623,6 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 		switches0_data |= (chip->cc_polarity == TYPEC_POLARITY_CC1) ?
 				  FUSB_REG_SWITCHES0_CC1_PU_EN :
 				  FUSB_REG_SWITCHES0_CC2_PU_EN;
-		mode = TOGGLING_MODE_SRC;
 		break;
 	default:
 		fusb302_log(chip, "unsupported cc value %s",
@@ -637,6 +633,12 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 
 	fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
 
+	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
+	if (ret < 0) {
+		fusb302_log(chip, "cannot set toggling mode, ret=%d", ret);
+		goto done;
+	}
+
 	ret = fusb302_i2c_mask_write(chip, FUSB_REG_SWITCHES0,
 				     switches0_mask, switches0_data);
 	if (ret < 0) {
@@ -655,10 +657,45 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
 		goto done;
 	}
 
-	ret = fusb302_set_toggling(chip, mode);
-	if (ret < 0)
-		fusb302_log(chip, "cannot set toggling mode, ret=%d", ret);
-
+	/* enable/disable interrupts, BC_LVL for SNK and COMP_CHNG for SRC */
+	switch (cc) {
+	case TYPEC_CC_RP_DEF:
+	case TYPEC_CC_RP_1_5:
+	case TYPEC_CC_RP_3_0:
+		rd_mda = rd_mda_value[cc_src_current[cc]];
+		ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
+		if (ret < 0) {
+			fusb302_log(chip,
+				    "cannot set SRC measure value, ret=%d",
+				    ret);
+			goto done;
+		}
+		ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK,
+					     FUSB_REG_MASK_BC_LVL |
+					     FUSB_REG_MASK_COMP_CHNG,
+					     FUSB_REG_MASK_COMP_CHNG);
+		if (ret < 0) {
+			fusb302_log(chip, "cannot set SRC interrupt, ret=%d",
+				    ret);
+			goto done;
+		}
+		chip->intr_comp_chng = true;
+		break;
+	case TYPEC_CC_RD:
+		ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK,
+					     FUSB_REG_MASK_BC_LVL |
+					     FUSB_REG_MASK_COMP_CHNG,
+					     FUSB_REG_MASK_BC_LVL);
+		if (ret < 0) {
+			fusb302_log(chip, "cannot set SRC interrupt, ret=%d",
+				    ret);
+			goto done;
+		}
+		chip->intr_bc_lvl = true;
+		break;
+	default:
+		break;
+	}
 done:
 	mutex_unlock(&chip->lock);
 
-- 
2.21.0

[2/3] usb: typec: fusb302: Implement start_srp_connection_detect

[Sergei Shtylyov]

Hello!

On 13.04.2019 23:39, Hans de Goede wrote:

> When in single role port mode, we most start single-role toggling to

    Must, perhaps?

> get an interrupt when a device / cable gets plugged into the port.
> 
> This commit implements the tcpc_dev start_srp_connection_detect callback
> for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/usb/typec/tcpm/fusb302.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 6ea6199caafa..30413a45104f 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -876,6 +876,34 @@ static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
>   	return ret;
>   }
>   
> +static int tcpm_start_srp_connection_detect(struct tcpc_dev *dev,
> +					    enum typec_cc_status cc)
> +{
> +	struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
> +						 tcpc_dev);
> +	int ret = 0;

    This initializer is useless.

> +
> +	mutex_lock(&chip->lock);
> +	ret = fusb302_set_src_current(chip, cc_src_current[cc]);
> +	if (ret < 0) {
> +		fusb302_log(chip, "unable to set src current %s, ret=%d",
> +			    typec_cc_status_name[cc], ret);
> +		goto done;
> +	}
> +	ret = fusb302_set_toggling(chip, (cc == TYPEC_CC_RD) ?
> +					 TOGGLING_MODE_SNK : TOGGLING_MODE_SRC);
> +	if (ret < 0) {
> +		fusb302_log(chip,
> +			    "unable to start srp toggling, ret=%d", ret);
> +		goto done;
> +	}
> +	fusb302_log(chip, "start srp toggling");
> +done:
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
>   static int tcpm_start_drp_toggling(struct tcpc_dev *dev,
>   				   enum typec_cc_status cc)
>   {
[...]

MBR, Sergei

Re: [PATCH 2/3] usb: typec: fusb302: Implement start_srp_connection_detect

[Sergei Shtylyov]

Hello!

On 13.04.2019 23:39, Hans de Goede wrote:

> When in single role port mode, we most start single-role toggling to

    Must, perhaps?

> get an interrupt when a device / cable gets plugged into the port.
> 
> This commit implements the tcpc_dev start_srp_connection_detect callback
> for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/usb/typec/tcpm/fusb302.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 6ea6199caafa..30413a45104f 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -876,6 +876,34 @@ static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
>   	return ret;
>   }
>   
> +static int tcpm_start_srp_connection_detect(struct tcpc_dev *dev,
> +					    enum typec_cc_status cc)
> +{
> +	struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
> +						 tcpc_dev);
> +	int ret = 0;

    This initializer is useless.

> +
> +	mutex_lock(&chip->lock);
> +	ret = fusb302_set_src_current(chip, cc_src_current[cc]);
> +	if (ret < 0) {
> +		fusb302_log(chip, "unable to set src current %s, ret=%d",
> +			    typec_cc_status_name[cc], ret);
> +		goto done;
> +	}
> +	ret = fusb302_set_toggling(chip, (cc == TYPEC_CC_RD) ?
> +					 TOGGLING_MODE_SNK : TOGGLING_MODE_SRC);
> +	if (ret < 0) {
> +		fusb302_log(chip,
> +			    "unable to start srp toggling, ret=%d", ret);
> +		goto done;
> +	}
> +	fusb302_log(chip, "start srp toggling");
> +done:
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
>   static int tcpm_start_drp_toggling(struct tcpc_dev *dev,
>   				   enum typec_cc_status cc)
>   {
[...]

MBR, Sergei

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Opensource [Adam Thomson]]

On 13 April 2019 21:40, Hans de Goede wrote:

> Some tcpc device-drivers need to explicitly be told to watch for connection
> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
> being plugged into the Type-C port will not be noticed.
> 
> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to watch for
> connection events. But for single-role ports we've so far been falling back to just
> calling tcpm_set_cc(). For some tcpc-s such as the
> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> 
> This commit adds a new start_srp_connection_detect callback to tcpc_dev and
> when this is implemented calls this in place of start_drp_toggling for SRP devices.
> 
> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>  include/linux/usb/tcpm.h      | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index a2233d72ae7c..1af54af90b50 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port
> *port,
>  			return true;
>  	}
> 
> +	if (port->tcpc->start_srp_connection_detect &&
> +	    port->port_type != TYPEC_PORT_DRP) {
> +		tcpm_log_force(port, "Start SRP connection detection");
> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> +		if (!ret)
> +			return true;
> +	}
> +

Is it a little misleading when reading the state machine code that we're calling
tcpm_start_drp_toggling() to actually start SRP detection? Maybe we could rename
this function and the associated state to cover both SRP and DRP, or
alternatively add a new state for SRP_DETECTION, just to keep logging and code
readability clear and then move the above code to a different function just for
SRP?

Functionally though, the change makes sense to me.

>  	return false;
>  }
> 
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
> 0c532ca3f079..bf2bbbf2e2b2 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -125,6 +125,10 @@ struct tcpc_config {
>   *		Optional; if supported by hardware, called to start DRP
>   *		toggling. DRP toggling is stopped automatically if
>   *		a connection is established.
> + * @start_srp_connection_detect:
> + *		Optional; if supported by hardware, called to start connection
> + *		detection for single role ports. Connection detection is stopped
> + *		automatically if a connection is established.
>   * @try_role:	Optional; called to set a preferred role
>   * @pd_transmit:Called to transmit PD message
>   * @mux:	Pointer to multiplexer data
> @@ -149,6 +153,8 @@ struct tcpc_dev {
>  			 enum typec_role role, enum typec_data_role data);
>  	int (*start_drp_toggling)(struct tcpc_dev *dev,
>  				  enum typec_cc_status cc);
> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> +					   enum typec_cc_status cc);
>  	int (*try_role)(struct tcpc_dev *dev, int role);
>  	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
> type,
>  			   const struct pd_message *msg);
> --
> 2.21.0

RE: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Adam Thomson]

On 13 April 2019 21:40, Hans de Goede wrote:

> Some tcpc device-drivers need to explicitly be told to watch for connection
> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
> being plugged into the Type-C port will not be noticed.
> 
> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to watch for
> connection events. But for single-role ports we've so far been falling back to just
> calling tcpm_set_cc(). For some tcpc-s such as the
> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> 
> This commit adds a new start_srp_connection_detect callback to tcpc_dev and
> when this is implemented calls this in place of start_drp_toggling for SRP devices.
> 
> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>  include/linux/usb/tcpm.h      | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index a2233d72ae7c..1af54af90b50 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port
> *port,
>  			return true;
>  	}
> 
> +	if (port->tcpc->start_srp_connection_detect &&
> +	    port->port_type != TYPEC_PORT_DRP) {
> +		tcpm_log_force(port, "Start SRP connection detection");
> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> +		if (!ret)
> +			return true;
> +	}
> +

Is it a little misleading when reading the state machine code that we're calling
tcpm_start_drp_toggling() to actually start SRP detection? Maybe we could rename
this function and the associated state to cover both SRP and DRP, or
alternatively add a new state for SRP_DETECTION, just to keep logging and code
readability clear and then move the above code to a different function just for
SRP?

Functionally though, the change makes sense to me.

>  	return false;
>  }
> 
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
> 0c532ca3f079..bf2bbbf2e2b2 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -125,6 +125,10 @@ struct tcpc_config {
>   *		Optional; if supported by hardware, called to start DRP
>   *		toggling. DRP toggling is stopped automatically if
>   *		a connection is established.
> + * @start_srp_connection_detect:
> + *		Optional; if supported by hardware, called to start connection
> + *		detection for single role ports. Connection detection is stopped
> + *		automatically if a connection is established.
>   * @try_role:	Optional; called to set a preferred role
>   * @pd_transmit:Called to transmit PD message
>   * @mux:	Pointer to multiplexer data
> @@ -149,6 +153,8 @@ struct tcpc_dev {
>  			 enum typec_role role, enum typec_data_role data);
>  	int (*start_drp_toggling)(struct tcpc_dev *dev,
>  				  enum typec_cc_status cc);
> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> +					   enum typec_cc_status cc);
>  	int (*try_role)(struct tcpc_dev *dev, int role);
>  	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
> type,
>  			   const struct pd_message *msg);
> --
> 2.21.0

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Hi,

On 15-04-19 12:31, Adam Thomson wrote:
> On 13 April 2019 21:40, Hans de Goede wrote:
> 
>> Some tcpc device-drivers need to explicitly be told to watch for connection
>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>> being plugged into the Type-C port will not be noticed.
>>
>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to watch for
>> connection events. But for single-role ports we've so far been falling back to just
>> calling tcpm_set_cc(). For some tcpc-s such as the
>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>
>> This commit adds a new start_srp_connection_detect callback to tcpc_dev and
>> when this is implemented calls this in place of start_drp_toggling for SRP devices.
>>
>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>   include/linux/usb/tcpm.h      | 6 ++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index a2233d72ae7c..1af54af90b50 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port
>> *port,
>>   			return true;
>>   	}
>>
>> +	if (port->tcpc->start_srp_connection_detect &&
>> +	    port->port_type != TYPEC_PORT_DRP) {
>> +		tcpm_log_force(port, "Start SRP connection detection");
>> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>> +		if (!ret)
>> +			return true;
>> +	}
>> +
> 
> Is it a little misleading when reading the state machine code that we're calling
> tcpm_start_drp_toggling() to actually start SRP detection? Maybe we could rename
> this function and the associated state to cover both SRP and DRP, or
> alternatively add a new state for SRP_DETECTION, just to keep logging and code
> readability clear and then move the above code to a different function just for
> SRP?

I'm not in fan of adding a separate state for this. Even though connection
detection is not really toggling, I've considered changing
tcpm_start_drp_toggling to tcpm_start_toggling and rename the state to match.
I think that if we want to drop drp/DRP from the names, that using
tcpm_start_toggling / TOGGLING is the best solution.

Also note that on the fusb302, which is the tcpc which needs SRP connection
detection, this is done using the toggling engine. So just changing the
names from drp-toggling to toggling seems best.

Regards,

Hans



> 
> Functionally though, the change makes sense to me.
> 
>>   	return false;
>>   }
>>
>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
>> 0c532ca3f079..bf2bbbf2e2b2 100644
>> --- a/include/linux/usb/tcpm.h
>> +++ b/include/linux/usb/tcpm.h
>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>    *		Optional; if supported by hardware, called to start DRP
>>    *		toggling. DRP toggling is stopped automatically if
>>    *		a connection is established.
>> + * @start_srp_connection_detect:
>> + *		Optional; if supported by hardware, called to start connection
>> + *		detection for single role ports. Connection detection is stopped
>> + *		automatically if a connection is established.
>>    * @try_role:	Optional; called to set a preferred role
>>    * @pd_transmit:Called to transmit PD message
>>    * @mux:	Pointer to multiplexer data
>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>   			 enum typec_role role, enum typec_data_role data);
>>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
>>   				  enum typec_cc_status cc);
>> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>> +					   enum typec_cc_status cc);
>>   	int (*try_role)(struct tcpc_dev *dev, int role);
>>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
>> type,
>>   			   const struct pd_message *msg);
>> --
>> 2.21.0

Re: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Hi,

On 15-04-19 12:31, Adam Thomson wrote:
> On 13 April 2019 21:40, Hans de Goede wrote:
> 
>> Some tcpc device-drivers need to explicitly be told to watch for connection
>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>> being plugged into the Type-C port will not be noticed.
>>
>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to watch for
>> connection events. But for single-role ports we've so far been falling back to just
>> calling tcpm_set_cc(). For some tcpc-s such as the
>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>
>> This commit adds a new start_srp_connection_detect callback to tcpc_dev and
>> when this is implemented calls this in place of start_drp_toggling for SRP devices.
>>
>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>   include/linux/usb/tcpm.h      | 6 ++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index a2233d72ae7c..1af54af90b50 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port
>> *port,
>>   			return true;
>>   	}
>>
>> +	if (port->tcpc->start_srp_connection_detect &&
>> +	    port->port_type != TYPEC_PORT_DRP) {
>> +		tcpm_log_force(port, "Start SRP connection detection");
>> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>> +		if (!ret)
>> +			return true;
>> +	}
>> +
> 
> Is it a little misleading when reading the state machine code that we're calling
> tcpm_start_drp_toggling() to actually start SRP detection? Maybe we could rename
> this function and the associated state to cover both SRP and DRP, or
> alternatively add a new state for SRP_DETECTION, just to keep logging and code
> readability clear and then move the above code to a different function just for
> SRP?

I'm not in fan of adding a separate state for this. Even though connection
detection is not really toggling, I've considered changing
tcpm_start_drp_toggling to tcpm_start_toggling and rename the state to match.
I think that if we want to drop drp/DRP from the names, that using
tcpm_start_toggling / TOGGLING is the best solution.

Also note that on the fusb302, which is the tcpc which needs SRP connection
detection, this is done using the toggling engine. So just changing the
names from drp-toggling to toggling seems best.

Regards,

Hans



> 
> Functionally though, the change makes sense to me.
> 
>>   	return false;
>>   }
>>
>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
>> 0c532ca3f079..bf2bbbf2e2b2 100644
>> --- a/include/linux/usb/tcpm.h
>> +++ b/include/linux/usb/tcpm.h
>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>    *		Optional; if supported by hardware, called to start DRP
>>    *		toggling. DRP toggling is stopped automatically if
>>    *		a connection is established.
>> + * @start_srp_connection_detect:
>> + *		Optional; if supported by hardware, called to start connection
>> + *		detection for single role ports. Connection detection is stopped
>> + *		automatically if a connection is established.
>>    * @try_role:	Optional; called to set a preferred role
>>    * @pd_transmit:Called to transmit PD message
>>    * @mux:	Pointer to multiplexer data
>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>   			 enum typec_role role, enum typec_data_role data);
>>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
>>   				  enum typec_cc_status cc);
>> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>> +					   enum typec_cc_status cc);
>>   	int (*try_role)(struct tcpc_dev *dev, int role);
>>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
>> type,
>>   			   const struct pd_message *msg);
>> --
>> 2.21.0

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Guenter Roeck]

On 4/15/19 3:37 AM, Hans de Goede wrote:
> Hi,
> 
> On 15-04-19 12:31, Adam Thomson wrote:
>> On 13 April 2019 21:40, Hans de Goede wrote:
>>
>>> Some tcpc device-drivers need to explicitly be told to watch for connection
>>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>>> being plugged into the Type-C port will not be noticed.
>>>
>>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to watch for
>>> connection events. But for single-role ports we've so far been falling back to just
>>> calling tcpm_set_cc(). For some tcpc-s such as the
>>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>>
>>> This commit adds a new start_srp_connection_detect callback to tcpc_dev and
>>> when this is implemented calls this in place of start_drp_toggling for SRP devices.
>>>
>>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>>   include/linux/usb/tcpm.h      | 6 ++++++
>>>   2 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>> index a2233d72ae7c..1af54af90b50 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port
>>> *port,
>>>               return true;
>>>       }
>>>
>>> +    if (port->tcpc->start_srp_connection_detect &&
>>> +        port->port_type != TYPEC_PORT_DRP) {
>>> +        tcpm_log_force(port, "Start SRP connection detection");
>>> +        ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>>> +        if (!ret)
>>> +            return true;
>>> +    }
>>> +
>>
>> Is it a little misleading when reading the state machine code that we're calling
>> tcpm_start_drp_toggling() to actually start SRP detection? Maybe we could rename
>> this function and the associated state to cover both SRP and DRP, or
>> alternatively add a new state for SRP_DETECTION, just to keep logging and code
>> readability clear and then move the above code to a different function just for
>> SRP?
> 
> I'm not in fan of adding a separate state for this. Even though connection
> detection is not really toggling, I've considered changing
> tcpm_start_drp_toggling to tcpm_start_toggling and rename the state to match.
> I think that if we want to drop drp/DRP from the names, that using
> tcpm_start_toggling / TOGGLING is the best solution.
> 
> Also note that on the fusb302, which is the tcpc which needs SRP connection
> detection, this is done using the toggling engine. So just changing the
> names from drp-toggling to toggling seems best.
> 
Agreed.

Guenter

> Regards,
> 
> Hans
> 
> 
> 
>>
>> Functionally though, the change makes sense to me.
>>
>>>       return false;
>>>   }
>>>
>>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
>>> 0c532ca3f079..bf2bbbf2e2b2 100644
>>> --- a/include/linux/usb/tcpm.h
>>> +++ b/include/linux/usb/tcpm.h
>>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>>    *        Optional; if supported by hardware, called to start DRP
>>>    *        toggling. DRP toggling is stopped automatically if
>>>    *        a connection is established.
>>> + * @start_srp_connection_detect:
>>> + *        Optional; if supported by hardware, called to start connection
>>> + *        detection for single role ports. Connection detection is stopped
>>> + *        automatically if a connection is established.
>>>    * @try_role:    Optional; called to set a preferred role
>>>    * @pd_transmit:Called to transmit PD message
>>>    * @mux:    Pointer to multiplexer data
>>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>>                enum typec_role role, enum typec_data_role data);
>>>       int (*start_drp_toggling)(struct tcpc_dev *dev,
>>>                     enum typec_cc_status cc);
>>> +    int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>>> +                       enum typec_cc_status cc);
>>>       int (*try_role)(struct tcpc_dev *dev, int role);
>>>       int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
>>> type,
>>>                  const struct pd_message *msg);
>>> -- 
>>> 2.21.0
>

Re: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Guenter Roeck]

On 4/15/19 3:37 AM, Hans de Goede wrote:
> Hi,
> 
> On 15-04-19 12:31, Adam Thomson wrote:
>> On 13 April 2019 21:40, Hans de Goede wrote:
>>
>>> Some tcpc device-drivers need to explicitly be told to watch for connection
>>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>>> being plugged into the Type-C port will not be noticed.
>>>
>>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to watch for
>>> connection events. But for single-role ports we've so far been falling back to just
>>> calling tcpm_set_cc(). For some tcpc-s such as the
>>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>>
>>> This commit adds a new start_srp_connection_detect callback to tcpc_dev and
>>> when this is implemented calls this in place of start_drp_toggling for SRP devices.
>>>
>>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>>   include/linux/usb/tcpm.h      | 6 ++++++
>>>   2 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>> index a2233d72ae7c..1af54af90b50 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port
>>> *port,
>>>               return true;
>>>       }
>>>
>>> +    if (port->tcpc->start_srp_connection_detect &&
>>> +        port->port_type != TYPEC_PORT_DRP) {
>>> +        tcpm_log_force(port, "Start SRP connection detection");
>>> +        ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>>> +        if (!ret)
>>> +            return true;
>>> +    }
>>> +
>>
>> Is it a little misleading when reading the state machine code that we're calling
>> tcpm_start_drp_toggling() to actually start SRP detection? Maybe we could rename
>> this function and the associated state to cover both SRP and DRP, or
>> alternatively add a new state for SRP_DETECTION, just to keep logging and code
>> readability clear and then move the above code to a different function just for
>> SRP?
> 
> I'm not in fan of adding a separate state for this. Even though connection
> detection is not really toggling, I've considered changing
> tcpm_start_drp_toggling to tcpm_start_toggling and rename the state to match.
> I think that if we want to drop drp/DRP from the names, that using
> tcpm_start_toggling / TOGGLING is the best solution.
> 
> Also note that on the fusb302, which is the tcpc which needs SRP connection
> detection, this is done using the toggling engine. So just changing the
> names from drp-toggling to toggling seems best.
> 
Agreed.

Guenter

> Regards,
> 
> Hans
> 
> 
> 
>>
>> Functionally though, the change makes sense to me.
>>
>>>       return false;
>>>   }
>>>
>>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
>>> 0c532ca3f079..bf2bbbf2e2b2 100644
>>> --- a/include/linux/usb/tcpm.h
>>> +++ b/include/linux/usb/tcpm.h
>>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>>    *        Optional; if supported by hardware, called to start DRP
>>>    *        toggling. DRP toggling is stopped automatically if
>>>    *        a connection is established.
>>> + * @start_srp_connection_detect:
>>> + *        Optional; if supported by hardware, called to start connection
>>> + *        detection for single role ports. Connection detection is stopped
>>> + *        automatically if a connection is established.
>>>    * @try_role:    Optional; called to set a preferred role
>>>    * @pd_transmit:Called to transmit PD message
>>>    * @mux:    Pointer to multiplexer data
>>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>>                enum typec_role role, enum typec_data_role data);
>>>       int (*start_drp_toggling)(struct tcpc_dev *dev,
>>>                     enum typec_cc_status cc);
>>> +    int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>>> +                       enum typec_cc_status cc);
>>>       int (*try_role)(struct tcpc_dev *dev, int role);
>>>       int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
>>> type,
>>>                  const struct pd_message *msg);
>>> -- 
>>> 2.21.0
> 

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Opensource [Adam Thomson]]

On 15 April 2019 11:38, Hans de Goede wrote:

> On 15-04-19 12:31, Adam Thomson wrote:
> > On 13 April 2019 21:40, Hans de Goede wrote:
> >
> >> Some tcpc device-drivers need to explicitly be told to watch for
> >> connection events, otherwise the tcpc will not generate any
> >> TCPM_CC_EVENTs and devices being plugged into the Type-C port will not be
> noticed.
> >>
> >> For dual-role ports tcpm_start_drp_toggling() is used to tell the
> >> tcpc to watch for connection events. But for single-role ports we've
> >> so far been falling back to just calling tcpm_set_cc(). For some
> >> tcpc-s such as the
> >> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> >>
> >> This commit adds a new start_srp_connection_detect callback to
> >> tcpc_dev and when this is implemented calls this in place of start_drp_toggling
> for SRP devices.
> >>
> >> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role
> >> ...")
> >> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
> >>   include/linux/usb/tcpm.h      | 6 ++++++
> >>   2 files changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c
> >> b/drivers/usb/typec/tcpm/tcpm.c index a2233d72ae7c..1af54af90b50
> >> 100644
> >> --- a/drivers/usb/typec/tcpm/tcpm.c
> >> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct
> >> tcpm_port *port,
> >>   			return true;
> >>   	}
> >>
> >> +	if (port->tcpc->start_srp_connection_detect &&
> >> +	    port->port_type != TYPEC_PORT_DRP) {
> >> +		tcpm_log_force(port, "Start SRP connection detection");
> >> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> >> +		if (!ret)
> >> +			return true;
> >> +	}
> >> +
> >
> > Is it a little misleading when reading the state machine code that
> > we're calling
> > tcpm_start_drp_toggling() to actually start SRP detection? Maybe we
> > could rename this function and the associated state to cover both SRP
> > and DRP, or alternatively add a new state for SRP_DETECTION, just to
> > keep logging and code readability clear and then move the above code
> > to a different function just for SRP?
> 
> I'm not in fan of adding a separate state for this. Even though connection
> detection is not really toggling, I've considered changing tcpm_start_drp_toggling
> to tcpm_start_toggling and rename the state to match.
> I think that if we want to drop drp/DRP from the names, that using
> tcpm_start_toggling / TOGGLING is the best solution.
> 
> Also note that on the fusb302, which is the tcpc which needs SRP connection
> detection, this is done using the toggling engine. So just changing the names from
> drp-toggling to toggling seems best.

Yep, that's fine with me. Thanks for the efforts on this.

> 
> Regards,
> 
> Hans
> 
> 
> 
> >
> > Functionally though, the change makes sense to me.
> >
> >>   	return false;
> >>   }
> >>
> >> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> >> index
> >> 0c532ca3f079..bf2bbbf2e2b2 100644
> >> --- a/include/linux/usb/tcpm.h
> >> +++ b/include/linux/usb/tcpm.h
> >> @@ -125,6 +125,10 @@ struct tcpc_config {
> >>    *		Optional; if supported by hardware, called to start DRP
> >>    *		toggling. DRP toggling is stopped automatically if
> >>    *		a connection is established.
> >> + * @start_srp_connection_detect:
> >> + *		Optional; if supported by hardware, called to start connection
> >> + *		detection for single role ports. Connection detection is stopped
> >> + *		automatically if a connection is established.
> >>    * @try_role:	Optional; called to set a preferred role
> >>    * @pd_transmit:Called to transmit PD message
> >>    * @mux:	Pointer to multiplexer data
> >> @@ -149,6 +153,8 @@ struct tcpc_dev {
> >>   			 enum typec_role role, enum typec_data_role data);
> >>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
> >>   				  enum typec_cc_status cc);
> >> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> >> +					   enum typec_cc_status cc);
> >>   	int (*try_role)(struct tcpc_dev *dev, int role);
> >>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
> >> type,
> >>   			   const struct pd_message *msg);
> >> --
> >> 2.21.0

RE: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Adam Thomson]

On 15 April 2019 11:38, Hans de Goede wrote:

> On 15-04-19 12:31, Adam Thomson wrote:
> > On 13 April 2019 21:40, Hans de Goede wrote:
> >
> >> Some tcpc device-drivers need to explicitly be told to watch for
> >> connection events, otherwise the tcpc will not generate any
> >> TCPM_CC_EVENTs and devices being plugged into the Type-C port will not be
> noticed.
> >>
> >> For dual-role ports tcpm_start_drp_toggling() is used to tell the
> >> tcpc to watch for connection events. But for single-role ports we've
> >> so far been falling back to just calling tcpm_set_cc(). For some
> >> tcpc-s such as the
> >> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> >>
> >> This commit adds a new start_srp_connection_detect callback to
> >> tcpc_dev and when this is implemented calls this in place of start_drp_toggling
> for SRP devices.
> >>
> >> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role
> >> ...")
> >> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
> >>   include/linux/usb/tcpm.h      | 6 ++++++
> >>   2 files changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c
> >> b/drivers/usb/typec/tcpm/tcpm.c index a2233d72ae7c..1af54af90b50
> >> 100644
> >> --- a/drivers/usb/typec/tcpm/tcpm.c
> >> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct
> >> tcpm_port *port,
> >>   			return true;
> >>   	}
> >>
> >> +	if (port->tcpc->start_srp_connection_detect &&
> >> +	    port->port_type != TYPEC_PORT_DRP) {
> >> +		tcpm_log_force(port, "Start SRP connection detection");
> >> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> >> +		if (!ret)
> >> +			return true;
> >> +	}
> >> +
> >
> > Is it a little misleading when reading the state machine code that
> > we're calling
> > tcpm_start_drp_toggling() to actually start SRP detection? Maybe we
> > could rename this function and the associated state to cover both SRP
> > and DRP, or alternatively add a new state for SRP_DETECTION, just to
> > keep logging and code readability clear and then move the above code
> > to a different function just for SRP?
> 
> I'm not in fan of adding a separate state for this. Even though connection
> detection is not really toggling, I've considered changing tcpm_start_drp_toggling
> to tcpm_start_toggling and rename the state to match.
> I think that if we want to drop drp/DRP from the names, that using
> tcpm_start_toggling / TOGGLING is the best solution.
> 
> Also note that on the fusb302, which is the tcpc which needs SRP connection
> detection, this is done using the toggling engine. So just changing the names from
> drp-toggling to toggling seems best.

Yep, that's fine with me. Thanks for the efforts on this.

> 
> Regards,
> 
> Hans
> 
> 
> 
> >
> > Functionally though, the change makes sense to me.
> >
> >>   	return false;
> >>   }
> >>
> >> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> >> index
> >> 0c532ca3f079..bf2bbbf2e2b2 100644
> >> --- a/include/linux/usb/tcpm.h
> >> +++ b/include/linux/usb/tcpm.h
> >> @@ -125,6 +125,10 @@ struct tcpc_config {
> >>    *		Optional; if supported by hardware, called to start DRP
> >>    *		toggling. DRP toggling is stopped automatically if
> >>    *		a connection is established.
> >> + * @start_srp_connection_detect:
> >> + *		Optional; if supported by hardware, called to start connection
> >> + *		detection for single role ports. Connection detection is stopped
> >> + *		automatically if a connection is established.
> >>    * @try_role:	Optional; called to set a preferred role
> >>    * @pd_transmit:Called to transmit PD message
> >>    * @mux:	Pointer to multiplexer data
> >> @@ -149,6 +153,8 @@ struct tcpc_dev {
> >>   			 enum typec_role role, enum typec_data_role data);
> >>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
> >>   				  enum typec_cc_status cc);
> >> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> >> +					   enum typec_cc_status cc);
> >>   	int (*try_role)(struct tcpc_dev *dev, int role);
> >>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type
> >> type,
> >>   			   const struct pd_message *msg);
> >> --
> >> 2.21.0

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Guenter Roeck]

On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
> Some tcpc device-drivers need to explicitly be told to watch for connection
> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
> being plugged into the Type-C port will not be noticed.
> 
> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
> watch for connection events. But for single-role ports we've so far been
> falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> 
> This commit adds a new start_srp_connection_detect callback to tcpc_dev
> and when this is implemented calls this in place of start_drp_toggling
> for SRP devices.
> 

Do we indeed need an additional callback for this purpose, or would a single
"start_connection_detect" call to replace start_drp_toggling be sufficient ?
If necessary, we could add the port type as argument (if the low level driver
doesn't already know that).

Thanks,
Guenter

> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>  include/linux/usb/tcpm.h      | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index a2233d72ae7c..1af54af90b50 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
>  			return true;
>  	}
>  
> +	if (port->tcpc->start_srp_connection_detect &&
> +	    port->port_type != TYPEC_PORT_DRP) {
> +		tcpm_log_force(port, "Start SRP connection detection");
> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> +		if (!ret)
> +			return true;
> +	}
> +
>  	return false;
>  }
>  
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 0c532ca3f079..bf2bbbf2e2b2 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -125,6 +125,10 @@ struct tcpc_config {
>   *		Optional; if supported by hardware, called to start DRP
>   *		toggling. DRP toggling is stopped automatically if
>   *		a connection is established.
> + * @start_srp_connection_detect:
> + *		Optional; if supported by hardware, called to start connection
> + *		detection for single role ports. Connection detection is stopped
> + *		automatically if a connection is established.
>   * @try_role:	Optional; called to set a preferred role
>   * @pd_transmit:Called to transmit PD message
>   * @mux:	Pointer to multiplexer data
> @@ -149,6 +153,8 @@ struct tcpc_dev {
>  			 enum typec_role role, enum typec_data_role data);
>  	int (*start_drp_toggling)(struct tcpc_dev *dev,
>  				  enum typec_cc_status cc);
> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> +					   enum typec_cc_status cc);
>  	int (*try_role)(struct tcpc_dev *dev, int role);
>  	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>  			   const struct pd_message *msg);
> -- 
> 2.21.0
>

Re: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Guenter Roeck]

On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
> Some tcpc device-drivers need to explicitly be told to watch for connection
> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
> being plugged into the Type-C port will not be noticed.
> 
> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
> watch for connection events. But for single-role ports we've so far been
> falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> 
> This commit adds a new start_srp_connection_detect callback to tcpc_dev
> and when this is implemented calls this in place of start_drp_toggling
> for SRP devices.
> 

Do we indeed need an additional callback for this purpose, or would a single
"start_connection_detect" call to replace start_drp_toggling be sufficient ?
If necessary, we could add the port type as argument (if the low level driver
doesn't already know that).

Thanks,
Guenter

> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>  include/linux/usb/tcpm.h      | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index a2233d72ae7c..1af54af90b50 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
>  			return true;
>  	}
>  
> +	if (port->tcpc->start_srp_connection_detect &&
> +	    port->port_type != TYPEC_PORT_DRP) {
> +		tcpm_log_force(port, "Start SRP connection detection");
> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> +		if (!ret)
> +			return true;
> +	}
> +
>  	return false;
>  }
>  
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 0c532ca3f079..bf2bbbf2e2b2 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -125,6 +125,10 @@ struct tcpc_config {
>   *		Optional; if supported by hardware, called to start DRP
>   *		toggling. DRP toggling is stopped automatically if
>   *		a connection is established.
> + * @start_srp_connection_detect:
> + *		Optional; if supported by hardware, called to start connection
> + *		detection for single role ports. Connection detection is stopped
> + *		automatically if a connection is established.
>   * @try_role:	Optional; called to set a preferred role
>   * @pd_transmit:Called to transmit PD message
>   * @mux:	Pointer to multiplexer data
> @@ -149,6 +153,8 @@ struct tcpc_dev {
>  			 enum typec_role role, enum typec_data_role data);
>  	int (*start_drp_toggling)(struct tcpc_dev *dev,
>  				  enum typec_cc_status cc);
> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> +					   enum typec_cc_status cc);
>  	int (*try_role)(struct tcpc_dev *dev, int role);
>  	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>  			   const struct pd_message *msg);
> -- 
> 2.21.0
> 

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Hi Guenter,

On 15-04-19 17:51, Guenter Roeck wrote:
> On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
>> Some tcpc device-drivers need to explicitly be told to watch for connection
>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>> being plugged into the Type-C port will not be noticed.
>>
>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
>> watch for connection events. But for single-role ports we've so far been
>> falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>
>> This commit adds a new start_srp_connection_detect callback to tcpc_dev
>> and when this is implemented calls this in place of start_drp_toggling
>> for SRP devices.
>>
> 
> Do we indeed need an additional callback for this purpose, or would a single
> "start_connection_detect" call to replace start_drp_toggling be sufficient ?
> If necessary, we could add the port type as argument (if the low level driver
> doesn't already know that).

A single "start_connection_detect" call to replace start_drp_toggling will be
sufficient AFAICT.

If you prefer that option, I can rework the patch-set accordingly.

Regards,

Hans



>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>   include/linux/usb/tcpm.h      | 6 ++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index a2233d72ae7c..1af54af90b50 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
>>   			return true;
>>   	}
>>   
>> +	if (port->tcpc->start_srp_connection_detect &&
>> +	    port->port_type != TYPEC_PORT_DRP) {
>> +		tcpm_log_force(port, "Start SRP connection detection");
>> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>> +		if (!ret)
>> +			return true;
>> +	}
>> +
>>   	return false;
>>   }
>>   
>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
>> index 0c532ca3f079..bf2bbbf2e2b2 100644
>> --- a/include/linux/usb/tcpm.h
>> +++ b/include/linux/usb/tcpm.h
>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>    *		Optional; if supported by hardware, called to start DRP
>>    *		toggling. DRP toggling is stopped automatically if
>>    *		a connection is established.
>> + * @start_srp_connection_detect:
>> + *		Optional; if supported by hardware, called to start connection
>> + *		detection for single role ports. Connection detection is stopped
>> + *		automatically if a connection is established.
>>    * @try_role:	Optional; called to set a preferred role
>>    * @pd_transmit:Called to transmit PD message
>>    * @mux:	Pointer to multiplexer data
>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>   			 enum typec_role role, enum typec_data_role data);
>>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
>>   				  enum typec_cc_status cc);
>> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>> +					   enum typec_cc_status cc);
>>   	int (*try_role)(struct tcpc_dev *dev, int role);
>>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>>   			   const struct pd_message *msg);
>> -- 
>> 2.21.0
>>

Re: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Hi Guenter,

On 15-04-19 17:51, Guenter Roeck wrote:
> On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
>> Some tcpc device-drivers need to explicitly be told to watch for connection
>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>> being plugged into the Type-C port will not be noticed.
>>
>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
>> watch for connection events. But for single-role ports we've so far been
>> falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>
>> This commit adds a new start_srp_connection_detect callback to tcpc_dev
>> and when this is implemented calls this in place of start_drp_toggling
>> for SRP devices.
>>
> 
> Do we indeed need an additional callback for this purpose, or would a single
> "start_connection_detect" call to replace start_drp_toggling be sufficient ?
> If necessary, we could add the port type as argument (if the low level driver
> doesn't already know that).

A single "start_connection_detect" call to replace start_drp_toggling will be
sufficient AFAICT.

If you prefer that option, I can rework the patch-set accordingly.

Regards,

Hans



>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>   include/linux/usb/tcpm.h      | 6 ++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index a2233d72ae7c..1af54af90b50 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
>>   			return true;
>>   	}
>>   
>> +	if (port->tcpc->start_srp_connection_detect &&
>> +	    port->port_type != TYPEC_PORT_DRP) {
>> +		tcpm_log_force(port, "Start SRP connection detection");
>> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>> +		if (!ret)
>> +			return true;
>> +	}
>> +
>>   	return false;
>>   }
>>   
>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
>> index 0c532ca3f079..bf2bbbf2e2b2 100644
>> --- a/include/linux/usb/tcpm.h
>> +++ b/include/linux/usb/tcpm.h
>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>    *		Optional; if supported by hardware, called to start DRP
>>    *		toggling. DRP toggling is stopped automatically if
>>    *		a connection is established.
>> + * @start_srp_connection_detect:
>> + *		Optional; if supported by hardware, called to start connection
>> + *		detection for single role ports. Connection detection is stopped
>> + *		automatically if a connection is established.
>>    * @try_role:	Optional; called to set a preferred role
>>    * @pd_transmit:Called to transmit PD message
>>    * @mux:	Pointer to multiplexer data
>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>   			 enum typec_role role, enum typec_data_role data);
>>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
>>   				  enum typec_cc_status cc);
>> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>> +					   enum typec_cc_status cc);
>>   	int (*try_role)(struct tcpc_dev *dev, int role);
>>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>>   			   const struct pd_message *msg);
>> -- 
>> 2.21.0
>>

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Guenter Roeck]

On Mon, Apr 15, 2019 at 07:53:58PM +0200, Hans de Goede wrote:
> Hi Guenter,
> 
> On 15-04-19 17:51, Guenter Roeck wrote:
> >On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
> >>Some tcpc device-drivers need to explicitly be told to watch for connection
> >>events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
> >>being plugged into the Type-C port will not be noticed.
> >>
> >>For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
> >>watch for connection events. But for single-role ports we've so far been
> >>falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
> >>fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> >>
> >>This commit adds a new start_srp_connection_detect callback to tcpc_dev
> >>and when this is implemented calls this in place of start_drp_toggling
> >>for SRP devices.
> >>
> >
> >Do we indeed need an additional callback for this purpose, or would a single
> >"start_connection_detect" call to replace start_drp_toggling be sufficient ?
> >If necessary, we could add the port type as argument (if the low level driver
> >doesn't already know that).
> 
> A single "start_connection_detect" call to replace start_drp_toggling will be
> sufficient AFAICT.
> 
> If you prefer that option, I can rework the patch-set accordingly.
> 

Yes, I do. I find it quite pointless to have two callbacks if exactly one
is ever called.

Thanks,
Guenter

> Regards,
> 
> Hans
> 
> 
> 
> >>Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
> >>Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>  drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
> >>  include/linux/usb/tcpm.h      | 6 ++++++
> >>  2 files changed, 14 insertions(+)
> >>
> >>diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >>index a2233d72ae7c..1af54af90b50 100644
> >>--- a/drivers/usb/typec/tcpm/tcpm.c
> >>+++ b/drivers/usb/typec/tcpm/tcpm.c
> >>@@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
> >>  			return true;
> >>  	}
> >>+	if (port->tcpc->start_srp_connection_detect &&
> >>+	    port->port_type != TYPEC_PORT_DRP) {
> >>+		tcpm_log_force(port, "Start SRP connection detection");
> >>+		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> >>+		if (!ret)
> >>+			return true;
> >>+	}
> >>+
> >>  	return false;
> >>  }
> >>diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> >>index 0c532ca3f079..bf2bbbf2e2b2 100644
> >>--- a/include/linux/usb/tcpm.h
> >>+++ b/include/linux/usb/tcpm.h
> >>@@ -125,6 +125,10 @@ struct tcpc_config {
> >>   *		Optional; if supported by hardware, called to start DRP
> >>   *		toggling. DRP toggling is stopped automatically if
> >>   *		a connection is established.
> >>+ * @start_srp_connection_detect:
> >>+ *		Optional; if supported by hardware, called to start connection
> >>+ *		detection for single role ports. Connection detection is stopped
> >>+ *		automatically if a connection is established.
> >>   * @try_role:	Optional; called to set a preferred role
> >>   * @pd_transmit:Called to transmit PD message
> >>   * @mux:	Pointer to multiplexer data
> >>@@ -149,6 +153,8 @@ struct tcpc_dev {
> >>  			 enum typec_role role, enum typec_data_role data);
> >>  	int (*start_drp_toggling)(struct tcpc_dev *dev,
> >>  				  enum typec_cc_status cc);
> >>+	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> >>+					   enum typec_cc_status cc);
> >>  	int (*try_role)(struct tcpc_dev *dev, int role);
> >>  	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
> >>  			   const struct pd_message *msg);
> >>-- 
> >>2.21.0
> >>

Re: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Guenter Roeck]

On Mon, Apr 15, 2019 at 07:53:58PM +0200, Hans de Goede wrote:
> Hi Guenter,
> 
> On 15-04-19 17:51, Guenter Roeck wrote:
> >On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
> >>Some tcpc device-drivers need to explicitly be told to watch for connection
> >>events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
> >>being plugged into the Type-C port will not be noticed.
> >>
> >>For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
> >>watch for connection events. But for single-role ports we've so far been
> >>falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
> >>fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
> >>
> >>This commit adds a new start_srp_connection_detect callback to tcpc_dev
> >>and when this is implemented calls this in place of start_drp_toggling
> >>for SRP devices.
> >>
> >
> >Do we indeed need an additional callback for this purpose, or would a single
> >"start_connection_detect" call to replace start_drp_toggling be sufficient ?
> >If necessary, we could add the port type as argument (if the low level driver
> >doesn't already know that).
> 
> A single "start_connection_detect" call to replace start_drp_toggling will be
> sufficient AFAICT.
> 
> If you prefer that option, I can rework the patch-set accordingly.
> 

Yes, I do. I find it quite pointless to have two callbacks if exactly one
is ever called.

Thanks,
Guenter

> Regards,
> 
> Hans
> 
> 
> 
> >>Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
> >>Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>  drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
> >>  include/linux/usb/tcpm.h      | 6 ++++++
> >>  2 files changed, 14 insertions(+)
> >>
> >>diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >>index a2233d72ae7c..1af54af90b50 100644
> >>--- a/drivers/usb/typec/tcpm/tcpm.c
> >>+++ b/drivers/usb/typec/tcpm/tcpm.c
> >>@@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
> >>  			return true;
> >>  	}
> >>+	if (port->tcpc->start_srp_connection_detect &&
> >>+	    port->port_type != TYPEC_PORT_DRP) {
> >>+		tcpm_log_force(port, "Start SRP connection detection");
> >>+		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
> >>+		if (!ret)
> >>+			return true;
> >>+	}
> >>+
> >>  	return false;
> >>  }
> >>diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> >>index 0c532ca3f079..bf2bbbf2e2b2 100644
> >>--- a/include/linux/usb/tcpm.h
> >>+++ b/include/linux/usb/tcpm.h
> >>@@ -125,6 +125,10 @@ struct tcpc_config {
> >>   *		Optional; if supported by hardware, called to start DRP
> >>   *		toggling. DRP toggling is stopped automatically if
> >>   *		a connection is established.
> >>+ * @start_srp_connection_detect:
> >>+ *		Optional; if supported by hardware, called to start connection
> >>+ *		detection for single role ports. Connection detection is stopped
> >>+ *		automatically if a connection is established.
> >>   * @try_role:	Optional; called to set a preferred role
> >>   * @pd_transmit:Called to transmit PD message
> >>   * @mux:	Pointer to multiplexer data
> >>@@ -149,6 +153,8 @@ struct tcpc_dev {
> >>  			 enum typec_role role, enum typec_data_role data);
> >>  	int (*start_drp_toggling)(struct tcpc_dev *dev,
> >>  				  enum typec_cc_status cc);
> >>+	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
> >>+					   enum typec_cc_status cc);
> >>  	int (*try_role)(struct tcpc_dev *dev, int role);
> >>  	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
> >>  			   const struct pd_message *msg);
> >>-- 
> >>2.21.0
> >>

[2/3] usb: typec: fusb302: Implement start_srp_connection_detect

[Hans de Goede]

Hi,

Thank you for the review.

On 14-04-19 09:40, Sergei Shtylyov wrote:
> Hello!
> 
> On 13.04.2019 23:39, Hans de Goede wrote:
> 
>> When in single role port mode, we most start single-role toggling to
> 
>     Must, perhaps?

Right, fixed for the upcoming v2 of the patch-set.

>> get an interrupt when a device / cable gets plugged into the port.
>>
>> This commit implements the tcpc_dev start_srp_connection_detect callback
>> for this.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/tcpm/fusb302.c | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>> index 6ea6199caafa..30413a45104f 100644
>> --- a/drivers/usb/typec/tcpm/fusb302.c
>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>> @@ -876,6 +876,34 @@ static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
>>       return ret;
>>   }
>> +static int tcpm_start_srp_connection_detect(struct tcpc_dev *dev,
>> +                        enum typec_cc_status cc)
>> +{
>> +    struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
>> +                         tcpc_dev);
>> +    int ret = 0;
> 
>     This initializer is useless.

Ack, this part is gone for v2 of the patch though.

Regards,

Hans



> 
>> +
>> +    mutex_lock(&chip->lock);
>> +    ret = fusb302_set_src_current(chip, cc_src_current[cc]);
>> +    if (ret < 0) {
>> +        fusb302_log(chip, "unable to set src current %s, ret=%d",
>> +                typec_cc_status_name[cc], ret);
>> +        goto done;
>> +    }
>> +    ret = fusb302_set_toggling(chip, (cc == TYPEC_CC_RD) ?
>> +                     TOGGLING_MODE_SNK : TOGGLING_MODE_SRC);
>> +    if (ret < 0) {
>> +        fusb302_log(chip,
>> +                "unable to start srp toggling, ret=%d", ret);
>> +        goto done;
>> +    }
>> +    fusb302_log(chip, "start srp toggling");
>> +done:
>> +    mutex_unlock(&chip->lock);
>> +
>> +    return ret;
>> +}
>> +
>>   static int tcpm_start_drp_toggling(struct tcpc_dev *dev,
>>                      enum typec_cc_status cc)
>>   {
> [...]
> 
> MBR, Sergei

Re: [PATCH 2/3] usb: typec: fusb302: Implement start_srp_connection_detect

[Hans de Goede]

Hi,

Thank you for the review.

On 14-04-19 09:40, Sergei Shtylyov wrote:
> Hello!
> 
> On 13.04.2019 23:39, Hans de Goede wrote:
> 
>> When in single role port mode, we most start single-role toggling to
> 
>     Must, perhaps?

Right, fixed for the upcoming v2 of the patch-set.

>> get an interrupt when a device / cable gets plugged into the port.
>>
>> This commit implements the tcpc_dev start_srp_connection_detect callback
>> for this.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/tcpm/fusb302.c | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>> index 6ea6199caafa..30413a45104f 100644
>> --- a/drivers/usb/typec/tcpm/fusb302.c
>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>> @@ -876,6 +876,34 @@ static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
>>       return ret;
>>   }
>> +static int tcpm_start_srp_connection_detect(struct tcpc_dev *dev,
>> +                        enum typec_cc_status cc)
>> +{
>> +    struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
>> +                         tcpc_dev);
>> +    int ret = 0;
> 
>     This initializer is useless.

Ack, this part is gone for v2 of the patch though.

Regards,

Hans



> 
>> +
>> +    mutex_lock(&chip->lock);
>> +    ret = fusb302_set_src_current(chip, cc_src_current[cc]);
>> +    if (ret < 0) {
>> +        fusb302_log(chip, "unable to set src current %s, ret=%d",
>> +                typec_cc_status_name[cc], ret);
>> +        goto done;
>> +    }
>> +    ret = fusb302_set_toggling(chip, (cc == TYPEC_CC_RD) ?
>> +                     TOGGLING_MODE_SNK : TOGGLING_MODE_SRC);
>> +    if (ret < 0) {
>> +        fusb302_log(chip,
>> +                "unable to start srp toggling, ret=%d", ret);
>> +        goto done;
>> +    }
>> +    fusb302_log(chip, "start srp toggling");
>> +done:
>> +    mutex_unlock(&chip->lock);
>> +
>> +    return ret;
>> +}
>> +
>>   static int tcpm_start_drp_toggling(struct tcpc_dev *dev,
>>                      enum typec_cc_status cc)
>>   {
> [...]
> 
> MBR, Sergei

[1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Hi,

On 15-04-19 20:38, Guenter Roeck wrote:
> On Mon, Apr 15, 2019 at 07:53:58PM +0200, Hans de Goede wrote:
>> Hi Guenter,
>>
>> On 15-04-19 17:51, Guenter Roeck wrote:
>>> On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
>>>> Some tcpc device-drivers need to explicitly be told to watch for connection
>>>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>>>> being plugged into the Type-C port will not be noticed.
>>>>
>>>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
>>>> watch for connection events. But for single-role ports we've so far been
>>>> falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
>>>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>>>
>>>> This commit adds a new start_srp_connection_detect callback to tcpc_dev
>>>> and when this is implemented calls this in place of start_drp_toggling
>>>> for SRP devices.
>>>>
>>>
>>> Do we indeed need an additional callback for this purpose, or would a single
>>> "start_connection_detect" call to replace start_drp_toggling be sufficient ?
>>> If necessary, we could add the port type as argument (if the low level driver
>>> doesn't already know that).
>>
>> A single "start_connection_detect" call to replace start_drp_toggling will be
>> sufficient AFAICT.
>>
>> If you prefer that option, I can rework the patch-set accordingly.
>>
> 
> Yes, I do. I find it quite pointless to have two callbacks if exactly one
> is ever called.

Ok, I've refactored this as requested for v2.

Regards,

Hans


>>>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>>>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>>>   include/linux/usb/tcpm.h      | 6 ++++++
>>>>   2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>>> index a2233d72ae7c..1af54af90b50 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
>>>>   			return true;
>>>>   	}
>>>> +	if (port->tcpc->start_srp_connection_detect &&
>>>> +	    port->port_type != TYPEC_PORT_DRP) {
>>>> +		tcpm_log_force(port, "Start SRP connection detection");
>>>> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>>>> +		if (!ret)
>>>> +			return true;
>>>> +	}
>>>> +
>>>>   	return false;
>>>>   }
>>>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
>>>> index 0c532ca3f079..bf2bbbf2e2b2 100644
>>>> --- a/include/linux/usb/tcpm.h
>>>> +++ b/include/linux/usb/tcpm.h
>>>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>>>    *		Optional; if supported by hardware, called to start DRP
>>>>    *		toggling. DRP toggling is stopped automatically if
>>>>    *		a connection is established.
>>>> + * @start_srp_connection_detect:
>>>> + *		Optional; if supported by hardware, called to start connection
>>>> + *		detection for single role ports. Connection detection is stopped
>>>> + *		automatically if a connection is established.
>>>>    * @try_role:	Optional; called to set a preferred role
>>>>    * @pd_transmit:Called to transmit PD message
>>>>    * @mux:	Pointer to multiplexer data
>>>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>>>   			 enum typec_role role, enum typec_data_role data);
>>>>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
>>>>   				  enum typec_cc_status cc);
>>>> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>>>> +					   enum typec_cc_status cc);
>>>>   	int (*try_role)(struct tcpc_dev *dev, int role);
>>>>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>>>>   			   const struct pd_message *msg);
>>>> -- 
>>>> 2.21.0
>>>>

Re: [PATCH 1/3] usb: typec: tcpm: Add start_srp_connection_detect callback

[Hans de Goede]

Hi,

On 15-04-19 20:38, Guenter Roeck wrote:
> On Mon, Apr 15, 2019 at 07:53:58PM +0200, Hans de Goede wrote:
>> Hi Guenter,
>>
>> On 15-04-19 17:51, Guenter Roeck wrote:
>>> On Sat, Apr 13, 2019 at 10:39:53PM +0200, Hans de Goede wrote:
>>>> Some tcpc device-drivers need to explicitly be told to watch for connection
>>>> events, otherwise the tcpc will not generate any TCPM_CC_EVENTs and devices
>>>> being plugged into the Type-C port will not be noticed.
>>>>
>>>> For dual-role ports tcpm_start_drp_toggling() is used to tell the tcpc to
>>>> watch for connection events. But for single-role ports we've so far been
>>>> falling back to just calling tcpm_set_cc(). For some tcpc-s such as the
>>>> fusb302 this is not enough and no TCPM_CC_EVENT will be generated.
>>>>
>>>> This commit adds a new start_srp_connection_detect callback to tcpc_dev
>>>> and when this is implemented calls this in place of start_drp_toggling
>>>> for SRP devices.
>>>>
>>>
>>> Do we indeed need an additional callback for this purpose, or would a single
>>> "start_connection_detect" call to replace start_drp_toggling be sufficient ?
>>> If necessary, we could add the port type as argument (if the low level driver
>>> doesn't already know that).
>>
>> A single "start_connection_detect" call to replace start_drp_toggling will be
>> sufficient AFAICT.
>>
>> If you prefer that option, I can rework the patch-set accordingly.
>>
> 
> Yes, I do. I find it quite pointless to have two callbacks if exactly one
> is ever called.

Ok, I've refactored this as requested for v2.

Regards,

Hans


>>>> Fixes: ea3b4d5523bc("usb: typec: fusb302: Resolve fixed power role ...")
>>>> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>   drivers/usb/typec/tcpm/tcpm.c | 8 ++++++++
>>>>   include/linux/usb/tcpm.h      | 6 ++++++
>>>>   2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>>> index a2233d72ae7c..1af54af90b50 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>> @@ -2553,6 +2553,14 @@ static bool tcpm_start_drp_toggling(struct tcpm_port *port,
>>>>   			return true;
>>>>   	}
>>>> +	if (port->tcpc->start_srp_connection_detect &&
>>>> +	    port->port_type != TYPEC_PORT_DRP) {
>>>> +		tcpm_log_force(port, "Start SRP connection detection");
>>>> +		ret = port->tcpc->start_srp_connection_detect(port->tcpc, cc);
>>>> +		if (!ret)
>>>> +			return true;
>>>> +	}
>>>> +
>>>>   	return false;
>>>>   }
>>>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
>>>> index 0c532ca3f079..bf2bbbf2e2b2 100644
>>>> --- a/include/linux/usb/tcpm.h
>>>> +++ b/include/linux/usb/tcpm.h
>>>> @@ -125,6 +125,10 @@ struct tcpc_config {
>>>>    *		Optional; if supported by hardware, called to start DRP
>>>>    *		toggling. DRP toggling is stopped automatically if
>>>>    *		a connection is established.
>>>> + * @start_srp_connection_detect:
>>>> + *		Optional; if supported by hardware, called to start connection
>>>> + *		detection for single role ports. Connection detection is stopped
>>>> + *		automatically if a connection is established.
>>>>    * @try_role:	Optional; called to set a preferred role
>>>>    * @pd_transmit:Called to transmit PD message
>>>>    * @mux:	Pointer to multiplexer data
>>>> @@ -149,6 +153,8 @@ struct tcpc_dev {
>>>>   			 enum typec_role role, enum typec_data_role data);
>>>>   	int (*start_drp_toggling)(struct tcpc_dev *dev,
>>>>   				  enum typec_cc_status cc);
>>>> +	int (*start_srp_connection_detect)(struct tcpc_dev *dev,
>>>> +					   enum typec_cc_status cc);
>>>>   	int (*try_role)(struct tcpc_dev *dev, int role);
>>>>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>>>>   			   const struct pd_message *msg);
>>>> -- 
>>>> 2.21.0
>>>>