[RFC PATCH 0/5] Adding graph handling to device connection API

[RFC PATCH 0/5] Adding graph handling to device connection API

[Heikki Krogerus]

Hi,

I'm only presenting my idea with these how I think we should be able
to deal with graphs in the API, so these are completely untested, and
obviously I can't say for certain if the idea works or not.

I will try to test these using custom ACPI tables, but of course these
should be tested on DT platform as well, so if somebody can do that, I
would much appreciate.

Thanks,

Heikki Krogerus (5):
  driver core: Add fwnode member to struct device_connection
  usb: typec: mux: Find the muxes by also matching against the device
    node
  usb: roles: Find the muxes by also matching against the device node
  usb: typec: Find the ports by also matching against the device node
  drivers core: Find device connections also from device graphs

 drivers/base/devcon.c      | 48 +++++++++++++++++++++++++++++++++++---
 drivers/usb/common/roles.c | 16 ++++++++++---
 drivers/usb/typec/class.c  | 19 ++++++++++++---
 drivers/usb/typec/mux.c    | 19 +++++++++++----
 include/linux/device.h     |  6 +++++
 5 files changed, 95 insertions(+), 13 deletions(-)

-- 
2.19.1

[RFC PATCH 1/5] driver core: Add fwnode member to struct device_connection

[Heikki Krogerus]

