[v2,4/6] usb: roles: add API to get usb_role_switch by node

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

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

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/roles/class.c | 30 ++++++++++++++++++++++++++++++
 include/linux/usb/role.h  |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 99116af07f1d..284b19856dc4 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -11,6 +11,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 
 static struct class *role_class;
@@ -121,6 +122,35 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get);
 
+static int __switch_match_node(struct device *dev, const void *node)
+{
+	return dev->parent->of_node == (const struct device_node *)node;
+}
+
+/**
+ * usb_role_switch_get_by_node - Find USB role switch by it's parent node
+ * @node: The node that register USB role switch
+ *
+ * Finds and returns role switch registered by @node. The reference count
+ * for the found switch is incremented.
+ */
+struct usb_role_switch *usb_role_switch_get_by_node(struct device_node *node)
+{
+	struct usb_role_switch *sw;
+	struct device *dev;
+
+	dev = class_find_device(role_class, NULL, node,
+				__switch_match_node);
+	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(usb_role_switch_get_by_node);
+
 /**
  * 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 edc51be4a77c..056498b83dee 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -42,6 +42,7 @@ struct usb_role_switch_desc {
 
 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_by_node(struct device_node *node);
 struct usb_role_switch *usb_role_switch_get(struct device *dev);
 void usb_role_switch_put(struct usb_role_switch *sw);
 

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Fri, Mar 15, 2019 at 03:38:31PM +0800, Chunfeng Yun wrote:
> Add usb_role_switch_get_by_node() to make easier to get
> usb_role_switch by node which register it.
> It's useful when there is not device_connection registered
> between two drivers and only knows the node which register
> usb_role_switch.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  drivers/usb/roles/class.c | 30 ++++++++++++++++++++++++++++++
>  include/linux/usb/role.h  |  1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index 99116af07f1d..284b19856dc4 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -11,6 +11,7 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  #include <linux/slab.h>
>  
>  static struct class *role_class;
> @@ -121,6 +122,35 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_get);
>  
> +static int __switch_match_node(struct device *dev, const void *node)
> +{
> +	return dev->parent->of_node == (const struct device_node *)node;
> +}

Andy already pointed out that you need to use fwnodes.

Rule of thumb: You always use fwnodes. Only if there is something that
can't be done with fwnodes you use DT or ACPI nodes directly.

In this case there is absolutely nothing that would prevent you from
using fwnodes.


thanks,

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Fri, Mar 15, 2019 at 10:18:35AM +0200, Heikki Krogerus wrote:
> Andy already pointed out that you need to use fwnodes.
> 
> Rule of thumb: You always use fwnodes. Only if there is something that
> can't be done with fwnodes you use DT or ACPI nodes directly.
> 
> In this case there is absolutely nothing that would prevent you from
> using fwnodes.

...

> +/**
> + * usb_role_switch_get_by_node - Find USB role switch by it's parent node
> + * @node: The node that register USB role switch
> + *
> + * Finds and returns role switch registered by @node. The reference count
> + * for the found switch is incremented.
> + */
> +struct usb_role_switch *usb_role_switch_get_by_node(struct device_node *node)

This is my proposal for function prototype:

struct usb_role_switch *
fwnode_usb_role_switch_get(struct fwnode_handle *fwnode);


thanks,

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi,
On Fri, 2019-03-15 at 10:18 +0200, Heikki Krogerus wrote:
> On Fri, Mar 15, 2019 at 03:38:31PM +0800, Chunfeng Yun wrote:
> > Add usb_role_switch_get_by_node() to make easier to get
> > usb_role_switch by node which register it.
> > It's useful when there is not device_connection registered
> > between two drivers and only knows the node which register
> > usb_role_switch.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  drivers/usb/roles/class.c | 30 ++++++++++++++++++++++++++++++
> >  include/linux/usb/role.h  |  1 +
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index 99116af07f1d..284b19856dc4 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/of.h>
> >  #include <linux/slab.h>
> >  
> >  static struct class *role_class;
> > @@ -121,6 +122,35 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_role_switch_get);
> >  
> > +static int __switch_match_node(struct device *dev, const void *node)
> > +{
> > +	return dev->parent->of_node == (const struct device_node *)node;
> > +}
> 
> Andy already pointed out that you need to use fwnodes.
> 
> Rule of thumb: You always use fwnodes. Only if there is something that
> can't be done with fwnodes you use DT or ACPI nodes directly.
> 
> In this case there is absolutely nothing that would prevent you from
> using fwnodes.
> 
Got it, will fix it in next version.

