linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2]  extcon: axp288: Move to swnodes
@ 2019-10-08 12:25 Heikki Krogerus
  2019-10-08 12:25 ` [PATCH v2 1/2] usb: roles: Add usb_role_switch_find_by_fwnode() Heikki Krogerus
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Heikki Krogerus @ 2019-10-08 12:25 UTC (permalink / raw)
  To: Hans de Goede
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

Hi Hans,

Fixed the compiler warning in this version. No other changes.

The original cover letter:

That AXP288 extcon driver is the last that uses build-in connection
description. I'm replacing it with a code that finds the role mux
software node instead.

I'm proposing also here a little helper
usb_role_switch_find_by_fwnode() that uses
class_find_device_by_fwnode() to find the role switches.

thanks,

Heikki Krogerus (2):
  usb: roles: Add usb_role_switch_find_by_fwnode()
  extcon: axp288: Remove the build-in connection description

 drivers/extcon/extcon-axp288.c | 38 ++++++++++++++++++++--------------
 drivers/usb/roles/class.c      | 21 +++++++++++++++++++
 include/linux/usb/role.h       |  3 +++
 3 files changed, 47 insertions(+), 15 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/2] usb: roles: Add usb_role_switch_find_by_fwnode()
  2019-10-08 12:25 [PATCH v2 0/2] extcon: axp288: Move to swnodes Heikki Krogerus
@ 2019-10-08 12:25 ` Heikki Krogerus
  2019-10-08 12:26 ` [PATCH v2 2/2] extcon: axp288: Remove the build-in connection description Heikki Krogerus
  2019-10-08 13:59 ` [PATCH v2 0/2] extcon: axp288: Move to swnodes Hans de Goede
  2 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2019-10-08 12:25 UTC (permalink / raw)
  To: Hans de Goede
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

Simple wrapper function that searches USB role switches with
class_find_device_by_fwnode().

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/roles/class.c | 21 +++++++++++++++++++++
 include/linux/usb/role.h  |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 94b4e7db2b94..8273126ffdf4 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -175,6 +175,27 @@ void usb_role_switch_put(struct usb_role_switch *sw)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_put);
 
+/**
+ * usb_role_switch_find_by_fwnode - Find USB role switch with its fwnode
+ * @fwnode: fwnode of the USB Role Switch
+ *
+ * Finds and returns role switch with @fwnode. The reference count for the
+ * found switch is incremented.
+ */
+struct usb_role_switch *
+usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
+{
+	struct device *dev;
+
+	if (!fwnode)
+		return NULL;
+
+	dev = class_find_device_by_fwnode(role_class, fwnode);
+
+	return dev ? to_role_switch(dev) : NULL;
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_find_by_fwnode);
+
 static umode_t
 usb_role_switch_is_visible(struct kobject *kobj, struct attribute *attr, int n)
 {
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index 2d77f97df72d..efac3af83d6b 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -50,6 +50,9 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev);
 struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *node);
 void usb_role_switch_put(struct usb_role_switch *sw);
 
+struct usb_role_switch *
+usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode);
+
 struct usb_role_switch *
 usb_role_switch_register(struct device *parent,
 			 const struct usb_role_switch_desc *desc);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] extcon: axp288: Remove the build-in connection description
  2019-10-08 12:25 [PATCH v2 0/2] extcon: axp288: Move to swnodes Heikki Krogerus
  2019-10-08 12:25 ` [PATCH v2 1/2] usb: roles: Add usb_role_switch_find_by_fwnode() Heikki Krogerus
@ 2019-10-08 12:26 ` Heikki Krogerus
  2019-10-08 13:59 ` [PATCH v2 0/2] extcon: axp288: Move to swnodes Hans de Goede
  2 siblings, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2019-10-08 12:26 UTC (permalink / raw)
  To: Hans de Goede
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

Getting handle to the USB role switch by first finding its
software fwnode.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/extcon/extcon-axp288.c | 38 ++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index 415afaf479e7..a7f216191493 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -322,6 +322,25 @@ static void axp288_put_role_sw(void *data)
 	usb_role_switch_put(info->role_sw);
 }
 
+static int axp288_extcon_find_role_sw(struct axp288_extcon_info *info)
+{
+	const struct software_node *swnode;
+	struct fwnode_handle *fwnode;
+
+	if (!x86_match_cpu(cherry_trail_cpu_ids))
+		return 0;
+
+	swnode = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
+	if (!swnode)
+		return -EPROBE_DEFER;
+
+	fwnode = software_node_fwnode(swnode);
+	info->role_sw = usb_role_switch_find_by_fwnode(fwnode);
+	fwnode_handle_put(fwnode);
+
+	return info->role_sw ? 0 : -EPROBE_DEFER;
+}
+
 static int axp288_extcon_probe(struct platform_device *pdev)
 {
 	struct axp288_extcon_info *info;
@@ -343,9 +362,10 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 
-	info->role_sw = usb_role_switch_get(dev);
-	if (IS_ERR(info->role_sw))
-		return PTR_ERR(info->role_sw);
+	ret = axp288_extcon_find_role_sw(info);
+	if (ret)
+		return ret;
+
 	if (info->role_sw) {
 		ret = devm_add_action_or_reset(dev, axp288_put_role_sw, info);
 		if (ret)
@@ -440,26 +460,14 @@ static struct platform_driver axp288_extcon_driver = {
 	},
 };
 
-static struct device_connection axp288_extcon_role_sw_conn = {
-	.endpoint[0] = "axp288_extcon",
-	.endpoint[1] = "intel_xhci_usb_sw-role-switch",
-	.id = "usb-role-switch",
-};
-
 static int __init axp288_extcon_init(void)
 {
-	if (x86_match_cpu(cherry_trail_cpu_ids))
-		device_connection_add(&axp288_extcon_role_sw_conn);
-
 	return platform_driver_register(&axp288_extcon_driver);
 }
 module_init(axp288_extcon_init);
 
 static void __exit axp288_extcon_exit(void)
 {
-	if (x86_match_cpu(cherry_trail_cpu_ids))
-		device_connection_remove(&axp288_extcon_role_sw_conn);
-
 	platform_driver_unregister(&axp288_extcon_driver);
 }
 module_exit(axp288_extcon_exit);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-08 12:25 [PATCH v2 0/2] extcon: axp288: Move to swnodes Heikki Krogerus
  2019-10-08 12:25 ` [PATCH v2 1/2] usb: roles: Add usb_role_switch_find_by_fwnode() Heikki Krogerus
  2019-10-08 12:26 ` [PATCH v2 2/2] extcon: axp288: Remove the build-in connection description Heikki Krogerus
@ 2019-10-08 13:59 ` Hans de Goede
  2019-10-08 14:01   ` Heikki Krogerus
  2019-11-04 13:09   ` Heikki Krogerus
  2 siblings, 2 replies; 12+ messages in thread
