Hi,
This removes the need for the drivers to always separately evaluate
the _PLD. With the USB Type-C connector and USB port mapping this
allows us to start using the component framework and remove the custom
APIs.
So far the only users of the _PLD information have been the USB
drivers, but it seems it will be used also at least in some camera
drivers later. These nevertheless touch mostly USB drivers.
Rafael, is it still OK if Greg takes these?
Prashant, can you test these?
thanks,
Heikki Krogerus (5):
acpi: Store the Physical Location of Device (_PLD) information
usb: Use the cached ACPI _PLD entry
usb: Link the ports to the connectors they are attached to
usb: typec: port-mapper: Convert to the component framework
usb: Remove usb_for_each_port()
Documentation/ABI/testing/sysfs-bus-usb | 9 +
drivers/acpi/scan.c | 79 +++++++
drivers/usb/core/port.c | 32 +++
drivers/usb/core/usb-acpi.c | 17 +-
drivers/usb/core/usb.c | 46 ----
drivers/usb/typec/Makefile | 3 +-
drivers/usb/typec/class.c | 2 -
drivers/usb/typec/class.h | 10 +-
drivers/usb/typec/port-mapper.c | 280 +++---------------------
include/acpi/acpi_bus.h | 14 ++
include/linux/usb.h | 9 -
include/linux/usb/typec.h | 12 -
12 files changed, 184 insertions(+), 329 deletions(-)
--
2.33.0
This will remove the need for the drivers to always evaluate
the _PLD separately.
Because the _PLD may be shared between devices - for example
the USB2 and USB3 ports that share the same connector have
always the same _PLD - every unique _PLD that is detected is
registered as a single entry and stored in a dedicated list.
Then each of those entries will hold a list of devices that
share the location - have identical _PLD.
The location entry can be acquired with a new function
acpi_device_get_location(). The location structure that the
function returns contrains the _PLD of the device and the
list the other devices that share it.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/acpi/scan.c | 79 +++++++++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 14 ++++++++
2 files changed, 93 insertions(+)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5991dddbc9ceb..9946ca4451ebc 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -19,6 +19,7 @@
#include <linux/dma-map-ops.h>
#include <linux/platform_data/x86/apple.h>
#include <linux/pgtable.h>
+#include <linux/crc32.h>
#include "internal.h"
@@ -42,6 +43,8 @@ static LIST_HEAD(acpi_scan_handlers_list);
DEFINE_MUTEX(acpi_device_lock);
LIST_HEAD(acpi_wakeup_device_list);
static DEFINE_MUTEX(acpi_hp_context_lock);
+static LIST_HEAD(acpi_location_list);
+static DEFINE_MUTEX(acpi_location_lock);
/*
* The UART device described by the SPCR table is the only object which needs
@@ -485,6 +488,7 @@ static void acpi_device_del(struct acpi_device *device)
break;
}
+ list_del(&device->location_list);
list_del(&device->wakeup_list);
mutex_unlock(&acpi_device_lock);
@@ -654,6 +658,78 @@ static int acpi_tie_acpi_dev(struct acpi_device *adev)
return 0;
}
+static void acpi_store_device_location(struct acpi_device *adev)
+{
+ struct acpi_device_location *location;
+ struct acpi_pld_info *pld;
+ acpi_status status;
+ u32 crc;
+
+ status = acpi_get_physical_device_location(adev->handle, &pld);
+ if (ACPI_FAILURE(status))
+ return;
+
+ crc = crc32(~0, pld, sizeof(*pld));
+
+ mutex_lock(&acpi_location_lock);
+
+ list_for_each_entry(location, &acpi_location_list, node) {
+ if (location->pld_crc == crc) {
+ ACPI_FREE(pld);
+ goto out_add_to_location;
+ }
+ }
+
+ /* The location does not exist yet so creating it. */
+
+ location = kzalloc(sizeof(*location), GFP_KERNEL);
+ if (!location) {
+ acpi_handle_err(adev->handle, "Unable to store location\n");
+ goto err_unlock;
+ }
+
+ list_add_tail(&location->node, &acpi_location_list);
+ INIT_LIST_HEAD(&location->devices);
+ location->pld = pld;
+ location->pld_crc = crc;
+
+out_add_to_location:
+ list_add_tail(&adev->location_list, &location->devices);
+
+err_unlock:
+ mutex_unlock(&acpi_location_lock);
+}
+
+/**
+ * acpi_device_get_location - Get the device location
+ * @adev: ACPI device handle
+ *
+ * Returns the location of @adev when it's known. The location is known for all
+ * ACPI devices that have _PLD (Physical Location of Device). When the location
+ * is not known, the function returns NULL.
+ */
+struct acpi_device_location *acpi_device_get_location(struct acpi_device *adev)
+{
+ struct acpi_device_location *location;
+ struct list_head *tmp;
+
+ mutex_lock(&acpi_location_lock);
+
+ list_for_each_entry(location, &acpi_location_list, node) {
+ list_for_each(tmp, &location->devices) {
+ if (tmp == &adev->location_list)
+ goto out_unlock;
+ }
+ }
+ location = NULL;
+
+out_unlock:
+ mutex_unlock(&acpi_location_lock);
+
+ return location;
+}
+EXPORT_SYMBOL_GPL(acpi_device_get_location);
+
static int __acpi_device_add(struct acpi_device *device,
void (*release)(struct device *))
{
@@ -670,6 +746,7 @@ static int __acpi_device_add(struct acpi_device *device,
INIT_LIST_HEAD(&device->wakeup_list);
INIT_LIST_HEAD(&device->physical_node_list);
INIT_LIST_HEAD(&device->del_list);
+ INIT_LIST_HEAD(&device->location_list);
mutex_init(&device->physical_node_lock);
mutex_lock(&acpi_device_lock);
@@ -712,6 +789,8 @@ static int __acpi_device_add(struct acpi_device *device,
if (device->wakeup.flags.valid)
list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
+ acpi_store_device_location(device);
+
mutex_unlock(&acpi_device_lock);
if (device->parent)
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index d6fe27b695c3d..1a4af747198a4 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -354,6 +354,13 @@ struct acpi_device_data {
struct list_head subnodes;
};
+struct acpi_device_location {
+ u32 pld_crc;
+ struct acpi_pld_info *pld;
+ struct list_head node;
+ struct list_head devices;
+};
+
struct acpi_gpio_mapping;
/* Device */
@@ -366,6 +373,7 @@ struct acpi_device {
struct list_head node;
struct list_head wakeup_list;
struct list_head del_list;
+ struct list_head location_list;
struct acpi_device_status status;
struct acpi_device_flags flags;
struct acpi_device_pnp pnp;
@@ -731,11 +739,17 @@ static inline void acpi_bus_put_acpi_device(struct acpi_device *adev)
{
acpi_dev_put(adev);
}
+
+struct acpi_device_location *acpi_device_get_location(struct acpi_device *adev);
#else /* CONFIG_ACPI */
static inline int register_acpi_bus_type(void *bus) { return 0; }
static inline int unregister_acpi_bus_type(void *bus) { return 0; }
+static inline struct acpi_device_location *acpi_device_get_location(struct acpi_device *adev)
+{
+ return NULL;
+}
#endif /* CONFIG_ACPI */
#endif /*__ACPI_BUS_H__*/
--
2.33.0
The _PLD is now stored, so there is no need to separately
evaluate it.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/core/usb-acpi.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 50b2fc7fcc0e3..3b21c15be548f 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -176,22 +176,19 @@ usb_acpi_get_companion_for_port(struct usb_port *port_dev)
static struct acpi_device *
usb_acpi_find_companion_for_port(struct usb_port *port_dev)
{
+ struct acpi_device_location *location;
struct acpi_device *adev;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
adev = usb_acpi_get_companion_for_port(port_dev);
if (!adev)
return NULL;
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_SUCCESS(status) && pld) {
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
+ location = acpi_device_get_location(adev);
+ if (location) {
+ port_dev->location = USB_ACPI_LOCATION_VALID |
+ location->pld->group_token << 8 |
+ location->pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(adev->handle, location->pld);
}
return adev;
--
2.33.0
Creating link to the USB Type-C connector for every new port
that is added when possible.
Signed-off-by: Heikki Krogerus <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++
drivers/usb/core/port.c | 32 +++++++++++++++++++++++++
2 files changed, 41 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 2ebe5708b4bc0..7efe31ed3a25c 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -244,6 +244,15 @@ Description:
is permitted, "u2" if only u2 is permitted, "u1_u2" if both u1 and
u2 are permitted.
+What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/connector
+Date: December 2021
+Contact: Heikki Krogerus <[email protected]>
+Description:
+ Link to the USB Type-C connector when available. This link is
+ only created when USB Type-C Connector Class is enabled, and
+ only if the system firmware is capable of describing the
+ connection between a port and its connector.
+
What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
Date: May 2013
Contact: Mathias Nyman <[email protected]>
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index dfcca9c876c73..c2bbf97a79bec 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -9,6 +9,7 @@
#include <linux/slab.h>
#include <linux/pm_qos.h>
+#include <linux/component.h>
#include "hub.h"
@@ -528,6 +529,32 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
link_peers_report(port_dev, peer);
}
+static int connector_bind(struct device *dev, struct device *connector, void *data)
+{
+ int ret;
+
+ ret = sysfs_create_link(&dev->kobj, &connector->kobj, "connector");
+ if (ret)
+ return ret;
+
+ ret = sysfs_create_link(&connector->kobj, &dev->kobj, dev_name(dev));
+ if (ret)
+ sysfs_remove_link(&dev->kobj, "connector");
+
+ return ret;
+}
+
+static void connector_unbind(struct device *dev, struct device *connector, void *data)
+{
+ sysfs_remove_link(&connector->kobj, dev_name(dev));
+ sysfs_remove_link(&dev->kobj, "connector");
+}
+
+static const struct component_ops connector_ops = {
+ .bind = connector_bind,
+ .unbind = connector_unbind,
+};
+
int usb_hub_create_port_device(struct usb_hub *hub, int port1)
{
struct usb_port *port_dev;
@@ -577,6 +604,10 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
find_and_link_peer(hub, port1);
+ retval = component_add(&port_dev->dev, &connector_ops);
+ if (retval)
+ dev_warn(&port_dev->dev, "failed to add component\n");
+
/*
* Enable runtime pm and hold a refernce that hub_configure()
* will drop once the PM_QOS_NO_POWER_OFF flag state has been set
@@ -619,5 +650,6 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
peer = port_dev->peer;
if (peer)
unlink_peers(port_dev, peer);
+ component_del(&port_dev->dev, &connector_ops);
device_unregister(&port_dev->dev);
}
--
2.33.0
Instead of trying to keep track of the connections to the
USB Type-C connectors separately, letting the component
framework take care of that.
From now on every USB Type-C connector will register itself
as "aggregate" - component master - and anything that can be
connected to it can then simply register itself as a generic
component.
The matching of the components and the connector shall rely
on ACPI _PLD initially. Before registering itself as the
aggregate, the connector will check the list of other
devices that share the same ACPI _PLD with it, and add a
component match entry for each one of them. Because only
ACPI is supported for now, the driver shall only be build
when ACPI is supported.
This removes the need for the custom API that the driver
exposed. The components and the connector can therefore
exist completely independently of each other. The order in
which they are registered, as well as are they modules or
not, is now irrelevant.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/Makefile | 3 +-
drivers/usb/typec/class.c | 2 -
drivers/usb/typec/class.h | 10 +-
drivers/usb/typec/port-mapper.c | 280 ++++----------------------------
include/linux/usb/typec.h | 12 --
5 files changed, 43 insertions(+), 264 deletions(-)
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index a0adb8947a301..57870a2bd7873 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_TYPEC) += typec.o
-typec-y := class.o mux.o bus.o port-mapper.o
+typec-y := class.o mux.o bus.o
+typec-$(CONFIG_ACPI) += port-mapper.o
obj-$(CONFIG_TYPEC) += altmodes/
obj-$(CONFIG_TYPEC_TCPM) += tcpm/
obj-$(CONFIG_TYPEC_UCSI) += ucsi/
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index aeef453aa6585..45a6f0c807cb5 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -2039,8 +2039,6 @@ struct typec_port *typec_register_port(struct device *parent,
ida_init(&port->mode_ids);
mutex_init(&port->port_type_lock);
- mutex_init(&port->port_list_lock);
- INIT_LIST_HEAD(&port->port_list);
port->id = id;
port->ops = cap->ops;
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index aef03eb7e1523..0f1bd6d19d67e 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -54,11 +54,6 @@ struct typec_port {
const struct typec_capability *cap;
const struct typec_operations *ops;
-
- struct list_head port_list;
- struct mutex port_list_lock; /* Port list lock */
-
- void *pld;
};
#define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
@@ -79,7 +74,12 @@ extern const struct device_type typec_port_dev_type;
extern struct class typec_mux_class;
extern struct class typec_class;
+#if defined(CONFIG_ACPI)
int typec_link_ports(struct typec_port *connector);
void typec_unlink_ports(struct typec_port *connector);
+#else
+static inline int typec_link_ports(struct typec_port *connector) { return 0; }
+static inline void typec_unlink_ports(struct typec_port *connector) { }
+#endif
#endif /* __USB_TYPEC_CLASS__ */
diff --git a/drivers/usb/typec/port-mapper.c b/drivers/usb/typec/port-mapper.c
index 9b0991bdf391a..03dce12bf11ff 100644
--- a/drivers/usb/typec/port-mapper.c
+++ b/drivers/usb/typec/port-mapper.c
@@ -7,273 +7,65 @@
*/
#include <linux/acpi.h>
-#include <linux/usb.h>
-#include <linux/usb/typec.h>
+#include <linux/component.h>
#include "class.h"
-struct port_node {
- struct list_head list;
- struct device *dev;
- void *pld;
-};
-
-static int acpi_pld_match(const struct acpi_pld_info *pld1,
- const struct acpi_pld_info *pld2)
-{
- if (!pld1 || !pld2)
- return 0;
-
- /*
- * To speed things up, first checking only the group_position. It seems
- * to often have the first unique value in the _PLD.
- */
- if (pld1->group_position == pld2->group_position)
- return !memcmp(pld1, pld2, sizeof(struct acpi_pld_info));
-
- return 0;
-}
-
-static void *get_pld(struct device *dev)
-{
-#ifdef CONFIG_ACPI
- struct acpi_pld_info *pld;
- acpi_status status;
-
- if (!has_acpi_companion(dev))
- return NULL;
-
- status = acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld);
- if (ACPI_FAILURE(status))
- return NULL;
-
- return pld;
-#else
- return NULL;
-#endif
-}
-
-static void free_pld(void *pld)
-{
-#ifdef CONFIG_ACPI
- ACPI_FREE(pld);
-#endif
-}
-
-static int __link_port(struct typec_port *con, struct port_node *node)
-{
- int ret;
-
- ret = sysfs_create_link(&node->dev->kobj, &con->dev.kobj, "connector");
- if (ret)
- return ret;
-
- ret = sysfs_create_link(&con->dev.kobj, &node->dev->kobj,
- dev_name(node->dev));
- if (ret) {
- sysfs_remove_link(&node->dev->kobj, "connector");
- return ret;
- }
-
- list_add_tail(&node->list, &con->port_list);
-
- return 0;
-}
-
-static int link_port(struct typec_port *con, struct port_node *node)
-{
- int ret;
-
- mutex_lock(&con->port_list_lock);
- ret = __link_port(con, node);
- mutex_unlock(&con->port_list_lock);
-
- return ret;
-}
-
-static void __unlink_port(struct typec_port *con, struct port_node *node)
+static int typec_aggregate_bind(struct device *dev)
{
- sysfs_remove_link(&con->dev.kobj, dev_name(node->dev));
- sysfs_remove_link(&node->dev->kobj, "connector");
- list_del(&node->list);
+ return component_bind_all(dev, NULL);
}
-static void unlink_port(struct typec_port *con, struct port_node *node)
+static void typec_aggregate_unbind(struct device *dev)
{
- mutex_lock(&con->port_list_lock);
- __unlink_port(con, node);
- mutex_unlock(&con->port_list_lock);
+ component_unbind_all(dev, NULL);
}
-static struct port_node *create_port_node(struct device *port)
-{
- struct port_node *node;
-
- node = kzalloc(sizeof(*node), GFP_KERNEL);
- if (!node)
- return ERR_PTR(-ENOMEM);
-
- node->dev = get_device(port);
- node->pld = get_pld(port);
-
- return node;
-}
-
-static void remove_port_node(struct port_node *node)
-{
- put_device(node->dev);
- free_pld(node->pld);
- kfree(node);
-}
-
-static int connector_match(struct device *dev, const void *data)
-{
- const struct port_node *node = data;
-
- if (!is_typec_port(dev))
- return 0;
-
- return acpi_pld_match(to_typec_port(dev)->pld, node->pld);
-}
+static const struct component_master_ops typec_aggregate_ops = {
+ .bind = typec_aggregate_bind,
+ .unbind = typec_aggregate_unbind,
+};
-static struct device *find_connector(struct port_node *node)
+static int typec_port_compare(struct device *dev, void *fwnode)
{
- if (!node->pld)
- return NULL;
-
- return class_find_device(&typec_class, NULL, node, connector_match);
+ return dev_fwnode(dev) == fwnode;
}
-/**
- * typec_link_port - Link a port to its connector
- * @port: The port device
- *
- * Find the connector of @port and create symlink named "connector" for it.
- * Returns 0 on success, or errno in case of a failure.
- *
- * NOTE. The function increments the reference count of @port on success.
- */
-int typec_link_port(struct device *port)
-{
- struct device *connector;
- struct port_node *node;
- int ret;
-
- node = create_port_node(port);
- if (IS_ERR(node))
- return PTR_ERR(node);
-
- connector = find_connector(node);
- if (!connector) {
- ret = 0;
- goto remove_node;
- }
-
- ret = link_port(to_typec_port(connector), node);
- if (ret)
- goto put_connector;
-
- return 0;
-
-put_connector:
- put_device(connector);
-remove_node:
- remove_port_node(node);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(typec_link_port);
-
-static int port_match_and_unlink(struct device *connector, void *port)
+int typec_link_ports(struct typec_port *con)
{
- struct port_node *node;
- struct port_node *tmp;
- int ret = 0;
+ struct acpi_device_location *location;
+ struct component_match *match = NULL;
+ struct acpi_device *adev;
- if (!is_typec_port(connector))
+ location = acpi_device_get_location(ACPI_COMPANION(&con->dev));
+ if (!location)
return 0;
- mutex_lock(&to_typec_port(connector)->port_list_lock);
- list_for_each_entry_safe(node, tmp, &to_typec_port(connector)->port_list, list) {
- ret = node->dev == port;
- if (ret) {
- unlink_port(to_typec_port(connector), node);
- remove_port_node(node);
- put_device(connector);
- break;
- }
- }
- mutex_unlock(&to_typec_port(connector)->port_list_lock);
+ /* Component match for every device that shares the same _PLD. */
+ list_for_each_entry(adev, &location->devices, location_list) {
+ if (adev == ACPI_COMPANION(&con->dev))
+ continue;
- return ret;
-}
-
-/**
- * typec_unlink_port - Unlink port from its connector
- * @port: The port device
- *
- * Removes the symlink "connector" and decrements the reference count of @port.
- */
-void typec_unlink_port(struct device *port)
-{
- class_for_each_device(&typec_class, NULL, port, port_match_and_unlink);
-}
-EXPORT_SYMBOL_GPL(typec_unlink_port);
-
-static int each_port(struct device *port, void *connector)
-{
- struct port_node *node;
- int ret;
-
- node = create_port_node(port);
- if (IS_ERR(node))
- return PTR_ERR(node);
-
- if (!connector_match(connector, node)) {
- remove_port_node(node);
- return 0;
+ component_match_add(&con->dev, &match, typec_port_compare,
+ acpi_fwnode_handle(adev));
}
- ret = link_port(to_typec_port(connector), node);
- if (ret) {
- remove_port_node(node->pld);
- return ret;
- }
-
- get_device(connector);
-
- return 0;
-}
-
-int typec_link_ports(struct typec_port *con)
-{
- int ret = 0;
-
- con->pld = get_pld(&con->dev);
- if (!con->pld)
- return 0;
-
- ret = usb_for_each_port(&con->dev, each_port);
- if (ret)
- typec_unlink_ports(con);
+ /*
+ * REVISIT: Now each connector can have only a single component master.
+ * So far only the USB ports connected to the USB Type-C connector share
+ * the _PLD with it, but if there one day is something else (like maybe
+ * the DisplayPort ACPI device object) that also shares the _PLD with
+ * the connector, every one of those needs to have its own component
+ * master, because each different type of component needs to be bind to
+ * the connector independently of the other components. That requires
+ * improvements to the component framework. Right now you can only have
+ * one master per device.
+ */
- return ret;
+ return component_master_add_with_match(&con->dev, &typec_aggregate_ops, match);
}
void typec_unlink_ports(struct typec_port *con)
{
- struct port_node *node;
- struct port_node *tmp;
-
- mutex_lock(&con->port_list_lock);
-
- list_for_each_entry_safe(node, tmp, &con->port_list, list) {
- __unlink_port(con, node);
- remove_port_node(node);
- put_device(&con->dev);
- }
-
- mutex_unlock(&con->port_list_lock);
-
- free_pld(con->pld);
+ component_master_del(&con->dev, &typec_aggregate_ops);
}
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index e2e44bb1dad85..7ba45a97eeae3 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -305,16 +305,4 @@ void typec_partner_set_svdm_version(struct typec_partner *partner,
enum usb_pd_svdm_ver svdm_version);
int typec_get_negotiated_svdm_version(struct typec_port *port);
-#if IS_REACHABLE(CONFIG_TYPEC)
-int typec_link_port(struct device *port);
-void typec_unlink_port(struct device *port);
-#else
-static inline int typec_link_port(struct device *port)
-{
- return 0;
-}
-
-static inline void typec_unlink_port(struct device *port) { }
-#endif
-
#endif /* __LINUX_USB_TYPEC_H */
--
2.33.0
There are no more users for the function.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/core/usb.c | 46 ------------------------------------------
include/linux/usb.h | 9 ---------
2 files changed, 55 deletions(-)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 62368c4ed37af..2ce3667ec6fae 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -398,52 +398,6 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
}
EXPORT_SYMBOL_GPL(usb_for_each_dev);
-struct each_hub_arg {
- void *data;
- int (*fn)(struct device *, void *);
-};
-
-static int __each_hub(struct usb_device *hdev, void *data)
-{
- struct each_hub_arg *arg = (struct each_hub_arg *)data;
- struct usb_hub *hub;
- int ret = 0;
- int i;
-
- hub = usb_hub_to_struct_hub(hdev);
- if (!hub)
- return 0;
-
- mutex_lock(&usb_port_peer_mutex);
-
- for (i = 0; i < hdev->maxchild; i++) {
- ret = arg->fn(&hub->ports[i]->dev, arg->data);
- if (ret)
- break;
- }
-
- mutex_unlock(&usb_port_peer_mutex);
-
- return ret;
-}
-
-/**
- * usb_for_each_port - interate over all USB ports in the system
- * @data: data pointer that will be handed to the callback function
- * @fn: callback function to be called for each USB port
- *
- * Iterate over all USB ports and call @fn for each, passing it @data. If it
- * returns anything other than 0, we break the iteration prematurely and return
- * that value.
- */
-int usb_for_each_port(void *data, int (*fn)(struct device *, void *))
-{
- struct each_hub_arg arg = {data, fn};
-
- return usb_for_each_dev(&arg, __each_hub);
-}
-EXPORT_SYMBOL_GPL(usb_for_each_port);
-
/**
* usb_release_dev - free a usb device structure when all users of it are finished.
* @dev: device that's been disconnected
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7ccaa76a9a968..200b7b79acb56 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -875,15 +875,6 @@ extern struct usb_host_interface *usb_find_alt_setting(
unsigned int iface_num,
unsigned int alt_num);
-#if IS_REACHABLE(CONFIG_USB)
-int usb_for_each_port(void *data, int (*fn)(struct device *, void *));
-#else
-static inline int usb_for_each_port(void *data, int (*fn)(struct device *, void *))
-{
- return 0;
-}
-#endif
-
/* port claiming functions */
int usb_hub_claim_port(struct usb_device *hdev, unsigned port1,
struct usb_dev_state *owner);
--
2.33.0
On Tue, Dec 07, 2021 at 05:37:56PM +0300, Heikki Krogerus wrote:
> Instead of trying to keep track of the connections to the
> USB Type-C connectors separately, letting the component
> framework take care of that.
>
> From now on every USB Type-C connector will register itself
> as "aggregate" - component master - and anything that can be
> connected to it can then simply register itself as a generic
> component.
>
> The matching of the components and the connector shall rely
> on ACPI _PLD initially. Before registering itself as the
> aggregate, the connector will check the list of other
> devices that share the same ACPI _PLD with it, and add a
> component match entry for each one of them. Because only
> ACPI is supported for now, the driver shall only be build
> when ACPI is supported.
>
> This removes the need for the custom API that the driver
> exposed. The components and the connector can therefore
> exist completely independently of each other. The order in
> which they are registered, as well as are they modules or
> not, is now irrelevant.
...
> +static int typec_port_compare(struct device *dev, void *fwnode)
> {
> + return dev_fwnode(dev) == fwnode;
> }
NIH device_match_fwnode()
...
> + /* Component match for every device that shares the same _PLD. */
> + list_for_each_entry(adev, &location->devices, location_list) {
> + if (adev == ACPI_COMPANION(&con->dev))
device_match_acpi_dev()
> + continue;
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 07, 2021 at 05:46:06PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 07, 2021 at 05:37:56PM +0300, Heikki Krogerus wrote:
> > Instead of trying to keep track of the connections to the
> > USB Type-C connectors separately, letting the component
> > framework take care of that.
> >
> > From now on every USB Type-C connector will register itself
> > as "aggregate" - component master - and anything that can be
> > connected to it can then simply register itself as a generic
> > component.
> >
> > The matching of the components and the connector shall rely
> > on ACPI _PLD initially. Before registering itself as the
> > aggregate, the connector will check the list of other
> > devices that share the same ACPI _PLD with it, and add a
> > component match entry for each one of them. Because only
> > ACPI is supported for now, the driver shall only be build
> > when ACPI is supported.
> >
> > This removes the need for the custom API that the driver
> > exposed. The components and the connector can therefore
> > exist completely independently of each other. The order in
> > which they are registered, as well as are they modules or
> > not, is now irrelevant.
>
> ...
>
> > +static int typec_port_compare(struct device *dev, void *fwnode)
> > {
>
> > + return dev_fwnode(dev) == fwnode;
> > }
>
> NIH device_match_fwnode()
>
> ...
>
> > + /* Component match for every device that shares the same _PLD. */
> > + list_for_each_entry(adev, &location->devices, location_list) {
>
> > + if (adev == ACPI_COMPANION(&con->dev))
>
> device_match_acpi_dev()
Ah, both look like great helpers. I'll this in v2. Thanks.
--
heikki
Hi Heikki,
On Tue, Dec 7, 2021 at 6:37 AM Heikki Krogerus
<[email protected]> wrote:
>
> Hi,
>
> This removes the need for the drivers to always separately evaluate
> the _PLD. With the USB Type-C connector and USB port mapping this
> allows us to start using the component framework and remove the custom
> APIs.
>
> So far the only users of the _PLD information have been the USB
> drivers, but it seems it will be used also at least in some camera
> drivers later. These nevertheless touch mostly USB drivers.
>
> Rafael, is it still OK if Greg takes these?
>
> Prashant, can you test these?
I've applied the patches to a system with the requisite _PLD entries
in firmware, and I'm not sure I can see the connectors getting created
correctly.
My setup is:
Chromebook ------> Dell WD19TB dock (in USB+DisplayPort Alternate
Mode) ----> USB Thumb drive.
Here is the lsusb -t output before connecting the dock (omitting
unrelated busses):
localhost ~ # lsusb -t
/: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
Here is the lsusb -t output (omitting unrelated busses):
localhost ~ # lsusb -t
/: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
|__ Port 2: Dev 15, If 0, Class=Hub, Driver=hub/4p, 10000M
|__ Port 3: Dev 16, If 0, Class=Hub, Driver=hub/4p, 5000M
|__ Port 3: Dev 18, If 0, Class=Mass Storage,
Driver=usb-storage, 5000M
|__ Port 4: Dev 17, If 0, Class=Vendor Specific Class,
Driver=r8152, 5000M
I see the connector symlink for the root hub:
localhost ~ # cd /sys/bus/usb/devices
localhost /sys/bus/usb/devices # ls 2-2/port/connector
data_role device firmware_node port1-cable port1-partner power
power_operation_mode power_role preferred_role subsystem
supported_accessory_modes uevent usb2-port2 usb3-port2
usb_power_delivery_revision usb_typec_revision vconn_source
But for none of the children devices:
localhost /sys/bus/usb/devices # ls 2-2.3/port/connector
ls: cannot access '2-2.3/port/connector': No such file or directory
localhost /sys/bus/usb/devices # ls 2-2.3.3/port/connector
ls: cannot access '2-2.3.3/port/connector': No such file or directory
localhost /sys/bus/usb/devices # ls 2-2.3\:1.0/port/connector
ls: cannot access '2-2.3:1.0/port/connector': No such file or directory
localhost /sys/bus/usb/devices # ls 2-2.3.3\:1.0/port/connector
ls: cannot access '2-2.3.3:1.0/port/connector': No such file or directory
Is this as you intended with the series? My interpretation was that
each connected usb device would get a "connector" symlink, but I may
have misinterpreted this.
Best regards,
-Prashant
Hi,
Thanks for testing these..
On Wed, Dec 08, 2021 at 07:45:26PM -0800, Prashant Malani wrote:
> Hi Heikki,
>
> On Tue, Dec 7, 2021 at 6:37 AM Heikki Krogerus
> <[email protected]> wrote:
> >
> > Hi,
> >
> > This removes the need for the drivers to always separately evaluate
> > the _PLD. With the USB Type-C connector and USB port mapping this
> > allows us to start using the component framework and remove the custom
> > APIs.
> >
> > So far the only users of the _PLD information have been the USB
> > drivers, but it seems it will be used also at least in some camera
> > drivers later. These nevertheless touch mostly USB drivers.
> >
> > Rafael, is it still OK if Greg takes these?
> >
> > Prashant, can you test these?
>
> I've applied the patches to a system with the requisite _PLD entries
> in firmware, and I'm not sure I can see the connectors getting created
> correctly.
>
> My setup is:
>
> Chromebook ------> Dell WD19TB dock (in USB+DisplayPort Alternate
> Mode) ----> USB Thumb drive.
>
> Here is the lsusb -t output before connecting the dock (omitting
> unrelated busses):
> localhost ~ # lsusb -t
> /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
>
> Here is the lsusb -t output (omitting unrelated busses):
> localhost ~ # lsusb -t
> /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
> |__ Port 2: Dev 15, If 0, Class=Hub, Driver=hub/4p, 10000M
> |__ Port 3: Dev 16, If 0, Class=Hub, Driver=hub/4p, 5000M
> |__ Port 3: Dev 18, If 0, Class=Mass Storage,
> Driver=usb-storage, 5000M
> |__ Port 4: Dev 17, If 0, Class=Vendor Specific Class,
> Driver=r8152, 5000M
>
> I see the connector symlink for the root hub:
>
> localhost ~ # cd /sys/bus/usb/devices
> localhost /sys/bus/usb/devices # ls 2-2/port/connector
> data_role device firmware_node port1-cable port1-partner power
> power_operation_mode power_role preferred_role subsystem
> supported_accessory_modes uevent usb2-port2 usb3-port2
> usb_power_delivery_revision usb_typec_revision vconn_source
>
> But for none of the children devices:
>
> localhost /sys/bus/usb/devices # ls 2-2.3/port/connector
> ls: cannot access '2-2.3/port/connector': No such file or directory
> localhost /sys/bus/usb/devices # ls 2-2.3.3/port/connector
> ls: cannot access '2-2.3.3/port/connector': No such file or directory
> localhost /sys/bus/usb/devices # ls 2-2.3\:1.0/port/connector
> ls: cannot access '2-2.3:1.0/port/connector': No such file or directory
> localhost /sys/bus/usb/devices # ls 2-2.3.3\:1.0/port/connector
> ls: cannot access '2-2.3.3:1.0/port/connector': No such file or directory
>
> Is this as you intended with the series? My interpretation was that
> each connected usb device would get a "connector" symlink, but I may
> have misinterpreted this.
It is as intended. The usb ports on the board will have the connector
symlink, not the devices attached to them - the firmware is only aware
of the connectors on the board of course. It looks like this series is
working as it should.
If you want to extend this solution so that also every device in the
usb topology will have the link to the connector on board, then that
should be now possible, but that is out side of the scope of this
series. You need to propose that separately.
But I must ask, why can't you just walk down the topology until you
reach the on-board ports that will have the connector links?
thanks,
--
heikki
Hey Heikki,
On Thu, Dec 9, 2021 at 2:06 AM Heikki Krogerus
<[email protected]> wrote:
>
> Hi,
>
> Thanks for testing these..
>
> On Wed, Dec 08, 2021 at 07:45:26PM -0800, Prashant Malani wrote:
> > Hi Heikki,
> >
> > On Tue, Dec 7, 2021 at 6:37 AM Heikki Krogerus
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > This removes the need for the drivers to always separately evaluate
> > > the _PLD. With the USB Type-C connector and USB port mapping this
> > > allows us to start using the component framework and remove the custom
> > > APIs.
> > >
> > > So far the only users of the _PLD information have been the USB
> > > drivers, but it seems it will be used also at least in some camera
> > > drivers later. These nevertheless touch mostly USB drivers.
> > >
> > > Rafael, is it still OK if Greg takes these?
> > >
> > > Prashant, can you test these?
> >
> > I've applied the patches to a system with the requisite _PLD entries
> > in firmware, and I'm not sure I can see the connectors getting created
> > correctly.
> >
> > My setup is:
> >
> > Chromebook ------> Dell WD19TB dock (in USB+DisplayPort Alternate
> > Mode) ----> USB Thumb drive.
> >
> > Here is the lsusb -t output before connecting the dock (omitting
> > unrelated busses):
> > localhost ~ # lsusb -t
> > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
> >
> > Here is the lsusb -t output (omitting unrelated busses):
> > localhost ~ # lsusb -t
> > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
> > |__ Port 2: Dev 15, If 0, Class=Hub, Driver=hub/4p, 10000M
> > |__ Port 3: Dev 16, If 0, Class=Hub, Driver=hub/4p, 5000M
> > |__ Port 3: Dev 18, If 0, Class=Mass Storage,
> > Driver=usb-storage, 5000M
> > |__ Port 4: Dev 17, If 0, Class=Vendor Specific Class,
> > Driver=r8152, 5000M
> >
> > I see the connector symlink for the root hub:
> >
> > localhost ~ # cd /sys/bus/usb/devices
> > localhost /sys/bus/usb/devices # ls 2-2/port/connector
> > data_role device firmware_node port1-cable port1-partner power
> > power_operation_mode power_role preferred_role subsystem
> > supported_accessory_modes uevent usb2-port2 usb3-port2
> > usb_power_delivery_revision usb_typec_revision vconn_source
> >
> > But for none of the children devices:
> >
> > localhost /sys/bus/usb/devices # ls 2-2.3/port/connector
> > ls: cannot access '2-2.3/port/connector': No such file or directory
> > localhost /sys/bus/usb/devices # ls 2-2.3.3/port/connector
> > ls: cannot access '2-2.3.3/port/connector': No such file or directory
> > localhost /sys/bus/usb/devices # ls 2-2.3\:1.0/port/connector
> > ls: cannot access '2-2.3:1.0/port/connector': No such file or directory
> > localhost /sys/bus/usb/devices # ls 2-2.3.3\:1.0/port/connector
> > ls: cannot access '2-2.3.3:1.0/port/connector': No such file or directory
> >
> > Is this as you intended with the series? My interpretation was that
> > each connected usb device would get a "connector" symlink, but I may
> > have misinterpreted this.
>
> It is as intended. The usb ports on the board will have the connector
> symlink, not the devices attached to them - the firmware is only aware
> of the connectors on the board of course. It looks like this series is
> working as it should.
Thanks for clarifying my understanding here.
>
> If you want to extend this solution so that also every device in the
> usb topology will have the link to the connector on board, then that
> should be now possible, but that is out side of the scope of this
> series. You need to propose that separately.
>
> But I must ask, why can't you just walk down the topology until you
> reach the on-board ports that will have the connector links?
>
Right, we can certainly do that; having it in each device is just
convenient. But as you said, that's the subject of another series.
You mentioned there would be a v2, so I'll add my Tested-By then.
Best regards,
On Thu, Dec 09, 2021 at 11:45:27AM -0800, Prashant Malani wrote:
> Hey Heikki,
>
> On Thu, Dec 9, 2021 at 2:06 AM Heikki Krogerus
> <[email protected]> wrote:
> >
> > Hi,
> >
> > Thanks for testing these..
> >
> > On Wed, Dec 08, 2021 at 07:45:26PM -0800, Prashant Malani wrote:
> > > Hi Heikki,
> > >
> > > On Tue, Dec 7, 2021 at 6:37 AM Heikki Krogerus
> > > <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > This removes the need for the drivers to always separately evaluate
> > > > the _PLD. With the USB Type-C connector and USB port mapping this
> > > > allows us to start using the component framework and remove the custom
> > > > APIs.
> > > >
> > > > So far the only users of the _PLD information have been the USB
> > > > drivers, but it seems it will be used also at least in some camera
> > > > drivers later. These nevertheless touch mostly USB drivers.
> > > >
> > > > Rafael, is it still OK if Greg takes these?
> > > >
> > > > Prashant, can you test these?
> > >
> > > I've applied the patches to a system with the requisite _PLD entries
> > > in firmware, and I'm not sure I can see the connectors getting created
> > > correctly.
> > >
> > > My setup is:
> > >
> > > Chromebook ------> Dell WD19TB dock (in USB+DisplayPort Alternate
> > > Mode) ----> USB Thumb drive.
> > >
> > > Here is the lsusb -t output before connecting the dock (omitting
> > > unrelated busses):
> > > localhost ~ # lsusb -t
> > > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
> > >
> > > Here is the lsusb -t output (omitting unrelated busses):
> > > localhost ~ # lsusb -t
> > > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/3p, 10000M/x2
> > > |__ Port 2: Dev 15, If 0, Class=Hub, Driver=hub/4p, 10000M
> > > |__ Port 3: Dev 16, If 0, Class=Hub, Driver=hub/4p, 5000M
> > > |__ Port 3: Dev 18, If 0, Class=Mass Storage,
> > > Driver=usb-storage, 5000M
> > > |__ Port 4: Dev 17, If 0, Class=Vendor Specific Class,
> > > Driver=r8152, 5000M
> > >
> > > I see the connector symlink for the root hub:
> > >
> > > localhost ~ # cd /sys/bus/usb/devices
> > > localhost /sys/bus/usb/devices # ls 2-2/port/connector
> > > data_role device firmware_node port1-cable port1-partner power
> > > power_operation_mode power_role preferred_role subsystem
> > > supported_accessory_modes uevent usb2-port2 usb3-port2
> > > usb_power_delivery_revision usb_typec_revision vconn_source
> > >
> > > But for none of the children devices:
> > >
> > > localhost /sys/bus/usb/devices # ls 2-2.3/port/connector
> > > ls: cannot access '2-2.3/port/connector': No such file or directory
> > > localhost /sys/bus/usb/devices # ls 2-2.3.3/port/connector
> > > ls: cannot access '2-2.3.3/port/connector': No such file or directory
> > > localhost /sys/bus/usb/devices # ls 2-2.3\:1.0/port/connector
> > > ls: cannot access '2-2.3:1.0/port/connector': No such file or directory
> > > localhost /sys/bus/usb/devices # ls 2-2.3.3\:1.0/port/connector
> > > ls: cannot access '2-2.3.3:1.0/port/connector': No such file or directory
> > >
> > > Is this as you intended with the series? My interpretation was that
> > > each connected usb device would get a "connector" symlink, but I may
> > > have misinterpreted this.
> >
> > It is as intended. The usb ports on the board will have the connector
> > symlink, not the devices attached to them - the firmware is only aware
> > of the connectors on the board of course. It looks like this series is
> > working as it should.
>
> Thanks for clarifying my understanding here.
>
> >
> > If you want to extend this solution so that also every device in the
> > usb topology will have the link to the connector on board, then that
> > should be now possible, but that is out side of the scope of this
> > series. You need to propose that separately.
> >
> > But I must ask, why can't you just walk down the topology until you
> > reach the on-board ports that will have the connector links?
> >
>
> Right, we can certainly do that; having it in each device is just
> convenient. But as you said, that's the subject of another series.
>
> You mentioned there would be a v2, so I'll add my Tested-By then.
Great!
I don't know does this help you, or if this is exactly what you had in
mind, but I am planning to link the USB device attached to the
connector to the USB Type-C partner device that we have (the USB
device attached to the connector is the Type-C partner).
So in your example, the Dell dock USB hub will have the symlink to
the Type-C partner device, but the USB mass storage device attached to
the dock will not (because it's not our USB Type-C partner - the dock
is our USB Type-C partner).
But I want to do that separately, as the next step in any case.
thanks,
--
heikki