[PATCH v3 00/13] Software fwnode references

[PATCH v3 00/13] Software fwnode references

[Heikki Krogerus]

Hi,

This is the third version of my proposal to add reference handling to
the software node code. In this version I renamed ACPI_NAME_SIZE to
ACPI_NAMESEG_SIZE in 6/13, and slit patch 9/13 in two separate patches
(9/13 and 10/13) as suggested by Andy. Patch 9/13 will now only move
the registration of max17047 out of probe, and 10/13 will introduce
the software nodes.

v2 commit message:

This is the second version of this series. In this version I'm
introducing a new helper device_find_child_by_name() as proposed
by Andy. Andy requested also another helper that could be used for
chaining the fwnodes, but I decided not to add that now. I would like
to still think about how we should handle exceptions like if there
already is a secondary node assigned for a node.

v1 commit message:

This series adds support for software fwnode reference handling. In
practice it means making fwnode_property_get_reference_args() function
usable in the drivers also with software nodes. I send the series
originally as RFC [1].

As the first user for the software node references, I'm converting
intel_cht_int33fe.c to use them as part of the series.

[1] https://lkml.org/lkml/2019/3/15/457

thanks,

Heikki Krogerus (13):
  software node: Allow node creation without properties
  software node: Simplify software_node_release() function
  software node: Add support for references
  software node: Implement .get_reference_args fwnode operation
  driver core: Add helper device_find_child_by_name()
  ACPI / property: Don't limit named child node matching to data nodes
  device connection: Find connections also by checking the references
  usb: typec: Registering real device entries for the muxes
  platform/x86: intel_cht_int33fe: Register max17047 in its own function
  platform/x86: intel_cht_int33fe: Provide software nodes for the
    devices
  platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector
  platform/x86: intel_cht_int33fe: Link with external dependencies using
    fwnodes
  platform/x86: intel_cht_int33fe: Replacing the old connections with
    references

 drivers/acpi/property.c                  |  26 +-
 drivers/base/core.c                      |  31 +++
 drivers/base/devcon.c                    |  28 ++
 drivers/base/swnode.c                    | 180 +++++++++++-
 drivers/platform/x86/intel_cht_int33fe.c | 339 +++++++++++++++++++----
 drivers/usb/typec/bus.h                  |  15 +
 drivers/usb/typec/class.c                |  17 +-
 drivers/usb/typec/mux.c                  | 221 ++++++++++-----
 drivers/usb/typec/mux/pi3usb30532.c      |  46 +--
 include/linux/device.h                   |   2 +
 include/linux/property.h                 |   8 +
 include/linux/usb/typec_mux.h            |  62 ++---
 12 files changed, 791 insertions(+), 184 deletions(-)

-- 
2.20.1

[PATCH v3 01/13] software node: Allow node creation without properties

[Heikki Krogerus]

Software nodes are not forced to have device properties.
Adding check to property_entries_dup() to make it possible
to create software nodes that don't have any properties.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 7fc5a18e02ad..30077454eb68 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -383,6 +383,9 @@ property_entries_dup(const struct property_entry *properties)
 	int i, n = 0;
 	int ret;
 
+	if (!properties)
+		return NULL;
+
 	while (properties[n].name)
 		n++;
 
-- 
2.20.1

[PATCH v3 02/13] software node: Simplify software_node_release() function

[Heikki Krogerus]

It's possible to release the node ID immediately when
fwnode_remove_software_node() is called, no need to wait for
software_node_release() with that. This change has no
functional effect.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 30077454eb68..7b321bf8424c 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -557,13 +557,6 @@ static void software_node_release(struct kobject *kobj)
 {
 	struct software_node *swnode = kobj_to_swnode(kobj);
 
-	if (swnode->parent) {
-		ida_simple_remove(&swnode->parent->child_ids, swnode->id);
-		list_del(&swnode->entry);
-	} else {
-		ida_simple_remove(&swnode_root_ids, swnode->id);
-	}
-
 	ida_destroy(&swnode->child_ids);
 	property_entries_free(swnode->properties);
 	kfree(swnode);
@@ -610,9 +603,6 @@ fwnode_create_software_node(const struct property_entry *properties,
 	INIT_LIST_HEAD(&swnode->children);
 	swnode->parent = p;
 
-	if (p)
-		list_add_tail(&swnode->entry, &p->children);
-
 	ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
 				   p ? &p->kobj : NULL, "node%d", swnode->id);
 	if (ret) {
@@ -626,6 +616,9 @@ fwnode_create_software_node(const struct property_entry *properties,
 		return ERR_PTR(ret);
 	}
 
+	if (p)
+		list_add_tail(&swnode->entry, &p->children);
+
 	kobject_uevent(&swnode->kobj, KOBJ_ADD);
 	return &swnode->fwnode;
 }
@@ -638,6 +631,13 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode)
 	if (!swnode)
 		return;
 
+	if (swnode->parent) {
+		ida_simple_remove(&swnode->parent->child_ids, swnode->id);
+		list_del(&swnode->entry);
+	} else {
+		ida_simple_remove(&swnode_root_ids, swnode->id);
+	}
+
 	kobject_put(&swnode->kobj);
 }
 EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
-- 
2.20.1

[PATCH v3 03/13] software node: Add support for references

[Heikki Krogerus]

Introducing functions that can be used for adding and
removing references to other software nodes. The goal is to
support fwnode_property_get_reference_args() also with
software nodes, however, get_reference_args fwnode operation
callback is not yet implemented in this commit for the
software nodes. This commit will only add support for
reference addition/removal.

The next example shows how to add references to GPIOs using the
format described in Documentation/acpi/gpio-properties.txt:

/* Array with two GPIOs */
static struct fwnode_reference_args gpioargs[] __initdata = {
	{
		.nargs = 3,
		.args[0]= 19, /* index */
		.args[1]= 0,  /* pin */
		.args[2]= 0,  /* active_low */
	},
	{
		.nargs = 3,
		.args[0]= 20, /* index */
		.args[1]= 0,  /* pin */
		.args[2]= 0,  /* active_low */
	},
	{ }
};

static int myinit(void)
{
	struct software_node_reference *ref;
	struct fwnode_handle *gpio_node;
	struct fwnode_handle *my_node;

	/* Creating the nodes */
	gpio_node = fwnode_create_software_node(gpio_props, NULL);
	...
	my_node = fwnode_create_software_node(my_props, NULL);
	...

	/* gpio_node is associated with a GPIO/Pin controller in this example */
	...

	/* Assigning the actual node references */
	gpioargs[0].fwnode = gpio_node;
	gpioargs[1].fwnode = gpio_node;

	/* my_node will now have a named ("gpios") reference to the two GPIOs */
	ref = fwnode_create_software_node_reference(my_node, "gpios", gpioargs);
	...
	return 0;
}

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c    | 101 +++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |   8 ++++
 2 files changed, 109 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 7b321bf8424c..39b8f8f35cfe 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -11,10 +11,19 @@
 #include <linux/property.h>
 #include <linux/slab.h>
 
+struct software_node_reference {
+	struct list_head list;
+	const char *name;
+	int nrefs;
+	struct fwnode_reference_args *args;
+};
+
 struct software_node {
 	int id;
 	struct kobject kobj;
 	struct fwnode_handle fwnode;
+	struct list_head references;
+	struct mutex lock; /* node lock */
 
 	/* hierarchy */
 	struct ida child_ids;
@@ -598,9 +607,11 @@ fwnode_create_software_node(const struct property_entry *properties,
 	swnode->kobj.kset = swnode_kset;
 	swnode->fwnode.ops = &software_node_ops;
 
+	mutex_init(&swnode->lock);
 	ida_init(&swnode->child_ids);
 	INIT_LIST_HEAD(&swnode->entry);
 	INIT_LIST_HEAD(&swnode->children);
+	INIT_LIST_HEAD(&swnode->references);
 	swnode->parent = p;
 
 	ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
@@ -631,6 +642,11 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode)
 	if (!swnode)
 		return;
 
+	mutex_lock(&swnode->lock);
+	WARN(!list_empty(&swnode->references),
+	     "\"%s\" has still references", kobject_name(&swnode->kobj));
+	mutex_unlock(&swnode->lock);
+
 	if (swnode->parent) {
 		ida_simple_remove(&swnode->parent->child_ids, swnode->id);
 		list_del(&swnode->entry);
@@ -642,6 +658,91 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
 
+/**
+ * fwnode_create_software_node_reference - Create named reference description
+ * @fwnode: The software node to have the references
+ * @name: Name given to reference description
+ * @args: Zero terminated array of software node references with arguments
+ *
+ * Associates software nodes listed in @args with @fwnode. The association is
+ * named @name. The reference count is incremented for the nodes in @args.
+ *
+ * Returns pointer to software node reference description on success, or ERR_PTR
+ * on failure.
+ */
+struct software_node_reference *
+fwnode_create_software_node_reference(const struct fwnode_handle *fwnode,
+				      const char *name,
+				      const struct fwnode_reference_args *args)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+	struct software_node_reference *ref;
+	int n;
+
+	if (!swnode)
+		return ERR_PTR(-EINVAL);
+
+	for (n = 0; args[n].fwnode; n++)
+		if (!is_software_node(args[n].fwnode))
+			return ERR_PTR(-EINVAL);
+
+	ref = kzalloc(sizeof(*ref), GFP_KERNEL);
+	if (!ref)
+		return ERR_PTR(-ENOMEM);
+
+	ref->nrefs = n;
+
+	ref->name = kstrdup(name, GFP_KERNEL);
+	if (!ref->name) {
+		kfree(ref);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	ref->args = kcalloc(ref->nrefs, sizeof(*ref->args), GFP_KERNEL);
+	if (!ref->args) {
+		kfree(ref->name);
+		kfree(ref);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (n = 0; n < ref->nrefs; n++) {
+		ref->args[n] = args[n];
+		software_node_get(ref->args[n].fwnode);
+	}
+
+	mutex_lock(&swnode->lock);
+	list_add_tail(&ref->list, &swnode->references);
+	mutex_unlock(&swnode->lock);
+
+	return ref;
+}
+EXPORT_SYMBOL_GPL(fwnode_create_software_node_reference);
+
+/**
+ * fwnode_remove_software_node_reference - Remove named reference description
+ * @ref: Software node reference description
+ *
+ * Remove named reference @ref. Decrements the software node reference count of
+ * each node in @ref, and removes the association that was created in
+ * fwnode_create_software_node_reference().
+ */
+void fwnode_remove_software_node_reference(struct software_node_reference *ref)
+{
+	int n;
+
+	if (IS_ERR_OR_NULL(ref))
+		return;
+
+	for (n = 0; n < ref->nrefs; n++)
+		kobject_put(&to_software_node(ref->args[n].fwnode)->kobj);
+
+	list_del(&ref->list);
+	kfree(ref->args);
+	kfree(ref->name);
+	kfree(ref);
+}
+EXPORT_SYMBOL_GPL(fwnode_remove_software_node_reference);
+
 int software_node_notify(struct device *dev, unsigned long action)
 {
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
diff --git a/include/linux/property.h b/include/linux/property.h
index 65d3420dd5d1..40e12ca43556 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -314,6 +314,8 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
+struct sofware_node_reference;
+
 bool is_software_node(const struct fwnode_handle *fwnode);
 
 int software_node_notify(struct device *dev, unsigned long action);
@@ -323,4 +325,10 @@ fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent);
 void fwnode_remove_software_node(struct fwnode_handle *fwnode);
 
+struct software_node_reference *
+fwnode_create_software_node_reference(const struct fwnode_handle *fwnode,
+				      const char *name,
+				      const struct fwnode_reference_args *args);
+void fwnode_remove_software_node_reference(struct software_node_reference *ref);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.20.1

[PATCH v3 04/13] software node: Implement .get_reference_args fwnode operation

[Heikki Krogerus]

This makes it possible to support drivers that use
fwnode_property_get_reference_args() function.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c | 56 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 39b8f8f35cfe..d6a9b56cb073 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -534,6 +534,61 @@ software_node_get_named_child_node(const struct fwnode_handle *fwnode,
 	return NULL;
 }
 
+static int
+software_node_get_reference_args(const struct fwnode_handle *fwnode,
+				 const char *propname, const char *nargs_prop,
+				 unsigned int nargs, unsigned int index,
+				 struct fwnode_reference_args *args)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+	struct software_node_reference *ref;
+	const struct property_entry *prop;
+	int ret = -ENOENT;
+	int i;
+
+	mutex_lock(&swnode->lock);
+
+	if (!swnode || list_empty(&swnode->references))
+		goto err_unlock;
+
+	if (nargs_prop) {
+		prop = property_entry_get(swnode->properties, nargs_prop);
+		if (!prop) {
+			ret = -EINVAL;
+			goto err_unlock;
+		}
+
+		nargs = prop->value.u32_data;
+	}
+
+	if (nargs > NR_FWNODE_REFERENCE_ARGS) {
+		ret = -EINVAL;
+		goto err_unlock;
+	}
+
+	list_for_each_entry(ref, &swnode->references, list) {
+		if (strcmp(ref->name, propname))
+			continue;
+
+		if (index > (ref->nrefs - 1))
+			break;
+
+		args->nargs = nargs;
+		args->fwnode = software_node_get(ref->args[index].fwnode);
+
+		for (i = 0; i < nargs; i++)
+			args->args[i] = ref->args[index].args[i];
+
+		ret = 0;
+		break;
+	}
+
+err_unlock:
+	mutex_unlock(&swnode->lock);
+
+	return ret;
+}
+
 static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
@@ -543,6 +598,7 @@ static const struct fwnode_operations software_node_ops = {
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
+	.get_reference_args = software_node_get_reference_args,
 };
 
 /* -------------------------------------------------------------------------- */
-- 
2.20.1

[PATCH v3 05/13] driver core: Add helper device_find_child_by_name()

[Heikki Krogerus]

It looks like the child device is often matched with a name.
This introduces a helper that does it automatically.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/core.c    | 31 +++++++++++++++++++++++++++++++
 include/linux/device.h |  2 ++
 2 files changed, 33 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4aeaa0c92bda..5819ba4c9d4c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2469,6 +2469,37 @@ struct device *device_find_child(struct device *parent, void *data,
 }
 EXPORT_SYMBOL_GPL(device_find_child);
 
+/**
+ * device_find_child_by_name - device iterator for locating a child device.
+ * @parent: parent struct device
+ * @name: name of the child device
+ *
+ * This is similar to the device_find_child() function above, but it
+ * returns a reference to a device that has the name @name.
+ *
+ * The callback should return NULL if the child is not found and a reference to
+ * the child device if it is found.
+ *
+ * NOTE: you will need to drop the reference with put_device() after use.
+ */
+struct device *device_find_child_by_name(struct device *parent,
+					 const char *name)
+{
+	struct klist_iter i;
+	struct device *child;
+
+	if (!parent)
+		return NULL;
+
+	klist_iter_init(&parent->p->klist_children, &i);
+	while ((child = next_device(&i)))
+		if (!strcmp(dev_name(child), name) && get_device(child))
+			break;
+	klist_iter_exit(&i);
+	return child;
+}
+EXPORT_SYMBOL_GPL(device_find_child_by_name);
+
 int __init devices_init(void)
 {
 	devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL);
diff --git a/include/linux/device.h b/include/linux/device.h
index 4457e560bc2b..fadb84dd07f9 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1250,6 +1250,8 @@ extern int device_for_each_child_reverse(struct device *dev, void *data,
 		     int (*fn)(struct device *dev, void *data));
 extern struct device *device_find_child(struct device *dev, void *data,
 				int (*match)(struct device *dev, void *data));
+extern struct device *device_find_child_by_name(struct device *parent,
+						const char *name);
 extern int device_rename(struct device *dev, const char *new_name);
 extern int device_move(struct device *dev, struct device *new_parent,
 		       enum dpm_order dpm_order);
-- 
2.20.1

Re: [PATCH v3 05/13] driver core: Add helper device_find_child_by_name()

[Greg Kroah-Hartman]

On Fri, Apr 12, 2019 at 04:41:14PM +0300, Heikki Krogerus wrote:
> It looks like the child device is often matched with a name.
> This introduces a helper that does it automatically.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v3 06/13] ACPI / property: Don't limit named child node matching to data nodes

[Heikki Krogerus]

There is no reason why we should limit the use of
fwnode_get_named_child_node() to data nodes only.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/property.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 77abe0ec4043..572a6868ff2e 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -602,15 +602,29 @@ static struct fwnode_handle *
 acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 				 const char *childname)
 {
+	char name[ACPI_PATH_SEGMENT_LENGTH];
 	struct fwnode_handle *child;
+	struct acpi_buffer path;
+	acpi_status status;
 
-	/*
-	 * Find first matching named child node of this fwnode.
-	 * For ACPI this will be a data only sub-node.
-	 */
-	fwnode_for_each_child_node(fwnode, child)
-		if (acpi_data_node_match(child, childname))
+	path.length = sizeof(name);
+	path.pointer = name;
+
+	fwnode_for_each_child_node(fwnode, child) {
+		if (is_acpi_data_node(child)) {
+			if (acpi_data_node_match(child, childname))
+				return child;
+			continue;
+		}
+
+		status = acpi_get_name(ACPI_HANDLE_FWNODE(child),
+				       ACPI_SINGLE_NAME, &path);
+		if (ACPI_FAILURE(status))
+			break;
+
+		if (!strncmp(name, childname, ACPI_NAMESEG_SIZE))
 			return child;
+	}
 
 	return NULL;
 }