From: Hans de Goede @ 2019-10-08 13:59 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

Hi,

On 08-10-2019 14:25, Heikki Krogerus wrote:
> Hi Hans,
> 
> Fixed the compiler warning in this version. No other changes.
> 
> The original cover letter:
> 
> That AXP288 extcon driver is the last that uses build-in connection
> description. I'm replacing it with a code that finds the role mux
> software node instead.
> 
> I'm proposing also here a little helper
> usb_role_switch_find_by_fwnode() that uses
> class_find_device_by_fwnode() to find the role switches.

Both patches look good to me and I can confirm that things still
work as they should on a CHT device with an AXP288 PMIC, so for both:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



p.s.

I guess this means we can remove the build-in connection (
device_connection_add / remove) stuff now?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-08 13:59 ` [PATCH v2 0/2] extcon: axp288: Move to swnodes Hans de Goede
@ 2019-10-08 14:01   ` Heikki Krogerus
  2019-10-10  8:31     ` Heikki Krogerus
  2019-11-04 13:09   ` Heikki Krogerus
  1 sibling, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2019-10-08 14:01 UTC (permalink / raw)
  To: Hans de Goede
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 08-10-2019 14:25, Heikki Krogerus wrote:
> > Hi Hans,
> > 
> > Fixed the compiler warning in this version. No other changes.
> > 
> > The original cover letter:
> > 
> > That AXP288 extcon driver is the last that uses build-in connection
> > description. I'm replacing it with a code that finds the role mux
> > software node instead.
> > 
> > I'm proposing also here a little helper
> > usb_role_switch_find_by_fwnode() that uses
> > class_find_device_by_fwnode() to find the role switches.
> 
> Both patches look good to me and I can confirm that things still
> work as they should on a CHT device with an AXP288 PMIC, so for both:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> I guess this means we can remove the build-in connection (
> device_connection_add / remove) stuff now?

