[PATCH 0/6] typec changes

[PATCH 0/6] typec changes

[Heikki Krogerus]

Hi Greg,

Here all the "trivial" changes I have for the USB Type-C class at this
stage. The first patch I already send before [1]. You told me to leave
the checks in, so I did. The rest of the patches in that series were
about USB4, which I'm not including here. I decided to propose them
separately.

The third patch in this series make's it possible to swap the power
role even when USB Power Delivery is not supported since the latest
specification now allows it. The rest of the patches are cleanups.

Let me know if you want me to change anything.

thanks,

[1] https://lore.kernel.org/linux-usb/20191230142611.24921-1-heikki.krogerus@linux.intel.com/

Heikki Krogerus (6):
  usb: typec: Make the attributes read-only when writing is not possible
  usb: typec: Hide the port_type attribute when it's not supported
  usb: typec: Allow power role swapping even without USB PD
  usb: typec: Fix the description of struct typec_capability
  usb: typec: altmode: Remove the notification chain
  usb: typec: mux: Drop support for device name matching

 Documentation/ABI/testing/sysfs-class-typec |  14 +-
 Documentation/driver-api/usb/typec_bus.rst  |  22 +---
 drivers/usb/typec/bus.c                     |  12 +-
 drivers/usb/typec/bus.h                     |   2 -
 drivers/usb/typec/class.c                   | 137 +++++++++-----------
 drivers/usb/typec/mux.c                     |  25 +---
 include/linux/usb/typec.h                   |   2 -
 include/linux/usb/typec_altmode.h           |   7 -
 8 files changed, 77 insertions(+), 144 deletions(-)

-- 
2.25.0

[PATCHv2 1/6] usb: typec: Make the attributes read-only when writing is not possible

[Heikki Krogerus]

This affects the read-writable attribute files. Before this
there was no way for the user to know is changing the value
supported or not.

From now on those attribute files will be made read-only
unless the underlying driver supports changing of the value.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---

Changes since v1:

- Left the checks in that make sure we don't crash the kernel even if
  somebody changes the permissions on the sysfs files.

---
 drivers/usb/typec/class.c | 65 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 7c44e930602f..a451ae181fe9 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -432,7 +432,28 @@ static struct attribute *typec_altmode_attrs[] = {
 	&dev_attr_vdo.attr,
 	NULL
 };