-- 
2.20.1

[PATCH v3 07/13] device connection: Find connections also by checking the references

[Heikki Krogerus]

We can also use this API to find named references that the
device nodes have by using fwnode_property_get_reference_args()
function.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/devcon.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 04db9ae235e4..4cdf95532b63 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -38,6 +38,30 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 	return NULL;
 }
 
+static void *
+fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
+		    void *data, devcon_match_fn_t match)
+{
+	struct device_connection con = { };
+	struct fwnode_reference_args args;
+	void *ret;
+	int i;
+
+	for (i = 0; ; i++) {
+		if (fwnode_property_get_reference_args(fwnode, con_id, NULL, 0,
+						       i, &args))
+			break;
+
+		con.fwnode = args.fwnode;
+		ret = match(&con, -1, data);
+		fwnode_handle_put(args.fwnode);
+		if (ret)
+			return ret;
+	}
+
+	return NULL;
+}
+
 /**
  * device_connection_find_match - Find physical connection to a device
  * @dev: Device with the connection
@@ -65,6 +89,10 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
 		ret = fwnode_graph_devcon_match(fwnode, con_id, data, match);
 		if (ret)
 			return ret;
+
+		ret = fwnode_devcon_match(fwnode, con_id, data, match);
+		if (ret)
+			return ret;
 	}
 
 	mutex_lock(&devcon_lock);
-- 
2.20.1

[PATCH v3 08/13] usb: typec: Registering real device entries for the muxes

[Heikki Krogerus]

Registering real device entries (struct device) for the mode
muxes as well as for the orientation switches.

The Type-C mux code was deliberately attempting to avoid
creation of separate device entries for the orientation
switch and the mode switch (alternate modes) because they
are not physical devices. They are functions of a single
physical multiplexer/demultiplexer switch device.

Unfortunately because of the dependency we still have on the
underlying mux device driver, we had to put in hacks like
the one in the commit 3e3b81965cbf ("usb: typec: mux: Take
care of driver module reference counting") to make sure the
driver does not disappear from underneath us. Even with
those hacks we were still left with a potential NUll pointer
dereference scenario, so just creating the device entries,
and letting the core take care of the dependencies. No more
hacks needed.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c |   4 +-
 drivers/usb/typec/bus.h                  |  15 ++
 drivers/usb/typec/class.c                |  17 +-
 drivers/usb/typec/mux.c                  | 221 ++++++++++++++++-------
 drivers/usb/typec/mux/pi3usb30532.c      |  46 +++--
 include/linux/usb/typec_mux.h            |  62 +++----
 6 files changed, 242 insertions(+), 123 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 6fa3cced6f8e..657b8d61554c 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -173,10 +173,10 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	}
 
 	data->connections[0].endpoint[0] = "port0";
-	data->connections[0].endpoint[1] = "i2c-pi3usb30532";
+	data->connections[0].endpoint[1] = "i2c-pi3usb30532-switch";
 	data->connections[0].id = "orientation-switch";
 	data->connections[1].endpoint[0] = "port0";
-	data->connections[1].endpoint[1] = "i2c-pi3usb30532";
+	data->connections[1].endpoint[1] = "i2c-pi3usb30532-mux";
 	data->connections[1].id = "mode-switch";
 	data->connections[2].endpoint[0] = "i2c-fusb302";
 	data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
diff --git a/drivers/usb/typec/bus.h b/drivers/usb/typec/bus.h
index db40e61d8b72..0c9661c96473 100644
--- a/drivers/usb/typec/bus.h
+++ b/drivers/usb/typec/bus.h
@@ -35,4 +35,19 @@ extern const struct device_type typec_port_dev_type;
 #define is_typec_altmode(_dev_) (_dev_->type == &typec_altmode_dev_type)
 #define is_typec_port(_dev_) (_dev_->type == &typec_port_dev_type)
 
+extern struct class typec_mux_class;
+
+struct typec_switch {
+	struct device dev;
+	typec_switch_set_fn_t set;
+};
+
+struct typec_mux {
+	struct device dev;
+	typec_mux_set_fn_t set;
+};
+
+#define to_typec_switch(_dev_) container_of(_dev_, struct typec_switch, dev)
+#define to_typec_mux(_dev_) container_of(_dev_, struct typec_mux, dev)
+
 #endif /* __USB_TYPEC_ALTMODE_H__ */
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 2eb623841847..a18285a990a8 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1646,13 +1646,25 @@ static int __init typec_init(void)
 	if (ret)
 		return ret;
 
+	ret = class_register(&typec_mux_class);
+	if (ret)
+		goto err_unregister_bus;
+
 	typec_class = class_create(THIS_MODULE, "typec");
 	if (IS_ERR(typec_class)) {
-		bus_unregister(&typec_bus);
-		return PTR_ERR(typec_class);
+		ret = PTR_ERR(typec_class);
+		goto err_unregister_mux_class;
 	}
 
 	return 0;
+
+err_unregister_mux_class:
+	class_unregister(&typec_mux_class);
+
+err_unregister_bus:
+	bus_unregister(&typec_bus);
+
+	return ret;
 }
 subsys_initcall(typec_init);
 
@@ -1661,6 +1673,7 @@ static void __exit typec_exit(void)
 	class_destroy(typec_class);
 	ida_destroy(&typec_index_ida);
 	bus_unregister(&typec_bus);
+	class_unregister(&typec_mux_class);
 }
 module_exit(typec_exit);
 
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 2ce54f3fc79c..15a0e76f792c 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -15,35 +15,35 @@
 #include <linux/slab.h>
 #include <linux/usb/typec_mux.h>
 
-static DEFINE_MUTEX(switch_lock);
-static DEFINE_MUTEX(mux_lock);
-static LIST_HEAD(switch_list);
-static LIST_HEAD(mux_list);
+#include "bus.h"
+
+static int name_match(struct device *dev, const void *name)
+{
+	return !strcmp((const char *)name, dev_name(dev));
+}
+
+static int fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
 
 static void *typec_switch_match(struct device_connection *con, int ep,
 				void *data)
 {
-	struct typec_switch *sw;
+	struct device *dev;
 
-	if (!con->fwnode) {
-		list_for_each_entry(sw, &switch_list, entry)
-			if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
-				return sw;
-		return ERR_PTR(-EPROBE_DEFER);
-	}
+	if (con->fwnode) {
+		if (con->id && !fwnode_property_present(con->fwnode, con->id))
+			return NULL;
 
-	/*
-	 * With OF graph the mux node must have a boolean device property named
-	 * "orientation-switch".
-	 */
-	if (con->id && !fwnode_property_present(con->fwnode, con->id))
-		return NULL;
-
-	list_for_each_entry(sw, &switch_list, entry)
-		if (dev_fwnode(sw->dev) == con->fwnode)
-			return sw;
+		dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
+					fwnode_match);
+	} else {
+		dev = class_find_device(&typec_mux_class, NULL,
+					con->endpoint[ep], name_match);
+	}
 
-	return con->id ? ERR_PTR(-EPROBE_DEFER) : NULL;
+	return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
 
 /**
@@ -59,14 +59,10 @@ struct typec_switch *typec_switch_get(struct device *dev)
 {
 	struct typec_switch *sw;
 
-	mutex_lock(&switch_lock);
 	sw = device_connection_find_match(dev, "orientation-switch", NULL,
 					  typec_switch_match);
-	if (!IS_ERR_OR_NULL(sw)) {
-		WARN_ON(!try_module_get(sw->dev->driver->owner));
-		get_device(sw->dev);
-	}
-	mutex_unlock(&switch_lock);
+	if (!IS_ERR_OR_NULL(sw))
+		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
 
 	return sw;
 }
@@ -81,28 +77,64 @@ EXPORT_SYMBOL_GPL(typec_switch_get);
 void typec_switch_put(struct typec_switch *sw)
 {
 	if (!IS_ERR_OR_NULL(sw)) {
-		module_put(sw->dev->driver->owner);
-		put_device(sw->dev);
+		module_put(sw->dev.parent->driver->owner);
+		put_device(&sw->dev);
 	}
 }
 EXPORT_SYMBOL_GPL(typec_switch_put);
 
+static void typec_switch_release(struct device *dev)
+{
+	kfree(to_typec_switch(dev));
+}
+
+static const struct device_type typec_switch_dev_type = {
+	.name = "orientation_switch",
+	.release = typec_switch_release,
+};
+
 /**
  * typec_switch_register - Register USB Type-C orientation switch
- * @sw: USB Type-C orientation switch
+ * @parent: Parent device
+ * @desc: Orientation switch description
  *
  * This function registers a switch that can be used for routing the correct
  * data pairs depending on the cable plug orientation from the USB Type-C
  * connector to the USB controllers. USB Type-C plugs can be inserted
  * right-side-up or upside-down.
  */
-int typec_switch_register(struct typec_switch *sw)
+struct typec_switch *
+typec_switch_register(struct device *parent,
+		      const struct typec_switch_desc *desc)
 {
-	mutex_lock(&switch_lock);
-	list_add_tail(&sw->entry, &switch_list);
-	mutex_unlock(&switch_lock);
+	struct typec_switch *sw;
+	int ret;
+
+	if (!desc || !desc->set)
+		return ERR_PTR(-EINVAL);
 
-	return 0;
+	sw = kzalloc(sizeof(*sw), GFP_KERNEL);
+	if (!sw)
+		return ERR_PTR(-ENOMEM);
+
+	sw->set = desc->set;
+
+	device_initialize(&sw->dev);
+	sw->dev.parent = parent;
+	sw->dev.fwnode = desc->fwnode;
+	sw->dev.class = &typec_mux_class;
+	sw->dev.type = &typec_switch_dev_type;
+	sw->dev.driver_data = desc->drvdata;
+	dev_set_name(&sw->dev, "%s-switch", dev_name(parent));
+
+	ret = device_add(&sw->dev);
+	if (ret) {
+		dev_err(parent, "failed to register switch (%d)\n", ret);
+		put_device(&sw->dev);
+		return ERR_PTR(ret);
+	}
+
+	return sw;
 }
 EXPORT_SYMBOL_GPL(typec_switch_register);
 
@@ -114,28 +146,39 @@ EXPORT_SYMBOL_GPL(typec_switch_register);
  */
 void typec_switch_unregister(struct typec_switch *sw)
 {
-	mutex_lock(&switch_lock);
-	list_del(&sw->entry);
-	mutex_unlock(&switch_lock);
+	if (!IS_ERR_OR_NULL(sw))
+		device_unregister(&sw->dev);
 }
 EXPORT_SYMBOL_GPL(typec_switch_unregister);
 
