Message-ID: <20160623130814.GD27263@kuha.fi.intel.com>
Date:	Thu, 23 Jun 2016 16:08:14 +0300
From:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:	Roger Quadros <rogerq@...com>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Guenter Roeck <linux@...ck-us.net>,
	Oliver Neukum <oneukum@...e.com>,
	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCHv3 1/2] usb: USB Type-C connector class

Hi Roger,

I'll fix all the typos you mention below...

On Thu, Jun 23, 2016 at 02:53:11PM +0300, Roger Quadros wrote:
> Hi Heikki,
> 
> On 21/06/16 17:51, 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 USB PD is
> 
> s/PD/PD (Power Delivery)
> 
> > available, also control over power role swapping and
> > Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec |  163 ++++
> >  Documentation/usb/typec.txt                 |  101 +++
> >  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                   | 1173 +++++++++++++++++++++++++++
> >  include/linux/usb/typec.h                   |  255 ++++++
> >  9 files changed, 1713 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..ce577f3
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -0,0 +1,163 @@
> > +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> > +
> > +What:		/sys/class/typec/<port>/current_data_role
> > +Data:		June 2016
> 
> s/Data/Date everywhere in this patch.
> 
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		The current USB data role the port is operating in. This
> > +		attribute can be used for requestion data role swapping on th
> 
> s/requestion/requesting
> 
> s/th/the
> 
> > +		port.
> 
> Is it better to list all the values that can be here with a short note for each?
> Same for all properties below?

Sure.

> Otherwise as a user I have no clue as to what all values I can set this to.
> 
> > +
> > +What:		/sys/class/typec/<port>/current_power_role
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		The current power role, source or sink, of the port. This
> > +		attribute can be used to request power role swap on the port
> > +		when USB Power Delivery is available.
> 
> So if it is not yet available, why add this now?

s/...when USB Power Delivery is available/...when the port supports
USB Power Delivery/

> > +
> > +What:		/sys/class/typec/<port>/current_vconn_role
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Shows the current VCONN role, source or sink, of the port. This
> > +		attribute can be used to request vconn role swap on the port
> 
> s/vconn/VCONN
> 
> > +		when USB Power Delivery is available.
> 
> Why add if not yet available?
> 
> > +
> > +What:		/sys/class/typec/<port>/power_operation_mode
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Shows the current power power mode the port is in. Reads as on
> 
> s/on/one
> 
> > +		of the following:
> > +		- 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 would it show when the port is disconnected?

USB. That is the default.

> Instead, is it somehow possible to list the actual negotiated current (in mA)
> here or in some other property, if you want to know the mode as well?

The current is not always going to be available for the port driver,
and in any case out side the scope of the class. Those details are
something that for example a USB Type-C PHY should always be able to
detect, except BC1.2... I think I'm dropping that one.

> > +
> > +What:		/sys/class/typec/<port>/preferred_role
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.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.
> 
> What would be the default setting?

Default is no preference. I'll fix this.

> > +
> > +What:		/sys/class/typec/<port>/supported_accessory_modes
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Lists the Accessory Modes, defined in the USB Type-C
> > +		specification, the port supports.
> > +
> > +What:		/sys/class/typec/<port>/supported_data_roles
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Lists the USB data roles, host or device, the port is capable
> > +		of supporting.
> 
> Can't it support both e.g in DRP case?

Yes. s/host or device/host and device/

> > +
> > +What:		/sys/class/typec/<port>/supported_power_roles
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Lists the power roles, source and/or sink, the port is capable
> > +		of supporting.
> > +
> > +What:		/sys/class/typec/<port>/supports_usb_power_delivery
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Shows if the port support USB Power Delivery.
> 
> s/support/supports
> 
> > +
> > +
> > +USB Type-C partner devices (eg. /sys/class/typec/usbc0-partner/)
> > +
> > +What:		/sys/class/typec/<port>-partner/accessory
> 
> <port>-partner/ or <port>/partner ?

<port>-partner/. It is a device.

> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Shows the name of the Accessory Mode if the partner attached to
> > +		the port is an accessory. Otherwise shows nothing.
> > +
> > +What:		/sys/class/typec/<port>-partner/type
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Shows the type of the partner. Can be one of the following:
> > +		- USB - When the partner is normal USB host/peripheral.
> > +		- Charger - When the partner has been identified as dedicated
> > +			    charger.
> > +		- Alternate Mode - When the partner supports Alternate Modes.
> > +		- Accessory - When the partner is one of the accessories with
> > +			      specific Accessory Mode defined in USB Type-C
> > +			      specification.
> > +
> > +
> > +USB Type-C cable devices (eg. /sys/class/typec/usbc0-cable/)
> > +
> > +Note: The two cable plugs will have their own devices presenting when available
> 
> I didn't get this part. How come two cable plugs can have 2 devices?

One plug has one struct device? A cable has two plugs.

> presenting what?

The plug? On active cables the plugs may have alternate modes.

> > +(eg. /sys/class/typec/usbc0-plug0). Both plugs may have their own active
> > +alternate modes as described in USB Type-C and USB Power Delivery
> > +specifications.
> > +
> > +What:		/sys/class/typec/<port>-cable/active
> 
> <port>-cable/ or <port>/cable ?

<port>-cable/

> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Shows if the cable is active or passive. Only active cables can
> > +		have configurable alternate modes, therefore only active cables
> > +		will have plug devices.
> > +
> > +What:		/sys/class/typec/<port>-cable/plug_type
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +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
> > +
> > +
> > +Alternate Mode devices  (For example,
> > +/sys/class/typec/usbc0-partner/usbc0-partner.svid:xxxx/)
> 
> Why should it show usbc0-partner twice?

Because device names need to be unique.

> > +
> > +What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/active
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +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.
> > +
> > +What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/description
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +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
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +Description:
> > +		Shows the VDO in hexadecimal returned from the Discover Modes
> > +		command.
> > +
> > +What:		/sys/class/typec/<port>/<port>.svid:<svid>/<mode>/supported_roles
> > +Data:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +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..ddeda97
> > --- /dev/null
> > +++ b/Documentation/usb/typec.txt
> > @@ -0,0 +1,101 @@
> > +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
> > +expect the user space interface implementation in hope that it can be utilized
> 
> s/expect/except/
> 
> > +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 offers 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.
> 
> We already know it is typec since it sits in /sys/class/typec
> 
> Why not just call the instances "0", "1" and so on?

A device named "0" or "1"... I wonder why other sub-systems are not
naming their devices like that :-)

> > +
> > +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".
> 
> Can the partner exist without the port? If not the partner should be located within
> the port directory.

It can't and won't exist without port, and the actual device does
exist in the port directory. However, from the classes point of view
that is irrelevant. A class will always create a link to its own group
for all the devices attached to it.

> > +
> > +The cable and the two plugs on it may also be optionally be 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".
> 
> If cables and plugs can't exist without the port they must come up within the port
> directory.

The same.

> > +
> > +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 e1b090f..0db33fb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11983,6 +11983,15 @@ F:	drivers/usb/
> >  F:	include/linux/usb.h
> >  F:	include/linux/usb/
> >  
> > +USB TYPEC SUBSYSTEM
> > +M:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > +L:	linux-usb@...r.kernel.org
> > +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@...land.harvard.edu>
> >  L:	linux-usb@...r.kernel.org
> > diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> > index 8689dcb..f42a3d3 100644
> > --- a/drivers/usb/Kconfig
> > +++ b/drivers/usb/Kconfig
> > @@ -150,6 +150,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 && USB_COMMON && LEDS_TRIGGERS
> > diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> > index dca7856..51e381e 100644
> > --- a/drivers/usb/Makefile
> > +++ b/drivers/usb/Makefile
> > @@ -61,3 +61,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"
> 
> USB PD is independent of Type C right? Why club them together here?

The are not tied to each other, that is for sure, but I don't think
there will be that many drivers for USB PD controller or PHYs that
would function without Type-C, so would separating them be that
useful?

> > +
> > +config TYPEC
> 
> No help description?

Nope. Alone the class is useless, so I don't want to make it something
that can be selected separately. And the port drivers will in any case
depend on it so why bother the users.


Thanks,

-- 
heikki

Powered by blists - more mailing lists