Re: [RFC PATCH] usb: typec: Various API updates and fixes
From: Heikki Krogerus
Date: Fri May 27 2016 - 03:55:52 EST
Hi,
On Wed, May 25, 2016 at 11:35:07AM -0700, Guenter Roeck wrote:
> From: Guenter Roeck <groeck@xxxxxxxxxxxx>
>
> New API functions (calls into class code)
> typec_set_usb_role()
> typec_set_pwr_role()
> typec_set_vconn_role()
> typec_set_pwr_opmode()
>
> Modified API functions (calls into class code):
> typec_register_port(dev, cap) ->
> typec_register_port(dev, cap, driver_data)
>
> Modified callback functions:
> dr_swap(port) -> dr_set(port, driver_data, role);
> pr_swap(port) -> pr_set(port, driver_data, role);
> vconn_swap(port) -> vconn_set(port, driver_data, role);
> fix_role(port) -> fix_role(port, driver_data, role);
> activate_mode(...) -> activate_mode(..., driver_data, ...);
>
> New sysfs attribute:
> current_vconn_role
>
> Other:
> - Extract role initialization to new function typec_init_roles()
> - Call driver code unconditionally on role changes
> - Add NULL check in typec_unregister_altmodes()
> - If an alternate mode description pointer is NULL, display
> an empty string.
>
> Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxxxx>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> This patch applies on top of '[RFC PATCHv2] usb: USB Type-C Connector Class'
> from Heikki Krogerus. It provided the changes I made to get the code
> operational.
>
> drivers/usb/type-c/typec.c | 134 ++++++++++++++++++++++++++++++++++++---------
> include/linux/usb/typec.h | 26 ++++++---
> 2 files changed, 125 insertions(+), 35 deletions(-)
Thanks Guenter. After a quick glance looks good to me.
I think I'll create a development branch to my github tree for this
class and add this on top of the original v2 patch as is. Just to have
some history for as long we develop this thing. Though I'm not sure
how useful it is in this case. I think we need to put this together
fast. I'm thinking we need to target v4.8 with this.
I'll merge this into any case to v3, and I'll send on Monday.
> diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
> index 8028b7df0951..6836e972b681 100644
> --- a/drivers/usb/type-c/typec.c
> +++ b/drivers/usb/type-c/typec.c
> @@ -27,6 +27,8 @@ struct typec_port {
> struct typec_partner *partner;
> struct typec_cable *cable;
>
> + void *driver_data;
> +
> unsigned int connected:1;
>
> int n_altmode;
> @@ -324,6 +326,20 @@ static void typec_remove_cable(struct typec_port *port)
> device_unregister(&port->cable->dev);
> }
>
> +static void typec_init_roles(struct typec_port *port)
> +{
> + if (port->fixed_role == TYPEC_PORT_DFP) {
> + port->usb_role = TYPEC_HOST;
> + port->pwr_role = TYPEC_PWR_SOURCE;
> + port->vconn_role = TYPEC_PWR_SOURCE;
> + } else {
> + /* Device mode as default also with DRP ports */
> + port->usb_role = TYPEC_DEVICE;
> + port->pwr_role = TYPEC_PWR_SINK;
> + port->vconn_role = TYPEC_PWR_SINK;
> + }
> +}
> +
> /* -------------------------------- */
>
> int typec_connect(struct typec_port *port, struct typec_connection *con)
> @@ -378,16 +394,7 @@ void typec_disconnect(struct typec_port *port)
>
> port->pwr_opmode = TYPEC_PWR_MODE_USB;
>
> - if (port->fixed_role == TYPEC_PORT_DFP) {
> - port->usb_role = TYPEC_HOST;
> - port->pwr_role = TYPEC_PWR_SOURCE;
> - port->vconn_role = TYPEC_PWR_SOURCE;
> - } else {
> - /* Device mode as default also with DRP ports */
> - port->usb_role = TYPEC_DEVICE;
> - port->pwr_role = TYPEC_PWR_SINK;
> - port->vconn_role = TYPEC_PWR_SINK;
> - }
> + typec_init_roles(port);
>
> kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> }
> @@ -405,6 +412,34 @@ struct typec_port *typec_dev2port(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(typec_dev2port);
>
> +/* --------------------------------------- */
> +/* Driver callbacks to report role updates */
> +
> +void typec_set_usb_role(struct typec_port *port, enum typec_usb_role role)
> +{
> + port->usb_role = role;
> +}
> +EXPORT_SYMBOL(typec_set_usb_role);
> +
> +void typec_set_pwr_role(struct typec_port *port, enum typec_pwr_role role)
> +{
> + port->pwr_role = role;
> +}
> +EXPORT_SYMBOL(typec_set_pwr_role);
> +
> +void typec_set_vconn_role(struct typec_port *port, enum typec_pwr_role role)
> +{
> + port->vconn_role = role;
> +}
> +EXPORT_SYMBOL(typec_set_vconn_role);
> +
> +void typec_set_pwr_opmode(struct typec_port *port,
> + enum typec_pwr_opmode opmode)
> +{
> + port->pwr_opmode = opmode;
> +}
> +EXPORT_SYMBOL(typec_set_pwr_opmode);
> +
> /* -------------------------------- */
> /* Alternate Modes */
>
> @@ -451,7 +486,7 @@ typec_altmode_desc_show(struct device *dev, struct device_attribute *attr,
> struct typec_mode *mode = container_of(attr, struct typec_mode,
> desc_attr);
>
> - return sprintf(buf, "%s\n", mode->desc);
> + return sprintf(buf, "%s\n", mode->desc ? mode->desc : "");
> }
>
> static ssize_t
> @@ -561,6 +596,9 @@ void typec_unregister_altmodes(struct typec_altmode *alt_modes)
> {
> struct typec_altmode *alt;
>
> + if (!alt_modes)
> + return;
> +
> for (alt = alt_modes; alt->svid; alt++)
> device_unregister(&alt->dev);
> }
> @@ -581,7 +619,7 @@ current_usb_data_role_store(struct device *dev, struct device_attribute *attr,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->dr_swap) {
> + if (!port->cap->dr_set) {
> dev_warn(dev, "data role swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -593,10 +631,7 @@ current_usb_data_role_store(struct device *dev, struct device_attribute *attr,
> else
> return -EINVAL;
>
> - if (port->usb_role == role || !port->partner)
> - return size;
> -
> - ret = port->cap->dr_swap(port);
> + ret = port->cap->dr_set(port, port->driver_data, role);
> if (ret)
> return ret;
>
> @@ -655,10 +690,7 @@ current_data_role_store(struct device *dev, struct device_attribute *attr,
> else
> return -EINVAL;
>
> - if (port->fixed_role == role)
> - return size;
> -
> - ret = port->cap->fix_role(port, role);
> + ret = port->cap->fix_role(port, port->driver_data, role);
> if (ret)
> return ret;
>
> @@ -688,7 +720,7 @@ static ssize_t current_power_role_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> - if (!port->cap->pr_swap) {
> + if (!port->cap->pr_set) {
> dev_warn(dev, "power role swapping not supported\n");
> return -EOPNOTSUPP;
> }
> @@ -705,10 +737,7 @@ static ssize_t current_power_role_store(struct device *dev,
> else
> return -EINVAL;
>
> - if (port->pwr_role == role || !port->partner)
> - return size;
> -
> - ret = port->cap->pr_swap(port);
> + ret = port->cap->pr_set(port, port->driver_data, role);
> if (ret)
> return ret;
>
> @@ -762,6 +791,54 @@ static ssize_t power_operation_mode_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(power_operation_mode);
>
> +static ssize_t current_vconn_role_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct typec_port *port = to_typec_port(dev);
> + enum typec_pwr_role role;
> + int ret;
> +
> + if (!port->cap->usb_pd) {
> + dev_dbg(dev, "vconn swap only supported with USB PD\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (!port->cap->vconn_set) {
> + dev_warn(dev, "vconn swapping not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (!strncmp(buf, "source", 6))
> + role = TYPEC_PWR_SOURCE;
> + else if (!strncmp(buf, "sink", 4))
> + role = TYPEC_PWR_SINK;
> + else
> + return -EINVAL;
> +
> + ret = port->cap->vconn_set(port, port->driver_data, role);
> + if (ret)
> + return ret;
> +
> + return size;
> +}
> +
> +static ssize_t current_vconn_role_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct typec_port *port = to_typec_port(dev);
> +
> + switch (port->vconn_role) {
> + case TYPEC_PWR_SOURCE:
> + return sprintf(buf, "source\n");
> + case TYPEC_PWR_SINK:
> + return sprintf(buf, "sink\n");
> + default:
> + return sprintf(buf, "unknown\n");
> + };
> +}
> +static DEVICE_ATTR_RW(current_vconn_role);
> +
> static ssize_t supports_audio_accessory_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -795,6 +872,7 @@ static DEVICE_ATTR_RO(supports_usb_power_delivery);
> static struct attribute *typec_attrs[] = {
> &dev_attr_current_data_role.attr,
> &dev_attr_current_power_role.attr,
> + &dev_attr_current_vconn_role.attr,
> &dev_attr_current_usb_data_role.attr,
> &dev_attr_power_operation_mode.attr,
> &dev_attr_supported_data_roles.attr,
> @@ -862,7 +940,8 @@ static struct device_type typec_port_dev_type = {
> };
>
> struct typec_port *typec_register_port(struct device *dev,
> - struct typec_capability *cap)
> + struct typec_capability *cap,
> + void *driver_data)
> {
> struct typec_port *port;
> int ret;
> @@ -880,6 +959,7 @@ struct typec_port *typec_register_port(struct device *dev,
>
> port->id = id;
> port->cap = cap;
> + port->driver_data = driver_data;
> port->dev.type = &typec_port_dev_type;
> port->dev.class = &typec_class;
> port->dev.parent = dev;
> @@ -888,6 +968,8 @@ struct typec_port *typec_register_port(struct device *dev,
>
> port->fixed_role = port->cap->role;
>
> + typec_init_roles(port);
> +
> ret = device_register(&port->dev);
> if (ret) {
> ida_simple_remove(&typec_index_ida, id);
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 86e5c867800b..d16a38de57ac 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -168,9 +168,9 @@ struct typec_partner {
> * @audio_accessory: Audio Accessory Adapter Mode support
> * @debug_accessory: Debug Accessory Mode support
> * @fix_role: Set a fixed data role for DRP port
> - * @dr_swap: Data Role Swap support
> - * @pr_swap: Power Role Swap support
> - * @vconn_swap: VCONN Swap support
> + * @dr_set: Set Data Role
> + * @pr_set: Set Power Role
> + * @vconn_set: Set VCONN Role
> * @activate_mode: Enter/exit given Alternate Mode
> *
> * Static capabilities of a single USB Type-C port.
> @@ -182,14 +182,14 @@ struct typec_capability {
> unsigned int audio_accessory:1;
> unsigned int debug_accessory:1;
>
> - int (*fix_role)(struct typec_port *,
> + int (*fix_role)(struct typec_port *, void *,
> enum typec_data_role);
>
> - int (*dr_swap)(struct typec_port *);
> - int (*pr_swap)(struct typec_port *);
> - int (*vconn_swap)(struct typec_port *);
> + int (*dr_set)(struct typec_port *, void *, enum typec_usb_role);
> + int (*pr_set)(struct typec_port *, void *, enum typec_pwr_role);
> + int (*vconn_set)(struct typec_port *, void *, enum typec_pwr_role);
>
> - int (*activate_mode)(struct typec_altmode *,
> + int (*activate_mode)(struct typec_altmode *, void *,
> int mode, int activate);
> };
>
> @@ -217,7 +217,8 @@ struct typec_connection {
> };
>
> struct typec_port *typec_register_port(struct device *dev,
> - struct typec_capability *cap);
> + struct typec_capability *cap,
> + void *driver_data);
> void typec_unregister_port(struct typec_port *port);
>
> int typec_connect(struct typec_port *port, struct typec_connection *con);
> @@ -227,4 +228,11 @@ void typec_disconnect(struct typec_port *port);
> struct device *typec_port2dev(struct typec_port *port);
> struct typec_port *typec_dev2port(struct device *dev);
>
> +/* Callbacks from driver */
> +
> +void typec_set_usb_role(struct typec_port *, enum typec_usb_role);
> +void typec_set_pwr_role(struct typec_port *, enum typec_pwr_role);
> +void typec_set_vconn_role(struct typec_port *, enum typec_pwr_role);
> +void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
> +
> #endif /* __LINUX_USB_TYPEC_H */
> --
> 2.5.0
--
heikki