+void typec_switch_set_drvdata(struct typec_switch *sw, void *data)
+{
+	dev_set_drvdata(&sw->dev, data);
+}
+EXPORT_SYMBOL_GPL(typec_switch_set_drvdata);
+
+void *typec_switch_get_drvdata(struct typec_switch *sw)
+{
+	return dev_get_drvdata(&sw->dev);
+}
+EXPORT_SYMBOL_GPL(typec_switch_get_drvdata);
+
 /* ------------------------------------------------------------------------- */
 
 static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 {
 	const struct typec_altmode_desc *desc = data;
-	struct typec_mux *mux;
-	int nval;
+	struct device *dev;
 	bool match;
+	int nval;
 	u16 *val;
 	int i;
 
 	if (!con->fwnode) {
-		list_for_each_entry(mux, &mux_list, entry)
-			if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
-				return mux;
-		return ERR_PTR(-EPROBE_DEFER);
+		dev = class_find_device(&typec_mux_class, NULL,
+					con->endpoint[ep], name_match);
+
+		return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 	}
 
 	/*
@@ -180,11 +223,10 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 	return NULL;
 
 find_mux:
-	list_for_each_entry(mux, &mux_list, entry)
-		if (dev_fwnode(mux->dev) == con->fwnode)
-			return mux;
+	dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
+				fwnode_match);
 
-	return ERR_PTR(-EPROBE_DEFER);
+	return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
 
 /**
@@ -202,14 +244,10 @@ struct typec_mux *typec_mux_get(struct device *dev,
 {
 	struct typec_mux *mux;
 
-	mutex_lock(&mux_lock);
 	mux = device_connection_find_match(dev, "mode-switch", (void *)desc,
 					   typec_mux_match);
-	if (!IS_ERR_OR_NULL(mux)) {
-		WARN_ON(!try_module_get(mux->dev->driver->owner));
-		get_device(mux->dev);
-	}
-	mutex_unlock(&mux_lock);
+	if (!IS_ERR_OR_NULL(mux))
+		WARN_ON(!try_module_get(mux->dev.parent->driver->owner));
 
 	return mux;
 }
@@ -224,28 +262,63 @@ EXPORT_SYMBOL_GPL(typec_mux_get);
 void typec_mux_put(struct typec_mux *mux)
 {
 	if (!IS_ERR_OR_NULL(mux)) {
-		module_put(mux->dev->driver->owner);
-		put_device(mux->dev);
+		module_put(mux->dev.parent->driver->owner);
+		put_device(&mux->dev);
 	}
 }
 EXPORT_SYMBOL_GPL(typec_mux_put);
 
+static void typec_mux_release(struct device *dev)
+{
+	kfree(to_typec_mux(dev));
+}
+
+static const struct device_type typec_mux_dev_type = {
+	.name = "mode_switch",
+	.release = typec_mux_release,
+};
+
 /**
  * typec_mux_register - Register Multiplexer routing USB Type-C pins
- * @mux: USB Type-C Connector Multiplexer/DeMultiplexer
+ * @parent: Parent device
+ * @desc: Multiplexer description
  *
  * USB Type-C connectors can be used for alternate modes of operation besides
  * USB when Accessory/Alternate Modes are supported. With some of those modes,
  * the pins on the connector need to be reconfigured. This function registers
  * multiplexer switches routing the pins on the connector.
  */
-int typec_mux_register(struct typec_mux *mux)
+struct typec_mux *
+typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
 {
-	mutex_lock(&mux_lock);
-	list_add_tail(&mux->entry, &mux_list);
-	mutex_unlock(&mux_lock);
+	struct typec_mux *mux;
+	int ret;
+
+	if (!desc || !desc->set)
+		return ERR_PTR(-EINVAL);
+
+	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
 
-	return 0;
+	mux->set = desc->set;
+
+	device_initialize(&mux->dev);
+	mux->dev.parent = parent;
+	mux->dev.fwnode = desc->fwnode;
+	mux->dev.class = &typec_mux_class;
+	mux->dev.type = &typec_mux_dev_type;
+	mux->dev.driver_data = desc->drvdata;
+	dev_set_name(&mux->dev, "%s-mux", dev_name(parent));
+
+	ret = device_add(&mux->dev);
+	if (ret) {
+		dev_err(parent, "failed to register mux (%d)\n", ret);
+		put_device(&mux->dev);
+		return ERR_PTR(ret);
+	}
+
+	return mux;
 }
 EXPORT_SYMBOL_GPL(typec_mux_register);
 
@@ -257,8 +330,24 @@ EXPORT_SYMBOL_GPL(typec_mux_register);
  */
 void typec_mux_unregister(struct typec_mux *mux)
 {
-	mutex_lock(&mux_lock);
-	list_del(&mux->entry);
-	mutex_unlock(&mux_lock);
+	if (!IS_ERR_OR_NULL(mux))
+		device_unregister(&mux->dev);
 }
 EXPORT_SYMBOL_GPL(typec_mux_unregister);
+
+void typec_mux_set_drvdata(struct typec_mux *mux, void *data)
+{
+	dev_set_drvdata(&mux->dev, data);
+}
+EXPORT_SYMBOL_GPL(typec_mux_set_drvdata);
+
+void *typec_mux_get_drvdata(struct typec_mux *mux)
+{
+	return dev_get_drvdata(&mux->dev);
+}
+EXPORT_SYMBOL_GPL(typec_mux_get_drvdata);
+
+struct class typec_mux_class = {
+	.name = "typec_mux",
+	.owner = THIS_MODULE,
+};
diff --git a/drivers/usb/typec/mux/pi3usb30532.c b/drivers/usb/typec/mux/pi3usb30532.c
index 9294e85fd34b..5585b109095b 100644
--- a/drivers/usb/typec/mux/pi3usb30532.c
+++ b/drivers/usb/typec/mux/pi3usb30532.c
@@ -23,8 +23,8 @@
 struct pi3usb30532 {
 	struct i2c_client *client;
 	struct mutex lock; /* protects the cached conf register */
-	struct typec_switch sw;
-	struct typec_mux mux;
+	struct typec_switch *sw;
+	struct typec_mux *mux;
 	u8 conf;
 };
 
@@ -48,7 +48,7 @@ static int pi3usb30532_set_conf(struct pi3usb30532 *pi, u8 new_conf)
 static int pi3usb30532_sw_set(struct typec_switch *sw,
 			      enum typec_orientation orientation)
 {
-	struct pi3usb30532 *pi = container_of(sw, struct pi3usb30532, sw);
+	struct pi3usb30532 *pi = typec_switch_get_drvdata(sw);
 	u8 new_conf;
 	int ret;
 
@@ -75,7 +75,7 @@ static int pi3usb30532_sw_set(struct typec_switch *sw,
 
 static int pi3usb30532_mux_set(struct typec_mux *mux, int state)
 {
-	struct pi3usb30532 *pi = container_of(mux, struct pi3usb30532, mux);
+	struct pi3usb30532 *pi = typec_mux_get_drvdata(mux);
 	u8 new_conf;
 	int ret;
 
@@ -113,6 +113,8 @@ static int pi3usb30532_mux_set(struct typec_mux *mux, int state)
 static int pi3usb30532_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
+	struct typec_switch_desc sw_desc;
+	struct typec_mux_desc mux_desc;
 	struct pi3usb30532 *pi;
 	int ret;
 
@@ -121,10 +123,6 @@ static int pi3usb30532_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	pi->client = client;
-	pi->sw.dev = dev;
-	pi->sw.set = pi3usb30532_sw_set;
-	pi->mux.dev = dev;
-	pi->mux.set = pi3usb30532_mux_set;
 	mutex_init(&pi->lock);
 
 	ret = i2c_smbus_read_byte_data(client, PI3USB30532_CONF);
@@ -134,17 +132,27 @@ static int pi3usb30532_probe(struct i2c_client *client)
 	}
 	pi->conf = ret;
 
-	ret = typec_switch_register(&pi->sw);
-	if (ret) {
-		dev_err(dev, "Error registering typec switch: %d\n", ret);
-		return ret;
+	sw_desc.drvdata = pi;
+	sw_desc.fwnode = dev->fwnode;
+	sw_desc.set = pi3usb30532_sw_set;
+
+	pi->sw = typec_switch_register(dev, &sw_desc);
+	if (IS_ERR(pi->sw)) {
+		dev_err(dev, "Error registering typec switch: %ld\n",
+			PTR_ERR(pi->sw));
+		return PTR_ERR(pi->sw);
 	}
 
-	ret = typec_mux_register(&pi->mux);
-	if (ret) {
-		typec_switch_unregister(&pi->sw);
-		dev_err(dev, "Error registering typec mux: %d\n", ret);
-		return ret;
+	mux_desc.drvdata = pi;
+	mux_desc.fwnode = dev->fwnode;
+	mux_desc.set = pi3usb30532_mux_set;
+
+	pi->mux = typec_mux_register(dev, &mux_desc);
+	if (IS_ERR(pi->mux)) {
+		typec_switch_unregister(pi->sw);
+		dev_err(dev, "Error registering typec mux: %ld\n",
+			PTR_ERR(pi->mux));
+		return PTR_ERR(pi->mux);
 	}
 
 	i2c_set_clientdata(client, pi);
@@ -155,8 +163,8 @@ static int pi3usb30532_remove(struct i2c_client *client)
 {
 	struct pi3usb30532 *pi = i2c_get_clientdata(client);
 
-	typec_mux_unregister(&pi->mux);
-	typec_switch_unregister(&pi->sw);
+	typec_mux_unregister(pi->mux);
+	typec_switch_unregister(pi->sw);
 	return 0;
 }
 
diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
index 43f40685e53c..873ace5b0cf8 100644
--- a/include/linux/usb/typec_mux.h
+++ b/include/linux/usb/typec_mux.h
@@ -3,54 +3,48 @@
 #ifndef __USB_TYPEC_MUX
 #define __USB_TYPEC_MUX
 
-#include <linux/list.h>
 #include <linux/usb/typec.h>
 
 struct device;
+struct typec_mux;
+struct typec_switch;
+struct fwnode_handle;
 
-/**
- * struct typec_switch - USB Type-C cable orientation switch
- * @dev: Switch device
- * @entry: List entry
- * @set: Callback to the driver for setting the orientation
- *
- * USB Type-C pin flipper switch routing the correct data pairs from the
- * connector to the USB controller depending on the orientation of the cable
- * plug.
- */
-struct typec_switch {
-	struct device *dev;
-	struct list_head entry;
-
-	int (*set)(struct typec_switch *sw, enum typec_orientation orientation);
-};
+typedef int (*typec_switch_set_fn_t)(struct typec_switch *sw,
+				     enum typec_orientation orientation);
 
-/**
- * struct typec_switch - USB Type-C connector pin mux
- * @dev: Mux device
- * @entry: List entry
- * @set: Callback to the driver for setting the state of the mux
- *
- * Pin Multiplexer/DeMultiplexer switch routing the USB Type-C connector pins to
- * different components depending on the requested mode of operation. Used with
- * Accessory/Alternate modes.
- */
-struct typec_mux {
-	struct device *dev;
-	struct list_head entry;
-
-	int (*set)(struct typec_mux *mux, int state);
+struct typec_switch_desc {
+	struct fwnode_handle *fwnode;
+	typec_switch_set_fn_t set;
+	void *drvdata;
 };
 
 struct typec_switch *typec_switch_get(struct device *dev);
 void typec_switch_put(struct typec_switch *sw);
-int typec_switch_register(struct typec_switch *sw);
+struct typec_switch *
+typec_switch_register(struct device *parent,
+		      const struct typec_switch_desc *desc);
 void typec_switch_unregister(struct typec_switch *sw);
 
+void typec_switch_set_drvdata(struct typec_switch *sw, void *data);
+void *typec_switch_get_drvdata(struct typec_switch *sw);
+
+typedef int (*typec_mux_set_fn_t)(struct typec_mux *mux, int state);
+
+struct typec_mux_desc {
+	struct fwnode_handle *fwnode;
+	typec_mux_set_fn_t set;
+	void *drvdata;
+};
+
 struct typec_mux *
 typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc);
 void typec_mux_put(struct typec_mux *mux);
-int typec_mux_register(struct typec_mux *mux);
+struct typec_mux *
+typec_mux_register(struct device *parent, const struct typec_mux_desc *desc);
 void typec_mux_unregister(struct typec_mux *mux);
 
+void typec_mux_set_drvdata(struct typec_mux *mux, void *data);
+void *typec_mux_get_drvdata(struct typec_mux *mux);
+
 #endif /* __USB_TYPEC_MUX */
-- 
2.20.1

[PATCH v3 09/13] platform/x86: intel_cht_int33fe: Register max17047 in its own function

[Heikki Krogerus]

Moving the registration of max17047 out of probe to its own
function. This is only a cleanup of the probe. There should
be no functional affect.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 64 +++++++++++++-----------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 657b8d61554c..d586bb22d7e8 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -63,14 +63,6 @@ static int cht_int33fe_check_for_max17047(struct device *dev, void *data)
 	return 1;
 }
 
-static struct i2c_client *cht_int33fe_find_max17047(void)
-{
-	struct i2c_client *max17047 = NULL;
-
-	i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047);
-	return max17047;
-}
-
 static const char * const max17047_suppliers[] = { "bq24190-charger" };
 
 static const struct property_entry max17047_props[] = {
@@ -78,6 +70,37 @@ static const struct property_entry max17047_props[] = {
 	{ }
 };
 
+static int
+cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
+{
+	struct i2c_client *max17047 = NULL;
+	struct i2c_board_info board_info;
+	int ret;
+
+	i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047);
+	if (max17047) {
+		/* Pre-existing i2c-client for the max17047, add device-props */
+		ret = device_add_properties(&max17047->dev, max17047_props);
+		if (ret)
+			return ret;
+		/* And re-probe to get the new device-props applied. */
+		ret = device_reprobe(&max17047->dev);
+		if (ret)
+			dev_warn(dev, "Reprobing max17047 error: %d\n", ret);
+		return 0;
+	}
+
+	memset(&board_info, 0, sizeof(board_info));
+	strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
+	board_info.dev_name = "max17047";
+	board_info.properties = max17047_props;
+	data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
+	if (IS_ERR(data->max17047))
+		return PTR_ERR(data->max17047);
+
+	return 0;
+}
+
 static const struct property_entry fusb302_props[] = {
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
 	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
@@ -91,7 +114,6 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct i2c_board_info board_info;
 	struct cht_int33fe_data *data;
-	struct i2c_client *max17047;
 	struct regulator *regulator;
 	unsigned long long ptyp;
 	acpi_status status;
@@ -151,26 +173,10 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
-	/* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */
-	max17047 = cht_int33fe_find_max17047();
-	if (max17047) {
-		/* Pre-existing i2c-client for the max17047, add device-props */
-		ret = device_add_properties(&max17047->dev, max17047_props);
-		if (ret)
-			return ret;
-		/* And re-probe to get the new device-props applied. */
-		ret = device_reprobe(&max17047->dev);
-		if (ret)
-			dev_warn(dev, "Reprobing max17047 error: %d\n", ret);
-	} else {
-		memset(&board_info, 0, sizeof(board_info));
-		strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
-		board_info.dev_name = "max17047";
-		board_info.properties = max17047_props;
-		data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
-		if (IS_ERR(data->max17047))
-			return PTR_ERR(data->max17047);
-	}
+	/* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */
+	ret = cht_int33fe_register_max17047(dev, data);
+	if (ret)
+		return ret;
 
 	data->connections[0].endpoint[0] = "port0";
 	data->connections[0].endpoint[1] = "i2c-pi3usb30532-switch";
-- 
2.20.1

[PATCH v3 10/13] platform/x86: intel_cht_int33fe: Provide software nodes for the devices

[Heikki Krogerus]

Software nodes provide two features that we will need later.
1) Software nodes can have references to other software nodes.
2) Software nodes can exist before a device entry is created.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 68 +++++++++++++++++++++---
 1 file changed, 62 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index d586bb22d7e8..863a792d9282 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -27,12 +27,21 @@
 
 #define EXPECTED_PTYPE		4
 
+enum {
+	INT33FE_NODE_FUSB302,
+	INT33FE_NODE_MAX17047,
+	INT33FE_NODE_PI3USB30532,
+	INT33FE_NODE_MAX,
+};
+
 struct cht_int33fe_data {
 	struct i2c_client *max17047;
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
 	/* Contain a list-head must be per device */
 	struct device_connection connections[4];
+
+	struct fwnode_handle *node[INT33FE_NODE_MAX];
 };
 
 /*
@@ -73,6 +82,7 @@ static const struct property_entry max17047_props[] = {
 static int
 cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
 {
+	struct fwnode_handle *fwnode = data->node[INT33FE_NODE_MAX17047];
 	struct i2c_client *max17047 = NULL;
 	struct i2c_board_info board_info;
 	int ret;
@@ -80,9 +90,7 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
 	i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047);
 	if (max17047) {
 		/* Pre-existing i2c-client for the max17047, add device-props */
-		ret = device_add_properties(&max17047->dev, max17047_props);
-		if (ret)
-			return ret;
+		max17047->dev.fwnode->secondary = fwnode;
 		/* And re-probe to get the new device-props applied. */
 		ret = device_reprobe(&max17047->dev);
 		if (ret)
@@ -93,7 +101,7 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
 	memset(&board_info, 0, sizeof(board_info));
 	strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
 	board_info.dev_name = "max17047";
-	board_info.properties = max17047_props;
+	board_info.fwnode = fwnode;
 	data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
 	if (IS_ERR(data->max17047))
 		return PTR_ERR(data->max17047);
@@ -109,6 +117,45 @@ static const struct property_entry fusb302_props[] = {
 	{ }
 };
 
+static const struct property_entry *props[] = {
+	[INT33FE_NODE_FUSB302]		= fusb302_props,
+	[INT33FE_NODE_MAX17047]		= max17047_props,
+	[INT33FE_NODE_PI3USB30532]	= NULL,
+};
+
+static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
+{
+	int i;
+
+	for (i = 0; i < INT33FE_NODE_MAX; i++) {
+		fwnode_remove_software_node(data->node[i]);
+		data->node[i] = NULL;
+	}
+}
+
+static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
+{
+	struct fwnode_handle *fwnode;
+	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(props); i++) {
+		fwnode = fwnode_create_software_node(props[i], NULL);
+		if (IS_ERR(fwnode)) {
+			ret = PTR_ERR(fwnode);
+			goto err_remove_nodes;
+		}
+		data->node[i] = fwnode;
+	}
+
+	return 0;
+
+err_remove_nodes:
+	cht_int33fe_remove_nodes(data);
+
+	return ret;
+}
+
 static int cht_int33fe_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -173,10 +220,14 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	ret = cht_int33fe_add_nodes(data);
+	if (ret)
+		return ret;
+
 	/* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */
 	ret = cht_int33fe_register_max17047(dev, data);
 	if (ret)
-		return ret;
+		goto out_remove_nodes;
 
 	data->connections[0].endpoint[0] = "port0";
 	data->connections[0].endpoint[1] = "i2c-pi3usb30532-switch";
@@ -193,7 +244,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	memset(&board_info, 0, sizeof(board_info));
 	strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
 	board_info.dev_name = "fusb302";
-	board_info.properties = fusb302_props;
+	board_info.fwnode = data->node[INT33FE_NODE_FUSB302];
 	board_info.irq = fusb302_irq;
 
 	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
@@ -204,6 +255,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 
 	memset(&board_info, 0, sizeof(board_info));
 	board_info.dev_name = "pi3usb30532";
+	board_info.fwnode = data->node[INT33FE_NODE_PI3USB30532];
 	strlcpy(board_info.type, "pi3usb30532", I2C_NAME_SIZE);
 
 	data->pi3usb30532 = i2c_acpi_new_device(dev, 3, &board_info);
@@ -224,6 +276,9 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 
 	device_connections_remove(data->connections);
 
+out_remove_nodes:
+	cht_int33fe_remove_nodes(data);
+
 	return ret;
 }
 
