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@xxxxxxxxxxx> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> --- 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