Yes. I'll prepare separate patches for that.

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-08 14:01   ` Heikki Krogerus
@ 2019-10-10  8:31     ` Heikki Krogerus
  2019-10-10  9:32       ` Hans de Goede
  0 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2019-10-10  8:31 UTC (permalink / raw)
  To: Hans de Goede
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

On Tue, Oct 08, 2019 at 05:02:04PM +0300, Heikki Krogerus wrote:
> On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote:
> > Hi,
> > 
> > On 08-10-2019 14:25, Heikki Krogerus wrote:
> > > Hi Hans,
> > > 
> > > Fixed the compiler warning in this version. No other changes.
> > > 
> > > The original cover letter:
> > > 
> > > That AXP288 extcon driver is the last that uses build-in connection
> > > description. I'm replacing it with a code that finds the role mux
> > > software node instead.
> > > 
> > > I'm proposing also here a little helper
> > > usb_role_switch_find_by_fwnode() that uses
> > > class_find_device_by_fwnode() to find the role switches.
> > 
> > Both patches look good to me and I can confirm that things still
> > work as they should on a CHT device with an AXP288 PMIC, so for both:
> > 
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Regards,
> > 
> > Hans
> > 
> > p.s.
> > 
> > I guess this means we can remove the build-in connection (
> > device_connection_add / remove) stuff now?
> 
> Yes. I'll prepare separate patches for that.

Actually, maybe it would make sense to just remove it in this series.
I'm attaching a patch that remove struct device_connection completely.
Can you check if it makes sense to you?

thanks,

-- 
heikki

[-- Attachment #2: 0001-device-connection-Remove-struct-device_connection.patch --]
[-- Type: text/plain, Size: 15827 bytes --]

From 9444a9498dd22fd88208d19ab3454689b953eb7b Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 9 Oct 2019 16:49:24 +0300
Subject: [PATCH] device connection: Remove struct device_connection

Now that we have software fwnodes to cover cases where a
firmware node does not describe the connections, there is no
need for dedicated device connection descriptors.

This removes struct device_connection completely. From now
on the processing is always done with the fwnode handle.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 .../driver-api/device_connection.rst          |  37 +-----
 drivers/base/devcon.c                         | 116 ++++--------------
 drivers/usb/roles/class.c                     |  12 +-
 drivers/usb/typec/class.c                     |  12 +-
 drivers/usb/typec/mux.c                       |  40 ++----
 include/linux/device.h                        |  49 +-------
 6 files changed, 48 insertions(+), 218 deletions(-)

diff --git a/Documentation/driver-api/device_connection.rst b/Documentation/driver-api/device_connection.rst
index ba364224c349..f1dfb426c0e8 100644
--- a/Documentation/driver-api/device_connection.rst
+++ b/Documentation/driver-api/device_connection.rst
@@ -5,39 +5,14 @@ Device connections
 Introduction
 ------------
 
-Devices often have connections to other devices that are outside of the direct
-child/parent relationship. A serial or network communication controller, which
-could be a PCI device, may need to be able to get a reference to its PHY
-component, which could be attached for example to the I2C bus. Some device
-drivers need to be able to control the clocks or the GPIOs for their devices,
-and so on.
-
-Device connections are generic descriptions of any type of connection between
-two separate devices.
-
-Device connections alone do not create a dependency between the two devices.
-They are only descriptions which are not tied to either of the devices directly.
-A dependency between the two devices exists only if one of the two endpoint
-devices requests a reference to the other. The descriptions themselves can be
-defined in firmware (not yet supported) or they can be built-in.
-
-Usage
------
-
-Device connections should exist before device ``->probe`` callback is called for
-either endpoint device in the description. If the connections are defined in
-firmware, this is not a problem. It should be considered if the connection
-descriptions are "built-in", and need to be added separately.
-
-The connection description consists of the names of the two devices with the
-connection, i.e. the endpoints, and unique identifier for the connection which
-is needed if there are multiple connections between the two devices.
-
-After a description exists, the devices in it can request reference to the other
-endpoint device, or they can request the description itself.
+Device connection API allows drivers to acquire references to devices that are,
+according to their firmware nodes, connected to the devices those drivers are
+controlling. The API is in practice a wrapper that for now utilizes
+``fwnode_graph*()`` family of functions and
+:c:func:`fwnode_property_get_reference_arg` function.
 
 API
 ---
 
 .. kernel-doc:: drivers/base/devcon.c
-   :functions: device_connection_find_match device_connection_find device_connection_add device_connection_remove
+   :functions: device_connection_find_match fwnode_connection_find_match device_connection_find
diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 14e2178e09f8..f7698362f267 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -9,24 +9,20 @@
 #include <linux/device.h>
 #include <linux/property.h>
 
-static DEFINE_MUTEX(devcon_lock);
-static LIST_HEAD(devcon_list);
-
 static void *
 fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 			  void *data, devcon_match_fn_t match)
 {
-	struct device_connection con = { .id = con_id };
 	struct fwnode_handle *ep;
 	void *ret;
 
 	fwnode_graph_for_each_endpoint(fwnode, ep) {
-		con.fwnode = fwnode_graph_get_remote_port_parent(ep);
-		if (!fwnode_device_is_available(con.fwnode))
+		fwnode = fwnode_graph_get_remote_port_parent(ep);
+		if (!fwnode_device_is_available(fwnode))
 			continue;
 
-		ret = match(&con, -1, data);
-		fwnode_handle_put(con.fwnode);
+		ret = match(fwnode, con_id, data);
+		fwnode_handle_put(fwnode);
 		if (ret) {
 			fwnode_handle_put(ep);
 			return ret;
@@ -39,17 +35,16 @@ static void *
 fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 		    void *data, devcon_match_fn_t match)
 {
-	struct device_connection con = { };
 	void *ret;
 	int i;
 
 	for (i = 0; ; i++) {
-		con.fwnode = fwnode_find_reference(fwnode, con_id, i);
-		if (IS_ERR(con.fwnode))
+		fwnode = fwnode_find_reference(fwnode, con_id, i);
+		if (IS_ERR(fwnode))
 			break;
 
-		ret = match(&con, -1, data);
-		fwnode_handle_put(con.fwnode);
+		ret = match(fwnode, NULL, data);
+		fwnode_handle_put(fwnode);
 		if (ret)
 			return ret;
 	}
@@ -99,37 +94,8 @@ EXPORT_SYMBOL_GPL(fwnode_connection_find_match);
 void *device_connection_find_match(struct device *dev, const char *con_id,
 				   void *data, devcon_match_fn_t match)
 {
-	struct fwnode_handle *fwnode = dev_fwnode(dev);
-	const char *devname = dev_name(dev);
-	struct device_connection *con;
-	void *ret = NULL;
-	int ep;
-
-	if (!match)
-		return NULL;
-
-	ret = fwnode_connection_find_match(fwnode, con_id, data, match);
-	if (ret)
-		return ret;
-
-	mutex_lock(&devcon_lock);
-
-	list_for_each_entry(con, &devcon_list, list) {
-		ep = match_string(con->endpoint, 2, devname);
-		if (ep < 0)
-			continue;
-
-		if (con_id && strcmp(con->id, con_id))
-			continue;
-
-		ret = match(con, !ep, data);
-		if (ret)
-			break;
-	}
-
-	mutex_unlock(&devcon_lock);
-
-	return ret;
+	return fwnode_connection_find_match(dev_fwnode(dev), con_id,
+					    data, match);
 }
 EXPORT_SYMBOL_GPL(device_connection_find_match);
 
@@ -152,41 +118,25 @@ static struct bus_type *generic_match_buses[] = {
 	NULL,
 };
 
-static void *device_connection_fwnode_match(struct device_connection *con)
+/* This tries to find the device from the most common bus types by name. */
+static void *generic_match(struct fwnode_handle *fwnode, const char *id,
+			   void *data)
 {
 	struct bus_type *bus;
 	struct device *dev;
+	void *ret = NULL;
 
 	for (bus = generic_match_buses[0]; bus; bus++) {
-		dev = bus_find_device_by_fwnode(bus, con->fwnode);
-		if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
-			return dev;
+		dev = bus_find_device_by_fwnode(bus, fwnode);
+		if (dev) {
+			if (!strncmp(dev_name(dev), id, strlen(id)))
+				return dev;
+			ret = ERR_PTR(-EPROBE_DEFER);
+		}
 
 		put_device(dev);
 	}
-	return NULL;
-}
-
-/* This tries to find the device from the most common bus types by name. */
-static void *generic_match(struct device_connection *con, int ep, void *data)
-{
-	struct bus_type *bus;
-	struct device *dev;
-
-	if (con->fwnode)
-		return device_connection_fwnode_match(con);
-
-	for (bus = generic_match_buses[0]; bus; bus++) {
-		dev = bus_find_device_by_name(bus, NULL, con->endpoint[ep]);
-		if (dev)
-			return dev;
-	}
-
-	/*
-	 * We only get called if a connection was found, tell the caller to
-	 * wait for the other device to show up.
-	 */
-	return ERR_PTR(-EPROBE_DEFER);
+	return ret;
 }
 
 /**
@@ -205,27 +155,3 @@ struct device *device_connection_find(struct device *dev, const char *con_id)
 	return device_connection_find_match(dev, con_id, NULL, generic_match);
 }
 EXPORT_SYMBOL_GPL(device_connection_find);
-
-/**
- * device_connection_add - Register a connection description
- * @con: The connection description to be registered
- */
-void device_connection_add(struct device_connection *con)
-{
-	mutex_lock(&devcon_lock);
-	list_add_tail(&con->list, &devcon_list);
-	mutex_unlock(&devcon_lock);
-}
-EXPORT_SYMBOL_GPL(device_connection_add);
-
-/**
- * device_connections_remove - Unregister connection description
- * @con: The connection description to be unregistered
- */
-void device_connection_remove(struct device_connection *con)
-{
-	mutex_lock(&devcon_lock);
-	list_del(&con->list);
-	mutex_unlock(&devcon_lock);
-}
-EXPORT_SYMBOL_GPL(device_connection_remove);
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 8273126ffdf4..67e48c733e92 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -85,19 +85,15 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
 
-static void *usb_role_switch_match(struct device_connection *con, int ep,
+static void *usb_role_switch_match(struct fwnode_handle *fwnode, const char *id,
 				   void *data)
 {
 	struct device *dev;
 
-	if (con->fwnode) {
-		if (con->id && !fwnode_property_present(con->fwnode, con->id))
-			return NULL;
+	if (id && !fwnode_property_present(fwnode, id))
+		return NULL;
 
-		dev = class_find_device_by_fwnode(role_class, con->fwnode);
-	} else {
-		dev = class_find_device_by_name(role_class, con->endpoint[ep]);
-	}
+	dev = class_find_device_by_fwnode(role_class, fwnode);
 
 	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 94a3eda62add..cccd60652618 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -205,20 +205,14 @@ 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)
+static void *typec_port_match(struct fwnode_handle *fwnode, const char *id,
+			      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);
+	return class_find_device_by_fwnode(typec_class, fwnode);
 }
 
 struct typec_altmode *
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 57907f26f681..957c405e4545 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);
@@ -39,21 +34,16 @@ static int switch_fwnode_match(struct device *dev, const void *fwnode)
 	return dev_fwnode(dev) == fwnode && dev_name_ends_with(dev, "-switch");
 }
 
-static void *typec_switch_match(struct device_connection *con, int ep,
+static void *typec_switch_match(struct fwnode_handle *fwnode, const char *id,
 				void *data)
 {
 	struct device *dev;
 
-	if (con->fwnode) {
-		if (con->id && !fwnode_property_present(con->fwnode, con->id))
-			return NULL;
+	if (id && !fwnode_property_present(fwnode, 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, fwnode,
+				switch_fwnode_match);
 
 	return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
@@ -182,7 +172,8 @@ static int mux_fwnode_match(struct device *dev, const void *fwnode)
 	return dev_fwnode(dev) == fwnode && dev_name_ends_with(dev, "-mux");
 }
 
-static void *typec_mux_match(struct device_connection *con, int ep, void *data)
+static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
+			     void *data)
 {
 	const struct typec_altmode_desc *desc = data;
 	struct device *dev;
@@ -191,31 +182,24 @@ 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.
 	 */
-	match = !con->id;
+	match = !id;
 	if (match)
 		goto find_mux;
 
 	/* Accessory Mode muxes */
 	if (!desc) {
-		match = fwnode_property_present(con->fwnode, "accessory");
+		match = fwnode_property_present(fwnode, "accessory");
 		if (match)
 			goto find_mux;
 		return NULL;
 	}
 
 	/* Alternate Mode muxes */
-	nval = fwnode_property_count_u16(con->fwnode, "svid");
+	nval = fwnode_property_count_u16(fwnode, "svid");
 	if (nval <= 0)
 		return NULL;
 
@@ -223,7 +207,7 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 	if (!val)
 		return ERR_PTR(-ENOMEM);
 
-	nval = fwnode_property_read_u16_array(con->fwnode, "svid", val, nval);
+	nval = fwnode_property_read_u16_array(fwnode, "svid", val, nval);
 	if (nval < 0) {
 		kfree(val);
 		return ERR_PTR(nval);
@@ -240,7 +224,7 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 	return NULL;
 
 find_mux:
-	dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
+	dev = class_find_device(&typec_mux_class, NULL, fwnode,
 				mux_fwnode_match);
 
 	return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
diff --git a/include/linux/device.h b/include/linux/device.h
index 9f2f2e169f95..26fcae1dae32 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1013,26 +1013,8 @@ struct device_dma_parameters {
 	unsigned long segment_boundary_mask;
 };
 
-/**
- * struct device_connection - Device Connection Descriptor
- * @fwnode: The device node of the connected device
- * @endpoint: The names of the two devices connected together
- * @id: Unique identifier for the connection
- * @list: List head, private, for internal use only
- *
- * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
- * platform firmware defines the connection. When the connection is registered
- * with device_connection_add() @endpoint is used instead.
- */
-struct device_connection {
-	struct fwnode_handle	*fwnode;
-	const char		*endpoint[2];
-	const char		*id;
-	struct list_head	list;
-};
-
-typedef void *(*devcon_match_fn_t)(struct device_connection *con, int ep,
-				   void *data);
+typedef void *(*devcon_match_fn_t)(struct fwnode_handle *fwnode,
+				   const char *id, void *data);
 
 void *fwnode_connection_find_match(struct fwnode_handle *fwnode,
 				   const char *con_id, void *data,
@@ -1042,33 +1024,6 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
 
 struct device *device_connection_find(struct device *dev, const char *con_id);
 
-void device_connection_add(struct device_connection *con);
-void device_connection_remove(struct device_connection *con);
-
-/**
- * device_connections_add - Add multiple device connections at once
- * @cons: Zero terminated array of device connection descriptors
- */
-static inline void device_connections_add(struct device_connection *cons)
-{
-	struct device_connection *c;
-
-	for (c = cons; c->endpoint[0]; c++)
-		device_connection_add(c);
-}
-
-/**
- * device_connections_remove - Remove multiple device connections at once
- * @cons: Zero terminated array of device connection descriptors
- */
-static inline void device_connections_remove(struct device_connection *cons)
-{
-	struct device_connection *c;
-
-	for (c = cons; c->endpoint[0]; c++)
-		device_connection_remove(c);
-}
-
 /**
  * enum device_link_state - Device link states.
  * @DL_STATE_NONE: The presence of the drivers is not being tracked.
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-10  8:31     ` Heikki Krogerus
@ 2019-10-10  9:32       ` Hans de Goede
  2019-10-10 11:16         ` Heikki Krogerus
  0 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2019-10-10  9:32 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