@@ -236,6 +291,7 @@ static int cht_int33fe_remove(struct platform_device *pdev)
 	i2c_unregister_device(data->max17047);
 
 	device_connections_remove(data->connections);
+	cht_int33fe_remove_nodes(data);
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH v3 10/13] platform/x86: intel_cht_int33fe: Provide software nodes for the devices

[Andy Shevchenko]

On Fri, Apr 12, 2019 at 4:41 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Software nodes provide two features that we will need later.
> 1) Software nodes can have references to other software nodes.
> 2) Software nodes can exist before a device entry is created.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/platform/x86/intel_cht_int33fe.c | 68 +++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index d586bb22d7e8..863a792d9282 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -27,12 +27,21 @@
>
>  #define EXPECTED_PTYPE         4
>
> +enum {
> +       INT33FE_NODE_FUSB302,
> +       INT33FE_NODE_MAX17047,
> +       INT33FE_NODE_PI3USB30532,
> +       INT33FE_NODE_MAX,
> +};
> +
>  struct cht_int33fe_data {
>         struct i2c_client *max17047;
>         struct i2c_client *fusb302;
>         struct i2c_client *pi3usb30532;
>         /* Contain a list-head must be per device */
>         struct device_connection connections[4];
> +
> +       struct fwnode_handle *node[INT33FE_NODE_MAX];
>  };
>
>  /*
> @@ -73,6 +82,7 @@ static const struct property_entry max17047_props[] = {
>  static int
>  cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
>  {
> +       struct fwnode_handle *fwnode = data->node[INT33FE_NODE_MAX17047];
>         struct i2c_client *max17047 = NULL;
>         struct i2c_board_info board_info;
>         int ret;
> @@ -80,9 +90,7 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
>         i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047);
>         if (max17047) {
>                 /* Pre-existing i2c-client for the max17047, add device-props */
> -               ret = device_add_properties(&max17047->dev, max17047_props);
> -               if (ret)
> -                       return ret;
> +               max17047->dev.fwnode->secondary = fwnode;
>                 /* And re-probe to get the new device-props applied. */
>                 ret = device_reprobe(&max17047->dev);
>                 if (ret)
> @@ -93,7 +101,7 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
>         memset(&board_info, 0, sizeof(board_info));
>         strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
>         board_info.dev_name = "max17047";
> -       board_info.properties = max17047_props;
> +       board_info.fwnode = fwnode;
>         data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
>         if (IS_ERR(data->max17047))
>                 return PTR_ERR(data->max17047);
> @@ -109,6 +117,45 @@ static const struct property_entry fusb302_props[] = {
>         { }
>  };
>
> +static const struct property_entry *props[] = {
> +       [INT33FE_NODE_FUSB302]          = fusb302_props,
> +       [INT33FE_NODE_MAX17047]         = max17047_props,
> +       [INT33FE_NODE_PI3USB30532]      = NULL,
> +};
> +
> +static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
> +{
> +       int i;
> +
> +       for (i = 0; i < INT33FE_NODE_MAX; i++) {
> +               fwnode_remove_software_node(data->node[i]);
> +               data->node[i] = NULL;
> +       }
> +}
> +
> +static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
> +{
> +       struct fwnode_handle *fwnode;
> +       int ret;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(props); i++) {
> +               fwnode = fwnode_create_software_node(props[i], NULL);
> +               if (IS_ERR(fwnode)) {
> +                       ret = PTR_ERR(fwnode);
> +                       goto err_remove_nodes;
> +               }
> +               data->node[i] = fwnode;
> +       }
> +
> +       return 0;
> +
> +err_remove_nodes:
> +       cht_int33fe_remove_nodes(data);
> +
> +       return ret;
> +}
> +
>  static int cht_int33fe_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -173,10 +220,14 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>         if (!data)
>                 return -ENOMEM;
>
> +       ret = cht_int33fe_add_nodes(data);
> +       if (ret)
> +               return ret;
> +
>         /* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */
>         ret = cht_int33fe_register_max17047(dev, data);
>         if (ret)
> -               return ret;
> +               goto out_remove_nodes;
>
>         data->connections[0].endpoint[0] = "port0";
>         data->connections[0].endpoint[1] = "i2c-pi3usb30532-switch";
> @@ -193,7 +244,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>         memset(&board_info, 0, sizeof(board_info));
>         strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
>         board_info.dev_name = "fusb302";
> -       board_info.properties = fusb302_props;
> +       board_info.fwnode = data->node[INT33FE_NODE_FUSB302];
>         board_info.irq = fusb302_irq;
>
>         data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
> @@ -204,6 +255,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>
>         memset(&board_info, 0, sizeof(board_info));
>         board_info.dev_name = "pi3usb30532";
> +       board_info.fwnode = data->node[INT33FE_NODE_PI3USB30532];
>         strlcpy(board_info.type, "pi3usb30532", I2C_NAME_SIZE);
>
>         data->pi3usb30532 = i2c_acpi_new_device(dev, 3, &board_info);
> @@ -224,6 +276,9 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>
>         device_connections_remove(data->connections);
>
> +out_remove_nodes:
> +       cht_int33fe_remove_nodes(data);
> +
>         return ret;
>  }
>
> @@ -236,6 +291,7 @@ static int cht_int33fe_remove(struct platform_device *pdev)
>         i2c_unregister_device(data->max17047);
>
>         device_connections_remove(data->connections);
> +       cht_int33fe_remove_nodes(data);
>
>         return 0;
>  }
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Heikki Krogerus]

In ACPI, and now also in DT, the USB connectors usually have
their own device nodes. In case of USB Type-C, those
connector (port) nodes are child nodes of the controller or
PHY device, in our case the fusb302. The software fwnodes
allow us to create a similar child node for fusb302 that
represents the connector also on Intel CHT.

This makes it possible replace the fusb302 specific device
properties which were deprecated with the common USB
connector properties that tcpm.c is able to use directly.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 37 ++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 863a792d9282..eff5990322ff 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/usb/pd.h>
 
 #define EXPECTED_PTYPE		4
 
@@ -31,6 +32,7 @@ enum {
 	INT33FE_NODE_FUSB302,
 	INT33FE_NODE_MAX17047,
 	INT33FE_NODE_PI3USB30532,
+	INT33FE_NODE_USB_CONNECTOR,
 	INT33FE_NODE_MAX,
 };
 
@@ -111,9 +113,29 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
 
 static const struct property_entry fusb302_props[] = {
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
-	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
-	PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
-	PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),
+	{ }
+};
+
+#define PDO_FIXED_FLAGS \
+	(PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)
+
+static const u32 src_pdo[] = {
+	PDO_FIXED(5000, 1500, PDO_FIXED_FLAGS),
+};
+
+static const u32 snk_pdo[] = {
+	PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
+	PDO_VAR(5000, 12000, 3000),
+};
+
+static const struct property_entry usb_connector_props[] = {
+	PROPERTY_ENTRY_STRING("name", "connector"),
+	PROPERTY_ENTRY_STRING("data-role", "dual"),
+	PROPERTY_ENTRY_STRING("power-role", "dual"),
+	PROPERTY_ENTRY_STRING("try-power-role", "sink"),
+	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
+	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
+	PROPERTY_ENTRY_U32("op-sink-microwatt", 36000000),
 	{ }
 };
 
@@ -148,6 +170,15 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 		data->node[i] = fwnode;
 	}
 
+	/* Node for the USB connector (FUSB302 is the parent) */
+	fwnode = fwnode_create_software_node(usb_connector_props,
+					     data->node[INT33FE_NODE_FUSB302]);
+	if (IS_ERR(fwnode)) {
+		ret = PTR_ERR(fwnode);
+		goto err_remove_nodes;
+	}
+	data->node[INT33FE_NODE_USB_CONNECTOR] = fwnode;
+
 	return 0;
 
 err_remove_nodes:
-- 
2.20.1

Re: [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Hans de Goede]

Hi,

On 12-04-19 15:41, Heikki Krogerus wrote:
> In ACPI, and now also in DT, the USB connectors usually have
> their own device nodes. In case of USB Type-C, those
> connector (port) nodes are child nodes of the controller or
> PHY device, in our case the fusb302. The software fwnodes
> allow us to create a similar child node for fusb302 that
> represents the connector also on Intel CHT.
> 
> This makes it possible replace the fusb302 specific device
> properties which were deprecated with the common USB
> connector properties that tcpm.c is able to use directly.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>   drivers/platform/x86/intel_cht_int33fe.c | 37 ++++++++++++++++++++++--
>   1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 863a792d9282..eff5990322ff 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -24,6 +24,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
> +#include <linux/usb/pd.h>
>   
>   #define EXPECTED_PTYPE		4
>   
> @@ -31,6 +32,7 @@ enum {
>   	INT33FE_NODE_FUSB302,
>   	INT33FE_NODE_MAX17047,
>   	INT33FE_NODE_PI3USB30532,
> +	INT33FE_NODE_USB_CONNECTOR,
>   	INT33FE_NODE_MAX,
>   };
>   
> @@ -111,9 +113,29 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
>   
>   static const struct property_entry fusb302_props[] = {
>   	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
> -	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
> -	PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
> -	PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),

Note that the 36000000 value being removed here is max-sink-microwatt,
esp. the _max_ part is important. And recent versions of the fusb302
code ignore this entirely.

> +	{ }
> +};
> +
> +#define PDO_FIXED_FLAGS \
> +	(PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)
> +
> +static const u32 src_pdo[] = {
> +	PDO_FIXED(5000, 1500, PDO_FIXED_FLAGS),
> +};
> +
> +static const u32 snk_pdo[] = {
> +	PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
> +	PDO_VAR(5000, 12000, 3000),
> +};
> +
> +static const struct property_entry usb_connector_props[] = {
> +	PROPERTY_ENTRY_STRING("name", "connector"),
> +	PROPERTY_ENTRY_STRING("data-role", "dual"),
> +	PROPERTY_ENTRY_STRING("power-role", "dual"),
> +	PROPERTY_ENTRY_STRING("try-power-role", "sink"),
> +	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
> +	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
> +	PROPERTY_ENTRY_U32("op-sink-microwatt", 36000000),

Where as "op-sink-microwatt" is more interpreted as a minimum
value for non PPS supplies not being able to deliver this causes
the Capability Mismatch to get set. But for PPS supplies if I'm
reading the code correctly, the entire PPS negotiation is failed
by tcpm.c if this cannot be matched. I guess / hope that there
is a fallback to none PPS PDOs then but I'm not sure.

Anyways the charger the GPD-win ships with is a non PD capable
5V/2A charger and the GPD-pocket ships with a charger which does
max 12V/2A. The device itself will work fine on around 10W and
even charge at that level (albeit slowly). So I believe that 10W
would be a better value for "op-sink-microwatt" (the dt-binding
says it is mandatory so we cannot just leave it out).

Regards,

Hans





>   	{ }
>   };
>   
> @@ -148,6 +170,15 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
>   		data->node[i] = fwnode;
>   	}
>   
> +	/* Node for the USB connector (FUSB302 is the parent) */
> +	fwnode = fwnode_create_software_node(usb_connector_props,
> +					     data->node[INT33FE_NODE_FUSB302]);
> +	if (IS_ERR(fwnode)) {
> +		ret = PTR_ERR(fwnode);
> +		goto err_remove_nodes;
> +	}
> +	data->node[INT33FE_NODE_USB_CONNECTOR] = fwnode;
> +
>   	return 0;
>   
>   err_remove_nodes:
> 

Re: [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Heikki Krogerus]

On Wed, Apr 17, 2019 at 11:52:00AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 12-04-19 15:41, Heikki Krogerus wrote:
> > In ACPI, and now also in DT, the USB connectors usually have
> > their own device nodes. In case of USB Type-C, those
> > connector (port) nodes are child nodes of the controller or
> > PHY device, in our case the fusb302. The software fwnodes
> > allow us to create a similar child node for fusb302 that
> > represents the connector also on Intel CHT.
> > 
> > This makes it possible replace the fusb302 specific device
> > properties which were deprecated with the common USB
> > connector properties that tcpm.c is able to use directly.
> > 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >   drivers/platform/x86/intel_cht_int33fe.c | 37 ++++++++++++++++++++++--
> >   1 file changed, 34 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> > index 863a792d9282..eff5990322ff 100644
> > --- a/drivers/platform/x86/intel_cht_int33fe.c
> > +++ b/drivers/platform/x86/intel_cht_int33fe.c
> > @@ -24,6 +24,7 @@
> >   #include <linux/platform_device.h>
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/slab.h>
> > +#include <linux/usb/pd.h>
> >   #define EXPECTED_PTYPE		4
> > @@ -31,6 +32,7 @@ enum {
> >   	INT33FE_NODE_FUSB302,
> >   	INT33FE_NODE_MAX17047,
> >   	INT33FE_NODE_PI3USB30532,
> > +	INT33FE_NODE_USB_CONNECTOR,
> >   	INT33FE_NODE_MAX,
> >   };
> > @@ -111,9 +113,29 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
> >   static const struct property_entry fusb302_props[] = {
> >   	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
> > -	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
> > -	PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
> > -	PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),
> 
> Note that the 36000000 value being removed here is max-sink-microwatt,
> esp. the _max_ part is important. And recent versions of the fusb302
> code ignore this entirely.
> 
> > +	{ }
> > +};
> > +
> > +#define PDO_FIXED_FLAGS \
> > +	(PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)
> > +
> > +static const u32 src_pdo[] = {
> > +	PDO_FIXED(5000, 1500, PDO_FIXED_FLAGS),
> > +};
> > +
> > +static const u32 snk_pdo[] = {
> > +	PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
> > +	PDO_VAR(5000, 12000, 3000),
> > +};
> > +
> > +static const struct property_entry usb_connector_props[] = {
> > +	PROPERTY_ENTRY_STRING("name", "connector"),
> > +	PROPERTY_ENTRY_STRING("data-role", "dual"),
> > +	PROPERTY_ENTRY_STRING("power-role", "dual"),
> > +	PROPERTY_ENTRY_STRING("try-power-role", "sink"),
> > +	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
> > +	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
> > +	PROPERTY_ENTRY_U32("op-sink-microwatt", 36000000),
> 
> Where as "op-sink-microwatt" is more interpreted as a minimum
> value for non PPS supplies not being able to deliver this causes
> the Capability Mismatch to get set. But for PPS supplies if I'm
> reading the code correctly, the entire PPS negotiation is failed
> by tcpm.c if this cannot be matched. I guess / hope that there
> is a fallback to none PPS PDOs then but I'm not sure.

OK. I'll change that to the current default value, 2500000.

> Anyways the charger the GPD-win ships with is a non PD capable
> 5V/2A charger and the GPD-pocket ships with a charger which does
> max 12V/2A. The device itself will work fine on around 10W and
> even charge at that level (albeit slowly). So I believe that 10W
> would be a better value for "op-sink-microwatt" (the dt-binding
> says it is mandatory so we cannot just leave it out).

I have no objections. If you prefer, I can include a separate patch
where I change the value to 10W.

thanks,

-- 
heikki

[PATCH v3 12/13] platform/x86: intel_cht_int33fe: Link with external dependencies using fwnodes

[Heikki Krogerus]

Supplying also external devices - the DisplayPort connector
and the USB role switch - software fwnodes. After this the
driver has access to all the components tied to the USB
Type-C connector and can start creating software node
references to actually associate them with the USB Type-C
connector device.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 93 ++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index eff5990322ff..7711667a3454 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -21,6 +21,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
@@ -32,6 +33,8 @@ enum {
 	INT33FE_NODE_FUSB302,
 	INT33FE_NODE_MAX17047,
 	INT33FE_NODE_PI3USB30532,
+	INT33FE_NODE_DISPLAYPORT,
+	INT33FE_NODE_ROLE_SWITCH,
 	INT33FE_NODE_USB_CONNECTOR,
 	INT33FE_NODE_MAX,
 };
@@ -43,6 +46,7 @@ struct cht_int33fe_data {
 	/* Contain a list-head must be per device */
 	struct device_connection connections[4];
 
+	struct fwnode_handle *dp;
 	struct fwnode_handle *node[INT33FE_NODE_MAX];
 };
 
@@ -143,8 +147,71 @@ static const struct property_entry *props[] = {
 	[INT33FE_NODE_FUSB302]		= fusb302_props,
 	[INT33FE_NODE_MAX17047]		= max17047_props,
 	[INT33FE_NODE_PI3USB30532]	= NULL,
+	[INT33FE_NODE_DISPLAYPORT]	= NULL,
+	[INT33FE_NODE_ROLE_SWITCH]	= NULL,
 };
 
+static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
+{
+	struct fwnode_handle *fwnode = data->node[INT33FE_NODE_ROLE_SWITCH];
+	struct pci_dev *pdev;
+	struct device *dev;
+	struct device *p;
+
+	/* First let's find xHCI PCI device */
+	pdev = pci_get_class(PCI_CLASS_SERIAL_USB_XHCI, NULL);
+	if (!pdev || (pdev->vendor != PCI_VENDOR_ID_INTEL))
+		return -ENODEV;
+
+	/* Then the child platform device */
+	p = device_find_child_by_name(&pdev->dev, "intel_xhci_usb_sw");
+	pci_dev_put(pdev);
+	if (!p)
+		return -EPROBE_DEFER;
+
+	/* Finally the mux device */
+	dev = device_find_child_by_name(p, "intel_xhci_usb_sw-role-switch");
+	put_device(p);
+	if (!dev)
+		return -EPROBE_DEFER;
+
+	/* If there already is a node for the mux, using that one. */
+	if (dev->fwnode) {
+		fwnode_handle_get(dev->fwnode);
+		fwnode_remove_software_node(fwnode);
+		data->node[INT33FE_NODE_ROLE_SWITCH] = dev->fwnode;
+	} else {
+		/* The node can be tied to the lifetime of the device. */
+		dev->fwnode = fwnode_handle_get(dev->fwnode);
+	}
+
+	put_device(dev);
+
+	return 0;
+}
+
+static int cht_int33fe_setup_dp(struct cht_int33fe_data *data)
+{
+	struct fwnode_handle *fwnode = data->node[INT33FE_NODE_DISPLAYPORT];
+	struct pci_dev *pdev;
+
+	/* First let's find the GPU PCI device */
+	pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, NULL);
+	if (!pdev || (pdev->vendor != PCI_VENDOR_ID_INTEL))
+		return -ENODEV;
+
+	/* Then the DP child device node */
+	data->dp = device_get_named_child_node(&pdev->dev, "DD02");
+	pci_dev_put(pdev);
+	if (!data->dp)
+		return -ENODEV;
+
+	fwnode->secondary = ERR_PTR(-ENODEV);
+	data->dp->secondary = fwnode;
+
+	return 0;
+}
+
 static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 {
 	int i;
@@ -153,6 +220,12 @@ static void cht_int33fe_remove_nodes(struct cht_int33fe_data *data)
 		fwnode_remove_software_node(data->node[i]);
 		data->node[i] = NULL;
 	}