This will prepare the device connection API for connections
described in firmware.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/device.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 90224e75ade4..a964a0d614fa 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -753,11 +753,17 @@ struct device_dma_parameters {
 
 /**
  * struct device_connection - Device Connection Descriptor
+ * @fwnode: The device node of the connected device
  * @endpoint: The names of the two devices connected together
  * @id: Unique identifier for the connection
  * @list: List head, private, for internal use only
+ *
+ * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
+ * platform firmware defines the connection. When the connection is registeded
+ * with device_connection_add() @endpoint is used instead.
  */
 struct device_connection {
+	struct fwnode_handle	*fwnode;
 	const char		*endpoint[2];
 	const char		*id;
 	struct list_head	list;
-- 
2.19.1

Re: [RFC PATCH 1/5] driver core: Add fwnode member to struct device_connection

[Randy Dunlap]

On 10/24/18 8:05 AM, Heikki Krogerus wrote:
> This will prepare the device connection API for connections
> described in firmware.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  include/linux/device.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 90224e75ade4..a964a0d614fa 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -753,11 +753,17 @@ struct device_dma_parameters {
>  
>  /**
>   * struct device_connection - Device Connection Descriptor
> + * @fwnode: The device node of the connected device
>   * @endpoint: The names of the two devices connected together
>   * @id: Unique identifier for the connection
>   * @list: List head, private, for internal use only
> + *
> + * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
> + * platform firmware defines the connection. When the connection is registeded

for your next version:)                                                registered

> + * with device_connection_add() @endpoint is used instead.
>   */
>  struct device_connection {
> +	struct fwnode_handle	*fwnode;
>  	const char		*endpoint[2];
>  	const char		*id;
>  	struct list_head	list;
> 

cheers.
-- 
~Randy

Re: [RFC PATCH 1/5] driver core: Add fwnode member to struct device_connection

[Heikki Krogerus]

On Wed, Oct 24, 2018 at 08:33:53AM -0700, Randy Dunlap wrote:
> On 10/24/18 8:05 AM, Heikki Krogerus wrote:
> > This will prepare the device connection API for connections
> > described in firmware.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  include/linux/device.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 90224e75ade4..a964a0d614fa 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -753,11 +753,17 @@ struct device_dma_parameters {
> >  
> >  /**
> >   * struct device_connection - Device Connection Descriptor
> > + * @fwnode: The device node of the connected device
> >   * @endpoint: The names of the two devices connected together
> >   * @id: Unique identifier for the connection
> >   * @list: List head, private, for internal use only
> > + *
> > + * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
> > + * platform firmware defines the connection. When the connection is registeded
> 
> for your next version:)                                                registered

OK.

thanks,

-- 
heikki

[RFC PATCH 2/5] usb: typec: mux: Find the muxes by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

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

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index d990aa510fab..161a96280296 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -11,6 +11,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/usb/typec_mux.h>
 
 static DEFINE_MUTEX(switch_lock);
@@ -23,9 +24,14 @@ static void *typec_switch_match(struct device_connection *con, int ep,
 {
 	struct typec_switch *sw;
 
-	list_for_each_entry(sw, &switch_list, entry)
-		if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
+	list_for_each_entry(sw, &switch_list, entry) {
+		if (con->fwnode) {
+			if (dev_fwnode(sw->dev) == con->fwnode)
+				return sw;
+		} else if (!strcmp(con->endpoint[ep], dev_name(sw->dev))) {
 			return sw;
+		}
+	}
 
 	/*
 	 * We only get called if a connection was found, tell the caller to
@@ -114,9 +120,14 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 {
 	struct typec_mux *mux;
 
-	list_for_each_entry(mux, &mux_list, entry)
-		if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
+	list_for_each_entry(mux, &mux_list, entry) {
+		if (con->fwnode) {
+			if (dev_fwnode(mux->dev) == con->fwnode)
+				return mux;
+		} else if (!strcmp(con->endpoint[ep], dev_name(mux->dev))) {
 			return mux;
+		}
+	}
 
 	/*
 	 * We only get called if a connection was found, tell the caller to
-- 
2.19.1

[RFC PATCH 3/5] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/common/roles.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/common/roles.c b/drivers/usb/common/roles.c
index 99116af07f1d..bb52e006d203 100644
--- a/drivers/usb/common/roles.c
+++ b/drivers/usb/common/roles.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/usb/role.h>
+#include <linux/property.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
 
-static int __switch_match(struct device *dev, const void *name)
+static int switch_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
+
+static int switch_name_match(struct device *dev, const void *name)
 {
 	return !strcmp((const char *)name, dev_name(dev));
 }
@@ -94,8 +100,12 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
 {
 	struct device *dev;
 
-	dev = class_find_device(role_class, NULL, con->endpoint[ep],
-				__switch_match);
+	if (con->fwnode)
+		dev = class_find_device(role_class, NULL, con->fwnode,
+					switch_fwnode_match);
+	else
+		dev = class_find_device(role_class, NULL, con->endpoint[ep],
+					switch_name_match);
 
 	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
-- 
2.19.1

Re: [RFC PATCH 3/5] usb: roles: Find the muxes by also matching against the device node

[Jun Li]

Hi Heikki,
Heikki Krogerus <heikki.krogerus@linux.intel.com> 于2018年10月24日周三 下午11:06写道：
>
> When the connections are defined in firmware, struct
> device_connection will have the fwnode member pointing to
> the device node (struct fwnode_handle) of the requested
> device, and the endpoint will not be used at all in that
> case.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/common/roles.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/common/roles.c b/drivers/usb/common/roles.c
> index 99116af07f1d..bb52e006d203 100644
> --- a/drivers/usb/common/roles.c
> +++ b/drivers/usb/common/roles.c
> @@ -8,6 +8,7 @@
>   */
>
>  #include <linux/usb/role.h>
> +#include <linux/property.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
>
> -static int __switch_match(struct device *dev, const void *name)
> +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> +{
> +       return dev_fwnode(dev) == fwnode;

Seems this should be dev_fwnode(dev->parent) == fwnode;
The role switch is the child dev of the caller of usb_role_switch_register().

Li Jun

> +}
> +
> +static int switch_name_match(struct device *dev, const void *name)
>  {
>         return !strcmp((const char *)name, dev_name(dev));
>  }
> @@ -94,8 +100,12 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
>  {
>         struct device *dev;
>
> -       dev = class_find_device(role_class, NULL, con->endpoint[ep],
> -                               __switch_match);
> +       if (con->fwnode)
> +               dev = class_find_device(role_class, NULL, con->fwnode,
> +                                       switch_fwnode_match);
> +       else
> +               dev = class_find_device(role_class, NULL, con->endpoint[ep],
> +                                       switch_name_match);
>
>         return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
>  }
> --
> 2.19.1
>

Re: [RFC PATCH 3/5] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

On Tue, Jan 22, 2019 at 01:40:02PM +0800, Jun Li wrote:
> Hi Heikki,
> Heikki Krogerus <heikki.krogerus@linux.intel.com> 于2018年10月24日周三 下午11:06写道：
> >
> > When the connections are defined in firmware, struct
> > device_connection will have the fwnode member pointing to
> > the device node (struct fwnode_handle) of the requested
> > device, and the endpoint will not be used at all in that
> > case.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/common/roles.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/common/roles.c b/drivers/usb/common/roles.c
> > index 99116af07f1d..bb52e006d203 100644
> > --- a/drivers/usb/common/roles.c
> > +++ b/drivers/usb/common/roles.c
> > @@ -8,6 +8,7 @@
> >   */
> >
> >  #include <linux/usb/role.h>
> > +#include <linux/property.h>
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> >
> > -static int __switch_match(struct device *dev, const void *name)
> > +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> > +{
> > +       return dev_fwnode(dev) == fwnode;
> 
> Seems this should be dev_fwnode(dev->parent) == fwnode;
> The role switch is the child dev of the caller of usb_role_switch_register().

Indeed.

thanks,

-- 
heikki

[RFC PATCH 4/5] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

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

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 5db0593ca0bd..fe6f3a932a88 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -204,15 +204,28 @@ static void typec_altmode_put_partner(struct altmode *altmode)
 	put_device(&adev->dev);
 }
 
-static int __typec_port_match(struct device *dev, const void *name)
+static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev_name(fwnode) == fwnode;
+}
+
+static int typec_port_name_match(struct device *dev, const void *name)
 {
 	return !strcmp((const char *)name, dev_name(dev));
 }
 
 static void *typec_port_match(struct device_connection *con, int ep, void *data)
 {
-	return class_find_device(typec_class, NULL, con->endpoint[ep],
-				 __typec_port_match);
+	struct device *dev;
+
+	if (con->fwnode)
+		dev = class_find_device(typec_class, NULL, con->fwnode,
+					typec_port_fwnode_match);
+	else
+		dev = class_find_device(typec_class, NULL, con->endpoint[ep],
+					typec_port_name_match);
+
+	return dev ? dev : ERR_PTR(-EPROBE_DEFER);
 }
 
 struct typec_altmode *
-- 
2.19.1

Re: [RFC PATCH 4/5] usb: typec: Find the ports by also matching against the device node

[Sergei Shtylyov]

Hello!

On 10/24/2018 06:05 PM, Heikki Krogerus wrote:

> When the connections are defined in firmware, struct
> device_connection will have the fwnode member pointing to
> the device node (struct fwnode_handle) of the requested
> device, and the endpoint will not be used at all in that
> case.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/class.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 5db0593ca0bd..fe6f3a932a88 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -204,15 +204,28 @@ static void typec_altmode_put_partner(struct altmode *altmode)
>  	put_device(&adev->dev);
>  }
>  
> -static int __typec_port_match(struct device *dev, const void *name)
> +static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
> +{
> +	return dev_name(fwnode) == fwnode;

   dev_name(dev), maybe?

[...]

MBR, Sergei

Re: [RFC PATCH 4/5] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

On Wed, Oct 24, 2018 at 08:07:09PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 10/24/2018 06:05 PM, Heikki Krogerus wrote:
> 
> > When the connections are defined in firmware, struct
> > device_connection will have the fwnode member pointing to
> > the device node (struct fwnode_handle) of the requested
> > device, and the endpoint will not be used at all in that
> > case.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/class.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 5db0593ca0bd..fe6f3a932a88 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -204,15 +204,28 @@ static void typec_altmode_put_partner(struct altmode *altmode)
> >  	put_device(&adev->dev);
> >  }
> >  
> > -static int __typec_port_match(struct device *dev, const void *name)
> > +static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
> > +{
> > +	return dev_name(fwnode) == fwnode;
> 
>    dev_name(dev), maybe?

Yes.

thanks,

-- 
heikki

Re: [RFC PATCH 4/5] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

On Thu, Oct 25, 2018 at 10:27:20AM +0300, Heikki Krogerus wrote:
> On Wed, Oct 24, 2018 at 08:07:09PM +0300, Sergei Shtylyov wrote:
> > Hello!
> > 
> > On 10/24/2018 06:05 PM, Heikki Krogerus wrote:
> > 
> > > When the connections are defined in firmware, struct
> > > device_connection will have the fwnode member pointing to
> > > the device node (struct fwnode_handle) of the requested
> > > device, and the endpoint will not be used at all in that
> > > case.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  drivers/usb/typec/class.c | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > index 5db0593ca0bd..fe6f3a932a88 100644
> > > --- a/drivers/usb/typec/class.c
> > > +++ b/drivers/usb/typec/class.c
> > > @@ -204,15 +204,28 @@ static void typec_altmode_put_partner(struct altmode *altmode)
> > >  	put_device(&adev->dev);
> > >  }
> > >  
> > > -static int __typec_port_match(struct device *dev, const void *name)
> > > +static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
> > > +{
> > > +	return dev_name(fwnode) == fwnode;
> > 
> >    dev_name(dev), maybe?

Actually:

        dev_fwnode(dev) == fwnode;


Cheers,

-- 
heikki

[RFC PATCH 5/5] drivers core: Find device connections also from device graphs

[Heikki Krogerus]

If connections between devices are described in OF graph or
ACPI device graph, we can find them by using the
fwnode_graph_*() functions.

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

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index d427e806cd73..9c446ef84dd1 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -7,10 +7,45 @@
  */
 
 #include <linux/device.h>
+#include <linux/property.h>
 
 static DEFINE_MUTEX(devcon_lock);
 static LIST_HEAD(devcon_list);
 
+static void *
+fwnode_graph_devcon_match(struct fwnode_handle *fwnode,
+			  const char *con_id, void *data,
+			  void *(*match)(struct device_connection *con,
+					 int ep, void *data))
+{
+	struct device_connection con = { .id = con_id };
+	struct fwnode_handle *ep;
+	const char *val;
+	void *ret;
+	int err;
+
+	fwnode_graph_for_each_endpoint(fwnode, ep) {
+		con.fwnode = fwnode_graph_get_remote_port_parent(ep);
+
+		if (con_id) {
+			err = fwnode_property_read_string(con.fwnode, "name",
+							  &val);
+			if (err || strcmp(con_id, val)) {
+				fwnode_handle_put(con.fwnode);
+				continue;
+			}
+		}
+
+		ret = match(&con, -1, data);
+		fwnode_handle_put(con.fwnode);
+		if (ret) {
+			fwnode_handle_put(ep);
+			return ret;
+		}
+	}
+	return NULL;
+}
+
 /**
  * device_connection_find_match - Find physical connection to a device
  * @dev: Device with the connection
@@ -23,10 +58,11 @@ static LIST_HEAD(devcon_list);
  * caller is expecting to be returned.
  */
 void *device_connection_find_match(struct device *dev, const char *con_id,
-			       void *data,
-			       void *(*match)(struct device_connection *con,
-					      int ep, void *data))
+				   void *data,
+				   void *(*match)(struct device_connection *con,
+						  int ep, void *data))
 {
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	const char *devname = dev_name(dev);
 	struct device_connection *con;
 	void *ret = NULL;
@@ -35,6 +71,12 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
 	if (!match)
 		return NULL;
 
+	if (fwnode) {
+		ret = fwnode_graph_devcon_match(fwnode, con_id, data, match);
+		if (ret)
+			return ret;
+	}
+
 	mutex_lock(&devcon_lock);
 
 	list_for_each_entry(con, &devcon_list, list) {
-- 
2.19.1