| Message ID | 1552635513-2378-5-git-send-email-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add USB Type-B GPIO based role switch driver | expand |
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,
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,
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, >
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, >
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? --- 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. thanks,
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, >
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,
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,
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);
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(+)