-ATTRIBUTE_GROUPS(typec_altmode);
+
+static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
+					     struct attribute *attr, int n)
+{
+	struct typec_altmode *adev = to_typec_altmode(kobj_to_dev(kobj));
+
+	if (attr == &dev_attr_active.attr)
+		if (!adev->ops || !adev->ops->activate)
+			return 0444;
+
+	return attr->mode;
+}
+
+static struct attribute_group typec_altmode_group = {
+	.is_visible = typec_altmode_attr_is_visible,
+	.attrs = typec_altmode_attrs,
+};
+
+static const struct attribute_group *typec_altmode_groups[] = {
+	&typec_altmode_group,
+	NULL
+};
 
 static int altmode_id_get(struct device *dev)
 {
@@ -1305,7 +1326,47 @@ static struct attribute *typec_attrs[] = {
 	&dev_attr_port_type.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(typec);
+
+static umode_t typec_attr_is_visible(struct kobject *kobj,
+				     struct attribute *attr, int n)
+{
+	struct typec_port *port = to_typec_port(kobj_to_dev(kobj));
+
+	if (attr == &dev_attr_data_role.attr) {
+		if (port->cap->data != TYPEC_PORT_DRD ||
+		    !port->ops || !port->ops->dr_set)
+			return 0444;
+	} else if (attr == &dev_attr_power_role.attr) {
+		if (port->cap->type != TYPEC_PORT_DRP ||
+		    !port->cap->pd_revision ||
+		    !port->ops || !port->ops->pr_set)
+			return 0444;
+	} else if (attr == &dev_attr_vconn_source.attr) {
+		if (!port->cap->pd_revision ||
+		    !port->ops || !port->ops->vconn_set)
+			return 0444;
+	} else if (attr == &dev_attr_preferred_role.attr) {
+		if (port->cap->type != TYPEC_PORT_DRP ||
+		    !port->ops || !port->ops->try_role)
+			return 0444;
+	} else if (attr == &dev_attr_port_type.attr) {
+		if (port->cap->type != TYPEC_PORT_DRP ||
+		    !port->ops || !port->ops->port_type_set)
+			return 0444;
+	}
+
+	return attr->mode;
+}
+
+static struct attribute_group typec_group = {
+	.is_visible = typec_attr_is_visible,
+	.attrs = typec_attrs,
+};
+
+static const struct attribute_group *typec_groups[] = {
+	&typec_group,
+	NULL
+};
 
 static int typec_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-- 
2.25.0

[PATCH 2/6] usb: typec: Hide the port_type attribute when it's not supported

[Heikki Krogerus]

The port_type attribute is special. It is meant to allow
changing the capability of the port in runtime. It is purely
Linux kernel specific feature, i.e. the feature is not
described in any of the USB specifications.

Because of the special nature of this attribute, handling it
differently compared to the other writable attributes, and
hiding it when the underlying port interface (or just the
driver) does not support the feature.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index a451ae181fe9..7fed6855ad59 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1350,8 +1350,9 @@ static umode_t typec_attr_is_visible(struct kobject *kobj,
 		    !port->ops || !port->ops->try_role)
 			return 0444;
 	} else if (attr == &dev_attr_port_type.attr) {
-		if (port->cap->type != TYPEC_PORT_DRP ||
-		    !port->ops || !port->ops->port_type_set)
+		if (!port->ops || !port->ops->port_type_set)
+			return 0;
+		if (port->cap->type != TYPEC_PORT_DRP)
 			return 0444;
 	}
 
-- 
2.25.0

[PATCH 3/6] usb: typec: Allow power role swapping even without USB PD

[Heikki Krogerus]

Even though originally the USB Type-C Specification did not
describe the steps for power role swapping without USB PD
contract in place, it did not actually mean power role swap
without USB PD was not allowed. The USB Type-C Specification
did not clearly separate the data and power roles until in
the release 1.2 which is why there also were no clear steps
for the scenario where only the power role was swapped
without USB PD contract before that.

Since in the latest version of the specification the power
role swap without USB PD is now clearly mentioned as allowed
operation, removing the check that prevented power role swap
without USB PD support.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-class-typec | 14 +++++++-------
 drivers/usb/typec/class.c                   |  6 ------
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index d7647b258c3c..0c2eb26fdc06 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -20,13 +20,13 @@ Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
 Description:
 		The supported power roles. 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. The current role is show in
-		brackets, for example "[source] sink" when in source mode.
+		power role swap 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. The current role is show in brackets, for example
+		"[source] sink" when in source mode.
 
 		Valid values: source, sink
 
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 7fed6855ad59..9cf4f6deb5a6 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1112,11 +1112,6 @@ static ssize_t power_role_store(struct device *dev,
 	struct typec_port *port = to_typec_port(dev);
 	int ret;
 
-	if (!port->cap->pd_revision) {
-		dev_dbg(dev, "USB Power Delivery not supported\n");
-		return -EOPNOTSUPP;
-	}
-
 	if (!port->ops || !port->ops->pr_set) {
 		dev_dbg(dev, "power role swapping not supported\n");
 		return -EOPNOTSUPP;
@@ -1338,7 +1333,6 @@ static umode_t typec_attr_is_visible(struct kobject *kobj,
 			return 0444;
 	} else if (attr == &dev_attr_power_role.attr) {
 		if (port->cap->type != TYPEC_PORT_DRP ||
-		    !port->cap->pd_revision ||
 		    !port->ops || !port->ops->pr_set)
 			return 0444;
 	} else if (attr == &dev_attr_vconn_source.attr) {
-- 
2.25.0

[PATCH 4/6] usb: typec: Fix the description of struct typec_capability

[Heikki Krogerus]

Removing descriptions of the mux and sw members. They are no
longer part of the structure.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/usb/typec.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index c358b3fd05c9..44d28387ced4 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -198,8 +198,6 @@ struct typec_operations {
  * @pd_revision: USB Power Delivery Specification revision if supported
  * @prefer_role: Initial role preference (DRP ports).
  * @accessory: Supported Accessory Modes
- * @sw: Cable plug orientation switch
- * @mux: Multiplexer switch for Alternate/Accessory Modes
  * @fwnode: Optional fwnode of the port
  * @driver_data: Private pointer for driver specific info
  * @ops: Port operations vector
-- 
2.25.0

[PATCH 5/6] usb: typec: altmode: Remove the notification chain

[Heikki Krogerus]

Using the generic notification chain is not reasonable with
the alternate modes because it would require dependencies
between the drivers of the components that need the
notifications, and the typec drivers.

There are no users for the alternate mode notifications, so
removing the chain and the API for it completely.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/driver-api/usb/typec_bus.rst | 22 +------
 drivers/usb/typec/bus.c                    | 12 +---
 drivers/usb/typec/bus.h                    |  2 -
 drivers/usb/typec/class.c                  | 67 +---------------------
 include/linux/usb/typec_altmode.h          |  7 ---
 5 files changed, 3 insertions(+), 107 deletions(-)

diff --git a/Documentation/driver-api/usb/typec_bus.rst b/Documentation/driver-api/usb/typec_bus.rst
index f47a69bff498..03dfa9c018b7 100644
--- a/Documentation/driver-api/usb/typec_bus.rst
+++ b/Documentation/driver-api/usb/typec_bus.rst
@@ -53,9 +53,7 @@ in need to reconfigure the pins on the connector, the alternate mode driver
 needs to notify the bus using :c:func:`typec_altmode_notify()`. The driver
 passes the negotiated SVID specific pin configuration value to the function as
 parameter. The bus driver will then configure the mux behind the connector using
-that value as the state value for the mux, and also call blocking notification
-chain to notify the external drivers about the state of the connector that need
-to know it.
+that value as the state value for the mux.
 
 NOTE: The SVID specific pin configuration values must always start from
 ``TYPEC_STATE_MODAL``. USB Type-C specification defines two default states for
@@ -80,19 +78,6 @@ Helper macro ``TYPEC_MODAL_STATE()`` can also be used::
 #define ALTMODEX_CONF_A = TYPEC_MODAL_STATE(0);
 #define ALTMODEX_CONF_B = TYPEC_MODAL_STATE(1);
 
-Notification chain
-~~~~~~~~~~~~~~~~~~
-
-The drivers for the components that the alternate modes are designed for need to
-get details regarding the results of the negotiation with the partner, and the
-pin configuration of the connector. In case of DisplayPort alternate mode for
-example, the GPU drivers will need to know those details. In case of
-Thunderbolt alternate mode, the thunderbolt drivers will need to know them, and
-so on.
-
-The notification chain is designed for this purpose. The drivers can register
-notifiers with :c:func:`typec_altmode_register_notifier()`.
-
 Cable plug alternate modes
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -129,8 +114,3 @@ Cable Plug operations
 
 .. kernel-doc:: drivers/usb/typec/bus.c
    :functions: typec_altmode_get_plug typec_altmode_put_plug
-
-Notifications
-~~~~~~~~~~~~~
-.. kernel-doc:: drivers/usb/typec/class.c
-   :functions: typec_altmode_register_notifier typec_altmode_unregister_notifier
diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
index 2e45eb479386..c823122f9cb7 100644
--- a/drivers/usb/typec/bus.c
+++ b/drivers/usb/typec/bus.c
@@ -30,17 +30,10 @@ static int typec_altmode_set_state(struct typec_altmode *adev,
 {
 	bool is_port = is_typec_port(adev->dev.parent);
 	struct altmode *port_altmode;
-	int ret;
 
 	port_altmode = is_port ? to_altmode(adev) : to_altmode(adev)->partner;
 
-	ret = typec_altmode_set_mux(port_altmode, conf, data);
-	if (ret)
-		return ret;
-
-	blocking_notifier_call_chain(&port_altmode->nh, conf, NULL);
-
-	return 0;
+	return typec_altmode_set_mux(port_altmode, conf, data);
 }
 
 /* -------------------------------------------------------------------------- */
@@ -82,9 +75,6 @@ int typec_altmode_notify(struct typec_altmode *adev,
 	if (ret)
 		return ret;
 
-	blocking_notifier_call_chain(is_port ? &altmode->nh : &partner->nh,
-				     conf, data);
-
 	if (partner->adev.ops && partner->adev.ops->notify)
 		return partner->adev.ops->notify(&partner->adev, conf, data);
 
diff --git a/drivers/usb/typec/bus.h b/drivers/usb/typec/bus.h
index 0c9661c96473..8ba8112d2740 100644
--- a/drivers/usb/typec/bus.h
+++ b/drivers/usb/typec/bus.h
@@ -22,8 +22,6 @@ struct altmode {
 
 	struct altmode			*partner;
 	struct altmode			*plug[2];
-
-	struct blocking_notifier_head	nh;
 };
 
 #define to_altmode(d) container_of(d, struct altmode, adev)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 9cf4f6deb5a6..12be5bb6d32c 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -206,69 +206,6 @@ static void typec_altmode_put_partner(struct altmode *altmode)
 	put_device(&adev->dev);
 }
 
-static void *typec_port_match(struct device_connection *con, int ep, void *data)
-{
-	struct device *dev;
-
-	/*
-	 * FIXME: Check does the fwnode supports the requested SVID. If it does
-	 * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
-	 */
-	if (con->fwnode)
-		return class_find_device_by_fwnode(typec_class, con->fwnode);
-
-	dev = class_find_device_by_name(typec_class, con->endpoint[ep]);
-
-	return dev ? dev : ERR_PTR(-EPROBE_DEFER);
-}
-
-struct typec_altmode *
-typec_altmode_register_notifier(struct device *dev, u16 svid, u8 mode,
-				struct notifier_block *nb)
-{
-	struct typec_device_id id = { svid, mode, };
-	struct device *altmode_dev;
-	struct device *port_dev;
-	struct altmode *altmode;
-	int ret;
-
-	/* Find the port linked to the caller */
-	port_dev = device_connection_find_match(dev, NULL, NULL,
-						typec_port_match);
-	if (IS_ERR_OR_NULL(port_dev))
-		return port_dev ? ERR_CAST(port_dev) : ERR_PTR(-ENODEV);
-
-	/* Find the altmode with matching svid */
-	altmode_dev = device_find_child(port_dev, &id, altmode_match);
-
-	put_device(port_dev);
-
-	if (!altmode_dev)
-		return ERR_PTR(-ENODEV);
-
-	altmode = to_altmode(to_typec_altmode(altmode_dev));
-
-	/* Register notifier */
-	ret = blocking_notifier_chain_register(&altmode->nh, nb);
-	if (ret) {
-		put_device(altmode_dev);
-		return ERR_PTR(ret);
-	}
-
-	return &altmode->adev;
-}
-EXPORT_SYMBOL_GPL(typec_altmode_register_notifier);
-
-void typec_altmode_unregister_notifier(struct typec_altmode *adev,
-				       struct notifier_block *nb)
-{
-	struct altmode *altmode = to_altmode(adev);
-
-	blocking_notifier_chain_unregister(&altmode->nh, nb);
-	put_device(&adev->dev);
-}
-EXPORT_SYMBOL_GPL(typec_altmode_unregister_notifier);
-
 /**
  * typec_altmode_update_active - Report Enter/Exit mode
  * @adev: Handle to the alternate mode
@@ -538,9 +475,7 @@ typec_register_altmode(struct device *parent,
 	dev_set_name(&alt->adev.dev, "%s.%u", dev_name(parent), id);
 
 	/* Link partners and plugs with the ports */
-	if (is_port)
-		BLOCKING_INIT_NOTIFIER_HEAD(&alt->nh);
-	else
+	if (!is_port)
 		typec_altmode_set_partner(alt);
 
 	/* The partners are bind to drivers */
diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
index 923ff3af0628..d834e236c6df 100644
--- a/include/linux/usb/typec_altmode.h
+++ b/include/linux/usb/typec_altmode.h
@@ -126,13 +126,6 @@ void typec_altmode_put_plug(struct typec_altmode *plug);
 struct typec_altmode *typec_match_altmode(struct typec_altmode **altmodes,
 					  size_t n, u16 svid, u8 mode);
 
-struct typec_altmode *
-typec_altmode_register_notifier(struct device *dev, u16 svid, u8 mode,
-				struct notifier_block *nb);
-
-void typec_altmode_unregister_notifier(struct typec_altmode *adev,
-				       struct notifier_block *nb);
-
 /**
  * typec_altmode_get_orientation - Get cable plug orientation
  * altmode: Handle to the alternate mode
-- 
2.25.0

[PATCH 6/6] usb: typec: mux: Drop support for device name matching

[Heikki Krogerus]

There are no more users for the old device connection
descriptions that used device names.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/mux.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 5baf0f416c73..b952fa2fff39 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -17,11 +17,6 @@
 
 #include "bus.h"
 
-static int name_match(struct device *dev, const void *name)
-{
-	return !strcmp((const char *)name, dev_name(dev));
-}
-
 static bool dev_name_ends_with(struct device *dev, const char *suffix)
 {
 	const char *name = dev_name(dev);
@@ -44,16 +39,11 @@ static void *typec_switch_match(struct device_connection *con, int ep,
 {
 	struct device *dev;
 
-	if (con->fwnode) {
-		if (con->id && !fwnode_property_present(con->fwnode, con->id))
-			return NULL;
+	if (con->id && !fwnode_property_present(con->fwnode, con->id))
+		return NULL;
 
-		dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
-					switch_fwnode_match);
-	} else {
-		dev = class_find_device(&typec_mux_class, NULL,
-					con->endpoint[ep], name_match);
-	}
+	dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
+				switch_fwnode_match);
 
 	return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
@@ -191,13 +181,6 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 	u16 *val;
 	int i;
 
-	if (!con->fwnode) {
-		dev = class_find_device(&typec_mux_class, NULL,
-					con->endpoint[ep], name_match);
-
-		return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
-	}
-
 	/*
 	 * Check has the identifier already been "consumed". If it
 	 * has, no need to do any extra connection identification.
-- 
2.25.0

Re: [PATCH 0/6] typec changes

[Greg Kroah-Hartman]

On Tue, Feb 11, 2020 at 02:25:25PM +0300, Heikki Krogerus wrote:
> Hi Greg,
> 
> Here all the "trivial" changes I have for the USB Type-C class at this
> stage. The first patch I already send before [1]. You told me to leave
> the checks in, so I did. The rest of the patches in that series were
> about USB4, which I'm not including here. I decided to propose them
> separately.
> 
> The third patch in this series make's it possible to swap the power
> role even when USB Power Delivery is not supported since the latest
> specification now allows it. The rest of the patches are cleanups.
> 
> Let me know if you want me to change anything.

Are these fixes for 5.6-rc2 or for new stuff for 5.7-rc1?

thanks,

greg k-h

Re: [PATCH 0/6] typec changes

[Heikki Krogerus]

On Tue, Feb 11, 2020 at 04:25:26AM -0800, Greg Kroah-Hartman wrote:
> On Tue, Feb 11, 2020 at 02:25:25PM +0300, Heikki Krogerus wrote:
> > Hi Greg,
> > 
> > Here all the "trivial" changes I have for the USB Type-C class at this
> > stage. The first patch I already send before [1]. You told me to leave
> > the checks in, so I did. The rest of the patches in that series were
> > about USB4, which I'm not including here. I decided to propose them
> > separately.
> > 
> > The third patch in this series make's it possible to swap the power
> > role even when USB Power Delivery is not supported since the latest
> > specification now allows it. The rest of the patches are cleanups.
> > 
> > Let me know if you want me to change anything.
> 
> Are these fixes for 5.6-rc2 or for new stuff for 5.7-rc1?

For 5.7-rc1. Sorry.

thanks,

-- 
heikki