+
+	if (data->dp) {
+		data->dp->secondary = NULL;
+		fwnode_handle_put(data->dp);
+		data->dp = NULL;
+	}
 }
 
 static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
@@ -179,6 +252,26 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 	}
 	data->node[INT33FE_NODE_USB_CONNECTOR] = fwnode;
 
+	/* The devices that are not created in this driver need extra steps. */
+
+	/*
+	 * There is no ACPI device node for the USB role mux, so we need to find
+	 * the mux device and assign our node directly to it. That means we
+	 * depend on the mux driver. This function will return -PROBE_DEFER
+	 * until the mux device is registered.
+	 */
+	ret = cht_int33fe_setup_mux(data);
+	if (ret)
+		goto err_remove_nodes;
+
+	/*
+	 * The DP connector does have ACPI device node. In this case we can just
+	 * find that ACPI node and assing our node as the secondary node to it.
+	 */
+	ret = cht_int33fe_setup_dp(data);
+	if (ret)
+		goto err_remove_nodes;
+
 	return 0;
 
 err_remove_nodes:
-- 
2.20.1

[PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Heikki Krogerus]

Now that the software nodes support references, and the
device connection API support parsing fwnode references,
replacing the old connection descriptions with software node
references. Relying on device names when matching the
connection would not have been possible to link the USB
Type-C connector and the DisplayPort connector together, but
with real references it's not problem.

The DisplayPort ACPI node is dag up, and the drivers own
software node for the DisplayPort is set as the secondary
node for it. The USB Type-C connector refers the software
node, but it is now tied to the ACPI node, and therefore any
device entry (struct drm_connector in practice) that the
node combo is assigned to.

The USB role switch device does not have ACPI node, so we
have to wait for the device to appear. Then we can simply
assign our software node for the to the device.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 93 +++++++++++++++++++-----
 1 file changed, 76 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 7711667a3454..e6a1ea7f33af 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -39,15 +39,22 @@ enum {
 	INT33FE_NODE_MAX,
 };
 
+enum {
+	INT33FE_REF_ORIENTATION,
+	INT33FE_REF_MODE_MUX,
+	INT33FE_REF_DISPLAYPORT,
+	INT33FE_REF_ROLE_SWITCH,
+	INT33FE_REF_MAX,
+};
+
 struct cht_int33fe_data {
 	struct i2c_client *max17047;
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
-	/* Contain a list-head must be per device */
-	struct device_connection connections[4];
 
 	struct fwnode_handle *dp;
 	struct fwnode_handle *node[INT33FE_NODE_MAX];
+	struct software_node_reference *ref[INT33FE_REF_MAX];
 };
 
 /*
@@ -280,6 +287,65 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
 	return ret;
 }
 
+static void cht_int33fe_remove_references(struct cht_int33fe_data *data)
+{
+	int i;
+
+	for (i = 0; i < INT33FE_REF_MAX; i++)
+		fwnode_remove_software_node_reference(data->ref[i]);
+}
+
+static int cht_int33fe_add_references(struct cht_int33fe_data *data)
+{
+	struct fwnode_reference_args args[2] = { };
+	struct software_node_reference *ref;
+	struct fwnode_handle *con;
+
+	con = data->node[INT33FE_NODE_USB_CONNECTOR];
+
+	/* USB Type-C muxes */
+	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
+	args[1].fwnode = NULL;
+
+	ref = fwnode_create_software_node_reference(con, "orientation-switch",
+						    args);
+	if (IS_ERR(ref))
+		return PTR_ERR(ref);
+	data->ref[INT33FE_REF_ORIENTATION] = ref;
+
+	ref = fwnode_create_software_node_reference(con, "mode-switch", args);
+	if (IS_ERR(ref)) {
+		cht_int33fe_remove_references(data);
+		return PTR_ERR(ref);
+	}
+	data->ref[INT33FE_REF_MODE_MUX] = ref;
+
+	/* USB role switch */
+	args[0].fwnode = data->node[INT33FE_NODE_ROLE_SWITCH];
+	args[1].fwnode = NULL;
+
+	ref = fwnode_create_software_node_reference(con, "usb-role-switch",
+						    args);
+	if (IS_ERR(ref)) {
+		cht_int33fe_remove_references(data);
+		return PTR_ERR(ref);
+	}
+	data->ref[INT33FE_REF_ROLE_SWITCH] = ref;
+
+	/* DisplayPort */
+	args[0].fwnode = data->node[INT33FE_NODE_DISPLAYPORT];
+	args[1].fwnode = NULL;
+
+	ref = fwnode_create_software_node_reference(con, "displayport", args);
+	if (IS_ERR(ref)) {
+		cht_int33fe_remove_references(data);
+		return PTR_ERR(ref);
+	}
+	data->ref[INT33FE_REF_DISPLAYPORT] = ref;
+
+	return 0;
+}
+
 static int cht_int33fe_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -348,22 +414,14 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */
-	ret = cht_int33fe_register_max17047(dev, data);
+	ret = cht_int33fe_add_references(data);
 	if (ret)
 		goto out_remove_nodes;
 
-	data->connections[0].endpoint[0] = "port0";
-	data->connections[0].endpoint[1] = "i2c-pi3usb30532-switch";
-	data->connections[0].id = "orientation-switch";
-	data->connections[1].endpoint[0] = "port0";
-	data->connections[1].endpoint[1] = "i2c-pi3usb30532-mux";
-	data->connections[1].id = "mode-switch";
-	data->connections[2].endpoint[0] = "i2c-fusb302";
-	data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
-	data->connections[2].id = "usb-role-switch";
-
-	device_connections_add(data->connections);
+	/* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */
+	ret = cht_int33fe_register_max17047(dev, data);
+	if (ret)
+		goto out_remove_references;
 
 	memset(&board_info, 0, sizeof(board_info));
 	strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
@@ -398,7 +456,8 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 out_unregister_max17047:
 	i2c_unregister_device(data->max17047);
 
-	device_connections_remove(data->connections);
+out_remove_references:
+	cht_int33fe_remove_references(data);
 
 out_remove_nodes:
 	cht_int33fe_remove_nodes(data);
@@ -414,7 +473,7 @@ static int cht_int33fe_remove(struct platform_device *pdev)
 	i2c_unregister_device(data->fusb302);
 	i2c_unregister_device(data->max17047);
 
-	device_connections_remove(data->connections);
+	cht_int33fe_remove_references(data);
 	cht_int33fe_remove_nodes(data);
 
 	return 0;
-- 
2.20.1

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede]

Hi,

On 12-04-19 15:41, Heikki Krogerus wrote:
> Now that the software nodes support references, and the
> device connection API support parsing fwnode references,
> replacing the old connection descriptions with software node
> references. Relying on device names when matching the
> connection would not have been possible to link the USB
> Type-C connector and the DisplayPort connector together, but
> with real references it's not problem.
> 
> The DisplayPort ACPI node is dag up, and the drivers own
> software node for the DisplayPort is set as the secondary
> node for it. The USB Type-C connector refers the software
> node, but it is now tied to the ACPI node, and therefore any
> device entry (struct drm_connector in practice) that the
> node combo is assigned to.
> 
> The USB role switch device does not have ACPI node, so we
> have to wait for the device to appear. Then we can simply
> assign our software node for the to the device.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

So as promised I've been testing this series and this commit
breaks type-c functionality on devices using this driver.

The problem is that typec_switch_get() and  typec_mux_get()
after this both return the same pointer, which is pointing
to the switch, so typec_mux_get() is returning the wrong
pointer.

This is not surprising since the references for both are
both pointing to the fwnode attached to the piusb30532 devices:

	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];

So the class_find_device here:

static void *typec_switch_match(struct device_connection *con, int ep,
                                 void *data)
{
         struct device *dev;

         if (con->fwnode) {
                 if (con->id && !fwnode_property_present(con->fwnode, con->id))
                         return NULL;

                 dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
                                         fwnode_match);
         } else {
                 dev = class_find_device(&typec_mux_class, NULL,
                                         con->endpoint[ep], name_match);
         }

         return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
}

Simply returns the first typec_mux_class device registered.

I see 2 possible solutions to this problem:

1) Use separate typec_mux_class and typec_orientation_switch_class-es

2) Merge struct typec_switch and struct typec_mux into a single struct,
so that all typec_mux_class devices have the same memory layout, add
a subclass enum to this new merged struct and use that to identify
which of the typec_mux_class devices with the same fwnode pointer we
want.

Any other suggestions?

Regards,

Hans





> ---
>   drivers/platform/x86/intel_cht_int33fe.c | 93 +++++++++++++++++++-----
>   1 file changed, 76 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 7711667a3454..e6a1ea7f33af 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -39,15 +39,22 @@ enum {
>   	INT33FE_NODE_MAX,
>   };
>   
> +enum {
> +	INT33FE_REF_ORIENTATION,
> +	INT33FE_REF_MODE_MUX,
> +	INT33FE_REF_DISPLAYPORT,
> +	INT33FE_REF_ROLE_SWITCH,
> +	INT33FE_REF_MAX,
> +};
> +
>   struct cht_int33fe_data {
>   	struct i2c_client *max17047;
>   	struct i2c_client *fusb302;
>   	struct i2c_client *pi3usb30532;
> -	/* Contain a list-head must be per device */
> -	struct device_connection connections[4];
>   
>   	struct fwnode_handle *dp;
>   	struct fwnode_handle *node[INT33FE_NODE_MAX];
> +	struct software_node_reference *ref[INT33FE_REF_MAX];
>   };
>   
>   /*
> @@ -280,6 +287,65 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data)
>   	return ret;
>   }
>   
> +static void cht_int33fe_remove_references(struct cht_int33fe_data *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < INT33FE_REF_MAX; i++)
> +		fwnode_remove_software_node_reference(data->ref[i]);
> +}
> +
> +static int cht_int33fe_add_references(struct cht_int33fe_data *data)
> +{
> +	struct fwnode_reference_args args[2] = { };
> +	struct software_node_reference *ref;
> +	struct fwnode_handle *con;
> +
> +	con = data->node[INT33FE_NODE_USB_CONNECTOR];
> +
> +	/* USB Type-C muxes */
> +	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
> +	args[1].fwnode = NULL;
> +
> +	ref = fwnode_create_software_node_reference(con, "orientation-switch",
> +						    args);
> +	if (IS_ERR(ref))
> +		return PTR_ERR(ref);
> +	data->ref[INT33FE_REF_ORIENTATION] = ref;
> +
> +	ref = fwnode_create_software_node_reference(con, "mode-switch", args);
> +	if (IS_ERR(ref)) {
> +		cht_int33fe_remove_references(data);
> +		return PTR_ERR(ref);
> +	}
> +	data->ref[INT33FE_REF_MODE_MUX] = ref;
> +
> +	/* USB role switch */
> +	args[0].fwnode = data->node[INT33FE_NODE_ROLE_SWITCH];
> +	args[1].fwnode = NULL;
> +
> +	ref = fwnode_create_software_node_reference(con, "usb-role-switch",
> +						    args);
> +	if (IS_ERR(ref)) {
> +		cht_int33fe_remove_references(data);
> +		return PTR_ERR(ref);
> +	}
> +	data->ref[INT33FE_REF_ROLE_SWITCH] = ref;
> +
> +	/* DisplayPort */
> +	args[0].fwnode = data->node[INT33FE_NODE_DISPLAYPORT];
> +	args[1].fwnode = NULL;
> +
> +	ref = fwnode_create_software_node_reference(con, "displayport", args);
> +	if (IS_ERR(ref)) {
> +		cht_int33fe_remove_references(data);
> +		return PTR_ERR(ref);
> +	}
> +	data->ref[INT33FE_REF_DISPLAYPORT] = ref;
> +
> +	return 0;
> +}
> +
>   static int cht_int33fe_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -348,22 +414,14 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	/* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */
> -	ret = cht_int33fe_register_max17047(dev, data);
> +	ret = cht_int33fe_add_references(data);
>   	if (ret)
>   		goto out_remove_nodes;
>   
> -	data->connections[0].endpoint[0] = "port0";
> -	data->connections[0].endpoint[1] = "i2c-pi3usb30532-switch";
> -	data->connections[0].id = "orientation-switch";
> -	data->connections[1].endpoint[0] = "port0";
> -	data->connections[1].endpoint[1] = "i2c-pi3usb30532-mux";
> -	data->connections[1].id = "mode-switch";
> -	data->connections[2].endpoint[0] = "i2c-fusb302";
> -	data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
> -	data->connections[2].id = "usb-role-switch";
> -
> -	device_connections_add(data->connections);
> +	/* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */
> +	ret = cht_int33fe_register_max17047(dev, data);
> +	if (ret)
> +		goto out_remove_references;
>   
>   	memset(&board_info, 0, sizeof(board_info));
>   	strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
> @@ -398,7 +456,8 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>   out_unregister_max17047:
>   	i2c_unregister_device(data->max17047);
>   
> -	device_connections_remove(data->connections);
> +out_remove_references:
> +	cht_int33fe_remove_references(data);
>   
>   out_remove_nodes:
>   	cht_int33fe_remove_nodes(data);
> @@ -414,7 +473,7 @@ static int cht_int33fe_remove(struct platform_device *pdev)
>   	i2c_unregister_device(data->fusb302);
>   	i2c_unregister_device(data->max17047);
>   
> -	device_connections_remove(data->connections);
> +	cht_int33fe_remove_references(data);
>   	cht_int33fe_remove_nodes(data);
>   
>   	return 0;
> 

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Heikki Krogerus]

