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@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.
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
