On Wed, Apr 19, 2017 at 8:14 AM, Guenter Roeck <li...@roeck-us.net> wrote: > On Wed, Apr 19, 2017 at 07:45:00AM -0700, Badhri Jagan Sridharan wrote: >> On Wed, Apr 19, 2017 at 4:23 AM, Heikki Krogerus >> <heikki.kroge...@linux.intel.com> wrote: >> > Hi, >> > >> > On Tue, Apr 18, 2017 at 11:52:33AM -0700, Badhri Jagan Sridharan wrote: >> >> Hi Heikki, >> >> >> >> I have a question regarding the preferred_role node. >> >> >> >> +What: /sys/class/typec/<port>/preferred_role >> >> +Date: March 2017 >> >> +Contact: Heikki Krogerus <heikki.kroge...@linux.intel.com> >> >> +Description: >> >> + The user space can notify the driver about the preferred >> >> role. >> >> + It should be handled as enabling of Try.SRC or Try.SNK, as >> >> + defined in USB Type-C specification, in the port drivers. >> >> By >> >> + default the preferred role should come from the platform. >> >> + >> >> + Valid values: source, sink, none (to remove preference) >> >> >> >> What is the expected behavior when the userspace changes the >> >> preferred_role node when the port is in connected state ? >> >> >> >> 1. the state machine re-resolves the port roles right away based on >> >> the new state machine in place ? (or) >> > >> > No! There are separate attributes for sending role swap requests. >> >> Right. But, that might not be helpful in cases when PD is not implemented. >> and Implementing PD is not mandatory according the spec :/ >> >> FYI quoting from the Type-C specification release(page 24), >> role swaps are not limited to devices that only support PD. >> >> "Two independent set of mechanisms are defined to allow a USB Type-C >> DRP to functionally swap power and data roles. When USB PD is >> supported, power and data role swapping is performed as a subsequent >> step following the initial connection process. For non-PD implementations, >> power/data role swapping can optionally be dealt with as part of the initial >> connection process." >> >> But, the current interface definition actually prevents current/data role >> swaps for non-pd devices. >>
> This is correct for the attribute definition, but it is not implemented > that way. Writing the attribute is only read-only for non-DRP ports. i.e. tcpm_dr_set/tcpm_pr_set at tcpm.c would return EINVAL when type is not TYPEC_PORT_DRP, is that what you are referring to ? if (port->typec_caps.type != TYPEC_PORT_DRP) { ret = -EINVAL; goto port_unlock; } I do agree that this is actually correct. I am referring to the case where port is dual-role-power and dual-role-data but NOT PD capable. > Given the standard, I would consider that to be intentional; it might > make sense to update the description accordingly. > > How about implementing a mechanism in the dr_set and pr_set code in tcpm > which would handle that situation ? Something along the line of > > if (!port->pd_capable && connected && current role != desired role) { > reset_port(); > goto done; > } By "desired role" you are referring to preferred_role right ? If so yes, That's a good idea as well and it might work as long as type-c connector class allows the call to reach tcpm code :) But the current connector class code does not allow that because the power_role and data_role nodes are defined that way. port->cap->pd_revision and the port->pwr_opmode check in the below code stub have to removed/refactored to make current_role/data_role writes to reach the tcpm code. +static ssize_t power_role_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + struct typec_port *port = to_typec_port(dev); + int ret = size; + + if (!port->cap->pd_revision) { + dev_dbg(dev, "USB Power Delivery not supported\n"); + return -EOPNOTSUPP; + } + + if (!port->cap->pr_set) { + dev_dbg(dev, "power role swapping not supported\n"); + return -EOPNOTSUPP; + } + + if (port->pwr_opmode != TYPEC_PWR_MODE_PD) { + dev_dbg(dev, "partner unable to swap power role\n"); + return -EIO; + } + + ret = sysfs_match_string(typec_roles, buf); + if (ret < 0) + return ret; + + ret = port->cap->pr_set(port->cap, ret); + if (ret) + return ret; + + return size; +} Thanks, Badhri > > My current code doesn't handle the !pd_capable state, so I'll need to do > something anyway. > > Thanks, > Guenter > >> > >> > The attribute will "enable" Try.SRC/SNK states, i.e. next time the >> > state machine is executed, those states need to be considered. >> > Changing the value of this attribute must not affect the current >> > connection. >> > >> >> 2. Wait till the subsequent connect for resolving port roles based on the >> >> new state machine. >> > >> > Yes. >> > >> >> For #1 to happen the policy_engine layer would have to reset the port >> >> to resolve the port roles based on the (Try.SRC /Try.SNK/ Default) >> >> new state machine preference. >> >> >> >> Say for example when two non-PD devices following none (default state >> >> machine) are connected, the port role resolution is going to be random. >> >> But, if the userspace in one of the devices later changes the >> >> preferred_role to source, then that device is most likely to become source >> >> if the Try.SRC state-machine is re-run. >> >> >> >> Does the above question fall under a policy decision ? If so, should there >> >> be another node to say if the port roles have to re-resolved based on the >> >> new state machine right away ? >> > >> > I don't think we should even consider option #1, but just to be sure, >> > Oliver, what do you say? >> >> Can we at least consider exposing a port_reset field so that the userspace >> at least has an option to make the state machine to kick in right away with >> a hard reset ? >> >> Please do consider. We can't expect all low-end phones and devices with >> smaller form factors then phones to implement PD as it might be an overkill >> for them. >> >> > >> > I guess we need to say in the documentation explicitly that changing >> > the value will not affect the current connection. >> > >> > >> > Thanks, >> > >> > -- >> > heikki