BTW:

I encounter a build error when CONFIG_USB_ROLE_SWITCH is not enabled,

drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_role_sw_register':
./drivers/usb/mtu3/mtu3_dr.c:460: undefined reference to
`usb_role_switch_register'
drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_otg_switch_exit':
./drivers/usb/mtu3/mtu3_dr.c:491: undefined reference to
`usb_role_switch_unregister'

the following patch has fixed the issue, but seems not get into kernel,
[v3,08/12] usb: roles: Add usb role switch notifier.
https://patchwork.kernel.org/patch/10836525/

What should I do if I add a new API? Thanks

> 
> thanks,
>

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi,
On Fri, 2019-03-15 at 11:11 +0200, Heikki Krogerus wrote:
> On Fri, Mar 15, 2019 at 10:18:35AM +0200, Heikki Krogerus wrote:
> > Andy already pointed out that you need to use fwnodes.
> > 
> > Rule of thumb: You always use fwnodes. Only if there is something that
> > can't be done with fwnodes you use DT or ACPI nodes directly.
> > 
> > In this case there is absolutely nothing that would prevent you from
> > using fwnodes.
> 
> ...
> 
> > +/**
> > + * usb_role_switch_get_by_node - Find USB role switch by it's parent node
> > + * @node: The node that register USB role switch
> > + *
> > + * Finds and returns role switch registered by @node. The reference count
> > + * for the found switch is incremented.
> > + */
> > +struct usb_role_switch *usb_role_switch_get_by_node(struct device_node *node)
> 
> This is my proposal for function prototype:
> 
> struct usb_role_switch *
> fwnode_usb_role_switch_get(struct fwnode_handle *fwnode);
Ok, thanks again
> 
> 
> thanks,
>

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

Hi Chunfeng,

On Fri, Mar 15, 2019 at 05:13:24PM +0800, Chunfeng Yun wrote:
> I encounter a build error when CONFIG_USB_ROLE_SWITCH is not enabled,
> 
> drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_role_sw_register':
> ./drivers/usb/mtu3/mtu3_dr.c:460: undefined reference to
> `usb_role_switch_register'
> drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_otg_switch_exit':
> ./drivers/usb/mtu3/mtu3_dr.c:491: undefined reference to
> `usb_role_switch_unregister'

So you need to add dependency on USB_ROLE_SWITCH, right?


> the following patch has fixed the issue, but seems not get into kernel,
> [v3,08/12] usb: roles: Add usb role switch notifier.
> https://patchwork.kernel.org/patch/10836525/

I don't understand how that fixes the problem? That patch will in any
case be targeting v5.2. We are in the middle of merge window, so
nothing is happening until v5.1-rc1 is tagged.


thanks,

--- a/drivers/usb/mtu3/Kconfig
+++ b/drivers/usb/mtu3/Kconfig
@@ -43,6 +43,7 @@ config USB_MTU3_DUAL_ROLE
        bool "Dual Role mode"
        depends on ((USB=y || USB=USB_MTU3) && (USB_GADGET=y || USB_GADGET=USB_MTU3))
        depends on (EXTCON=y || EXTCON=USB_MTU3)
+       depends on USB_ROLE_SWITCH
        help
          This is the default mode of working of MTU3 controller where
          both host and gadget features are enabled.

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi,
On Fri, 2019-03-15 at 11:26 +0200, Heikki Krogerus wrote:
> Hi Chunfeng,
> 
> On Fri, Mar 15, 2019 at 05:13:24PM +0800, Chunfeng Yun wrote:
> > I encounter a build error when CONFIG_USB_ROLE_SWITCH is not enabled,
> > 
> > drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_role_sw_register':
> > ./drivers/usb/mtu3/mtu3_dr.c:460: undefined reference to
> > `usb_role_switch_register'
> > drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_otg_switch_exit':
> > ./drivers/usb/mtu3/mtu3_dr.c:491: undefined reference to
> > `usb_role_switch_unregister'
> 
> So you need to add dependency on USB_ROLE_SWITCH, right?
Yes

