Re: [PATCHv11 2/3] usb: USB Type-C connector class

From: Greg KH
Date: Mon Nov 21 2016 - 05:35:45 EST

On Thu, Nov 17, 2016 at 12:50:35PM +0200, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> Documentation/ABI/testing/sysfs-class-typec | 222 ++++++
> Documentation/usb/typec.txt | 103 +++
> MAINTAINERS | 9 +
> drivers/usb/Kconfig | 2 +
> drivers/usb/Makefile | 2 +
> drivers/usb/typec/Kconfig | 7 +
> drivers/usb/typec/Makefile | 1 +
> drivers/usb/typec/typec.c | 1012 +++++++++++++++++++++++++++
> include/linux/usb/typec.h | 252 +++++++
> 9 files changed, 1610 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-typec
> create mode 100644 Documentation/usb/typec.txt
> create mode 100644 drivers/usb/typec/Kconfig
> create mode 100644 drivers/usb/typec/Makefile
> create mode 100644 drivers/usb/typec/typec.c
> create mode 100644 include/linux/usb/typec.h
>
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> new file mode 100644
> index 0000000..4fac77c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,222 @@
> +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> +
> +What: /sys/class/typec/<port>/current_data_role
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + The current USB data role the port is operating in. This
> + attribute can be used for requesting data role swapping on the
> + port. Swapping is supported as synchronous operation, so
> + write(2) to the attribute will not return until the operation
> + has finished. The attribute is notified about role changes so
> + that poll(2) on the attribute wakes up. Change on the role will
> + also generate uevent KOBJ_CHANGE on the port.
> +
> + Valid values:
> + - host
> + - device
> +
> +What: /sys/class/typec/<port>/current_power_role
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + The current power role of the port. This attribute can be used
> + to request power role swap on the port when the port supports
> + USB Power Delivery. Swapping is supported as synchronous
> + operation, so write(2) to the attribute will not return until
> + the operation has finished. The attribute is notified about role
> + changes so that poll(2) on the attribute wakes up. Change on the
> + role will also generate uevent KOBJ_CHANGE.
> +
> + Valid values:
> + - source
> + - sink
> +
> +What: /sys/class/typec/<port>/vconn_source
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows is the port VCONN Source. This attribute can be used to
> + request VCONN swap to change the VCONN Source during connection
> + when both the port and the partner support USB Power Delivery.
> + Swapping is supported as synchronous operation, so write(2) to
> + the attribute will not return until the operation has finished.
> + The attribute is notified about VCONN source changes so that
> + poll(2) on the attribute wakes up. Change on VCONN source also
> + generates uevent KOBJ_CHANGE.
> +
> + Valid values are:
> + - 0 when the port is not the VCONN Source
> + - 1 when the port is the VCONN Source
> +
> +What: /sys/class/typec/<port>/power_operation_mode
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows the current power operational mode the port is in.
> +
> + Valid values:
> + - USB - Normal power levels defined in USB specifications
> + - BC1.2 - Power levels defined in Battery Charging Specification
> + v1.2
> + - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
> + specification.
> + - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
> + specification.
> + - USB Power Delivery - The voltages and currents defined in USB
> + Power Delivery specification
> +
> +What: /sys/class/typec/<port>/preferred_role
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +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 there is no preferred role.
> +
> + Valid values:
> + - source
> + - sink
> + - none - to remove preference
> +
> +What: /sys/class/typec/<port>/supported_accessory_modes
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Lists the Accessory Modes, defined in the USB Type-C
> + specification, the port supports.
> +
> +What: /sys/class/typec/<port>/supported_data_roles
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Lists the USB data roles the port is capable of supporting.
> +
> + Valid values:
> + - device
> + - host
> + - device, host (DRD as defined in USB Type-C specification v1.2)
> +
> +What: /sys/class/typec/<port>/supported_power_roles
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Lists the power roles the port is capable of supporting.
> +
> + Valid values:
> + - source
> + - sink
> + - source, sink (DRP as defined in USB Type-C specification v1.2)
> +
> +What: /sys/class/typec/<port>/supports_usb_power_delivery
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows if the port supports USB Power Delivery.
> + - 1 if USB Power Delivery is supported
> + - 0 when it's not
> +
> +
> +USB Type-C partner devices (eg. /sys/class/typec/usbc0-partner/)
> +
> +What: /sys/class/typec/<port>-partner/accessory_mode
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows the Accessory Mode name, or "no" when the partner does not
> + support Accesory Modes. The Accessory Modes are defined in USB
> + Type-C Specification.
> +
> +What: /sys/class/typec/<port>-partner/supports_usb_power_delivery
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows if the partner supports USB Power Delivery:
> + - 0 when USB Power Delivery is not supported
> + - 1 when USB Power Delivery is supported
> +
> +
> +USB Type-C cable devices (eg. /sys/class/typec/usbc0-cable/)
> +
> +Note: Electronically Marked Cables will have a device also for one cable plug
> +(eg. /sys/class/typec/usbc0-plug0). If the cable is active and has also SOP
> +Double Prime controller (USB Power Deliver specification ch. 2.4) it will have
> +second device also for the other plug. Both plugs may have their alternate modes
> +as described in USB Type-C and USB Power Delivery specifications.
> +
> +What: /sys/class/typec/<port>-cable/active
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows if the cable is active or passive.
> +
> + Valid values:
> + - 0 when the cable is passive
> + - 1 when the cable is active
> +
> +What: /sys/class/typec/<port>-cable/plug_type
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows type of the plug on the cable:
> + - Type-A - Standard A
> + - Type-B - Standard B
> + - Type-C - USB Type-C
> + - Captive - Non-standard
> +
> +What: /sys/class/typec/<port>-cable/supports_usb_power_delivery
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows if the cable supports USB Power Delivery:
> + - 0 when USB Power Delivery is not supported
> + - 1 when USB Power Delivery is supported
> +
> +
> +Alternate Mode devices (For example,
> +/sys/class/typec/usbc0-partner/usbc0-partner.svid:xxxx/). The ports, partners
> +and cable plugs can have alternate modes.
> +
> +What: /sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/active
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows if the mode is active or not. The attribute can be used
> + for entering/exiting the mode with partners and cable plugs, and
> + with the port alternate modes it can be used for disabling
> + support for specific alternate modes. Entering/exiting modes is
> + supported as synchronous operation so write(2) to the attribute
> + does not return until the enter/exit mode operation has
> + finished. The attribute is notified when the mode is
> + entered/exited so poll(2) on the attribute wakes up.
> + Entering/exiting a mode will also generate uevent KOBJ_CHANGE.
> +
> + Valid values:
> + - 0 when the mode is deactive
> + - 1 when the mode is active
> +
> +What: /sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/description
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows description of the mode. The description is optional for
> + the drivers, just like with the Billboard Devices.
> +
> +What: /sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/vdo
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows the VDO in hexadecimal returned from the Discover Modes
> + command.
> +
> +What: /sys/class/typec/<port>/<port>.svid:<svid>/<mode>/supported_roles
> +Date: December 2016
> +Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +Description:
> + Shows the roles, source or sink, the mode is supported with.
> +
> + This attribute is available for the devices describing the
> + alternate modes a port supports, and it will not be exposed with
> + the devices presenting the alternate modes the partners or cable
> + plugs support.
> diff --git a/Documentation/usb/typec.txt b/Documentation/usb/typec.txt
> new file mode 100644
> index 0000000..7095a2a
> --- /dev/null
> +++ b/Documentation/usb/typec.txt
> @@ -0,0 +1,103 @@
> +USB Type-C connector class
> +==========================
> +
> +Introduction
> +------------
> +The typec class is meant for describing the USB Type-C ports in a system to the
> +user space in unified fashion. The class is designed to provide nothing else
> +except the user space interface implementation in hope that it can be utilized
> +on as many platforms as possible.
> +
> +The platforms are expected to register every USB Type-C port they have with the
> +class. In a normal case the registration will be done by a USB Type-C or PD PHY
> +driver, but it may be a driver for firmware interface such as UCSI, driver for
> +USB PD controller or even driver for Thunderbolt3 controller. This document
> +considers the component registering the USB Type-C ports with the class as "port
> +driver".
> +
> +On top of showing the capabilities, the class also offer the user space control
> +over the roles and alternate modes they support when the port driver is capable
> +of supporting those features.
> +
> +The class provides an API for the port drivers described in this document. The
> +attributes are described in Documentation/ABI/testing/sysfs-class-typec.
> +
> +
> +Interface
> +---------
> +Every port will be presented as its own device under /sys/class/typec/. The
> +first port will be named "usbc0", the second "usbc1" and so on.
> +
> +When connected, the partner will be presented also as its own device under
> +/sys/class/typec/. The parent of the partner device will always be the port. The
> +partner attached to port "usbc0" will be named "usbc0-partner". Full path to the
> +device would be /sys/class/typec/usb0/usb0-partner/.
> +
> +The cable and the two plugs on it may also be optionally presented as their own
> +devices under /sys/class/typec/. The cable attached to the port "usbc0" port
> +will be named usbc0-cable and the plug on the SOP Prime end (see USB Power
> +Delivery Specification ch. 2.4) will be named "usbc-plug0" and on the SOP Double
> +Prime end "usbc0-plug1". The parent of a cable will always be the port, and the
> +parent of the cable plugs will always be the cable.
> +
> +If the port, partner or cable plug support Alternate Modes, every Alternate Mode
> +SVID will have their own device describing them. The Alternate Modes will not be
> +attached to the typec class. For the port's "usbc0" partner, the Alternate Modes
> +would have devices presented under /sys/class/typec/usbc0-partner/. Every mode
> +that is supported will have its own group under the Alternate Mode device named
> +"mode<id>". For example /sys/class/typec/usbc0/usbc0.svid:xxxx/mode0/. The
> +requests for entering/exiting the modes happens with the "active" attribute in
> +that group.
> +
> +
> +API
> +---
> +
> +* Registering the ports
> +
> +The port drivers will describe every Type-C port they control with struct
> +typec_capability data structure, and register them with the following API:
> +
> +struct typec_port *typec_register_port(struct device *dev,
> + const struct typec_capability *cap);
> +
> +The class will provide handle to struct typec_port on success and ERR_PTR on
> +failure. The un-registration of the port happens with the following API:
> +
> +void typec_unregister_port(struct typec_port *port);
> +
> +
> +* Notifications
> +
> +When connection happens on a port, the port driver fills struct typec_connection
> +which is passed to the class. The class provides the following API for reporting
> +connection/disconnection:
> +
> +int typec_connect(struct typec_port *port, struct typec_connection *);
> +void typec_disconnect(struct typec_port *);
> +
> +When the partner end has executed a role change, the port driver uses the
> +following APIs to report it to the class:
> +
> +void typec_set_data_role(struct typec_port *, enum typec_data_role);
> +void typec_set_pwr_role(struct typec_port *, enum typec_role);
> +void typec_set_vconn_role(struct typec_port *, enum typec_role);
> +void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
> +
> +
> +* Alternate Modes
> +
> +After connection, the port drivers register the alternate modes the partner
> +and/or cable plugs support. And before reporting disconnection, the port driver
> +_must_ unregister all the alternate modes registered for the partner and cable
> +plugs. The API takes the struct device of the partner or the cable plug as
> +parameter:
> +
> +int typec_register_altmodes(struct device *, struct typec_altmode *);
> +void typec_unregister_altmodes(struct device *);
> +
> +When the partner end enters or exits the modes, the port driver needs to notify
> +the class with the following API:
> +
> +void typec_altmode_update_active(struct typec_altmode *alt, int mode,
> + bool active);
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5e25ba1..baa2322 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12761,6 +12761,15 @@ F: drivers/usb/
> F: include/linux/usb.h
> F: include/linux/usb/
>
> +USB TYPEC SUBSYSTEM
> +M: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> +L: linux-usb@xxxxxxxxxxxxxxx
> +S: Maintained
> +F: Documentation/ABI/testing/sysfs-class-typec
> +F: Documentation/usb/typec.txt
> +F: drivers/usb/typec/
> +F: include/linux/usb/typec.h
> +
> USB UHCI DRIVER
> M: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> L: linux-usb@xxxxxxxxxxxxxxx
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index fbe493d..89c322e 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -152,6 +152,8 @@ source "drivers/usb/phy/Kconfig"
>
> source "drivers/usb/gadget/Kconfig"
>
> +source "drivers/usb/typec/Kconfig"
> +
> config USB_LED_TRIG
> bool "USB LED Triggers"
> depends on LEDS_CLASS && LEDS_TRIGGERS
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index 7791af6..c7f4098 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -62,3 +62,5 @@ obj-$(CONFIG_USB_GADGET) += gadget/
> obj-$(CONFIG_USB_COMMON) += common/
>
> obj-$(CONFIG_USBIP_CORE) += usbip/
> +
> +obj-$(CONFIG_TYPEC) += typec/
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> new file mode 100644
> index 0000000..b229fb9
> --- /dev/null
> +++ b/drivers/usb/typec/Kconfig
> @@ -0,0 +1,7 @@
> +
> +menu "USB PD and Type-C drivers"
> +
> +config TYPEC
> + tristate
> +
> +endmenu
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> new file mode 100644
> index 0000000..1012a8b
> --- /dev/null
> +++ b/drivers/usb/typec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TYPEC) += typec.o
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> new file mode 100644
> index 0000000..e2909a6
> --- /dev/null
> +++ b/drivers/usb/typec/typec.c
> @@ -0,0 +1,1012 @@
> +/*
> + * USB Type-C Connector Class
> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb/typec.h>
> +
> +struct typec_port {
> + unsigned int id;
> + struct device dev;
> +
> + int prefer_role;
> +
> + enum typec_data_role data_role;
> + enum typec_role pwr_role;
> + enum typec_role vconn_role;
> + enum typec_pwr_opmode pwr_opmode;
> +
> + struct typec_partner *partner;
> + struct typec_cable *cable;
> +
> + unsigned int connected:1;
> +
> + const struct typec_capability *cap;
> +};
> +
> +#define to_typec_port(p) container_of(p, struct typec_port, dev)
> +#define to_typec_plug(p) container_of(p, struct typec_plug, dev)
> +#define to_typec_cable(p) container_of(p, struct typec_cable, dev)
> +#define to_typec_partner(p) container_of(p, struct typec_partner, dev)
> +
> +static DEFINE_IDA(typec_index_ida);
> +
> +static struct class typec_class = {
> + .name = "typec",
> +};
> +
> +static const char * const typec_accessory_modes[] = {
> + [TYPEC_ACCESSORY_NONE] = "no",
> + [TYPEC_ACCESSORY_AUDIO] = "Audio Adapter Accessory Mode",
> + [TYPEC_ACCESSORY_DEBUG] = "Debug Accessory Mode",
> +};
> +
> +static struct device_type typec_partner_dev_type;
> +static struct device_type typec_cable_dev_type;
> +static struct device_type typec_port_dev_type;
> +
> +static ssize_t supports_usb_power_delivery_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + if (dev->type == &typec_partner_dev_type) {
> + struct typec_partner *p = to_typec_partner(dev);
> +
> + return sprintf(buf, "%d\n", p->usb_pd);
> + } else if (dev->type == &typec_partner_dev_type) {
> + struct typec_cable *p = to_typec_cable(dev);
> +
> + return sprintf(buf, "%d\n", p->usb_pd);
> + } else {
> + struct typec_port *p = to_typec_port(dev);
> +
> + return sprintf(buf, "%d\n", p->cap->usb_pd);
> + }
> +}
> +static DEVICE_ATTR_RO(supports_usb_power_delivery);
> +
> +/* ------------------------------------------------------------------------- */
> +/* Type-C Partners */
> +
> +static ssize_t accessory_mode_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct typec_partner *p = to_typec_partner(dev);
> +
> + return sprintf(buf, "%s\n", typec_accessory_modes[p->accessory]);
> +}
> +static DEVICE_ATTR_RO(accessory_mode);
> +
> +static struct attribute *typec_partner_attrs[] = {
> + &dev_attr_accessory_mode.attr,
> + &dev_attr_supports_usb_power_delivery.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(typec_partner);
> +
> +static void typec_partner_release(struct device *dev)
> +{
> + struct typec_port *port = to_typec_port(dev->parent);
> +
> + typec_unregister_altmodes(dev);
> + port->partner = NULL;
> +}

This doesn't feel right, why are you both exporting
typec_unregister_altmodes() and also calling it from release callbacks?
That implies there is two way to clean stuff up, so what is a driver
writer to use? Please simplify and force it to be one way or the other.

Also typec_unregister_altmodes() doesn't free memory, which release is
supposed to be doing, which also implies that the reference counting
logic is all wrong here. Please fix, like I asked you to previously.

thanks,

greg k-h