2019-05-14 08:49:13

by Chunfeng Yun

Subject: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Add fwnode_usb_role_switch_get() to make easier to get
usb_role_switch by fwnode which register it.
It's useful when there is not device_connection registered
between two drivers and only knows the fwnode which register
usb_role_switch.

Signed-off-by: Chunfeng Yun <[email protected]>
Tested-by: Biju Das <[email protected]>
---
v5 changes:
1. remove linux/of.h suggested by Biju
2. add tested by Biju

Note: still depends on [1]
[1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
https://patchwork.kernel.org/patch/10909971/

v4 changes:
1. use switch_fwnode_match() to find fwnode suggested by Heikki
2. this patch now depends on [1]

[1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
https://patchwork.kernel.org/patch/10909971/

v3 changes:
1. use fwnodes instead of node suggested by Andy
2. rebuild the API suggested by Heikki

v2 no changes
---
drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
include/linux/usb/role.h | 8 ++++++++
2 files changed, 32 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f45d8df5cfb8..4a1f09a41ec0 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
}
EXPORT_SYMBOL_GPL(usb_role_switch_get);

+/**
+ * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
+ * @fwnode: The fwnode that register USB role switch
+ *
+ * Finds and returns role switch registered by @fwnode. The reference count
+ * for the found switch is incremented.
+ */
+struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
+{
+ struct usb_role_switch *sw;
+ struct device *dev;
+
+ dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
+ if (!dev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ sw = to_role_switch(dev);
+ WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+
+ return sw;
+}
+EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
+
/**
* usb_role_switch_put - Release handle to a switch
* @sw: USB Role Switch
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index da2b9641b877..35d460f9ec40 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -48,6 +48,8 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role);
enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw);
struct usb_role_switch *usb_role_switch_get(struct device *dev);
void usb_role_switch_put(struct usb_role_switch *sw);
+struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode);

struct usb_role_switch *
usb_role_switch_register(struct device *parent,
@@ -72,6 +74,12 @@ static inline struct usb_role_switch *usb_role_switch_get(struct device *dev)

static inline void usb_role_switch_put(struct usb_role_switch *sw) { }

+static inline struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline struct usb_role_switch *
usb_role_switch_register(struct device *parent,
const struct usb_role_switch_desc *desc)
--
2.21.0

2019-05-17 10:46:10

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> Add fwnode_usb_role_switch_get() to make easier to get
> usb_role_switch by fwnode which register it.
> It's useful when there is not device_connection registered
> between two drivers and only knows the fwnode which register
> usb_role_switch.
>
> Signed-off-by: Chunfeng Yun <[email protected]>
> Tested-by: Biju Das <[email protected]>

Acked-by: Heikki Krogerus <[email protected]>

> ---
> v5 changes:
> 1. remove linux/of.h suggested by Biju
> 2. add tested by Biju
>
> Note: still depends on [1]
> [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> https://patchwork.kernel.org/patch/10909971/
>
> v4 changes:
> 1. use switch_fwnode_match() to find fwnode suggested by Heikki
> 2. this patch now depends on [1]
>
> [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> https://patchwork.kernel.org/patch/10909971/
>
> v3 changes:
> 1. use fwnodes instead of node suggested by Andy
> 2. rebuild the API suggested by Heikki
>
> v2 no changes
> ---
> drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> include/linux/usb/role.h | 8 ++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f45d8df5cfb8..4a1f09a41ec0 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(usb_role_switch_get);
>
> +/**
> + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> + * @fwnode: The fwnode that register USB role switch
> + *
> + * Finds and returns role switch registered by @fwnode. The reference count
> + * for the found switch is incremented.
> + */
> +struct usb_role_switch *
> +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> +{
> + struct usb_role_switch *sw;
> + struct device *dev;
> +
> + dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> + if (!dev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + sw = to_role_switch(dev);
> + WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> +
> + return sw;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
> +
> /**
> * usb_role_switch_put - Release handle to a switch
> * @sw: USB Role Switch
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index da2b9641b877..35d460f9ec40 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -48,6 +48,8 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role);
> enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw);
> struct usb_role_switch *usb_role_switch_get(struct device *dev);
> void usb_role_switch_put(struct usb_role_switch *sw);
> +struct usb_role_switch *
> +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode);
>
> struct usb_role_switch *
> usb_role_switch_register(struct device *parent,
> @@ -72,6 +74,12 @@ static inline struct usb_role_switch *usb_role_switch_get(struct device *dev)
>
> static inline void usb_role_switch_put(struct usb_role_switch *sw) { }
>
> +static inline struct usb_role_switch *
> +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> static inline struct usb_role_switch *
> usb_role_switch_register(struct device *parent,
> const struct usb_role_switch_desc *desc)
> --
> 2.21.0

thanks,

--
heikki

2019-05-17 13:08:38

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi,

On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > Add fwnode_usb_role_switch_get() to make easier to get
> > usb_role_switch by fwnode which register it.
> > It's useful when there is not device_connection registered
> > between two drivers and only knows the fwnode which register
> > usb_role_switch.
> >
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > Tested-by: Biju Das <[email protected]>
>
> Acked-by: Heikki Krogerus <[email protected]>

Hold on. I just noticed Rob's comment on patch 2/6, where he points out
that you don't need to use device graph since the controller is the
parent of the connector. Doesn't that mean you don't really need this
API?

> > ---
> > v5 changes:
> > 1. remove linux/of.h suggested by Biju
> > 2. add tested by Biju
> >
> > Note: still depends on [1]
> > [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > https://patchwork.kernel.org/patch/10909971/
> >
> > v4 changes:
> > 1. use switch_fwnode_match() to find fwnode suggested by Heikki
> > 2. this patch now depends on [1]
> >
> > [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > https://patchwork.kernel.org/patch/10909971/
> >
> > v3 changes:
> > 1. use fwnodes instead of node suggested by Andy
> > 2. rebuild the API suggested by Heikki
> >
> > v2 no changes
> > ---
> > drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> > include/linux/usb/role.h | 8 ++++++++
> > 2 files changed, 32 insertions(+)
> >
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index f45d8df5cfb8..4a1f09a41ec0 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> > }
> > EXPORT_SYMBOL_GPL(usb_role_switch_get);
> >
> > +/**
> > + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> > + * @fwnode: The fwnode that register USB role switch
> > + *
> > + * Finds and returns role switch registered by @fwnode. The reference count
> > + * for the found switch is incremented.
> > + */
> > +struct usb_role_switch *
> > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> > +{
> > + struct usb_role_switch *sw;
> > + struct device *dev;
> > +
> > + dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> > + if (!dev)
> > + return ERR_PTR(-EPROBE_DEFER);
> > +
> > + sw = to_role_switch(dev);
> > + WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> > +
> > + return sw;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);

This function only basically converts the fwnode to usb_role_switch,
but I would actually prefer that we walked through the device graph
here instead of expecting the caller to do that.

So this function should probable be called fwnode_to_usb_role_switch()
and not fwnode_usb_role_switch_get(), but I guess you don't need it
at all, right?


thanks,

--
heikki

2019-05-20 03:04:55

by Chunfeng Yun

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi,
On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> Hi,
>
> On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > Add fwnode_usb_role_switch_get() to make easier to get
> > > usb_role_switch by fwnode which register it.
> > > It's useful when there is not device_connection registered
> > > between two drivers and only knows the fwnode which register
> > > usb_role_switch.
> > >
> > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > Tested-by: Biju Das <[email protected]>
> >
> > Acked-by: Heikki Krogerus <[email protected]>
>
> Hold on. I just noticed Rob's comment on patch 2/6, where he points out
> that you don't need to use device graph since the controller is the
> parent of the connector. Doesn't that mean you don't really need this
> API?
No, I still need it.
The change is about the way how to get fwnode;
when use device graph, get fwnode by of_graph_get_remote_node();
but now will get fwnode by of_get_parent();

>
> > > ---
> > > v5 changes:
> > > 1. remove linux/of.h suggested by Biju
> > > 2. add tested by Biju
> > >
> > > Note: still depends on [1]
> > > [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > > https://patchwork.kernel.org/patch/10909971/
> > >
> > > v4 changes:
> > > 1. use switch_fwnode_match() to find fwnode suggested by Heikki
> > > 2. this patch now depends on [1]
> > >
> > > [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > > https://patchwork.kernel.org/patch/10909971/
> > >
> > > v3 changes:
> > > 1. use fwnodes instead of node suggested by Andy
> > > 2. rebuild the API suggested by Heikki
> > >
> > > v2 no changes
> > > ---
> > > drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> > > include/linux/usb/role.h | 8 ++++++++
> > > 2 files changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > > index f45d8df5cfb8..4a1f09a41ec0 100644
> > > --- a/drivers/usb/roles/class.c
> > > +++ b/drivers/usb/roles/class.c
> > > @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> > > }
> > > EXPORT_SYMBOL_GPL(usb_role_switch_get);
> > >
> > > +/**
> > > + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> > > + * @fwnode: The fwnode that register USB role switch
> > > + *
> > > + * Finds and returns role switch registered by @fwnode. The reference count
> > > + * for the found switch is incremented.
> > > + */
> > > +struct usb_role_switch *
> > > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> > > +{
> > > + struct usb_role_switch *sw;
> > > + struct device *dev;
> > > +
> > > + dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> > > + if (!dev)
> > > + return ERR_PTR(-EPROBE_DEFER);
> > > +
> > > + sw = to_role_switch(dev);
> > > + WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> > > +
> > > + return sw;
> > > +}
> > > +EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
>
> This function only basically converts the fwnode to usb_role_switch,
> but I would actually prefer that we walked through the device graph
> here instead of expecting the caller to do that.
>
> So this function should probable be called fwnode_to_usb_role_switch()
> and not fwnode_usb_role_switch_get(), but I guess you don't need it
> at all, right?
>
>
> thanks,
>

2019-05-20 08:19:57

by Biju Das

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Heikki,

> -----Original Message-----
> From: Heikki Krogerus <[email protected]>
> Sent: Monday, May 20, 2019 9:04 AM
> To: Chunfeng Yun <[email protected]>
> Cc: Rob Herring <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Mark Rutland <[email protected]>;
> Matthias Brugger <[email protected]>; Adam Thomson
> <[email protected]>; Li Jun <[email protected]>;
> Badhri Jagan Sridharan <[email protected]>; Hans de Goede
> <[email protected]>; Andy Shevchenko
> <[email protected]>; Min Guo <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Biju Das <[email protected]>; Linus
> Walleij <[email protected]>
> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
>
> On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > Hi,
> > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > usb_role_switch by fwnode which register it.
> > > > > It's useful when there is not device_connection registered
> > > > > between two drivers and only knows the fwnode which register
> > > > > usb_role_switch.
> > > > >
> > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > Tested-by: Biju Das <[email protected]>
> > > >
> > > > Acked-by: Heikki Krogerus <[email protected]>
> > >
> > > Hold on. I just noticed Rob's comment on patch 2/6, where he points
> > > out that you don't need to use device graph since the controller is
> > > the parent of the connector. Doesn't that mean you don't really need
> > > this API?
> > No, I still need it.
> > The change is about the way how to get fwnode; when use device graph,
> > get fwnode by of_graph_get_remote_node(); but now will get fwnode by
> > of_get_parent();
>
> OK, I get that, but I'm still not convinced about if something like this function
> is needed at all. I also have concerns regarding how you are using the
> function. I'll explain in comment to the patch 5/6 in this series...

FYI, Currently I am also using this api in my patch series.
https://patchwork.kernel.org/patch/10944637/

regards,
Biju

> > > > > ---
> > > > > v5 changes:
> > > > > 1. remove linux/of.h suggested by Biju 2. add tested by Biju
> > > > >
> > > > > Note: still depends on [1]
> > > > > [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in
> role.h
> > > > > https://patchwork.kernel.org/patch/10909971/
> > > > >
> > > > > v4 changes:
> > > > > 1. use switch_fwnode_match() to find fwnode suggested by Heikki
> > > > > 2. this patch now depends on [1]
> > > > >
> > > > > [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in
> role.h
> > > > > https://patchwork.kernel.org/patch/10909971/
> > > > >
> > > > > v3 changes:
> > > > > 1. use fwnodes instead of node suggested by Andy
> > > > > 2. rebuild the API suggested by Heikki
> > > > >
> > > > > v2 no changes
> > > > > ---
> > > > > drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> > > > > include/linux/usb/role.h | 8 ++++++++
> > > > > 2 files changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/roles/class.c
> > > > > b/drivers/usb/roles/class.c index f45d8df5cfb8..4a1f09a41ec0
> > > > > 100644
> > > > > --- a/drivers/usb/roles/class.c
> > > > > +++ b/drivers/usb/roles/class.c
> > > > > @@ -135,6 +135,30 @@ struct usb_role_switch
> > > > > *usb_role_switch_get(struct device *dev) }
> > > > > EXPORT_SYMBOL_GPL(usb_role_switch_get);
> > > > >
> > > > > +/**
> > > > > + * fwnode_usb_role_switch_get - Find USB role switch by it's
> > > > > +parent fwnode
> > > > > + * @fwnode: The fwnode that register USB role switch
> > > > > + *
> > > > > + * Finds and returns role switch registered by @fwnode. The
> > > > > +reference count
> > > > > + * for the found switch is incremented.
> > > > > + */
> > > > > +struct usb_role_switch *
> > > > > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode) {
> > > > > + struct usb_role_switch *sw;
> > > > > + struct device *dev;
> > > > > +
> > > > > + dev = class_find_device(role_class, NULL, fwnode,
> switch_fwnode_match);
> > > > > + if (!dev)
> > > > > + return ERR_PTR(-EPROBE_DEFER);
> > > > > +
> > > > > + sw = to_role_switch(dev);
> > > > > + WARN_ON(!try_module_get(sw->dev.parent->driver-
> >owner));
> > > > > +
> > > > > + return sw;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
> > >
> > > This function only basically converts the fwnode to usb_role_switch,
> > > but I would actually prefer that we walked through the device graph
> > > here instead of expecting the caller to do that.
> > >
> > > So this function should probable be called
> > > fwnode_to_usb_role_switch() and not fwnode_usb_role_switch_get(),
> > > but I guess you don't need it at all, right?
> > >
> > >
> > > thanks,
> > >
> >
>
> --
> heikki

2019-05-20 09:06:00

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> Hi,
> On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > Hi,
> >
> > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > usb_role_switch by fwnode which register it.
> > > > It's useful when there is not device_connection registered
> > > > between two drivers and only knows the fwnode which register
> > > > usb_role_switch.
> > > >
> > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > Tested-by: Biju Das <[email protected]>
> > >
> > > Acked-by: Heikki Krogerus <[email protected]>
> >
> > Hold on. I just noticed Rob's comment on patch 2/6, where he points out
> > that you don't need to use device graph since the controller is the
> > parent of the connector. Doesn't that mean you don't really need this
> > API?
> No, I still need it.
> The change is about the way how to get fwnode;
> when use device graph, get fwnode by of_graph_get_remote_node();
> but now will get fwnode by of_get_parent();

OK, I get that, but I'm still not convinced about if something like
this function is needed at all. I also have concerns regarding how you
are using the function. I'll explain in comment to the patch 5/6 in
this series...

> > > > ---
> > > > v5 changes:
> > > > 1. remove linux/of.h suggested by Biju
> > > > 2. add tested by Biju
> > > >
> > > > Note: still depends on [1]
> > > > [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > > > https://patchwork.kernel.org/patch/10909971/
> > > >
> > > > v4 changes:
> > > > 1. use switch_fwnode_match() to find fwnode suggested by Heikki
> > > > 2. this patch now depends on [1]
> > > >
> > > > [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > > > https://patchwork.kernel.org/patch/10909971/
> > > >
> > > > v3 changes:
> > > > 1. use fwnodes instead of node suggested by Andy
> > > > 2. rebuild the API suggested by Heikki
> > > >
> > > > v2 no changes
> > > > ---
> > > > drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> > > > include/linux/usb/role.h | 8 ++++++++
> > > > 2 files changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > > > index f45d8df5cfb8..4a1f09a41ec0 100644
> > > > --- a/drivers/usb/roles/class.c
> > > > +++ b/drivers/usb/roles/class.c
> > > > @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> > > > }
> > > > EXPORT_SYMBOL_GPL(usb_role_switch_get);
> > > >
> > > > +/**
> > > > + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> > > > + * @fwnode: The fwnode that register USB role switch
> > > > + *
> > > > + * Finds and returns role switch registered by @fwnode. The reference count
> > > > + * for the found switch is incremented.
> > > > + */
> > > > +struct usb_role_switch *
> > > > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> > > > +{
> > > > + struct usb_role_switch *sw;
> > > > + struct device *dev;
> > > > +
> > > > + dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> > > > + if (!dev)
> > > > + return ERR_PTR(-EPROBE_DEFER);
> > > > +
> > > > + sw = to_role_switch(dev);
> > > > + WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> > > > +
> > > > + return sw;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
> >
> > This function only basically converts the fwnode to usb_role_switch,
> > but I would actually prefer that we walked through the device graph
> > here instead of expecting the caller to do that.
> >
> > So this function should probable be called fwnode_to_usb_role_switch()
> > and not fwnode_usb_role_switch_get(), but I guess you don't need it
> > at all, right?
> >
> >
> > thanks,
> >
>

--
heikki

2019-05-20 09:53:43

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> Hi Heikki,
>
> > -----Original Message-----
> > From: Heikki Krogerus <[email protected]>
> > Sent: Monday, May 20, 2019 9:04 AM
> > To: Chunfeng Yun <[email protected]>
> > Cc: Rob Herring <[email protected]>; Greg Kroah-Hartman
> > <[email protected]>; Mark Rutland <[email protected]>;
> > Matthias Brugger <[email protected]>; Adam Thomson
> > <[email protected]>; Li Jun <[email protected]>;
> > Badhri Jagan Sridharan <[email protected]>; Hans de Goede
> > <[email protected]>; Andy Shevchenko
> > <[email protected]>; Min Guo <[email protected]>;
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; linux-
> > [email protected]; Biju Das <[email protected]>; Linus
> > Walleij <[email protected]>
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> >
> > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > Hi,
> > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > Hi,
> > > >
> > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > usb_role_switch by fwnode which register it.
> > > > > > It's useful when there is not device_connection registered
> > > > > > between two drivers and only knows the fwnode which register
> > > > > > usb_role_switch.
> > > > > >
> > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > Tested-by: Biju Das <[email protected]>
> > > > >
> > > > > Acked-by: Heikki Krogerus <[email protected]>
> > > >
> > > > Hold on. I just noticed Rob's comment on patch 2/6, where he points
> > > > out that you don't need to use device graph since the controller is
> > > > the parent of the connector. Doesn't that mean you don't really need
> > > > this API?
> > > No, I still need it.
> > > The change is about the way how to get fwnode; when use device graph,
> > > get fwnode by of_graph_get_remote_node(); but now will get fwnode by
> > > of_get_parent();
> >
> > OK, I get that, but I'm still not convinced about if something like this function
> > is needed at all. I also have concerns regarding how you are using the
> > function. I'll explain in comment to the patch 5/6 in this series...
>
> FYI, Currently I am also using this api in my patch series.
> https://patchwork.kernel.org/patch/10944637/

Yes, and I have the same question for you I jusb asked in comment I
added to the patch 5/6 of this series. Why isn't usb_role_switch_get()
enough?

thanks,

--
heikki

2019-05-20 13:53:04

by Biju Das

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Heikki,

Thanks for the feedback.

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
>
> On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > Hi Heikki,
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > Hi,
> > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > It's useful when there is not device_connection registered
> > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > usb_role_switch.
> > > > > > >
> > > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > >
> > > > > > Acked-by: Heikki Krogerus <[email protected]>
> > > > >
> > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > points out that you don't need to use device graph since the
> > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > don't really need this API?
> > > > No, I still need it.
> > > > The change is about the way how to get fwnode; when use device
> > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > fwnode by of_get_parent();
> > >
> > > OK, I get that, but I'm still not convinced about if something like
> > > this function is needed at all. I also have concerns regarding how
> > > you are using the function. I'll explain in comment to the patch 5/6 in this
> series...
> >
> > FYI, Currently I am also using this api in my patch series.
> > https://patchwork.kernel.org/patch/10944637/
>
> Yes, and I have the same question for you I jusb asked in comment I added
> to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?

Currently no issue. It will work with this api as well, since the port node is part of controller node.
For eg:-
https://patchwork.kernel.org/patch/10944627/

However if any one adds port node inside the connector node, then this api may won't work as expected.
Currently I get below error

[ 2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47

For eg:-

hd3ss3220@47 {
compatible = "ti,hd3ss3220";
...
....
usb_con: connector {
....
....
port {
hd3ss3220_ep: endpoint@0 {
reg = <0>;
remote-endpoint = <&usb3peri_role_switch>;
};
};
};
};

Regards,
Biju

2019-05-21 07:37:55

by Chunfeng Yun

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi,
On Mon, 2019-05-20 at 09:45 +0000, Biju Das wrote:
>
> Hi Heikki,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> >
> > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > Hi,
> > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > usb_role_switch.
> > > > > > > >
> > > > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > >
> > > > > > > Acked-by: Heikki Krogerus <[email protected]>
> > > > > >
> > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > points out that you don't need to use device graph since the
> > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > don't really need this API?
> > > > > No, I still need it.
> > > > > The change is about the way how to get fwnode; when use device
> > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > fwnode by of_get_parent();
> > > >
> > > > OK, I get that, but I'm still not convinced about if something like
> > > > this function is needed at all. I also have concerns regarding how
> > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > series...
> > >
> > > FYI, Currently I am also using this api in my patch series.
> > > https://patchwork.kernel.org/patch/10944637/
> >
> > Yes, and I have the same question for you I jusb asked in comment I added
> > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
>
> Currently no issue. It will work with this api as well, since the port node is part of controller node.
> For eg:-
> https://patchwork.kernel.org/patch/10944627/
>
> However if any one adds port node inside the connector node, then this api may won't work as expected.
> Currently I get below error
>
> [ 2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47
>
> For eg:-
>
> hd3ss3220@47 {
> compatible = "ti,hd3ss3220";
> ...
> ....
> usb_con: connector {
> ....
> ....
> port {
> hd3ss3220_ep: endpoint@0 {
> reg = <0>;
> remote-endpoint = <&usb3peri_role_switch>;
> };
> };
> };
> };
>
> Regards,
> Biju

I tested 3 cases:

case 1:

connector {
compatible = "linux,typeb-conn-gpio", "usb-b-connector";
label = "micro-USB";
type = "micro";
id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
vbus-supply = <&usb_p0_vbus>;

port {
bconn_ep: endpoint@0 {
remote-endpoint = <&usb_role_sw>;
};
};
};

&mtu3 {
usb-role-switch;

port {
usb_role_sw: endpoint@0 {
remote-endpoint = <&bconn_ep>;
};
};
};

the driver of connector could use usb_role_switch_get(dev) to get
mtu3's USB Role Switch. (dev is the device of connector)

case 2:

&mtu3 {
usb-role-switch;

connector {
compatible = "linux,typeb-conn-gpio", "usb-b-connector";
label = "micro-USB";
type = "micro";
id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
vbus-supply = <&usb_p0_vbus>;
};
};

the driver of connector using usb_role_switch_get(dev) failed to get
mtu3's USB Role Switch.
error log:
#OF: graph: no port node found in /usb@11271000/connector
this is because connector hasn't child node connected to remote
endpoint which register USB Role Switch

case 3:

rsw_iddig: role_sw_iddig {
compatible = "linux,typeb-conn-gpio";
status = "okay";

connector {
compatible = "usb-b-connector";
label = "micro-USB";
type = "micro";
id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
vbus-supply = <&usb_p0_vbus>;

port {
bconn_ep: endpoint@0 {
remote-endpoint = <&usb_role_sw>;
};
};
};
};

&mtu3 {
usb-role-switch;

port {
usb_role_sw: endpoint@0 {
remote-endpoint = <&bconn_ep>;
};
};
};


the driver of connector using usb_role_switch_get(dev) also failed to
get mtu3's USB Role Switch. Because usb_role_switch_get() only search
its child nodes (connector node), but not child's child (port node)
This case is the same as Biju's

Usually type-c is similar with case 3;
the next version v6 of this series will use case 2 as Rob suggested,
see [v5, 2/6]

for case 2, will need the new API fwnode_usb_role_switch_get();
for case 3, use the new API, or need modify usb_role_switch_get();

2019-05-21 10:00:57

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
>
>
> Hi Heikki,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> >
> > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > Hi,
> > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > usb_role_switch.
> > > > > > > >
> > > > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > >
> > > > > > > Acked-by: Heikki Krogerus <[email protected]>
> > > > > >
> > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > points out that you don't need to use device graph since the
> > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > don't really need this API?
> > > > > No, I still need it.
> > > > > The change is about the way how to get fwnode; when use device
> > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > fwnode by of_get_parent();
> > > >
> > > > OK, I get that, but I'm still not convinced about if something like
> > > > this function is needed at all. I also have concerns regarding how
> > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > series...
> > >
> > > FYI, Currently I am also using this api in my patch series.
> > > https://patchwork.kernel.org/patch/10944637/
> >
> > Yes, and I have the same question for you I jusb asked in comment I added
> > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
>
> Currently no issue. It will work with this api as well, since the port node is part of controller node.
> For eg:-
> https://patchwork.kernel.org/patch/10944627/
>
> However if any one adds port node inside the connector node, then this api may won't work as expected.
> Currently I get below error
>
> [ 2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47

We need to understand why is that happening?

It looks like we have an issue somewhere in the code, and instead of
fixing that, you are working around it. Let's not do that.

> For eg:-
>
> hd3ss3220@47 {
> compatible = "ti,hd3ss3220";
> ...
> ....
> usb_con: connector {
> ....
> ....
> port {
> hd3ss3220_ep: endpoint@0 {
> reg = <0>;
> remote-endpoint = <&usb3peri_role_switch>;
> };
> };
> };
> };
>
> Regards,
> Biju

thanks,

--
heikki

2019-05-21 10:36:16

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Tue, May 21, 2019 at 03:35:04PM +0800, Chunfeng Yun wrote:
> Hi,
> On Mon, 2019-05-20 at 09:45 +0000, Biju Das wrote:
> >
> > Hi Heikki,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > node
> > >
> > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > Hi,
> > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > > usb_role_switch.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > >
> > > > > > > > Acked-by: Heikki Krogerus <[email protected]>
> > > > > > >
> > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > points out that you don't need to use device graph since the
> > > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > > don't really need this API?
> > > > > > No, I still need it.
> > > > > > The change is about the way how to get fwnode; when use device
> > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > > fwnode by of_get_parent();
> > > > >
> > > > > OK, I get that, but I'm still not convinced about if something like
> > > > > this function is needed at all. I also have concerns regarding how
> > > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > > series...
> > > >
> > > > FYI, Currently I am also using this api in my patch series.
> > > > https://patchwork.kernel.org/patch/10944637/
> > >
> > > Yes, and I have the same question for you I jusb asked in comment I added
> > > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
> >
> > Currently no issue. It will work with this api as well, since the port node is part of controller node.
> > For eg:-
> > https://patchwork.kernel.org/patch/10944627/
> >
> > However if any one adds port node inside the connector node, then this api may won't work as expected.
> > Currently I get below error
> >
> > [ 2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47
> >
> > For eg:-
> >
> > hd3ss3220@47 {
> > compatible = "ti,hd3ss3220";
> > ...
> > ....
> > usb_con: connector {
> > ....
> > ....
> > port {
> > hd3ss3220_ep: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&usb3peri_role_switch>;
> > };
> > };
> > };
> > };
> >
> > Regards,
> > Biju
>
> I tested 3 cases:
>
> case 1:
>
> connector {
> compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> label = "micro-USB";
> type = "micro";
> id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> vbus-supply = <&usb_p0_vbus>;
>
> port {
> bconn_ep: endpoint@0 {
> remote-endpoint = <&usb_role_sw>;
> };
> };
> };
>
> &mtu3 {
> usb-role-switch;
>
> port {
> usb_role_sw: endpoint@0 {
> remote-endpoint = <&bconn_ep>;
> };
> };
> };
>
> the driver of connector could use usb_role_switch_get(dev) to get
> mtu3's USB Role Switch. (dev is the device of connector)
>
> case 2:
>
> &mtu3 {
> usb-role-switch;
>
> connector {
> compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> label = "micro-USB";
> type = "micro";
> id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> vbus-supply = <&usb_p0_vbus>;
> };
> };
>
> the driver of connector using usb_role_switch_get(dev) failed to get
> mtu3's USB Role Switch.
> error log:
> #OF: graph: no port node found in /usb@11271000/connector
> this is because connector hasn't child node connected to remote
> endpoint which register USB Role Switch
>
> case 3:
>
> rsw_iddig: role_sw_iddig {
> compatible = "linux,typeb-conn-gpio";
> status = "okay";
>
> connector {
> compatible = "usb-b-connector";
> label = "micro-USB";
> type = "micro";
> id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> vbus-supply = <&usb_p0_vbus>;
>
> port {
> bconn_ep: endpoint@0 {
> remote-endpoint = <&usb_role_sw>;
> };
> };
> };
> };
>
> &mtu3 {
> usb-role-switch;
>
> port {
> usb_role_sw: endpoint@0 {
> remote-endpoint = <&bconn_ep>;
> };
> };
> };
>
>
> the driver of connector using usb_role_switch_get(dev) also failed to
> get mtu3's USB Role Switch. Because usb_role_switch_get() only search
> its child nodes (connector node), but not child's child (port node)
> This case is the same as Biju's
>
> Usually type-c is similar with case 3;
> the next version v6 of this series will use case 2 as Rob suggested,
> see [v5, 2/6]
>
> for case 2, will need the new API fwnode_usb_role_switch_get();

Thanks for the explanation.

In this case, if I understood this correctly, the USB controller, which
is also the role switch, is the parent of the connector. So shouldn't
we simply consider that in the current API?

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f45d8df5cfb8..2f898167b99a 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -125,6 +125,13 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
{
struct usb_role_switch *sw;

+ /*
+ * Simplest case is that a connector is looking for the controller,
+ * which is its parent.
+ */
+ if (device_property_present(dev->parent, "usb-role-switch"))
+ return to_role_switch(dev->parent);
+
sw = device_connection_find_match(dev, "usb-role-switch", NULL,
usb_role_switch_match);


> for case 3, use the new API, or need modify usb_role_switch_get();

I did not completely understand this case, but isn't it the same as
case 2 in the end, after you change it as Rob suggested?


thanks,

--
heikki

2019-05-22 03:39:29

by Chunfeng Yun

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Tue, 2019-05-21 at 13:33 +0300, Heikki Krogerus wrote:
> On Tue, May 21, 2019 at 03:35:04PM +0800, Chunfeng Yun wrote:
> > Hi,
> > On Mon, 2019-05-20 at 09:45 +0000, Biju Das wrote:
> > >
> > > Hi Heikki,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > > node
> > > >
> > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > Hi,
> > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > > > usb_role_switch.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > >
> > > > > > > > > Acked-by: Heikki Krogerus <[email protected]>
> > > > > > > >
> > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > > points out that you don't need to use device graph since the
> > > > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > > > don't really need this API?
> > > > > > > No, I still need it.
> > > > > > > The change is about the way how to get fwnode; when use device
> > > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > > > fwnode by of_get_parent();
> > > > > >
> > > > > > OK, I get that, but I'm still not convinced about if something like
> > > > > > this function is needed at all. I also have concerns regarding how
> > > > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > > > series...
> > > > >
> > > > > FYI, Currently I am also using this api in my patch series.
> > > > > https://patchwork.kernel.org/patch/10944637/
> > > >
> > > > Yes, and I have the same question for you I jusb asked in comment I added
> > > > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
> > >
> > > Currently no issue. It will work with this api as well, since the port node is part of controller node.
> > > For eg:-
> > > https://patchwork.kernel.org/patch/10944627/
> > >
> > > However if any one adds port node inside the connector node, then this api may won't work as expected.
> > > Currently I get below error
> > >
> > > [ 2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47
> > >
> > > For eg:-
> > >
> > > hd3ss3220@47 {
> > > compatible = "ti,hd3ss3220";
> > > ...
> > > ....
> > > usb_con: connector {
> > > ....
> > > ....
> > > port {
> > > hd3ss3220_ep: endpoint@0 {
> > > reg = <0>;
> > > remote-endpoint = <&usb3peri_role_switch>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > Regards,
> > > Biju
> >
> > I tested 3 cases:
> >
> > case 1:
> >
> > connector {
> > compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> > label = "micro-USB";
> > type = "micro";
> > id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> > vbus-supply = <&usb_p0_vbus>;
> >
> > port {
> > bconn_ep: endpoint@0 {
> > remote-endpoint = <&usb_role_sw>;
> > };
> > };
> > };
> >
> > &mtu3 {
> > usb-role-switch;
> >
> > port {
> > usb_role_sw: endpoint@0 {
> > remote-endpoint = <&bconn_ep>;
> > };
> > };
> > };
> >
> > the driver of connector could use usb_role_switch_get(dev) to get
> > mtu3's USB Role Switch. (dev is the device of connector)
> >
> > case 2:
> >
> > &mtu3 {
> > usb-role-switch;
> >
> > connector {
> > compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> > label = "micro-USB";
> > type = "micro";
> > id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> > vbus-supply = <&usb_p0_vbus>;
> > };
> > };
> >
> > the driver of connector using usb_role_switch_get(dev) failed to get
> > mtu3's USB Role Switch.
> > error log:
> > #OF: graph: no port node found in /usb@11271000/connector
> > this is because connector hasn't child node connected to remote
> > endpoint which register USB Role Switch
> >
> > case 3:
> >
> > rsw_iddig: role_sw_iddig {
> > compatible = "linux,typeb-conn-gpio";
> > status = "okay";
> >
> > connector {
> > compatible = "usb-b-connector";
> > label = "micro-USB";
> > type = "micro";
> > id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> > vbus-supply = <&usb_p0_vbus>;
> >
> > port {
> > bconn_ep: endpoint@0 {
> > remote-endpoint = <&usb_role_sw>;
> > };
> > };
> > };
> > };
> >
> > &mtu3 {
> > usb-role-switch;
> >
> > port {
> > usb_role_sw: endpoint@0 {
> > remote-endpoint = <&bconn_ep>;
> > };
> > };
> > };
> >
> >
> > the driver of connector using usb_role_switch_get(dev) also failed to
> > get mtu3's USB Role Switch. Because usb_role_switch_get() only search
> > its child nodes (connector node), but not child's child (port node)
> > This case is the same as Biju's
> >
> > Usually type-c is similar with case 3;
> > the next version v6 of this series will use case 2 as Rob suggested,
> > see [v5, 2/6]
> >
> > for case 2, will need the new API fwnode_usb_role_switch_get();
>
> Thanks for the explanation.
>
> In this case, if I understood this correctly, the USB controller, which
> is also the role switch, is the parent of the connector. So shouldn't
> we simply consider that in the current API?
It's better if can be added into the current API.
I'll try it.
>
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f45d8df5cfb8..2f898167b99a 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -125,6 +125,13 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> {
> struct usb_role_switch *sw;
>
> + /*
> + * Simplest case is that a connector is looking for the controller,
> + * which is its parent.
> + */
> + if (device_property_present(dev->parent, "usb-role-switch"))
> + return to_role_switch(dev->parent);
> +
> sw = device_connection_find_match(dev, "usb-role-switch", NULL,
> usb_role_switch_match);
>
>
> > for case 3, use the new API, or need modify usb_role_switch_get();
>
> I did not completely understand this case, but isn't it the same as
> case 2 in the end, after you change it as Rob suggested?
I'm afraid not, their bindings are different, see
connector/usb-connector.txt
>
>
> thanks,
>

2019-05-22 08:07:06

by Biju Das

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Heikki,

Thanks for the feedback.

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
>
> On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> >
> >
> > Hi Heikki,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > Hi,
> > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> wrote:
> > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > registered between two drivers and only knows the fwnode
> > > > > > > > > which register usb_role_switch.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > >
> > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > <[email protected]>
> > > > > > >
> > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > points out that you don't need to use device graph since the
> > > > > > > controller is the parent of the connector. Doesn't that mean
> > > > > > > you don't really need this API?
> > > > > > No, I still need it.
> > > > > > The change is about the way how to get fwnode; when use device
> > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will
> > > > > > get fwnode by of_get_parent();
> > > > >
> > > > > OK, I get that, but I'm still not convinced about if something
> > > > > like this function is needed at all. I also have concerns
> > > > > regarding how you are using the function. I'll explain in
> > > > > comment to the patch 5/6 in this
> > > series...
> > > >
> > > > FYI, Currently I am also using this api in my patch series.
> > > > https://patchwork.kernel.org/patch/10944637/
> > >
> > > Yes, and I have the same question for you I jusb asked in comment I
> > > added to the patch 5/6 of this series. Why isn't usb_role_switch_get()
> enough?
> >
> > Currently no issue. It will work with this api as well, since the port node is
> part of controller node.
> > For eg:-
> > https://patchwork.kernel.org/patch/10944627/
> >
> > However if any one adds port node inside the connector node, then this
> api may won't work as expected.
> > Currently I get below error
> >
> > [ 2.299703] OF: graph: no port node found in
> /soc/i2c@e6500000/hd3ss3220@47
>
> We need to understand why is that happening?
>

Form the stack trace the parent node is "parent_node=hd3ss3220@47" , instead of the "connector" node.
That is the reason for the above error.

[ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
[ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
[ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
[ 2.458374] device_connection_find_match+0x74/0x1a0
[ 2.463399] usb_role_switch_get+0x20/0x28
[ 2.467542] hd3ss3220_probe+0xc4/0x218

The use case is

&i2c0 {
hd3ss3220@47 {
compatible = "ti,hd3ss3220";

usb_con: connector {
compatible = "usb-c-connector";
port {
hd3ss3220_ep: endpoint {
remote-endpoint = <&usb3_role_switch>;
};
};
};
};
};

&usb3_peri0 {
companion = <&xhci0>;
usb-role-switch;

port {
usb3_role_switch: endpoint {
remote-endpoint = <&hd3ss3220_ep>;
};
};
};

Q1) How do we modify the usb_role_switch_get() function to search
Child(connector) and child's endpoint?

> It looks like we have an issue somewhere in the code, and instead of fixing
> that, you are working around it. Let's not do that.

OK.

Regards,
Biju

2019-05-22 09:33:20

by Chunfeng Yun

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Biju,
On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> Hi Heikki,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> >
> > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > >
> > >
> > > Hi Heikki,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > Hi,
> > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > wrote:
> > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > registered between two drivers and only knows the fwnode
> > > > > > > > > > which register usb_role_switch.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > >
> > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > <[email protected]>
> > > > > > > >
> > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > > points out that you don't need to use device graph since the
> > > > > > > > controller is the parent of the connector. Doesn't that mean
> > > > > > > > you don't really need this API?
> > > > > > > No, I still need it.
> > > > > > > The change is about the way how to get fwnode; when use device
> > > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will
> > > > > > > get fwnode by of_get_parent();
> > > > > >
> > > > > > OK, I get that, but I'm still not convinced about if something
> > > > > > like this function is needed at all. I also have concerns
> > > > > > regarding how you are using the function. I'll explain in
> > > > > > comment to the patch 5/6 in this
> > > > series...
> > > > >
> > > > > FYI, Currently I am also using this api in my patch series.
> > > > > https://patchwork.kernel.org/patch/10944637/
> > > >
> > > > Yes, and I have the same question for you I jusb asked in comment I
> > > > added to the patch 5/6 of this series. Why isn't usb_role_switch_get()
> > enough?
> > >
> > > Currently no issue. It will work with this api as well, since the port node is
> > part of controller node.
> > > For eg:-
> > > https://patchwork.kernel.org/patch/10944627/
> > >
> > > However if any one adds port node inside the connector node, then this
> > api may won't work as expected.
> > > Currently I get below error
> > >
> > > [ 2.299703] OF: graph: no port node found in
> > /soc/i2c@e6500000/hd3ss3220@47
> >
> > We need to understand why is that happening?
> >
>
> Form the stack trace the parent node is "parent_node=hd3ss3220@47" , instead of the "connector" node.
> That is the reason for the above error.
>
> [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> [ 2.458374] device_connection_find_match+0x74/0x1a0
> [ 2.463399] usb_role_switch_get+0x20/0x28
> [ 2.467542] hd3ss3220_probe+0xc4/0x218
>
> The use case is
>
> &i2c0 {
> hd3ss3220@47 {
> compatible = "ti,hd3ss3220";
>
> usb_con: connector {
> compatible = "usb-c-connector";
> port {
> hd3ss3220_ep: endpoint {
> remote-endpoint = <&usb3_role_switch>;
> };
> };
> };
> };
> };
>
> &usb3_peri0 {
> companion = <&xhci0>;
> usb-role-switch;
>
> port {
> usb3_role_switch: endpoint {
> remote-endpoint = <&hd3ss3220_ep>;
> };
> };
> };
>
> Q1) How do we modify the usb_role_switch_get() function to search
> Child(connector) and child's endpoint?
How about firstly finding connector node in fwnode_graph_devcon_match(),
then search each endpoint?

>
> > It looks like we have an issue somewhere in the code, and instead of fixing
> > that, you are working around it. Let's not do that.
>
> OK.
>
> Regards,
> Biju
>

2019-05-22 10:57:45

by Biju Das

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Chunfeng Yun,

Thanks for the feedback.

> Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
>
> Hi Biju,
> On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > Hi Heikki,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > >
> > > >
> > > > Hi Heikki,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > Hi Heikki,
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > Hi,
> > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> wrote:
> > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > wrote:
> > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > <[email protected]>
> > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > >
> > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > <[email protected]>
> > > > > > > > >
> > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > No, I still need it.
> > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > >
> > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > something like this function is needed at all. I also have
> > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > explain in comment to the patch 5/6 in this
> > > > > series...
> > > > > >
> > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > >
> > > > > Yes, and I have the same question for you I jusb asked in
> > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > usb_role_switch_get()
> > > enough?
> > > >
> > > > Currently no issue. It will work with this api as well, since the
> > > > port node is
> > > part of controller node.
> > > > For eg:-
> > > > https://patchwork.kernel.org/patch/10944627/
> > > >
> > > > However if any one adds port node inside the connector node, then
> > > > this
> > > api may won't work as expected.
> > > > Currently I get below error
> > > >
> > > > [ 2.299703] OF: graph: no port node found in
> > > /soc/i2c@e6500000/hd3ss3220@47
> > >
> > > We need to understand why is that happening?
> > >
> >
> > Form the stack trace the parent node is "parent_node=hd3ss3220@47" ,
> instead of the "connector" node.
> > That is the reason for the above error.
> >
> > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > [ 2.463399] usb_role_switch_get+0x20/0x28
> > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> >
> > The use case is
> >
> > &i2c0 {
> > hd3ss3220@47 {
> > compatible = "ti,hd3ss3220";
> >
> > usb_con: connector {
> > compatible = "usb-c-connector";
> > port {
> > hd3ss3220_ep: endpoint {
> > remote-endpoint =
> <&usb3_role_switch>;
> > };
> > };
> > };
> > };
> > };
> >
> > &usb3_peri0 {
> > companion = <&xhci0>;
> > usb-role-switch;
> >
> > port {
> > usb3_role_switch: endpoint {
> > remote-endpoint = <&hd3ss3220_ep>;
> > };
> > };
> > };
> >
> > Q1) How do we modify the usb_role_switch_get() function to search
> > Child(connector) and child's endpoint?
> How about firstly finding connector node in fwnode_graph_devcon_match(),
> then search each endpoint?

I have done a quick prototyping with the changes you suggested and it works.

- struct fwnode_handle *ep;
+ struct fwnode_handle *ep,*child,*tmp = fwnode;

- fwnode_graph_for_each_endpoint(fwnode, ep) {
+ child = fwnode_get_named_child_node(fwnode, "connector");
+ if (child)
+ tmp = child;
+
+ fwnode_graph_for_each_endpoint(tmp, ep) {

Form the stack trace the parent node is "parent_node= connector" .

[ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
[ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
[ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
[ 2.456866] device_connection_find_match+0x84/0x1c0
[ 2.461888] usb_role_switch_get+0x20/0x28

Heikki,
Are you ok with the above changes?

Regards,
Biju
> >
> > > It looks like we have an issue somewhere in the code, and instead of
> > > fixing that, you are working around it. Let's not do that.
> >
> > OK.
> >
> > Regards,
> > Biju
> >
>

2019-05-22 14:28:51

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> Hi Chunfeng Yun,
>
> Thanks for the feedback.
>
> > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> >
> > Hi Biju,
> > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > >
> > > > >
> > > > > Hi Heikki,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > Hi,
> > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > wrote:
> > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > wrote:
> > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > <[email protected]>
> > > > > > > > > >
> > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > No, I still need it.
> > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > >
> > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > something like this function is needed at all. I also have
> > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > series...
> > > > > > >
> > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > >
> > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > usb_role_switch_get()
> > > > enough?
> > > > >
> > > > > Currently no issue. It will work with this api as well, since the
> > > > > port node is
> > > > part of controller node.
> > > > > For eg:-
> > > > > https://patchwork.kernel.org/patch/10944627/
> > > > >
> > > > > However if any one adds port node inside the connector node, then
> > > > > this
> > > > api may won't work as expected.
> > > > > Currently I get below error
> > > > >
> > > > > [ 2.299703] OF: graph: no port node found in
> > > > /soc/i2c@e6500000/hd3ss3220@47
> > > >
> > > > We need to understand why is that happening?
> > > >
> > >
> > > Form the stack trace the parent node is "parent_node=hd3ss3220@47" ,
> > instead of the "connector" node.
> > > That is the reason for the above error.
> > >
> > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > >
> > > The use case is
> > >
> > > &i2c0 {
> > > hd3ss3220@47 {
> > > compatible = "ti,hd3ss3220";
> > >
> > > usb_con: connector {
> > > compatible = "usb-c-connector";
> > > port {
> > > hd3ss3220_ep: endpoint {
> > > remote-endpoint =
> > <&usb3_role_switch>;
> > > };
> > > };
> > > };
> > > };
> > > };
> > >
> > > &usb3_peri0 {
> > > companion = <&xhci0>;
> > > usb-role-switch;
> > >
> > > port {
> > > usb3_role_switch: endpoint {
> > > remote-endpoint = <&hd3ss3220_ep>;
> > > };
> > > };
> > > };
> > >
> > > Q1) How do we modify the usb_role_switch_get() function to search
> > > Child(connector) and child's endpoint?
> > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > then search each endpoint?
>
> I have done a quick prototyping with the changes you suggested and it works.
>
> - struct fwnode_handle *ep;
> + struct fwnode_handle *ep,*child,*tmp = fwnode;
>
> - fwnode_graph_for_each_endpoint(fwnode, ep) {
> + child = fwnode_get_named_child_node(fwnode, "connector");
> + if (child)
> + tmp = child;
> +
> + fwnode_graph_for_each_endpoint(tmp, ep) {
>
> Form the stack trace the parent node is "parent_node= connector" .
>
> [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> [ 2.456866] device_connection_find_match+0x84/0x1c0
> [ 2.461888] usb_role_switch_get+0x20/0x28
>
> Heikki,
> Are you ok with the above changes?

Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
I proposed, there is no problem? You just find the "connector" child
node in your driver, and pass that to fwnode_usb_role_switch_get():

struct fwnode_handle *connector;
...
connector = device_get_named_child_node(&client->dev, "connector");
if (IS_ERR(connector))
<do something>

hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
...

The difference is that instead of just converting a device node of an
usb role switch to the usb role switch, it works just like
usb_role_switch_get(), just taking fwnode instead of device entry as
parameter.

I prepared the patches implementing fwnode_usb_role_switch_get() the
way I though it needs to work for my own tests. Please find the
patches attached.

thanks,

--
heikki

Attachments:

2019-05-22 15:33:52

by Biju Das

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Heikki,

Thanks for the patch

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
>
> On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > Hi Chunfeng Yun,
> >
> > Thanks for the feedback.
> >
> > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > Hi Biju,
> > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > >
> > > > > >
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun
> wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki
> > > > > > > > > > > Krogerus
> > > wrote:
> > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng
> > > > > > > > > > > > Yun
> > > > > wrote:
> > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier
> > > > > > > > > > > > > to get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > registered between two drivers and only knows
> > > > > > > > > > > > > the fwnode which register usb_role_switch.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > where he points out that you don't need to use
> > > > > > > > > > > device graph since the controller is the parent of
> > > > > > > > > > > the connector. Doesn't that mean you don't really need
> this API?
> > > > > > > > > > No, I still need it.
> > > > > > > > > > The change is about the way how to get fwnode; when
> > > > > > > > > > use device graph, get fwnode by
> > > > > > > > > > of_graph_get_remote_node(); but now will get fwnode by
> > > > > > > > > > of_get_parent();
> > > > > > > > >
> > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > something like this function is needed at all. I also
> > > > > > > > > have concerns regarding how you are using the function.
> > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > series...
> > > > > > > >
> > > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > >
> > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > usb_role_switch_get()
> > > > > enough?
> > > > > >
> > > > > > Currently no issue. It will work with this api as well, since
> > > > > > the port node is
> > > > > part of controller node.
> > > > > > For eg:-
> > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > >
> > > > > > However if any one adds port node inside the connector node,
> > > > > > then this
> > > > > api may won't work as expected.
> > > > > > Currently I get below error
> > > > > >
> > > > > > [ 2.299703] OF: graph: no port node found in
> > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > >
> > > > > We need to understand why is that happening?
> > > > >
> > > >
> > > > Form the stack trace the parent node is
> > > > "parent_node=hd3ss3220@47" ,
> > > instead of the "connector" node.
> > > > That is the reason for the above error.
> > > >
> > > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > > >
> > > > The use case is
> > > >
> > > > &i2c0 {
> > > > hd3ss3220@47 {
> > > > compatible = "ti,hd3ss3220";
> > > >
> > > > usb_con: connector {
> > > > compatible = "usb-c-connector";
> > > > port {
> > > > hd3ss3220_ep: endpoint {
> > > > remote-endpoint =
> > > <&usb3_role_switch>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > &usb3_peri0 {
> > > > companion = <&xhci0>;
> > > > usb-role-switch;
> > > >
> > > > port {
> > > > usb3_role_switch: endpoint {
> > > > remote-endpoint = <&hd3ss3220_ep>;
> > > > };
> > > > };
> > > > };
> > > >
> > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > Child(connector) and child's endpoint?
> > > How about firstly finding connector node in
> > > fwnode_graph_devcon_match(), then search each endpoint?
> >
> > I have done a quick prototyping with the changes you suggested and it
> works.
> >
> > - struct fwnode_handle *ep;
> > + struct fwnode_handle *ep,*child,*tmp = fwnode;
> >
> > - fwnode_graph_for_each_endpoint(fwnode, ep) {
> > + child = fwnode_get_named_child_node(fwnode, "connector");
> > + if (child)
> > + tmp = child;
> > +
> > + fwnode_graph_for_each_endpoint(tmp, ep) {
> >
> > Form the stack trace the parent node is "parent_node= connector" .
> >
> > [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> > [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> > [ 2.456866] device_connection_find_match+0x84/0x1c0
> > [ 2.461888] usb_role_switch_get+0x20/0x28
> >
> > Heikki,
> > Are you ok with the above changes?
>
> Doesn't that mean that if we made fwnode_usb_role_switch_get() the way I
> proposed, there is no problem? You just find the "connector" child node in
> your driver, and pass that to fwnode_usb_role_switch_get():

Yes, That is correct.

> struct fwnode_handle *connector;
> ...
> connector = device_get_named_child_node(&client->dev, "connector");
> if (IS_ERR(connector))
> <do something>
>
> hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> ...
>
> The difference is that instead of just converting a device node of an usb role
> switch to the usb role switch, it works just like usb_role_switch_get(), just
> taking fwnode instead of device entry as parameter.
>
> I prepared the patches implementing fwnode_usb_role_switch_get() the
> way I though it needs to work for my own tests. Please find the patches
> attached.

I have tested this patches and conform it works.
Do you plan to post this patches to ML?

Regards,
Biju

2019-05-23 10:18:03

by Chunfeng Yun

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Heikki,
On Wed, 2019-05-22 at 17:26 +0300, Heikki Krogerus wrote:
> On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > Hi Chunfeng Yun,
> >
> > Thanks for the feedback.
> >
> > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > node
> > >
> > > Hi Biju,
> > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > >
> > > > > >
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > > wrote:
> > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > > wrote:
> > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > > No, I still need it.
> > > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > > >
> > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > something like this function is needed at all. I also have
> > > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > > series...
> > > > > > > >
> > > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > >
> > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > usb_role_switch_get()
> > > > > enough?
> > > > > >
> > > > > > Currently no issue. It will work with this api as well, since the
> > > > > > port node is
> > > > > part of controller node.
> > > > > > For eg:-
> > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > >
> > > > > > However if any one adds port node inside the connector node, then
> > > > > > this
> > > > > api may won't work as expected.
> > > > > > Currently I get below error
> > > > > >
> > > > > > [ 2.299703] OF: graph: no port node found in
> > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > >
> > > > > We need to understand why is that happening?
> > > > >
> > > >
> > > > Form the stack trace the parent node is "parent_node=hd3ss3220@47" ,
> > > instead of the "connector" node.
> > > > That is the reason for the above error.
> > > >
> > > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > > >
> > > > The use case is
> > > >
> > > > &i2c0 {
> > > > hd3ss3220@47 {
> > > > compatible = "ti,hd3ss3220";
> > > >
> > > > usb_con: connector {
> > > > compatible = "usb-c-connector";
> > > > port {
> > > > hd3ss3220_ep: endpoint {
> > > > remote-endpoint =
> > > <&usb3_role_switch>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > &usb3_peri0 {
> > > > companion = <&xhci0>;
> > > > usb-role-switch;
> > > >
> > > > port {
> > > > usb3_role_switch: endpoint {
> > > > remote-endpoint = <&hd3ss3220_ep>;
> > > > };
> > > > };
> > > > };
> > > >
> > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > Child(connector) and child's endpoint?
> > > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > > then search each endpoint?
> >
> > I have done a quick prototyping with the changes you suggested and it works.
> >
> > - struct fwnode_handle *ep;
> > + struct fwnode_handle *ep,*child,*tmp = fwnode;
> >
> > - fwnode_graph_for_each_endpoint(fwnode, ep) {
> > + child = fwnode_get_named_child_node(fwnode, "connector");
> > + if (child)
> > + tmp = child;
> > +
> > + fwnode_graph_for_each_endpoint(tmp, ep) {
> >
> > Form the stack trace the parent node is "parent_node= connector" .
> >
> > [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> > [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> > [ 2.456866] device_connection_find_match+0x84/0x1c0
> > [ 2.461888] usb_role_switch_get+0x20/0x28
> >
> > Heikki,
> > Are you ok with the above changes?
>
> Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
> I proposed, there is no problem? You just find the "connector" child
> node in your driver, and pass that to fwnode_usb_role_switch_get():
>
> struct fwnode_handle *connector;
> ...
> connector = device_get_named_child_node(&client->dev, "connector");
> if (IS_ERR(connector))
> <do something>
>
> hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> ...
>
> The difference is that instead of just converting a device node of an
> usb role switch to the usb role switch, it works just like
> usb_role_switch_get(), just taking fwnode instead of device entry as
> parameter.
>
> I prepared the patches implementing fwnode_usb_role_switch_get() the
> way I though it needs to work for my own tests. Please find the
> patches attached.
I tested these patches, it didn't work for case as following:

case 2:

&mtu3 {
usb-role-switch;

connector {
compatible = "linux,typeb-conn-gpio", "usb-b-connector";
label = "micro-USB";
type = "micro";
id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
vbus-supply = <&usb_p0_vbus>;
};
};

so I consider this case in the function fwnode_graph_devcon_match()
(based on your patches)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 8311b70..1dae8b7 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -18,9 +18,11 @@
{
struct device_connection con = { .id = con_id };
struct fwnode_handle *ep;
+ int ep_count = 0;
void *ret;

fwnode_graph_for_each_endpoint(fwnode, ep) {
+ ep_count++;
con.fwnode = fwnode_graph_get_remote_port_parent(ep);
if (!fwnode_device_is_available(con.fwnode))
continue;
@@ -32,6 +34,19 @@
return ret;
}
}
+
+ /* if the connector has no remote endpoint, check its parent */
+ if (!ep_count) {
+ con.fwnode = fwnode_get_parent(fwnode);
+ if (!con.fwnode)
+ return NULL;
+
+ ret = match(&con, -1, data);
+ fwnode_handle_put(con.fwnode);
+ if (ret)
+ return ret;
+ }
+
return NULL;
}

see attached patch.

then both usb_role_switch_get(dev) or fwnode_usb_role_switch_get(fwnode)
work well;

but I don't know which way is better when consider this specific case,
put into class.c as you suggested before or put into devcon.c like
above.

Thanks a lot

>
> thanks,
>

Attachments:

2019-05-24 11:42:07

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Thu, May 23, 2019 at 06:16:10PM +0800, Chunfeng Yun wrote:
> Hi Heikki,
> On Wed, 2019-05-22 at 17:26 +0300, Heikki Krogerus wrote:
> > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > Hi Chunfeng Yun,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > > node
> > > >
> > > > Hi Biju,
> > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > Thanks for the feedback.
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > Hi Heikki,
> > > > > > > > >
> > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > usb_role_switch by node
> > > > > > > > > >
> > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > > > wrote:
> > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > > > wrote:
> > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > > > No, I still need it.
> > > > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > > > >
> > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > something like this function is needed at all. I also have
> > > > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > > > series...
> > > > > > > > >
> > > > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > >
> > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > usb_role_switch_get()
> > > > > > enough?
> > > > > > >
> > > > > > > Currently no issue. It will work with this api as well, since the
> > > > > > > port node is
> > > > > > part of controller node.
> > > > > > > For eg:-
> > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > >
> > > > > > > However if any one adds port node inside the connector node, then
> > > > > > > this
> > > > > > api may won't work as expected.
> > > > > > > Currently I get below error
> > > > > > >
> > > > > > > [ 2.299703] OF: graph: no port node found in
> > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > >
> > > > > > We need to understand why is that happening?
> > > > > >
> > > > >
> > > > > Form the stack trace the parent node is "parent_node=hd3ss3220@47" ,
> > > > instead of the "connector" node.
> > > > > That is the reason for the above error.
> > > > >
> > > > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > > > >
> > > > > The use case is
> > > > >
> > > > > &i2c0 {
> > > > > hd3ss3220@47 {
> > > > > compatible = "ti,hd3ss3220";
> > > > >
> > > > > usb_con: connector {
> > > > > compatible = "usb-c-connector";
> > > > > port {
> > > > > hd3ss3220_ep: endpoint {
> > > > > remote-endpoint =
> > > > <&usb3_role_switch>;
> > > > > };
> > > > > };
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > > > &usb3_peri0 {
> > > > > companion = <&xhci0>;
> > > > > usb-role-switch;
> > > > >
> > > > > port {
> > > > > usb3_role_switch: endpoint {
> > > > > remote-endpoint = <&hd3ss3220_ep>;
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > Child(connector) and child's endpoint?
> > > > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > > > then search each endpoint?
> > >
> > > I have done a quick prototyping with the changes you suggested and it works.
> > >
> > > - struct fwnode_handle *ep;
> > > + struct fwnode_handle *ep,*child,*tmp = fwnode;
> > >
> > > - fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > + child = fwnode_get_named_child_node(fwnode, "connector");
> > > + if (child)
> > > + tmp = child;
> > > +
> > > + fwnode_graph_for_each_endpoint(tmp, ep) {
> > >
> > > Form the stack trace the parent node is "parent_node= connector" .
> > >
> > > [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> > > [ 2.456866] device_connection_find_match+0x84/0x1c0
> > > [ 2.461888] usb_role_switch_get+0x20/0x28
> > >
> > > Heikki,
> > > Are you ok with the above changes?
> >
> > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
> > I proposed, there is no problem? You just find the "connector" child
> > node in your driver, and pass that to fwnode_usb_role_switch_get():
> >
> > struct fwnode_handle *connector;
> > ...
> > connector = device_get_named_child_node(&client->dev, "connector");
> > if (IS_ERR(connector))
> > <do something>
> >
> > hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > ...
> >
> > The difference is that instead of just converting a device node of an
> > usb role switch to the usb role switch, it works just like
> > usb_role_switch_get(), just taking fwnode instead of device entry as
> > parameter.
> >
> > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > way I though it needs to work for my own tests. Please find the
> > patches attached.
> I tested these patches, it didn't work for case as following:
>
> case 2:
>
> &mtu3 {
> usb-role-switch;
>
> connector {
> compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> label = "micro-USB";
> type = "micro";
> id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> vbus-supply = <&usb_p0_vbus>;
> };
> };
>
> so I consider this case in the function fwnode_graph_devcon_match()
> (based on your patches)

IMO that case should be handled in USB role switch API initially, not
in the device connection API. If there is another user for it one day,
then we can move it to device connection API, but not before that.

How about this on top of the two patches I sent:

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index aab795b54c7f..36ac9d181e09 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -114,6 +114,19 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
}

+static struct usb_role_switch *
+usb_role_switch_is_parent(struct fwnode_handle *parent)
+{
+ struct device *dev;
+
+ if (!parent ||?!fwnode_property_present(parent, "usb-role-switch"))
+ return NULL;
+
+ dev = class_find_device(role_class, NULL, parent, switch_fwnode_match);
+
+ return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
+}
+
/**
* usb_role_switch_get - Find USB role switch linked with the caller
* @dev: The caller device
@@ -125,6 +138,10 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
{
struct usb_role_switch *sw;

+ sw = usb_role_switch_is_parent(fwnode_get_parent(dev_fwnode(dev)));
+ if (sw)
+ return sw;
+
sw = device_connection_find_match(dev, "usb-role-switch", NULL,
usb_role_switch_match);

@@ -146,6 +163,10 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
{
struct usb_role_switch *sw;

+ sw = usb_role_switch_is_parent(fwnode_get_parent(fwnode));
+ if (sw)
+ return sw;
+
sw = fwnode_connection_find_match(fwnode, "usb-role-switch", NULL,
usb_role_switch_match);
if (!IS_ERR_OR_NULL(sw))


thanks,

--
heikki

2019-05-24 12:47:22

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Wed, May 22, 2019 at 02:57:33PM +0000, Biju Das wrote:
> Hi Heikki,
>
> Thanks for the patch
>
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> >
> > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > Hi Chunfeng Yun,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > Hi Biju,
> > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > Thanks for the feedback.
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > Hi Heikki,
> > > > > > > > >
> > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > usb_role_switch by node
> > > > > > > > > >
> > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun
> > wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki
> > > > > > > > > > > > Krogerus
> > > > wrote:
> > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng
> > > > > > > > > > > > > Yun
> > > > > > wrote:
> > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier
> > > > > > > > > > > > > > to get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > registered between two drivers and only knows
> > > > > > > > > > > > > > the fwnode which register usb_role_switch.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > where he points out that you don't need to use
> > > > > > > > > > > > device graph since the controller is the parent of
> > > > > > > > > > > > the connector. Doesn't that mean you don't really need
> > this API?
> > > > > > > > > > > No, I still need it.
> > > > > > > > > > > The change is about the way how to get fwnode; when
> > > > > > > > > > > use device graph, get fwnode by
> > > > > > > > > > > of_graph_get_remote_node(); but now will get fwnode by
> > > > > > > > > > > of_get_parent();
> > > > > > > > > >
> > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > something like this function is needed at all. I also
> > > > > > > > > > have concerns regarding how you are using the function.
> > > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > > series...
> > > > > > > > >
> > > > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > >
> > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > usb_role_switch_get()
> > > > > > enough?
> > > > > > >
> > > > > > > Currently no issue. It will work with this api as well, since
> > > > > > > the port node is
> > > > > > part of controller node.
> > > > > > > For eg:-
> > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > >
> > > > > > > However if any one adds port node inside the connector node,
> > > > > > > then this
> > > > > > api may won't work as expected.
> > > > > > > Currently I get below error
> > > > > > >
> > > > > > > [ 2.299703] OF: graph: no port node found in
> > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > >
> > > > > > We need to understand why is that happening?
> > > > > >
> > > > >
> > > > > Form the stack trace the parent node is
> > > > > "parent_node=hd3ss3220@47" ,
> > > > instead of the "connector" node.
> > > > > That is the reason for the above error.
> > > > >
> > > > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > > > >
> > > > > The use case is
> > > > >
> > > > > &i2c0 {
> > > > > hd3ss3220@47 {
> > > > > compatible = "ti,hd3ss3220";
> > > > >
> > > > > usb_con: connector {
> > > > > compatible = "usb-c-connector";
> > > > > port {
> > > > > hd3ss3220_ep: endpoint {
> > > > > remote-endpoint =
> > > > <&usb3_role_switch>;
> > > > > };
> > > > > };
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > > > &usb3_peri0 {
> > > > > companion = <&xhci0>;
> > > > > usb-role-switch;
> > > > >
> > > > > port {
> > > > > usb3_role_switch: endpoint {
> > > > > remote-endpoint = <&hd3ss3220_ep>;
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > Child(connector) and child's endpoint?
> > > > How about firstly finding connector node in
> > > > fwnode_graph_devcon_match(), then search each endpoint?
> > >
> > > I have done a quick prototyping with the changes you suggested and it
> > works.
> > >
> > > - struct fwnode_handle *ep;
> > > + struct fwnode_handle *ep,*child,*tmp = fwnode;
> > >
> > > - fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > + child = fwnode_get_named_child_node(fwnode, "connector");
> > > + if (child)
> > > + tmp = child;
> > > +
> > > + fwnode_graph_for_each_endpoint(tmp, ep) {
> > >
> > > Form the stack trace the parent node is "parent_node= connector" .
> > >
> > > [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> > > [ 2.456866] device_connection_find_match+0x84/0x1c0
> > > [ 2.461888] usb_role_switch_get+0x20/0x28
> > >
> > > Heikki,
> > > Are you ok with the above changes?
> >
> > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way I
> > proposed, there is no problem? You just find the "connector" child node in
> > your driver, and pass that to fwnode_usb_role_switch_get():
>
> Yes, That is correct.
>
> > struct fwnode_handle *connector;
> > ...
> > connector = device_get_named_child_node(&client->dev, "connector");
> > if (IS_ERR(connector))
> > <do something>
> >
> > hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > ...
> >
> > The difference is that instead of just converting a device node of an usb role
> > switch to the usb role switch, it works just like usb_role_switch_get(), just
> > taking fwnode instead of device entry as parameter.
> >
> > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > way I though it needs to work for my own tests. Please find the patches
> > attached.
>
> I have tested this patches and conform it works.
> Do you plan to post this patches to ML?

Could make them part of this series?


thanks,

--
heikki

2019-05-27 03:09:02

by Chunfeng Yun

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

On Fri, 2019-05-24 at 14:40 +0300, Heikki Krogerus wrote:
> On Thu, May 23, 2019 at 06:16:10PM +0800, Chunfeng Yun wrote:
> > Hi Heikki,
> > On Wed, 2019-05-22 at 17:26 +0300, Heikki Krogerus wrote:
> > > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > > Hi Chunfeng Yun,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > > > node
> > > > >
> > > > > Hi Biju,
> > > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > Thanks for the feedback.
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > > Hi Heikki,
> > > > > > > > > >
> > > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > > usb_role_switch by node
> > > > > > > > > > >
> > > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > > > > wrote:
> > > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > > > > wrote:
> > > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > > > > No, I still need it.
> > > > > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > > > > >
> > > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > > something like this function is needed at all. I also have
> > > > > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > > > > series...
> > > > > > > > > >
> > > > > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > > >
> > > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > > usb_role_switch_get()
> > > > > > > enough?
> > > > > > > >
> > > > > > > > Currently no issue. It will work with this api as well, since the
> > > > > > > > port node is
> > > > > > > part of controller node.
> > > > > > > > For eg:-
> > > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > > >
> > > > > > > > However if any one adds port node inside the connector node, then
> > > > > > > > this
> > > > > > > api may won't work as expected.
> > > > > > > > Currently I get below error
> > > > > > > >
> > > > > > > > [ 2.299703] OF: graph: no port node found in
> > > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > > >
> > > > > > > We need to understand why is that happening?
> > > > > > >
> > > > > >
> > > > > > Form the stack trace the parent node is "parent_node=hd3ss3220@47" ,
> > > > > instead of the "connector" node.
> > > > > > That is the reason for the above error.
> > > > > >
> > > > > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > > > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > > > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > > > > >
> > > > > > The use case is
> > > > > >
> > > > > > &i2c0 {
> > > > > > hd3ss3220@47 {
> > > > > > compatible = "ti,hd3ss3220";
> > > > > >
> > > > > > usb_con: connector {
> > > > > > compatible = "usb-c-connector";
> > > > > > port {
> > > > > > hd3ss3220_ep: endpoint {
> > > > > > remote-endpoint =
> > > > > <&usb3_role_switch>;
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > >
> > > > > > &usb3_peri0 {
> > > > > > companion = <&xhci0>;
> > > > > > usb-role-switch;
> > > > > >
> > > > > > port {
> > > > > > usb3_role_switch: endpoint {
> > > > > > remote-endpoint = <&hd3ss3220_ep>;
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > >
> > > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > > Child(connector) and child's endpoint?
> > > > > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > > > > then search each endpoint?
> > > >
> > > > I have done a quick prototyping with the changes you suggested and it works.
> > > >
> > > > - struct fwnode_handle *ep;
> > > > + struct fwnode_handle *ep,*child,*tmp = fwnode;
> > > >
> > > > - fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > > + child = fwnode_get_named_child_node(fwnode, "connector");
> > > > + if (child)
> > > > + tmp = child;
> > > > +
> > > > + fwnode_graph_for_each_endpoint(tmp, ep) {
> > > >
> > > > Form the stack trace the parent node is "parent_node= connector" .
> > > >
> > > > [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [ 2.456866] device_connection_find_match+0x84/0x1c0
> > > > [ 2.461888] usb_role_switch_get+0x20/0x28
> > > >
> > > > Heikki,
> > > > Are you ok with the above changes?
> > >
> > > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
> > > I proposed, there is no problem? You just find the "connector" child
> > > node in your driver, and pass that to fwnode_usb_role_switch_get():
> > >
> > > struct fwnode_handle *connector;
> > > ...
> > > connector = device_get_named_child_node(&client->dev, "connector");
> > > if (IS_ERR(connector))
> > > <do something>
> > >
> > > hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > > ...
> > >
> > > The difference is that instead of just converting a device node of an
> > > usb role switch to the usb role switch, it works just like
> > > usb_role_switch_get(), just taking fwnode instead of device entry as
> > > parameter.
> > >
> > > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > > way I though it needs to work for my own tests. Please find the
> > > patches attached.
> > I tested these patches, it didn't work for case as following:
> >
> > case 2:
> >
> > &mtu3 {
> > usb-role-switch;
> >
> > connector {
> > compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> > label = "micro-USB";
> > type = "micro";
> > id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> > vbus-supply = <&usb_p0_vbus>;
> > };
> > };
> >
> > so I consider this case in the function fwnode_graph_devcon_match()
> > (based on your patches)
>
> IMO that case should be handled in USB role switch API initially, not
> in the device connection API. If there is another user for it one day,
> then we can move it to device connection API, but not before that.
Ok
>
> How about this on top of the two patches I sent:
I just test it, it works well.
I'll prepare a patch and put into this series.

>
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index aab795b54c7f..36ac9d181e09 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -114,6 +114,19 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
> return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> }
>
> +static struct usb_role_switch *
> +usb_role_switch_is_parent(struct fwnode_handle *parent)
> +{
> + struct device *dev;
> +
> + if (!parent || !fwnode_property_present(parent, "usb-role-switch"))
> + return NULL;
> +
> + dev = class_find_device(role_class, NULL, parent, switch_fwnode_match);
> +
> + return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> +}
> +
> /**
> * usb_role_switch_get - Find USB role switch linked with the caller
> * @dev: The caller device
> @@ -125,6 +138,10 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> {
> struct usb_role_switch *sw;
>
> + sw = usb_role_switch_is_parent(fwnode_get_parent(dev_fwnode(dev)));
> + if (sw)
> + return sw;
Do we also need to get parent module before return?

Thanks.
> +
> sw = device_connection_find_match(dev, "usb-role-switch", NULL,
> usb_role_switch_match);
>
> @@ -146,6 +163,10 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> {
> struct usb_role_switch *sw;
>
> + sw = usb_role_switch_is_parent(fwnode_get_parent(fwnode));
> + if (sw)
> + return sw;
> +
> sw = fwnode_connection_find_match(fwnode, "usb-role-switch", NULL,
> usb_role_switch_match);
> if (!IS_ERR_OR_NULL(sw))
>
>
> thanks,
>

2019-05-27 03:10:14

by Chunfeng Yun

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Heikki & Biju,
On Fri, 2019-05-24 at 15:44 +0300, Heikki Krogerus wrote:
> On Wed, May 22, 2019 at 02:57:33PM +0000, Biju Das wrote:
> > Hi Heikki,
> >
> > Thanks for the patch
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > node
> > >
> > > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > > Hi Chunfeng Yun,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > Hi Biju,
> > > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > Thanks for the feedback.
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > > Hi Heikki,
> > > > > > > > > >
> > > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > > usb_role_switch by node
> > > > > > > > > > >
> > > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun
> > > wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki
> > > > > > > > > > > > > Krogerus
> > > > > wrote:
> > > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng
> > > > > > > > > > > > > > Yun
> > > > > > > wrote:
> > > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier
> > > > > > > > > > > > > > > to get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > > registered between two drivers and only knows
> > > > > > > > > > > > > > > the fwnode which register usb_role_switch.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > > Tested-by: Biju Das <[email protected]>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > > where he points out that you don't need to use
> > > > > > > > > > > > > device graph since the controller is the parent of
> > > > > > > > > > > > > the connector. Doesn't that mean you don't really need
> > > this API?
> > > > > > > > > > > > No, I still need it.
> > > > > > > > > > > > The change is about the way how to get fwnode; when
> > > > > > > > > > > > use device graph, get fwnode by
> > > > > > > > > > > > of_graph_get_remote_node(); but now will get fwnode by
> > > > > > > > > > > > of_get_parent();
> > > > > > > > > > >
> > > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > > something like this function is needed at all. I also
> > > > > > > > > > > have concerns regarding how you are using the function.
> > > > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > > > series...
> > > > > > > > > >
> > > > > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > > >
> > > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > > usb_role_switch_get()
> > > > > > > enough?
> > > > > > > >
> > > > > > > > Currently no issue. It will work with this api as well, since
> > > > > > > > the port node is
> > > > > > > part of controller node.
> > > > > > > > For eg:-
> > > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > > >
> > > > > > > > However if any one adds port node inside the connector node,
> > > > > > > > then this
> > > > > > > api may won't work as expected.
> > > > > > > > Currently I get below error
> > > > > > > >
> > > > > > > > [ 2.299703] OF: graph: no port node found in
> > > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > > >
> > > > > > > We need to understand why is that happening?
> > > > > > >
> > > > > >
> > > > > > Form the stack trace the parent node is
> > > > > > "parent_node=hd3ss3220@47" ,
> > > > > instead of the "connector" node.
> > > > > > That is the reason for the above error.
> > > > > >
> > > > > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > > > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > > > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > > > > >
> > > > > > The use case is
> > > > > >
> > > > > > &i2c0 {
> > > > > > hd3ss3220@47 {
> > > > > > compatible = "ti,hd3ss3220";
> > > > > >
> > > > > > usb_con: connector {
> > > > > > compatible = "usb-c-connector";
> > > > > > port {
> > > > > > hd3ss3220_ep: endpoint {
> > > > > > remote-endpoint =
> > > > > <&usb3_role_switch>;
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > >
> > > > > > &usb3_peri0 {
> > > > > > companion = <&xhci0>;
> > > > > > usb-role-switch;
> > > > > >
> > > > > > port {
> > > > > > usb3_role_switch: endpoint {
> > > > > > remote-endpoint = <&hd3ss3220_ep>;
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > >
> > > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > > Child(connector) and child's endpoint?
> > > > > How about firstly finding connector node in
> > > > > fwnode_graph_devcon_match(), then search each endpoint?
> > > >
> > > > I have done a quick prototyping with the changes you suggested and it
> > > works.
> > > >
> > > > - struct fwnode_handle *ep;
> > > > + struct fwnode_handle *ep,*child,*tmp = fwnode;
> > > >
> > > > - fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > > + child = fwnode_get_named_child_node(fwnode, "connector");
> > > > + if (child)
> > > > + tmp = child;
> > > > +
> > > > + fwnode_graph_for_each_endpoint(tmp, ep) {
> > > >
> > > > Form the stack trace the parent node is "parent_node= connector" .
> > > >
> > > > [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [ 2.456866] device_connection_find_match+0x84/0x1c0
> > > > [ 2.461888] usb_role_switch_get+0x20/0x28
> > > >
> > > > Heikki,
> > > > Are you ok with the above changes?
> > >
> > > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way I
> > > proposed, there is no problem? You just find the "connector" child node in
> > > your driver, and pass that to fwnode_usb_role_switch_get():
> >
> > Yes, That is correct.
> >
> > > struct fwnode_handle *connector;
> > > ...
> > > connector = device_get_named_child_node(&client->dev, "connector");
> > > if (IS_ERR(connector))
> > > <do something>
> > >
> > > hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > > ...
> > >
> > > The difference is that instead of just converting a device node of an usb role
> > > switch to the usb role switch, it works just like usb_role_switch_get(), just
> > > taking fwnode instead of device entry as parameter.
> > >
> > > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > > way I though it needs to work for my own tests. Please find the patches
> > > attached.
> >
> > I have tested this patches and conform it works.
> > Do you plan to post this patches to ML?
>
> Could make them part of this series?
I'll do it, thanks
>
>
> thanks,
>

2019-05-27 10:46:39

by Heikki Krogerus

Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi,

> > IMO that case should be handled in USB role switch API initially, not
> > in the device connection API. If there is another user for it one day,
> > then we can move it to device connection API, but not before that.
> Ok
> >
> > How about this on top of the two patches I sent:
> I just test it, it works well.
> I'll prepare a patch and put into this series.
>
> >
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index aab795b54c7f..36ac9d181e09 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -114,6 +114,19 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
> > return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> > }
> >
> > +static struct usb_role_switch *
> > +usb_role_switch_is_parent(struct fwnode_handle *parent)
> > +{
> > + struct device *dev;
> > +
> > + if (!parent || !fwnode_property_present(parent, "usb-role-switch"))
> > + return NULL;
> > +
> > + dev = class_find_device(role_class, NULL, parent, switch_fwnode_match);
> > +
> > + return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> > +}
> > +
> > /**
> > * usb_role_switch_get - Find USB role switch linked with the caller
> > * @dev: The caller device
> > @@ -125,6 +138,10 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> > {
> > struct usb_role_switch *sw;
> >
> > + sw = usb_role_switch_is_parent(fwnode_get_parent(dev_fwnode(dev)));
> > + if (sw)
> > + return sw;
> Do we also need to get parent module before return?

Yes.

thanks,

--
heikki

2019-05-28 06:55:38

by Biju Das

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Chunfeng Yun,

+ Chen Yu

Thanks for the feedback.

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
>
> Hi Heikki & Biju,
> On Fri, 2019-05-24 at 15:44 +0300, Heikki Krogerus wrote:
> > On Wed, May 22, 2019 at 02:57:33PM +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > Thanks for the patch
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > > > Hi Chunfeng Yun,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > Hi Biju,
> > > > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > Thanks for the feedback.
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi Heikki,
> > > > > > > > >
> > > > > > > > > Thanks for the feedback.
> > > > > > > > >
> > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > usb_role_switch by node
> > > > > > > > > >
> > > > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > > > Hi Heikki,
> > > > > > > > > > >
> > > > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to
> > > > > > > > > > > > get usb_role_switch by node
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng
> > > > > > > > > > > > Yun
> > > > wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus
> wrote:
> > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300,
> > > > > > > > > > > > > > Heikki Krogerus
> > > > > > wrote:
> > > > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800,
> > > > > > > > > > > > > > > Chunfeng Yun
> > > > > > > > wrote:
> > > > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make
> > > > > > > > > > > > > > > > easier to get usb_role_switch by fwnode which
> register it.
> > > > > > > > > > > > > > > > It's useful when there is not
> > > > > > > > > > > > > > > > device_connection registered between two
> > > > > > > > > > > > > > > > drivers and only knows the fwnode which register
> usb_role_switch.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > > > Tested-by: Biju Das
> > > > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch
> > > > > > > > > > > > > > 2/6, where he points out that you don't need
> > > > > > > > > > > > > > to use device graph since the controller is
> > > > > > > > > > > > > > the parent of the connector. Doesn't that mean
> > > > > > > > > > > > > > you don't really need
> > > > this API?
> > > > > > > > > > > > > No, I still need it.
> > > > > > > > > > > > > The change is about the way how to get fwnode;
> > > > > > > > > > > > > when use device graph, get fwnode by
> > > > > > > > > > > > > of_graph_get_remote_node(); but now will get
> > > > > > > > > > > > > fwnode by of_get_parent();
> > > > > > > > > > > >
> > > > > > > > > > > > OK, I get that, but I'm still not convinced about
> > > > > > > > > > > > if something like this function is needed at all.
> > > > > > > > > > > > I also have concerns regarding how you are using the
> function.
> > > > > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > > > > series...
> > > > > > > > > > >
> > > > > > > > > > > FYI, Currently I am also using this api in my patch series.
> > > > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > > > >
> > > > > > > > > > Yes, and I have the same question for you I jusb asked
> > > > > > > > > > in comment I added to the patch 5/6 of this series.
> > > > > > > > > > Why isn't
> > > > > > > > > > usb_role_switch_get()
> > > > > > > > enough?
> > > > > > > > >
> > > > > > > > > Currently no issue. It will work with this api as well,
> > > > > > > > > since the port node is
> > > > > > > > part of controller node.
> > > > > > > > > For eg:-
> > > > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > > > >
> > > > > > > > > However if any one adds port node inside the connector
> > > > > > > > > node, then this
> > > > > > > > api may won't work as expected.
> > > > > > > > > Currently I get below error
> > > > > > > > >
> > > > > > > > > [ 2.299703] OF: graph: no port node found in
> > > > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > > > >
> > > > > > > > We need to understand why is that happening?
> > > > > > > >
> > > > > > >
> > > > > > > Form the stack trace the parent node is
> > > > > > > "parent_node=hd3ss3220@47" ,
> > > > > > instead of the "connector" node.
> > > > > > > That is the reason for the above error.
> > > > > > >
> > > > > > > [ 2.442429] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > > > [ 2.447889] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > > > [ 2.453267] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > > > [ 2.458374] device_connection_find_match+0x74/0x1a0
> > > > > > > [ 2.463399] usb_role_switch_get+0x20/0x28
> > > > > > > [ 2.467542] hd3ss3220_probe+0xc4/0x218
> > > > > > >
> > > > > > > The use case is
> > > > > > >
> > > > > > > &i2c0 {
> > > > > > > hd3ss3220@47 {
> > > > > > > compatible = "ti,hd3ss3220";
> > > > > > >
> > > > > > > usb_con: connector {
> > > > > > > compatible = "usb-c-connector";
> > > > > > > port {
> > > > > > > hd3ss3220_ep: endpoint {
> > > > > > > remote-endpoint =
> > > > > > <&usb3_role_switch>;
> > > > > > > };
> > > > > > > };
> > > > > > > };
> > > > > > > };
> > > > > > > };
> > > > > > >
> > > > > > > &usb3_peri0 {
> > > > > > > companion = <&xhci0>;
> > > > > > > usb-role-switch;
> > > > > > >
> > > > > > > port {
> > > > > > > usb3_role_switch: endpoint {
> > > > > > > remote-endpoint = <&hd3ss3220_ep>;
> > > > > > > };
> > > > > > > };
> > > > > > > };
> > > > > > >
> > > > > > > Q1) How do we modify the usb_role_switch_get() function to
> > > > > > > search
> > > > > > > Child(connector) and child's endpoint?
> > > > > > How about firstly finding connector node in
> > > > > > fwnode_graph_devcon_match(), then search each endpoint?
> > > > >
> > > > > I have done a quick prototyping with the changes you suggested
> > > > > and it
> > > > works.
> > > > >
> > > > > - struct fwnode_handle *ep;
> > > > > + struct fwnode_handle *ep,*child,*tmp = fwnode;
> > > > >
> > > > > - fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > > > + child = fwnode_get_named_child_node(fwnode, "connector");
> > > > > + if (child)
> > > > > + tmp = child;
> > > > > +
> > > > > + fwnode_graph_for_each_endpoint(tmp, ep) {
> > > > >
> > > > > Form the stack trace the parent node is "parent_node= connector" .
> > > > >
> > > > > [ 2.440922] of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > [ 2.446381] of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > [ 2.451758] fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > [ 2.456866] device_connection_find_match+0x84/0x1c0
> > > > > [ 2.461888] usb_role_switch_get+0x20/0x28
> > > > >
> > > > > Heikki,
> > > > > Are you ok with the above changes?
> > > >
> > > > Doesn't that mean that if we made fwnode_usb_role_switch_get() the
> > > > way I proposed, there is no problem? You just find the "connector"
> > > > child node in your driver, and pass that to
> fwnode_usb_role_switch_get():
> > >
> > > Yes, That is correct.
> > >
> > > > struct fwnode_handle *connector;
> > > > ...
> > > > connector = device_get_named_child_node(&client->dev,
> "connector");
> > > > if (IS_ERR(connector))
> > > > <do something>
> > > >
> > > > hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > > > ...
> > > >
> > > > The difference is that instead of just converting a device node of
> > > > an usb role switch to the usb role switch, it works just like
> > > > usb_role_switch_get(), just taking fwnode instead of device entry as
> parameter.
> > > >
> > > > I prepared the patches implementing fwnode_usb_role_switch_get()
> > > > the way I though it needs to work for my own tests. Please find
> > > > the patches attached.
> > >
> > > I have tested this patches and conform it works.
> > > Do you plan to post this patches to ML?
> >
> > Could make them part of this series?
> I'll do it, thanks

Just a suggestion, Do you think, is it worth to add the below patch[1] also part of this series? So that we have all common patches in this series.

"usb: roles: Introduce stubs for the exiting functions in role.h."
[1] https://patchwork.kernel.org/patch/10909971/

Regards,
Biju

2019-05-28 09:25:17

by Chunfeng Yun

Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

Hi Biju & Yu,

On Tue, 2019-05-28 at 06:52 +0000, Biju Das wrote:
> Hi Chunfeng Yun,
>
> + Chen Yu
>
> Thanks for the feedback.
[...]
>
> Just a suggestion, Do you think, is it worth to add the below patch[1] also part of this series? So that we have all common patches in this series.
>
Or resend it as a single patch?

> "usb: roles: Introduce stubs for the exiting functions in role.h."
> [1] https://patchwork.kernel.org/patch/10909971/
>

> Regards,
> Biju