On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 12-04-19 15:41, Heikki Krogerus wrote:
> > Now that the software nodes support references, and the
> > device connection API support parsing fwnode references,
> > replacing the old connection descriptions with software node
> > references. Relying on device names when matching the
> > connection would not have been possible to link the USB
> > Type-C connector and the DisplayPort connector together, but
> > with real references it's not problem.
> > 
> > The DisplayPort ACPI node is dag up, and the drivers own
> > software node for the DisplayPort is set as the secondary
> > node for it. The USB Type-C connector refers the software
> > node, but it is now tied to the ACPI node, and therefore any
> > device entry (struct drm_connector in practice) that the
> > node combo is assigned to.
> > 
> > The USB role switch device does not have ACPI node, so we
> > have to wait for the device to appear. Then we can simply
> > assign our software node for the to the device.
> > 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> So as promised I've been testing this series and this commit
> breaks type-c functionality on devices using this driver.
> 
> The problem is that typec_switch_get() and  typec_mux_get()
> after this both return the same pointer, which is pointing
> to the switch, so typec_mux_get() is returning the wrong
> pointer.
> 
> This is not surprising since the references for both are
> both pointing to the fwnode attached to the piusb30532 devices:
> 
> 	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
> 
> So the class_find_device here:
> 
> static void *typec_switch_match(struct device_connection *con, int ep,
>                                 void *data)
> {
>         struct device *dev;
> 
>         if (con->fwnode) {
>                 if (con->id && !fwnode_property_present(con->fwnode, con->id))
>                         return NULL;
> 
>                 dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
>                                         fwnode_match);
>         } else {
>                 dev = class_find_device(&typec_mux_class, NULL,
>                                         con->endpoint[ep], name_match);
>         }
> 
>         return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> }
> 
> Simply returns the first typec_mux_class device registered.
> 
> I see 2 possible solutions to this problem:
> 
> 1) Use separate typec_mux_class and typec_orientation_switch_class-es
> 
> 2) Merge struct typec_switch and struct typec_mux into a single struct,
> so that all typec_mux_class devices have the same memory layout, add
> a subclass enum to this new merged struct and use that to identify
> which of the typec_mux_class devices with the same fwnode pointer we
> want.
> 
> Any other suggestions?

I think the correct fix is that we supply separate nodes for both
device entries.


thanks,

-- 
heikki

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede]

Hi,

On 17-04-19 08:39, Heikki Krogerus wrote:
> On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 12-04-19 15:41, Heikki Krogerus wrote:
>>> Now that the software nodes support references, and the
>>> device connection API support parsing fwnode references,
>>> replacing the old connection descriptions with software node
>>> references. Relying on device names when matching the
>>> connection would not have been possible to link the USB
>>> Type-C connector and the DisplayPort connector together, but
>>> with real references it's not problem.
>>>
>>> The DisplayPort ACPI node is dag up, and the drivers own
>>> software node for the DisplayPort is set as the secondary
>>> node for it. The USB Type-C connector refers the software
>>> node, but it is now tied to the ACPI node, and therefore any
>>> device entry (struct drm_connector in practice) that the
>>> node combo is assigned to.
>>>
>>> The USB role switch device does not have ACPI node, so we
>>> have to wait for the device to appear. Then we can simply
>>> assign our software node for the to the device.
>>>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> So as promised I've been testing this series and this commit
>> breaks type-c functionality on devices using this driver.
>>
>> The problem is that typec_switch_get() and  typec_mux_get()
>> after this both return the same pointer, which is pointing
>> to the switch, so typec_mux_get() is returning the wrong
>> pointer.
>>
>> This is not surprising since the references for both are
>> both pointing to the fwnode attached to the piusb30532 devices:
>>
>> 	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
>>
>> So the class_find_device here:
>>
>> static void *typec_switch_match(struct device_connection *con, int ep,
>>                                  void *data)
>> {
>>          struct device *dev;
>>
>>          if (con->fwnode) {
>>                  if (con->id && !fwnode_property_present(con->fwnode, con->id))
>>                          return NULL;
>>
>>                  dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
>>                                          fwnode_match);
>>          } else {
>>                  dev = class_find_device(&typec_mux_class, NULL,
>>                                          con->endpoint[ep], name_match);
>>          }
>>
>>          return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
>> }
>>
>> Simply returns the first typec_mux_class device registered.
>>
>> I see 2 possible solutions to this problem:
>>
>> 1) Use separate typec_mux_class and typec_orientation_switch_class-es
>>
>> 2) Merge struct typec_switch and struct typec_mux into a single struct,
>> so that all typec_mux_class devices have the same memory layout, add
>> a subclass enum to this new merged struct and use that to identify
>> which of the typec_mux_class devices with the same fwnode pointer we
>> want.
>>
>> Any other suggestions?
> 
> I think the correct fix is that we supply separate nodes for both
> device entries.

That is not going to work since the (virtual) mux / orientation-switch
devices are only registered once the driver binds to the piusb30532 i2c
device, so when creating the nodes we only have the piusb30532 i2c device.

I've been thinking some more about this and an easy fix is to have separate
fwnode_match functions for typec_switch_match and typec_mux_match and have
them check that the dev_name ends in "-mux" resp. "-switch" that requires
only a very minimal change to "usb: typec: Registering real device entries for the muxes"
and then everything should be fine.

Note that another problem with this series which I noticed while testing
is that the usb-role-switch is not being found at all anymore after
this ("Replacing the old connections with references") patch. I still need
start debugging that.

Regards,

Hans

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Heikki Krogerus]

On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 17-04-19 08:39, Heikki Krogerus wrote:
> > On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 12-04-19 15:41, Heikki Krogerus wrote:
> > > > Now that the software nodes support references, and the
> > > > device connection API support parsing fwnode references,
> > > > replacing the old connection descriptions with software node
> > > > references. Relying on device names when matching the
> > > > connection would not have been possible to link the USB
> > > > Type-C connector and the DisplayPort connector together, but
> > > > with real references it's not problem.
> > > > 
> > > > The DisplayPort ACPI node is dag up, and the drivers own
> > > > software node for the DisplayPort is set as the secondary
> > > > node for it. The USB Type-C connector refers the software
> > > > node, but it is now tied to the ACPI node, and therefore any
> > > > device entry (struct drm_connector in practice) that the
> > > > node combo is assigned to.
> > > > 
> > > > The USB role switch device does not have ACPI node, so we
> > > > have to wait for the device to appear. Then we can simply
> > > > assign our software node for the to the device.
> > > > 
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > > So as promised I've been testing this series and this commit
> > > breaks type-c functionality on devices using this driver.
> > > 
> > > The problem is that typec_switch_get() and  typec_mux_get()
> > > after this both return the same pointer, which is pointing
> > > to the switch, so typec_mux_get() is returning the wrong
> > > pointer.
> > > 
> > > This is not surprising since the references for both are
> > > both pointing to the fwnode attached to the piusb30532 devices:
> > > 
> > > 	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
> > > 
> > > So the class_find_device here:
> > > 
> > > static void *typec_switch_match(struct device_connection *con, int ep,
> > >                                  void *data)
> > > {
> > >          struct device *dev;
> > > 
> > >          if (con->fwnode) {
> > >                  if (con->id && !fwnode_property_present(con->fwnode, con->id))
> > >                          return NULL;
> > > 
> > >                  dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
> > >                                          fwnode_match);
> > >          } else {
> > >                  dev = class_find_device(&typec_mux_class, NULL,
> > >                                          con->endpoint[ep], name_match);
> > >          }
> > > 
> > >          return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> > > }
> > > 
> > > Simply returns the first typec_mux_class device registered.
> > > 
> > > I see 2 possible solutions to this problem:
> > > 
> > > 1) Use separate typec_mux_class and typec_orientation_switch_class-es
> > > 
> > > 2) Merge struct typec_switch and struct typec_mux into a single struct,
> > > so that all typec_mux_class devices have the same memory layout, add
> > > a subclass enum to this new merged struct and use that to identify
> > > which of the typec_mux_class devices with the same fwnode pointer we
> > > want.
> > > 
> > > Any other suggestions?
> > 
> > I think the correct fix is that we supply separate nodes for both
> > device entries.
> 
> That is not going to work since the (virtual) mux / orientation-switch
> devices are only registered once the driver binds to the piusb30532 i2c
> device, so when creating the nodes we only have the piusb30532 i2c device.

It's not a problem, that's why we have the software nodes. The nodes
can be created before the device entires. The node for pi3usb30532
will just be the parent node for the new nodes we add for the mux and
switch.

> I've been thinking some more about this and an easy fix is to have separate
> fwnode_match functions for typec_switch_match and typec_mux_match and have
> them check that the dev_name ends in "-mux" resp. "-switch" that requires
> only a very minimal change to "usb: typec: Registering real device entries for the muxes"
> and then everything should be fine.

I don't want to do anymore device name matching unless we have to, and
here we don't have to. We can name the nodes for those virtual mux and
switch, and then just do fwnode_find_named_child_node() in
pi3usb30532.c for both of them.

> Note that another problem with this series which I noticed while testing
> is that the usb-role-switch is not being found at all anymore after
> this ("Replacing the old connections with references") patch. I still need
> start debugging that.

OK, I'll wait for you results on that.


thanks,

-- 
heikki

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede]

Hi,

On 17-04-19 11:32, Heikki Krogerus wrote:
> On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 17-04-19 08:39, Heikki Krogerus wrote:
>>> On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 12-04-19 15:41, Heikki Krogerus wrote:
>>>>> Now that the software nodes support references, and the
>>>>> device connection API support parsing fwnode references,
>>>>> replacing the old connection descriptions with software node
>>>>> references. Relying on device names when matching the
>>>>> connection would not have been possible to link the USB
>>>>> Type-C connector and the DisplayPort connector together, but
>>>>> with real references it's not problem.
>>>>>
>>>>> The DisplayPort ACPI node is dag up, and the drivers own
>>>>> software node for the DisplayPort is set as the secondary
>>>>> node for it. The USB Type-C connector refers the software
>>>>> node, but it is now tied to the ACPI node, and therefore any
>>>>> device entry (struct drm_connector in practice) that the
>>>>> node combo is assigned to.
>>>>>
>>>>> The USB role switch device does not have ACPI node, so we
>>>>> have to wait for the device to appear. Then we can simply
>>>>> assign our software node for the to the device.
>>>>>
>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>
>>>> So as promised I've been testing this series and this commit
>>>> breaks type-c functionality on devices using this driver.
>>>>
>>>> The problem is that typec_switch_get() and  typec_mux_get()
>>>> after this both return the same pointer, which is pointing
>>>> to the switch, so typec_mux_get() is returning the wrong
>>>> pointer.
>>>>
>>>> This is not surprising since the references for both are
>>>> both pointing to the fwnode attached to the piusb30532 devices:
>>>>
>>>> 	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
>>>>
>>>> So the class_find_device here:
>>>>
>>>> static void *typec_switch_match(struct device_connection *con, int ep,
>>>>                                   void *data)
>>>> {
>>>>           struct device *dev;
>>>>
>>>>           if (con->fwnode) {
>>>>                   if (con->id && !fwnode_property_present(con->fwnode, con->id))
>>>>                           return NULL;
>>>>
>>>>                   dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
>>>>                                           fwnode_match);
>>>>           } else {
>>>>                   dev = class_find_device(&typec_mux_class, NULL,
>>>>                                           con->endpoint[ep], name_match);
>>>>           }
>>>>
>>>>           return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
>>>> }
>>>>
>>>> Simply returns the first typec_mux_class device registered.
>>>>
>>>> I see 2 possible solutions to this problem:
>>>>
>>>> 1) Use separate typec_mux_class and typec_orientation_switch_class-es
>>>>
>>>> 2) Merge struct typec_switch and struct typec_mux into a single struct,
>>>> so that all typec_mux_class devices have the same memory layout, add
>>>> a subclass enum to this new merged struct and use that to identify
>>>> which of the typec_mux_class devices with the same fwnode pointer we
>>>> want.
>>>>
>>>> Any other suggestions?
>>>
>>> I think the correct fix is that we supply separate nodes for both
>>> device entries.
>>
>> That is not going to work since the (virtual) mux / orientation-switch
>> devices are only registered once the driver binds to the piusb30532 i2c
>> device, so when creating the nodes we only have the piusb30532 i2c device.
> 
> It's not a problem, that's why we have the software nodes. The nodes
> can be created before the device entires. The node for pi3usb30532
> will just be the parent node for the new nodes we add for the mux and
> switch.
> 
>> I've been thinking some more about this and an easy fix is to have separate
>> fwnode_match functions for typec_switch_match and typec_mux_match and have
>> them check that the dev_name ends in "-mux" resp. "-switch" that requires
>> only a very minimal change to "usb: typec: Registering real device entries for the muxes"
>> and then everything should be fine.
> 
> I don't want to do anymore device name matching unless we have to, and
> here we don't have to. We can name the nodes for those virtual mux and
> switch, and then just do fwnode_find_named_child_node() in
> pi3usb30532.c for both of them.

Ok.

>> Note that another problem with this series which I noticed while testing
>> is that the usb-role-switch is not being found at all anymore after
>> this ("Replacing the old connections with references") patch. I still need
>> start debugging that.
> 
> OK, I'll wait for you results on that.

I understand that you want to wait with doing a v4 until this is resolved,
but can you (privately) send me a v3.5 fixing the mux/switch issue so that
I can test that (and get the problem out of the way for debugging the other
stuff) ?

I've also just replied to the following patch with a review remark:
[PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

Since you need to respin the series anyways, it would be good if you can
address that too.

Regards,

Hans

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Heikki Krogerus]