Hi,

On 10-10-2019 10:31, Heikki Krogerus wrote:
> On Tue, Oct 08, 2019 at 05:02:04PM +0300, Heikki Krogerus wrote:
>> On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 08-10-2019 14:25, Heikki Krogerus wrote:
>>>> Hi Hans,
>>>>
>>>> Fixed the compiler warning in this version. No other changes.
>>>>
>>>> The original cover letter:
>>>>
>>>> That AXP288 extcon driver is the last that uses build-in connection
>>>> description. I'm replacing it with a code that finds the role mux
>>>> software node instead.
>>>>
>>>> I'm proposing also here a little helper
>>>> usb_role_switch_find_by_fwnode() that uses
>>>> class_find_device_by_fwnode() to find the role switches.
>>>
>>> Both patches look good to me and I can confirm that things still
>>> work as they should on a CHT device with an AXP288 PMIC, so for both:
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>> p.s.
>>>
>>> I guess this means we can remove the build-in connection (
>>> device_connection_add / remove) stuff now?
>>
>> Yes. I'll prepare separate patches for that.
> 
> Actually, maybe it would make sense to just remove it in this series.
> I'm attaching a patch that remove struct device_connection completely.
> Can you check if it makes sense to you?

This bit seems broken:

  static void *
  fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
                           void *data, devcon_match_fn_t match)
  {
-       struct device_connection con = { .id = con_id };
         struct fwnode_handle *ep;
         void *ret;

         fwnode_graph_for_each_endpoint(fwnode, ep) {
-               con.fwnode = fwnode_graph_get_remote_port_parent(ep);
-               if (!fwnode_device_is_available(con.fwnode))
+               fwnode = fwnode_graph_get_remote_port_parent(ep);

You are no replacing the passed in fwnode with the fwnode for the
remote_port_parent and then the next time through the loop you will
look at the wrong fwnode as the fwnode argument to
fwnode_graph_for_each_endpoint() gets evaluated multiple times:

#define fwnode_graph_for_each_endpoint(fwnode, child)                   \
         for (child = NULL;                                              \
              (child = fwnode_graph_get_next_endpoint(fwnode, child)); )

###

And there is a similar problem here, where you again use the fwmode
argument also as local variable in a loop where the function
argument should be evaluated more then once:

  fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
                     void *data, devcon_match_fn_t match)
  {
-       struct device_connection con = { };
         void *ret;
         int i;

         for (i = 0; ; i++) {
-               con.fwnode = fwnode_find_reference(fwnode, con_id, i);
-               if (IS_ERR(con.fwnode))
+               fwnode = fwnode_find_reference(fwnode, con_id, i);

###

And it seems that this bit where you introduce -EPROBE_DEFER is an unrelated
behavior change? :

+static void *generic_match(struct fwnode_handle *fwnode, const char *id,
+                          void *data)
  {
         struct bus_type *bus;
         struct device *dev;
+       void *ret = NULL;

         for (bus = generic_match_buses[0]; bus; bus++) {
-               dev = bus_find_device_by_fwnode(bus, con->fwnode);
-               if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
-                       return dev;
+               dev = bus_find_device_by_fwnode(bus, fwnode);
+               if (dev) {
+                       if (!strncmp(dev_name(dev), id, strlen(id)))
+                               return dev;
+                       ret = ERR_PTR(-EPROBE_DEFER);
+               }


Note that the old generic_match code had:

-       if (con->fwnode)
-               return device_connection_fwnode_match(con);

Which will simply always return either the dev or NULL, so as said this
is a behavior change AFAICT.

I've been trying to figure out what you are trying to do here and
I found a troublesome path through the old code:

1. device_connection_find() gets called on a device with a fwnode
2. device_connection_find() calls device_connection_find_match()
3. device_connection_find_match() calls fwnode_connection_find_match()
4. fwnode_connection_find_match() calls fwnode_graph_devcon_match() this returns NULL
5. fwnode_connection_find_match() calls fwnode_devcon_match()
6. fwnode_devcon_match() creates a struct device_connection with just fwnode set, the rest is 0/NULL
7. fwnode_devcon_match() calls generic_match() with this struct
8. generic_match() calls device_connection_fwnode_match() because con->fwnode is set
9. device_connection_fwnode_match() does the following if a device is found:
    if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
        return dev;
    but con->id is NULL here, so we have a NULL pointer deref here!

We are currently not hitting this path because there are no callers of
device_connection_find() all devcon users currently use device_connection_find_match()

Note I believe the code after your patch still has this problem. Also doing
the strcmp on the dev_name seems wrong? At least for the above code path, where
fwnode_devcon_match() has already used / consumed the id.

So at a minimum we need to add a id == NULL check to generic_match (*), but
Since there are no users and the strcmp to to dev_name is weird, I believe that
it would be better to just remove device_connection_find() (and generic_match, etc.) ?

This could be a preparation patch for the patch you attached, this would simplify
this patch a bit.

###

If we end up with something like your suggested patch I think it might be good to
do a follow up where device_connection_find_match callers simply call
fwnode_connection_find_match directly and we remove device_connection_find_match ?
Or maybe turn it into a static inline function?

Regards,

Hans

*)  Note that the typec related callers of device_connection_find_match() all 3
either already have an id == NULL check, or just ignore id completely.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-10  9:32       ` Hans de Goede
@ 2019-10-10 11:16         ` Heikki Krogerus
  2019-10-10 11:58           ` Heikki Krogerus
  0 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2019-10-10 11:16 UTC (permalink / raw)
  To: Hans de Goede
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

On Thu, Oct 10, 2019 at 11:32:23AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10-10-2019 10:31, Heikki Krogerus wrote:
> > On Tue, Oct 08, 2019 at 05:02:04PM +0300, Heikki Krogerus wrote:
> > > On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 08-10-2019 14:25, Heikki Krogerus wrote:
> > > > > Hi Hans,
> > > > > 
> > > > > Fixed the compiler warning in this version. No other changes.
> > > > > 
> > > > > The original cover letter:
> > > > > 
> > > > > That AXP288 extcon driver is the last that uses build-in connection
> > > > > description. I'm replacing it with a code that finds the role mux
> > > > > software node instead.
> > > > > 
> > > > > I'm proposing also here a little helper
> > > > > usb_role_switch_find_by_fwnode() that uses
> > > > > class_find_device_by_fwnode() to find the role switches.
> > > > 
> > > > Both patches look good to me and I can confirm that things still
> > > > work as they should on a CHT device with an AXP288 PMIC, so for both:
> > > > 
> > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > > > 
> > > > Regards,
> > > > 
> > > > Hans
> > > > 
> > > > p.s.
> > > > 
> > > > I guess this means we can remove the build-in connection (
> > > > device_connection_add / remove) stuff now?
> > > 
> > > Yes. I'll prepare separate patches for that.
> > 
> > Actually, maybe it would make sense to just remove it in this series.
> > I'm attaching a patch that remove struct device_connection completely.
> > Can you check if it makes sense to you?
> 
> This bit seems broken:
> 
>  static void *
>  fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
>                           void *data, devcon_match_fn_t match)
>  {
> -       struct device_connection con = { .id = con_id };
>         struct fwnode_handle *ep;
>         void *ret;
> 
>         fwnode_graph_for_each_endpoint(fwnode, ep) {
> -               con.fwnode = fwnode_graph_get_remote_port_parent(ep);
> -               if (!fwnode_device_is_available(con.fwnode))
> +               fwnode = fwnode_graph_get_remote_port_parent(ep);
> 
> You are no replacing the passed in fwnode with the fwnode for the
> remote_port_parent and then the next time through the loop you will
> look at the wrong fwnode as the fwnode argument to
> fwnode_graph_for_each_endpoint() gets evaluated multiple times:
> 
> #define fwnode_graph_for_each_endpoint(fwnode, child)                   \
>         for (child = NULL;                                              \
>              (child = fwnode_graph_get_next_endpoint(fwnode, child)); )
> 
> ###
> 
> And there is a similar problem here, where you again use the fwmode
> argument also as local variable in a loop where the function
> argument should be evaluated more then once:
> 
>  fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
>                     void *data, devcon_match_fn_t match)
>  {
> -       struct device_connection con = { };
>         void *ret;
>         int i;
> 
>         for (i = 0; ; i++) {
> -               con.fwnode = fwnode_find_reference(fwnode, con_id, i);
> -               if (IS_ERR(con.fwnode))
> +               fwnode = fwnode_find_reference(fwnode, con_id, i);
> 
> ###
> 
> And it seems that this bit where you introduce -EPROBE_DEFER is an unrelated
> behavior change? :
> 
> +static void *generic_match(struct fwnode_handle *fwnode, const char *id,
> +                          void *data)
>  {
>         struct bus_type *bus;
>         struct device *dev;
> +       void *ret = NULL;
> 
>         for (bus = generic_match_buses[0]; bus; bus++) {
> -               dev = bus_find_device_by_fwnode(bus, con->fwnode);
> -               if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
> -                       return dev;
> +               dev = bus_find_device_by_fwnode(bus, fwnode);
> +               if (dev) {
> +                       if (!strncmp(dev_name(dev), id, strlen(id)))
> +                               return dev;
> +                       ret = ERR_PTR(-EPROBE_DEFER);
> +               }
> 
> 
> Note that the old generic_match code had:
> 
> -       if (con->fwnode)
> -               return device_connection_fwnode_match(con);
> 
> Which will simply always return either the dev or NULL, so as said this
> is a behavior change AFAICT.
> 
> I've been trying to figure out what you are trying to do here and
> I found a troublesome path through the old code:
> 
> 1. device_connection_find() gets called on a device with a fwnode
> 2. device_connection_find() calls device_connection_find_match()
> 3. device_connection_find_match() calls fwnode_connection_find_match()
> 4. fwnode_connection_find_match() calls fwnode_graph_devcon_match() this returns NULL
> 5. fwnode_connection_find_match() calls fwnode_devcon_match()
> 6. fwnode_devcon_match() creates a struct device_connection with just fwnode set, the rest is 0/NULL
> 7. fwnode_devcon_match() calls generic_match() with this struct
> 8. generic_match() calls device_connection_fwnode_match() because con->fwnode is set
> 9. device_connection_fwnode_match() does the following if a device is found:
>    if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
>        return dev;
>    but con->id is NULL here, so we have a NULL pointer deref here!
> 
> We are currently not hitting this path because there are no callers of
> device_connection_find() all devcon users currently use device_connection_find_match()
> 
> Note I believe the code after your patch still has this problem. Also doing
> the strcmp on the dev_name seems wrong? At least for the above code path, where
> fwnode_devcon_match() has already used / consumed the id.
> 
> So at a minimum we need to add a id == NULL check to generic_match (*), but
> Since there are no users and the strcmp to to dev_name is weird, I believe that
> it would be better to just remove device_connection_find() (and generic_match, etc.) ?

Yes! That makes sense to me. That function really serves no purpose.

> This could be a preparation patch for the patch you attached, this would simplify
> this patch a bit.
> 
> ###
> 
> If we end up with something like your suggested patch I think it might be good to
> do a follow up where device_connection_find_match callers simply call
> fwnode_connection_find_match directly and we remove device_connection_find_match ?
> Or maybe turn it into a static inline function?

Sure. I think it would make sense to move that
fwnode_connection_find_match() function under drivers/base/property.c
at the same time, and get rid of this devcon.c file completely.

But before that, I would like to do a change to the software node
support, if that's OK. I have to now add support for the "device
graph" to the software nodes in any case (not because of this).

I'm really not a fan of that "device graph" thing (I think it's over
engineered) but if the software nodes support it, I think we probable
need to always use it to describe these connections (because that's
what DT uses). That would leave us with only one method of describing
these connections (the device graph) compared to the three we have
now (built-in, device graph and the "references").

In any case, do we leave this series as it is now, or should I add two
patches to it - one that just removes device_connection_add/remove
functions without any other changes, and another patch that removes
that device_connection_find() function (together with generic_match
etc.)?

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-10 11:16         ` Heikki Krogerus
@ 2019-10-10 11:58           ` Heikki Krogerus
  2019-10-10 12:06             ` Hans de Goede
  0 siblings, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2019-10-10 11:58 UTC (permalink / raw)
  To: Hans de Goede
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

On Thu, Oct 10, 2019 at 02:16:23PM +0300, Heikki Krogerus wrote:
> In any case, do we leave this series as it is now, or should I add two
> patches to it - one that just removes device_connection_add/remove
> functions without any other changes, and another patch that removes
> that device_connection_find() function (together with generic_match
> etc.)?

Forget about it. Let's leave this series as it is now.

The device_connection_find() function we can remove separately.

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-10 11:58           ` Heikki Krogerus
@ 2019-10-10 12:06             ` Hans de Goede
  0 siblings, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2019-10-10 12:06 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: MyungJoo Ham, Chanwoo Choi, Greg Kroah-Hartman, linux-kernel, linux-usb

Hi,

On 10-10-2019 13:58, Heikki Krogerus wrote:
> On Thu, Oct 10, 2019 at 02:16:23PM +0300, Heikki Krogerus wrote:
>> In any case, do we leave this series as it is now, or should I add two
>> patches to it - one that just removes device_connection_add/remove
>> functions without any other changes, and another patch that removes
>> that device_connection_find() function (together with generic_match
>> etc.)?
> 
> Forget about it. Let's leave this series as it is now.
> 
> The device_connection_find() function we can remove separately.

That sounds fine to me. Note as mentioned I would remove the
device_connection_find() function before removing the builtin
connection support, that will make the builtin connection support
removal patch a bit cleaner.

Regards,

Hans


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-10-08 13:59 ` [PATCH v2 0/2] extcon: axp288: Move to swnodes Hans de Goede
  2019-10-08 14:01   ` Heikki Krogerus
@ 2019-11-04 13:09   ` Heikki Krogerus
  2019-11-04 14:04     ` Greg Kroah-Hartman
  1 sibling, 1 reply; 12+ messages in thread
From: Heikki Krogerus @ 2019-11-04 13:09 UTC (permalink / raw)
  To: Hans de Goede, Greg Kroah-Hartman
  Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel, linux-usb

Hi Greg,

On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 08-10-2019 14:25, Heikki Krogerus wrote:
> > Hi Hans,
> > 
> > Fixed the compiler warning in this version. No other changes.
> > 
> > The original cover letter:
> > 
> > That AXP288 extcon driver is the last that uses build-in connection
> > description. I'm replacing it with a code that finds the role mux
> > software node instead.
> > 
> > I'm proposing also here a little helper
> > usb_role_switch_find_by_fwnode() that uses
> > class_find_device_by_fwnode() to find the role switches.
> 
> Both patches look good to me and I can confirm that things still
> work as they should on a CHT device with an AXP288 PMIC, so for both:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>

These two patches in this series are basically about the usb role API,
so can you take them?

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] extcon: axp288: Move to swnodes
  2019-11-04 13:09   ` Heikki Krogerus
@ 2019-11-04 14:04     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2019-11-04 14:04 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Hans de Goede, MyungJoo Ham, Chanwoo Choi, linux-kernel, linux-usb

On Mon, Nov 04, 2019 at 03:09:04PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Tue, Oct 08, 2019 at 03:59:23PM +0200, Hans de Goede wrote:
> > Hi,
> > 
> > On 08-10-2019 14:25, Heikki Krogerus wrote:
> > > Hi Hans,
> > > 
> > > Fixed the compiler warning in this version. No other changes.
> > > 
> > > The original cover letter:
> > > 
> > > That AXP288 extcon driver is the last that uses build-in connection
> > > description. I'm replacing it with a code that finds the role mux
> > > software node instead.
> > > 
> > > I'm proposing also here a little helper
> > > usb_role_switch_find_by_fwnode() that uses
> > > class_find_device_by_fwnode() to find the role switches.
> > 
> > Both patches look good to me and I can confirm that things still
> > work as they should on a CHT device with an AXP288 PMIC, so for both:
> > 
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> These two patches in this series are basically about the usb role API,
> so can you take them?

Sure, will do that, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-11-04 14:04 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 12:25 [PATCH v2 0/2] extcon: axp288: Move to swnodes Heikki Krogerus
2019-10-08 12:25 ` [PATCH v2 1/2] usb: roles: Add usb_role_switch_find_by_fwnode() Heikki Krogerus
2019-10-08 12:26 ` [PATCH v2 2/2] extcon: axp288: Remove the build-in connection description Heikki Krogerus
2019-10-08 13:59 ` [PATCH v2 0/2] extcon: axp288: Move to swnodes Hans de Goede
2019-10-08 14:01   ` Heikki Krogerus
2019-10-10  8:31     ` Heikki Krogerus
2019-10-10  9:32       ` Hans de Goede
2019-10-10 11:16         ` Heikki Krogerus
2019-10-10 11:58           ` Heikki Krogerus
2019-10-10 12:06             ` Hans de Goede
2019-11-04 13:09   ` Heikki Krogerus
2019-11-04 14:04     ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).