Messages in this thread | | | Subject | Re: [PATCH] usb: typec: Add a sysfs node to manage port type | From | Guenter Roeck <> | Date | Tue, 23 May 2017 06:16:28 -0700 |
| |
On 05/23/2017 03:46 AM, Heikki Krogerus wrote: > Hi, > > On Mon, May 22, 2017 at 01:05:42PM -0700, Badhri Jagan Sridharan wrote: >> User space applications in some cases have the need to enforce a >> specific port type(DFP/UFP/DRP). This change allows userspace to >> attempt setting the desired port type. Low level drivers can >> however reject the request if the specific port type is not supported. >> >> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com> >> --- >> Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++ >> drivers/usb/typec/typec.c | 40 +++++++++++++++++++++++++++++ >> include/linux/usb/typec.h | 4 +++ >> 3 files changed, 57 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec >> index d4a3d23eb09c..853b2ef73641 100644 >> --- a/Documentation/ABI/testing/sysfs-class-typec >> +++ b/Documentation/ABI/testing/sysfs-class-typec >> @@ -73,6 +73,19 @@ Description: >> >> Valid values: source, sink, none (to remove preference) >> >> +What: /sys/class/typec/<port>/port_type >> +Date: May 2017 >> +Description: >> + Indicates the type of the port. This attribute can be used for >> + requesting a change in the port type. Port type change is >> + supported as a synchronous operation, so write(2) to the >> + attribute will not return until the operation has finished. >> + >> + Valid values: >> + - DRP >> + - DFP >> + - UFP >> >> What: /sys/class/typec/<port>/supported_accessory_modes >> Date: April 2017 >> Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com> >> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c >> index 89e540bb7ff3..684a13bb744d 100644 >> --- a/drivers/usb/typec/typec.c >> +++ b/drivers/usb/typec/typec.c >> @@ -789,6 +789,12 @@ static const char * const typec_data_roles[] = { >> [TYPEC_HOST] = "host", >> }; >> >> +static const char * const typec_port_types[] = { >> + [TYPEC_PORT_DFP] = "dfp", >> + [TYPEC_PORT_UFP] = "ufp", >> + [TYPEC_PORT_DRP] = "drp", >> +}; > > The meaning of those abbreviations has changed in every version of the > spec since v1.0 which makes me a bit uncomfortable using them with the > attributes. In USB Type-C specification v1.2, DRP now means > Dual-Role-Power, but DFP and UFP are used with USB data operation. > > I would prefer "source, "sink" and "drp". Actually, I don't even like > "drp". How about "dual" instead? > >> static ssize_t >> preferred_role_store(struct device *dev, struct device_attribute *attr, >> const char *buf, size_t size) >> @@ -926,6 +932,39 @@ static ssize_t power_role_show(struct device *dev, >> } >> static DEVICE_ATTR_RW(power_role); >> >> +static ssize_t >> +port_type_store(struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + struct typec_port *port = to_typec_port(dev); >> + int ret; >> + >> + if (!port->cap->port_type_set) { >> + dev_dbg(dev, "changing port type not supported\n"); >> + return -EOPNOTSUPP; >> + } >> + >> + ret = sysfs_match_string(typec_port_types, buf); >> + if (ret < 0) >> + return ret; >> + >> + ret = port->cap->port_type_set(port->cap, ret); >> + if (ret) >> + return ret; >> + >> + return size; >> +} >> + >> +static ssize_t >> +port_type_show(struct device *dev, struct device_attribute *attr, >> + char *buf) >> +{ >> + struct typec_port *port = to_typec_port(dev); >> + >> + return sprintf(buf, "%s\n", typec_port_types[port->cap->type]); >> +} >> +static DEVICE_ATTR_RW(port_type); > > This doesn't tell the user the capabilities of the port. All the > supported roles should be listed here like with the other attributes, > the active one in brackets. This probable means we need a small > addition/change to the API too. > typec_capability already lists the port type. Presumably it can be restricted to TYPEC_PORT_DFP or TYPEC_PORT_UFP only if it is reported as TYPEC_PORT_DRP. Am I missing something ?
> I do like the idea of being able to fix the role, assuming others are > OK with it too. >
I am definitely ok with it.
Thanks, Guenter
| |