> 
> --- a/drivers/usb/mtu3/Kconfig
> +++ b/drivers/usb/mtu3/Kconfig
> @@ -43,6 +43,7 @@ config USB_MTU3_DUAL_ROLE
>         bool "Dual Role mode"
>         depends on ((USB=y || USB=USB_MTU3) && (USB_GADGET=y || USB_GADGET=USB_MTU3))
>         depends on (EXTCON=y || EXTCON=USB_MTU3)
> +       depends on USB_ROLE_SWITCH
>         help
>           This is the default mode of working of MTU3 controller where
>           both host and gadget features are enabled.
> 
> > the following patch has fixed the issue, but seems not get into kernel,
> > [v3,08/12] usb: roles: Add usb role switch notifier.
> > https://patchwork.kernel.org/patch/10836525/
> 
> I don't understand how that fixes the problem? That patch will in any
> case be targeting v5.2. We are in the middle of merge window, so
> nothing is happening until v5.1-rc1 is tagged.
It provides some dummy inline functions when USB_ROLE_SWITCH is not
enabled, this will avoid build error

> 
> 
> thanks,
>

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Fri, Mar 15, 2019 at 05:32:59PM +0800, Chunfeng Yun wrote:
> Hi,
> On Fri, 2019-03-15 at 11:26 +0200, Heikki Krogerus wrote:
> > Hi Chunfeng,
> > 
> > On Fri, Mar 15, 2019 at 05:13:24PM +0800, Chunfeng Yun wrote:
> > > I encounter a build error when CONFIG_USB_ROLE_SWITCH is not enabled,
> > > 
> > > drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_role_sw_register':
> > > ./drivers/usb/mtu3/mtu3_dr.c:460: undefined reference to
> > > `usb_role_switch_register'
> > > drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_otg_switch_exit':
> > > ./drivers/usb/mtu3/mtu3_dr.c:491: undefined reference to
> > > `usb_role_switch_unregister'
> > 
> > So you need to add dependency on USB_ROLE_SWITCH, right?
> Yes
> 
> > 
> > --- a/drivers/usb/mtu3/Kconfig
> > +++ b/drivers/usb/mtu3/Kconfig
> > @@ -43,6 +43,7 @@ config USB_MTU3_DUAL_ROLE
> >         bool "Dual Role mode"
> >         depends on ((USB=y || USB=USB_MTU3) && (USB_GADGET=y || USB_GADGET=USB_MTU3))
> >         depends on (EXTCON=y || EXTCON=USB_MTU3)
> > +       depends on USB_ROLE_SWITCH
> >         help
> >           This is the default mode of working of MTU3 controller where
> >           both host and gadget features are enabled.
> > 
> > > the following patch has fixed the issue, but seems not get into kernel,
> > > [v3,08/12] usb: roles: Add usb role switch notifier.
> > > https://patchwork.kernel.org/patch/10836525/
> > 
> > I don't understand how that fixes the problem? That patch will in any
> > case be targeting v5.2. We are in the middle of merge window, so
> > nothing is happening until v5.1-rc1 is tagged.
> It provides some dummy inline functions when USB_ROLE_SWITCH is not
> enabled, this will avoid build error

Ah, true. Those should brobable be introduced in their own patch.


thanks,

[v2,4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Fri, Mar 15, 2019 at 05:13:24PM +0800, Chunfeng Yun wrote:
> I encounter a build error when CONFIG_USB_ROLE_SWITCH is not enabled,
> 
> drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_role_sw_register':
> ./drivers/usb/mtu3/mtu3_dr.c:460: undefined reference to
> `usb_role_switch_register'
> drivers/usb/mtu3/mtu3_dr.o: In function `ssusb_otg_switch_exit':
> ./drivers/usb/mtu3/mtu3_dr.c:491: undefined reference to
> `usb_role_switch_unregister'
> 
> the following patch has fixed the issue, but seems not get into kernel,
> [v3,08/12] usb: roles: Add usb role switch notifier.
> https://patchwork.kernel.org/patch/10836525/
> 
> What should I do if I add a new API? Thanks

So if you are asking should you supply dummy functions for the new API,
then I would just say that if you do so, you need to prepare these
patches on top of that series from Yu Chen. In general I'm not sure we
need dummy functions with this API.

Hans, comments?


thanks,