On Wed, Apr 17, 2019 at 11:52:15AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 17-04-19 11:32, Heikki Krogerus wrote:
> > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 17-04-19 08:39, Heikki Krogerus wrote:
> > > > On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 12-04-19 15:41, Heikki Krogerus wrote:
> > > > > > Now that the software nodes support references, and the
> > > > > > device connection API support parsing fwnode references,
> > > > > > replacing the old connection descriptions with software node
> > > > > > references. Relying on device names when matching the
> > > > > > connection would not have been possible to link the USB
> > > > > > Type-C connector and the DisplayPort connector together, but
> > > > > > with real references it's not problem.
> > > > > > 
> > > > > > The DisplayPort ACPI node is dag up, and the drivers own
> > > > > > software node for the DisplayPort is set as the secondary
> > > > > > node for it. The USB Type-C connector refers the software
> > > > > > node, but it is now tied to the ACPI node, and therefore any
> > > > > > device entry (struct drm_connector in practice) that the
> > > > > > node combo is assigned to.
> > > > > > 
> > > > > > The USB role switch device does not have ACPI node, so we
> > > > > > have to wait for the device to appear. Then we can simply
> > > > > > assign our software node for the to the device.
> > > > > > 
> > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > 
> > > > > So as promised I've been testing this series and this commit
> > > > > breaks type-c functionality on devices using this driver.
> > > > > 
> > > > > The problem is that typec_switch_get() and  typec_mux_get()
> > > > > after this both return the same pointer, which is pointing
> > > > > to the switch, so typec_mux_get() is returning the wrong
> > > > > pointer.
> > > > > 
> > > > > This is not surprising since the references for both are
> > > > > both pointing to the fwnode attached to the piusb30532 devices:
> > > > > 
> > > > > 	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
> > > > > 
> > > > > So the class_find_device here:
> > > > > 
> > > > > static void *typec_switch_match(struct device_connection *con, int ep,
> > > > >                                   void *data)
> > > > > {
> > > > >           struct device *dev;
> > > > > 
> > > > >           if (con->fwnode) {
> > > > >                   if (con->id && !fwnode_property_present(con->fwnode, con->id))
> > > > >                           return NULL;
> > > > > 
> > > > >                   dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
> > > > >                                           fwnode_match);
> > > > >           } else {
> > > > >                   dev = class_find_device(&typec_mux_class, NULL,
> > > > >                                           con->endpoint[ep], name_match);
> > > > >           }
> > > > > 
> > > > >           return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> > > > > }
> > > > > 
> > > > > Simply returns the first typec_mux_class device registered.
> > > > > 
> > > > > I see 2 possible solutions to this problem:
> > > > > 
> > > > > 1) Use separate typec_mux_class and typec_orientation_switch_class-es
> > > > > 
> > > > > 2) Merge struct typec_switch and struct typec_mux into a single struct,
> > > > > so that all typec_mux_class devices have the same memory layout, add
> > > > > a subclass enum to this new merged struct and use that to identify
> > > > > which of the typec_mux_class devices with the same fwnode pointer we
> > > > > want.
> > > > > 
> > > > > Any other suggestions?
> > > > 
> > > > I think the correct fix is that we supply separate nodes for both
> > > > device entries.
> > > 
> > > That is not going to work since the (virtual) mux / orientation-switch
> > > devices are only registered once the driver binds to the piusb30532 i2c
> > > device, so when creating the nodes we only have the piusb30532 i2c device.
> > 
> > It's not a problem, that's why we have the software nodes. The nodes
> > can be created before the device entires. The node for pi3usb30532
> > will just be the parent node for the new nodes we add for the mux and
> > switch.
> > 
> > > I've been thinking some more about this and an easy fix is to have separate
> > > fwnode_match functions for typec_switch_match and typec_mux_match and have
> > > them check that the dev_name ends in "-mux" resp. "-switch" that requires
> > > only a very minimal change to "usb: typec: Registering real device entries for the muxes"
> > > and then everything should be fine.
> > 
> > I don't want to do anymore device name matching unless we have to, and
> > here we don't have to. We can name the nodes for those virtual mux and
> > switch, and then just do fwnode_find_named_child_node() in
> > pi3usb30532.c for both of them.
> 
> Ok.
> 
> > > Note that another problem with this series which I noticed while testing
> > > is that the usb-role-switch is not being found at all anymore after
> > > this ("Replacing the old connections with references") patch. I still need
> > > start debugging that.
> > 
> > OK, I'll wait for you results on that.
> 
> I understand that you want to wait with doing a v4 until this is resolved,
> but can you (privately) send me a v3.5 fixing the mux/switch issue so that
> I can test that (and get the problem out of the way for debugging the other
> stuff) ?
> 
> I've also just replied to the following patch with a review remark:
> [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector
> 
> Since you need to respin the series anyways, it would be good if you can
> address that too.

Sure thing. I don't have time today to prepare the patches for you to
test today, nor tomorrow depending I'm a travelling or not, so this may
have wait for next week (Friday's off) :-(


thanks,

-- 
heikki

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede]

Hi,

On 17-04-19 13:04, Heikki Krogerus wrote:
> On Wed, Apr 17, 2019 at 11:52:15AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 17-04-19 11:32, Heikki Krogerus wrote:
>>> On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 17-04-19 08:39, Heikki Krogerus wrote:
>>>>> On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 12-04-19 15:41, Heikki Krogerus wrote:
>>>>>>> Now that the software nodes support references, and the
>>>>>>> device connection API support parsing fwnode references,
>>>>>>> replacing the old connection descriptions with software node
>>>>>>> references. Relying on device names when matching the
>>>>>>> connection would not have been possible to link the USB
>>>>>>> Type-C connector and the DisplayPort connector together, but
>>>>>>> with real references it's not problem.
>>>>>>>
>>>>>>> The DisplayPort ACPI node is dag up, and the drivers own
>>>>>>> software node for the DisplayPort is set as the secondary
>>>>>>> node for it. The USB Type-C connector refers the software
>>>>>>> node, but it is now tied to the ACPI node, and therefore any
>>>>>>> device entry (struct drm_connector in practice) that the
>>>>>>> node combo is assigned to.
>>>>>>>
>>>>>>> The USB role switch device does not have ACPI node, so we
>>>>>>> have to wait for the device to appear. Then we can simply
>>>>>>> assign our software node for the to the device.
>>>>>>>
>>>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>>>
>>>>>> So as promised I've been testing this series and this commit
>>>>>> breaks type-c functionality on devices using this driver.
>>>>>>
>>>>>> The problem is that typec_switch_get() and  typec_mux_get()
>>>>>> after this both return the same pointer, which is pointing
>>>>>> to the switch, so typec_mux_get() is returning the wrong
>>>>>> pointer.
>>>>>>
>>>>>> This is not surprising since the references for both are
>>>>>> both pointing to the fwnode attached to the piusb30532 devices:
>>>>>>
>>>>>> 	args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
>>>>>>
>>>>>> So the class_find_device here:
>>>>>>
>>>>>> static void *typec_switch_match(struct device_connection *con, int ep,
>>>>>>                                    void *data)
>>>>>> {
>>>>>>            struct device *dev;
>>>>>>
>>>>>>            if (con->fwnode) {
>>>>>>                    if (con->id && !fwnode_property_present(con->fwnode, con->id))
>>>>>>                            return NULL;
>>>>>>
>>>>>>                    dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
>>>>>>                                            fwnode_match);
>>>>>>            } else {
>>>>>>                    dev = class_find_device(&typec_mux_class, NULL,
>>>>>>                                            con->endpoint[ep], name_match);
>>>>>>            }
>>>>>>
>>>>>>            return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
>>>>>> }
>>>>>>
>>>>>> Simply returns the first typec_mux_class device registered.
>>>>>>
>>>>>> I see 2 possible solutions to this problem:
>>>>>>
>>>>>> 1) Use separate typec_mux_class and typec_orientation_switch_class-es
>>>>>>
>>>>>> 2) Merge struct typec_switch and struct typec_mux into a single struct,
>>>>>> so that all typec_mux_class devices have the same memory layout, add
>>>>>> a subclass enum to this new merged struct and use that to identify
>>>>>> which of the typec_mux_class devices with the same fwnode pointer we
>>>>>> want.
>>>>>>
>>>>>> Any other suggestions?
>>>>>
>>>>> I think the correct fix is that we supply separate nodes for both
>>>>> device entries.
>>>>
>>>> That is not going to work since the (virtual) mux / orientation-switch
>>>> devices are only registered once the driver binds to the piusb30532 i2c
>>>> device, so when creating the nodes we only have the piusb30532 i2c device.
>>>
>>> It's not a problem, that's why we have the software nodes. The nodes
>>> can be created before the device entires. The node for pi3usb30532
>>> will just be the parent node for the new nodes we add for the mux and
>>> switch.
>>>
>>>> I've been thinking some more about this and an easy fix is to have separate
>>>> fwnode_match functions for typec_switch_match and typec_mux_match and have
>>>> them check that the dev_name ends in "-mux" resp. "-switch" that requires
>>>> only a very minimal change to "usb: typec: Registering real device entries for the muxes"
>>>> and then everything should be fine.
>>>
>>> I don't want to do anymore device name matching unless we have to, and
>>> here we don't have to. We can name the nodes for those virtual mux and
>>> switch, and then just do fwnode_find_named_child_node() in
>>> pi3usb30532.c for both of them.
>>
>> Ok.
>>
>>>> Note that another problem with this series which I noticed while testing
>>>> is that the usb-role-switch is not being found at all anymore after
>>>> this ("Replacing the old connections with references") patch. I still need
>>>> start debugging that.
>>>
>>> OK, I'll wait for you results on that.
>>
>> I understand that you want to wait with doing a v4 until this is resolved,
>> but can you (privately) send me a v3.5 fixing the mux/switch issue so that
>> I can test that (and get the problem out of the way for debugging the other
>> stuff) ?
>>
>> I've also just replied to the following patch with a review remark:
>> [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector
>>
>> Since you need to respin the series anyways, it would be good if you can
>> address that too.
> 
> Sure thing. I don't have time today to prepare the patches for you to
> test today, nor tomorrow depending I'm a travelling or not, so this may
> have wait for next week (Friday's off) :-(

Note I'm on vacation myself next week, Monday I'm still reading email and
I can run some tests, next week Tuesday - Friday I'm off.

Regards,

Hans

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede]

Hi,

On 17-04-19 11:32, Heikki Krogerus wrote:
> On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:

<snip>

>> That is not going to work since the (virtual) mux / orientation-switch
>> devices are only registered once the driver binds to the piusb30532 i2c
>> device, so when creating the nodes we only have the piusb30532 i2c device.
> 
> It's not a problem, that's why we have the software nodes. The nodes
> can be created before the device entires. The node for pi3usb30532
> will just be the parent node for the new nodes we add for the mux and
> switch.
> 
>> I've been thinking some more about this and an easy fix is to have separate
>> fwnode_match functions for typec_switch_match and typec_mux_match and have
>> them check that the dev_name ends in "-mux" resp. "-switch" that requires
>> only a very minimal change to "usb: typec: Registering real device entries for the muxes"
>> and then everything should be fine.
> 
> I don't want to do anymore device name matching unless we have to, and
> here we don't have to. We can name the nodes for those virtual mux and
> switch, and then just do fwnode_find_named_child_node() in
> pi3usb30532.c for both of them.

Thinking more about this, I have a feeling that this makes things needlessly
complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be
100% reliable since we call:

         dev_set_name(&sw->dev, "%s-switch", dev_name(parent));
         dev_set_name(&mux->dev, "%s-mux", dev_name(parent));

When registering the switch / mux, so I believe doing name (suffix) comparison
here is fine and much simpler. Anyways this is just my 2 cents on this, I'm
happy with either solution, your choice.

Regards,

Hans

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Heikki Krogerus]

On Wed, Apr 17, 2019 at 12:15:18PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 17-04-19 11:32, Heikki Krogerus wrote:
> > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:
> 
> <snip>
> 
> > > That is not going to work since the (virtual) mux / orientation-switch
> > > devices are only registered once the driver binds to the piusb30532 i2c
> > > device, so when creating the nodes we only have the piusb30532 i2c device.
> > 
> > It's not a problem, that's why we have the software nodes. The nodes
> > can be created before the device entires. The node for pi3usb30532
> > will just be the parent node for the new nodes we add for the mux and
> > switch.
> > 
> > > I've been thinking some more about this and an easy fix is to have separate
> > > fwnode_match functions for typec_switch_match and typec_mux_match and have
> > > them check that the dev_name ends in "-mux" resp. "-switch" that requires
> > > only a very minimal change to "usb: typec: Registering real device entries for the muxes"
> > > and then everything should be fine.
> > 
> > I don't want to do anymore device name matching unless we have to, and
> > here we don't have to. We can name the nodes for those virtual mux and
> > switch, and then just do fwnode_find_named_child_node() in
> > pi3usb30532.c for both of them.
> 
> Thinking more about this, I have a feeling that this makes things needlessly
> complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be
> 100% reliable since we call:
> 
>         dev_set_name(&sw->dev, "%s-switch", dev_name(parent));
>         dev_set_name(&mux->dev, "%s-mux", dev_name(parent));
> 
> When registering the switch / mux, so I believe doing name (suffix) comparison
> here is fine and much simpler. Anyways this is just my 2 cents on this, I'm
> happy with either solution, your choice.

You do have a point. I'll take a look how the two options look like,
but maybe your way is better after all.

thanks,

-- 
heikki

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede]

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

Hi,

On 17-04-19 12:44, Heikki Krogerus wrote:
> On Wed, Apr 17, 2019 at 12:15:18PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 17-04-19 11:32, Heikki Krogerus wrote:
>>> On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> That is not going to work since the (virtual) mux / orientation-switch
>>>> devices are only registered once the driver binds to the piusb30532 i2c
>>>> device, so when creating the nodes we only have the piusb30532 i2c device.
>>>
>>> It's not a problem, that's why we have the software nodes. The nodes
>>> can be created before the device entires. The node for pi3usb30532
>>> will just be the parent node for the new nodes we add for the mux and
>>> switch.
>>>
>>>> I've been thinking some more about this and an easy fix is to have separate
>>>> fwnode_match functions for typec_switch_match and typec_mux_match and have
>>>> them check that the dev_name ends in "-mux" resp. "-switch" that requires
>>>> only a very minimal change to "usb: typec: Registering real device entries for the muxes"
>>>> and then everything should be fine.
>>>
>>> I don't want to do anymore device name matching unless we have to, and
>>> here we don't have to. We can name the nodes for those virtual mux and
>>> switch, and then just do fwnode_find_named_child_node() in
>>> pi3usb30532.c for both of them.
>>
>> Thinking more about this, I have a feeling that this makes things needlessly
>> complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be
>> 100% reliable since we call:
>>
>>          dev_set_name(&sw->dev, "%s-switch", dev_name(parent));
>>          dev_set_name(&mux->dev, "%s-mux", dev_name(parent));
>>
>> When registering the switch / mux, so I believe doing name (suffix) comparison
>> here is fine and much simpler. Anyways this is just my 2 cents on this, I'm
>> happy with either solution, your choice.
> 
> You do have a point. I'll take a look how the two options look like,
> but maybe your way is better after all.

I whipped up a quick fix using my approach so that I can start working
on debugging the usb_role_switch_get call in tcpm.c returning NULL.

I've attached it, feel free to use this for v4 of the series if you
decide to go with this approach.

Regards,

Hans


> 
> thanks,
> 

[-- Attachment #2: 0001-FIXUP-usb-typec-Registering-real-device-entries-for-.patch --]
[-- Type: text/x-patch, Size: 2522 bytes --]

From 47154195c05dc7c8b3373de9603b06c2f435588a Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 17 Apr 2019 17:58:17 +0200
Subject: [PATCH v2] FIXUP: "usb: typec: Registering real device entries for
 the muxes"

Check the dev_name suffix so that we do not return the first registered
device when a mux and switch share the same parent and fwnode.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/mux.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index c7d4a2dd454e..a28803544301 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -22,9 +22,21 @@ static int name_match(struct device *dev, const void *name)
 	return !strcmp((const char *)name, dev_name(dev));
 }
 
-static int fwnode_match(struct device *dev, const void *fwnode)
+static bool dev_name_ends_with(struct device *dev, const char *suffix)
 {
-	return dev_fwnode(dev) == fwnode;
+	const char *name = dev_name(dev);
+	const int name_len = strlen(name);
+	const int suffix_len = strlen(suffix);
+
+	if (suffix_len > name_len)
+		return false;
+
+	return strcmp(name + (name_len - suffix_len), suffix) == 0;
+}
+
+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,
@@ -37,7 +49,7 @@ static void *typec_switch_match(struct device_connection *con, int ep,
 			return NULL;
 
 		dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
-					fwnode_match);
+					switch_fwnode_match);
 	} else {
 		dev = class_find_device(&typec_mux_class, NULL,
 					con->endpoint[ep], name_match);
@@ -167,6 +179,11 @@ EXPORT_SYMBOL_GPL(typec_switch_get_drvdata);
 
 /* ------------------------------------------------------------------------- */
 
+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)
 {
 	const struct typec_altmode_desc *desc = data;
@@ -226,7 +243,7 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 
 find_mux:
 	dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
-				fwnode_match);
+				mux_fwnode_match);
 
 	return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
-- 
2.21.0

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Heikki Krogerus]

On Wed, Apr 17, 2019 at 06:03:03PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 17-04-19 12:44, Heikki Krogerus wrote:
> > On Wed, Apr 17, 2019 at 12:15:18PM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 17-04-19 11:32, Heikki Krogerus wrote:
> > > > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote:
> > > 
> > > <snip>
> > > 
> > > > > That is not going to work since the (virtual) mux / orientation-switch
> > > > > devices are only registered once the driver binds to the piusb30532 i2c
> > > > > device, so when creating the nodes we only have the piusb30532 i2c device.
> > > > 
> > > > It's not a problem, that's why we have the software nodes. The nodes
> > > > can be created before the device entires. The node for pi3usb30532
> > > > will just be the parent node for the new nodes we add for the mux and
> > > > switch.
> > > > 
> > > > > I've been thinking some more about this and an easy fix is to have separate
> > > > > fwnode_match functions for typec_switch_match and typec_mux_match and have
> > > > > them check that the dev_name ends in "-mux" resp. "-switch" that requires
> > > > > only a very minimal change to "usb: typec: Registering real device entries for the muxes"
> > > > > and then everything should be fine.
> > > > 
> > > > I don't want to do anymore device name matching unless we have to, and
> > > > here we don't have to. We can name the nodes for those virtual mux and
> > > > switch, and then just do fwnode_find_named_child_node() in
> > > > pi3usb30532.c for both of them.
> > > 
> > > Thinking more about this, I have a feeling that this makes things needlessly
> > > complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be
> > > 100% reliable since we call:
> > > 
> > >          dev_set_name(&sw->dev, "%s-switch", dev_name(parent));
> > >          dev_set_name(&mux->dev, "%s-mux", dev_name(parent));
> > > 
> > > When registering the switch / mux, so I believe doing name (suffix) comparison
> > > here is fine and much simpler. Anyways this is just my 2 cents on this, I'm
> > > happy with either solution, your choice.
> > 
> > You do have a point. I'll take a look how the two options look like,
> > but maybe your way is better after all.
> 
> I whipped up a quick fix using my approach so that I can start working
> on debugging the usb_role_switch_get call in tcpm.c returning NULL.
> 
> I've attached it, feel free to use this for v4 of the series if you
> decide to go with this approach.

Thanks. I'll probable use that as is.


> >From 47154195c05dc7c8b3373de9603b06c2f435588a Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Wed, 17 Apr 2019 17:58:17 +0200
> Subject: [PATCH v2] FIXUP: "usb: typec: Registering real device entries for
>  the muxes"
> 
> Check the dev_name suffix so that we do not return the first registered
> device when a mux and switch share the same parent and fwnode.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/mux.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index c7d4a2dd454e..a28803544301 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -22,9 +22,21 @@ static int name_match(struct device *dev, const void *name)
>  	return !strcmp((const char *)name, dev_name(dev));
>  }
>  
> -static int fwnode_match(struct device *dev, const void *fwnode)
> +static bool dev_name_ends_with(struct device *dev, const char *suffix)
>  {
> -	return dev_fwnode(dev) == fwnode;
> +	const char *name = dev_name(dev);
> +	const int name_len = strlen(name);
> +	const int suffix_len = strlen(suffix);
> +
> +	if (suffix_len > name_len)
> +		return false;
> +
> +	return strcmp(name + (name_len - suffix_len), suffix) == 0;
> +}
> +
> +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,
> @@ -37,7 +49,7 @@ static void *typec_switch_match(struct device_connection *con, int ep,
>  			return NULL;
>  
>  		dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
> -					fwnode_match);
> +					switch_fwnode_match);
>  	} else {
>  		dev = class_find_device(&typec_mux_class, NULL,
>  					con->endpoint[ep], name_match);
> @@ -167,6 +179,11 @@ EXPORT_SYMBOL_GPL(typec_switch_get_drvdata);
>  
>  /* ------------------------------------------------------------------------- */
>  
> +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)
>  {
>  	const struct typec_altmode_desc *desc = data;
> @@ -226,7 +243,7 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
>  
>  find_mux:
>  	dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
> -				fwnode_match);
> +				mux_fwnode_match);
>  
>  	return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER);
>  }
> -- 
> 2.21.0
> 

cheers,

-- 
heikki

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede]

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

Hi,

On 17-04-19 11:19, Hans de Goede wrote:
> Note that another problem with this series which I noticed while testing
> is that the usb-role-switch is not being found at all anymore after
> this ("Replacing the old connections with references") patch. I still need
> start debugging that.

Ok, I've just finished debugging this and I'm attaching 2 FIXUP
patches (to be squased) and a new patch, which those 3 small fixes
added the problem of tcpm.c being unable to get the role-switch
goes away.

The second FIXUP patch might be a bit controversial; and we may
need another solution for the problem it fixes. I've tried to
explain it in more detail in the commit msg.

Regards,

Hans

[-- Attachment #2: 0001-FIXUP-platform-x86-intel_cht_int33fe-Link-with-exter.patch --]
[-- Type: text/x-patch, Size: 1139 bytes --]

From 3a2e047608a53caaefe8364eceb7e315ec413698 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 17 Apr 2019 22:54:47 +0200
Subject: [PATCH v2 1/3] FIXUP: "platform/x86: intel_cht_int33fe: Link with
 external dependencies using fwnodes"

In the else path of: if (dev->fwnode) ... else ..., we should set
dev->fwnode to our own fwnode not to dev->fwnode, which is NULL as we
just tested.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index e6a1ea7f33af..07bf92ece6cd 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -189,7 +189,7 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
 		data->node[INT33FE_NODE_ROLE_SWITCH] = dev->fwnode;
 	} else {
 		/* The node can be tied to the lifetime of the device. */
-		dev->fwnode = fwnode_handle_get(dev->fwnode);
+		dev->fwnode = fwnode_handle_get(fwnode);
 	}
 
 	put_device(dev);
-- 
2.21.0


[-- Attachment #3: 0002-FIXUP-device-connection-Find-connections-also-by-che.patch --]
[-- Type: text/x-patch, Size: 1923 bytes --]

From 5133467f116dff6e111d4bc0610ccbcedb397f1d Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 17 Apr 2019 23:00:59 +0200
Subject: [PATCH v2 2/3] FIXUP: "device connection: Find connections also by
 checking the references"

The reference we are looking for might be in a child node, rather then
directly in the device's own fwnode. A typical example of this is a
usb connector node with references to various muxes / switches.

Note that we do not hit this problem for the device_connection_find_match
calls in typec_switch_get and typec_mux_get because these get called
from typec_register_port and typec_register_port creates a new device
with its fwnode pointing to the usb-connector fwnode and that new
device (rather then the parent) is passed to typec_switch/mux_get and
thus to device_connection_find_match.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/base/devcon.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 4cdf95532b63..6f6f870c21eb 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -76,7 +76,7 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 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);
+	struct fwnode_handle *child, *fwnode = dev_fwnode(dev);
 	const char *devname = dev_name(dev);
 	struct device_connection *con;
 	void *ret = NULL;
@@ -93,6 +93,12 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
 		ret = fwnode_devcon_match(fwnode, con_id, data, match);
 		if (ret)
 			return ret;
+
+		fwnode_for_each_child_node(fwnode, child) {
+			ret = fwnode_devcon_match(child, con_id, data, match);
+			if (ret)
+				return ret;
+		}
 	}
 
 	mutex_lock(&devcon_lock);
-- 
2.21.0


[-- Attachment #4: 0003-usb-roles-Check-for-NULL-con_id.patch --]
[-- Type: text/x-patch, Size: 1140 bytes --]

From a69f76993dfe5f43d3e6c4b2bcfbaacf2c247d6e Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 17 Apr 2019 22:57:00 +0200
Subject: [PATCH v2 3/3] usb: roles: Check for NULL con_id

When usb_role_switch_match gets called by device_connection_find_match
because of a fwnode_reference matching the con_id passed to
device_connection_find_match, then con->id will be NULL and in this
case we do not need to check it since our caller has already checked it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/roles/class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f45d8df5cfb8..86defca6623e 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -101,7 +101,7 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
 	struct device *dev;
 
 	if (con->fwnode) {
-		if (!fwnode_property_present(con->fwnode, con->id))
+		if (con->id && !fwnode_property_present(con->fwnode, con->id))
 			return NULL;
 
 		dev = class_find_device(role_class, NULL, con->fwnode,
-- 
2.21.0

Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Heikki Krogerus]

On Wed, Apr 17, 2019 at 11:14:19PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 17-04-19 11:19, Hans de Goede wrote:
> > Note that another problem with this series which I noticed while testing
> > is that the usb-role-switch is not being found at all anymore after
> > this ("Replacing the old connections with references") patch. I still need
> > start debugging that.
> 
> Ok, I've just finished debugging this and I'm attaching 2 FIXUP
> patches (to be squased) and a new patch, which those 3 small fixes
> added the problem of tcpm.c being unable to get the role-switch
> goes away.
> 
> The second FIXUP patch might be a bit controversial; and we may
> need another solution for the problem it fixes. I've tried to
> explain it in more detail in the commit msg.

Thanks. I'll go over those, and probable squash them in. I'll think
about the second patch.

I'm going to first split the series in two so that I'll first
introduce all the other changes, and then in a separate series the
node reference stuff. I think it makes sense, since there are really
to major changes here: firstly starting to use the software nodes with
the connector fwnode and others, and secondly introducing the software
node references.

I'll send you an RFC of the first patches soon. Hope you have time to
check and test it.

> >From 3a2e047608a53caaefe8364eceb7e315ec413698 Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Wed, 17 Apr 2019 22:54:47 +0200
> Subject: [PATCH v2 1/3] FIXUP: "platform/x86: intel_cht_int33fe: Link with
>  external dependencies using fwnodes"
> 
> In the else path of: if (dev->fwnode) ... else ..., we should set
> dev->fwnode to our own fwnode not to dev->fwnode, which is NULL as we
> just tested.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/intel_cht_int33fe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index e6a1ea7f33af..07bf92ece6cd 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -189,7 +189,7 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
>  		data->node[INT33FE_NODE_ROLE_SWITCH] = dev->fwnode;
>  	} else {
>  		/* The node can be tied to the lifetime of the device. */
> -		dev->fwnode = fwnode_handle_get(dev->fwnode);
> +		dev->fwnode = fwnode_handle_get(fwnode);
>  	}
>  
>  	put_device(dev);
> -- 
> 2.21.0
> 

> >From 5133467f116dff6e111d4bc0610ccbcedb397f1d Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Wed, 17 Apr 2019 23:00:59 +0200
> Subject: [PATCH v2 2/3] FIXUP: "device connection: Find connections also by
>  checking the references"
> 
> The reference we are looking for might be in a child node, rather then
> directly in the device's own fwnode. A typical example of this is a
> usb connector node with references to various muxes / switches.
> 
> Note that we do not hit this problem for the device_connection_find_match
> calls in typec_switch_get and typec_mux_get because these get called
> from typec_register_port and typec_register_port creates a new device
> with its fwnode pointing to the usb-connector fwnode and that new
> device (rather then the parent) is passed to typec_switch/mux_get and
> thus to device_connection_find_match.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/base/devcon.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> index 4cdf95532b63..6f6f870c21eb 100644
> --- a/drivers/base/devcon.c
> +++ b/drivers/base/devcon.c
> @@ -76,7 +76,7 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
>  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);
> +	struct fwnode_handle *child, *fwnode = dev_fwnode(dev);
>  	const char *devname = dev_name(dev);
>  	struct device_connection *con;
>  	void *ret = NULL;
> @@ -93,6 +93,12 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
>  		ret = fwnode_devcon_match(fwnode, con_id, data, match);
>  		if (ret)
>  			return ret;
> +
> +		fwnode_for_each_child_node(fwnode, child) {
> +			ret = fwnode_devcon_match(child, con_id, data, match);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	mutex_lock(&devcon_lock);
> -- 
> 2.21.0
> 

> >From a69f76993dfe5f43d3e6c4b2bcfbaacf2c247d6e Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Wed, 17 Apr 2019 22:57:00 +0200
> Subject: [PATCH v2 3/3] usb: roles: Check for NULL con_id
> 
> When usb_role_switch_match gets called by device_connection_find_match
> because of a fwnode_reference matching the con_id passed to
> device_connection_find_match, then con->id will be NULL and in this
> case we do not need to check it since our caller has already checked it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/roles/class.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f45d8df5cfb8..86defca6623e 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -101,7 +101,7 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
>  	struct device *dev;
>  
>  	if (con->fwnode) {
> -		if (!fwnode_property_present(con->fwnode, con->id))
> +		if (con->id && !fwnode_property_present(con->fwnode, con->id))
>  			return NULL;
>  
>  		dev = class_find_device(role_class, NULL, con->fwnode,
> -- 
> 2.21.0
> 

thanks,

-- 
heikki

Re: [PATCH v3 00/13] Software fwnode references

[Rafael J. Wysocki]

On Fri, Apr 12, 2019 at 3:42 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi,
>
> This is the third version of my proposal to add reference handling to
> the software node code. In this version I renamed ACPI_NAME_SIZE to
> ACPI_NAMESEG_SIZE in 6/13, and slit patch 9/13 in two separate patches
> (9/13 and 10/13) as suggested by Andy. Patch 9/13 will now only move
> the registration of max17047 out of probe, and 10/13 will introduce
> the software nodes.
>
> v2 commit message:
>
> This is the second version of this series. In this version I'm
> introducing a new helper device_find_child_by_name() as proposed
> by Andy. Andy requested also another helper that could be used for
> chaining the fwnodes, but I decided not to add that now. I would like
> to still think about how we should handle exceptions like if there
> already is a secondary node assigned for a node.
>
> v1 commit message:
>
> This series adds support for software fwnode reference handling. In
> practice it means making fwnode_property_get_reference_args() function
> usable in the drivers also with software nodes. I send the series
> originally as RFC [1].
>
> As the first user for the software node references, I'm converting
> intel_cht_int33fe.c to use them as part of the series.
>
> [1] https://lkml.org/lkml/2019/3/15/457

I added this series to the bleeding-edge branch last night and it
triggered some warnings from various checkers and there was a breakage
report from Hans.  All of that needs to be fixed IMO, so I'm expecting
an update.

Thanks!

Re: [PATCH v3 00/13] Software fwnode references

[Heikki Krogerus]

On Wed, Apr 17, 2019 at 09:54:19AM +0200, Rafael J. Wysocki wrote:
> On Fri, Apr 12, 2019 at 3:42 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi,
> >
> > This is the third version of my proposal to add reference handling to
> > the software node code. In this version I renamed ACPI_NAME_SIZE to
> > ACPI_NAMESEG_SIZE in 6/13, and slit patch 9/13 in two separate patches
> > (9/13 and 10/13) as suggested by Andy. Patch 9/13 will now only move
> > the registration of max17047 out of probe, and 10/13 will introduce
> > the software nodes.
> >
> > v2 commit message:
> >
> > This is the second version of this series. In this version I'm
> > introducing a new helper device_find_child_by_name() as proposed
> > by Andy. Andy requested also another helper that could be used for
> > chaining the fwnodes, but I decided not to add that now. I would like
> > to still think about how we should handle exceptions like if there
> > already is a secondary node assigned for a node.
> >
> > v1 commit message:
> >
> > This series adds support for software fwnode reference handling. In
> > practice it means making fwnode_property_get_reference_args() function
> > usable in the drivers also with software nodes. I send the series
> > originally as RFC [1].
> >
> > As the first user for the software node references, I'm converting
> > intel_cht_int33fe.c to use them as part of the series.
> >
> > [1] https://lkml.org/lkml/2019/3/15/457
> 
> I added this series to the bleeding-edge branch last night and it
> triggered some warnings from various checkers and there was a breakage
> report from Hans.  All of that needs to be fixed IMO, so I'm expecting
> an update.

I'm on it.

thanks,

-- 
heikki