Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754376AbcDTGvh (ORCPT ); Wed, 20 Apr 2016 02:51:37 -0400 Received: from devils.ext.ti.com ([198.47.26.153]:37987 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752748AbcDTGvf (ORCPT ); Wed, 20 Apr 2016 02:51:35 -0400 Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core To: Peter Chen References: <1459865117-7032-1-git-send-email-rogerq@ti.com> <1459865117-7032-10-git-send-email-rogerq@ti.com> <20160418065954.GB4477@shlinux2.ap.freescale.net> CC: , , , , , , , , , , , , , From: Roger Quadros Message-ID: <571726DA.6090402@ti.com> Date: Wed, 20 Apr 2016 09:51:06 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160418065954.GB4477@shlinux2.ap.freescale.net> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6056 Lines: 208 On 18/04/16 09:59, Peter Chen wrote: > On Tue, Apr 05, 2016 at 05:05:14PM +0300, Roger Quadros wrote: >> The OTG state machine needs a mechanism to start and >> stop the gadget controller. Add usb_gadget_start() >> and usb_gadget_stop(). >> >> Introduce usb_otg_add_gadget_udc() to allow controller drivers >> to register a gadget controller that is part of an OTG instance. >> >> Register with OTG core when gadget function driver >> is available and unregister when function driver is unbound. >> >> We need to unlock the usb_lock mutexbefore calling > > ...mutex before... OK. > >> usb_otg_register_gadget() in udc_bind_to_driver() and >> usb_gadget_remove_driver() else it will cause a circular >> locking dependency. >> >> Ignore softconnect sysfs control when we're in OTG >> mode as OTG FSM takes care of gadget softconnect using >> the b_bus_req mechanism. >> >> Signed-off-by: Roger Quadros >> --- >> drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++--- >> include/linux/usb/gadget.h | 4 + >> 2 files changed, 161 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c >> index 4151597..9b9702f 100644 >> --- a/drivers/usb/gadget/udc/udc-core.c >> +++ b/drivers/usb/gadget/udc/udc-core.c >> @@ -28,6 +28,10 @@ >> #include >> #include >> #include >> +#include >> + >> +#include >> +#include >> >> /** >> * struct usb_udc - describes one usb device controller >> @@ -304,6 +308,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); >> */ >> static inline int usb_gadget_udc_start(struct usb_udc *udc) >> { >> + dev_dbg(&udc->dev, "%s\n", __func__); >> return udc->gadget->ops->udc_start(udc->gadget, udc->driver); >> } > > You may delete the debug message next time. OK for this and all the next comments to remove debug messages. > >> >> @@ -321,10 +326,81 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) >> */ >> static inline void usb_gadget_udc_stop(struct usb_udc *udc) >> { >> + dev_dbg(&udc->dev, "%s\n", __func__); >> udc->gadget->ops->udc_stop(udc->gadget); >> } > > The same. > >> >> /** >> + * usb_gadget_start - start the usb gadget controller and connect to bus >> + * @gadget: the gadget device to start >> + * >> + * This is external API for use by OTG core. >> + * >> + * Start the usb device controller and connect to bus (enable pull). >> + */ >> +static int usb_gadget_start(struct usb_gadget *gadget) >> +{ >> + int ret; >> + struct usb_udc *udc = NULL; >> + >> + dev_dbg(&gadget->dev, "%s\n", __func__); > > The same > >> + mutex_lock(&udc_lock); >> + list_for_each_entry(udc, &udc_list, list) >> + if (udc->gadget == gadget) >> + goto found; >> + >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >> + __func__); >> + mutex_unlock(&udc_lock); >> + return -EINVAL; >> + >> +found: >> + ret = usb_gadget_udc_start(udc); >> + if (ret) >> + dev_err(&udc->dev, "USB Device Controller didn't start: %d\n", >> + ret); >> + else >> + usb_udc_connect_control(udc); >> + >> + mutex_unlock(&udc_lock); >> + >> + return ret; >> +} >> + >> +/** >> + * usb_gadget_stop - disconnect from bus and stop the usb gadget >> + * @gadget: The gadget device we want to stop >> + * >> + * This is external API for use by OTG core. >> + * >> + * Disconnect from the bus (disable pull) and stop the >> + * gadget controller. >> + */ >> +static int usb_gadget_stop(struct usb_gadget *gadget) >> +{ >> + struct usb_udc *udc = NULL; >> + >> + dev_dbg(&gadget->dev, "%s\n", __func__); > > The same > >> + mutex_lock(&udc_lock); >> + list_for_each_entry(udc, &udc_list, list) >> + if (udc->gadget == gadget) >> + goto found; >> + >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >> + __func__); >> + mutex_unlock(&udc_lock); >> + return -EINVAL; > > The above finding gadget code is the same with usb_gadget_start, can we > use one common API to instead of it? Sure. > >> + >> +found: >> + usb_gadget_disconnect(udc->gadget); >> + udc->driver->disconnect(udc->gadget); >> + usb_gadget_udc_stop(udc); >> + mutex_unlock(&udc_lock); >> + >> + return 0; >> +} >> + >> +/** >> * usb_udc_release - release the usb_udc struct >> * @dev: the dev member within usb_udc >> * >> @@ -486,6 +562,48 @@ int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget) >> } >> EXPORT_SYMBOL_GPL(usb_add_gadget_udc); >> >> +/** >> + * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list >> + * @parent: the parent device to this udc. Usually the controller >> + * driver's device. >> + * @gadget: the gadget to be added to the list >> + * @otg_dev: the OTG controller device >> + * >> + * If otg_dev is NULL then device tree node is checked >> + * for OTG controller via the otg-controller property. >> + * Returns zero on success, negative errno otherwise. >> + */ >> +int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget, >> + struct device *otg_dev) >> +{ >> + if (!otg_dev) { >> + struct device_node *np; >> + struct platform_device *pdev; >> + >> + np = of_parse_phandle(parent->of_node, "otg-controller", 0); >> + if (!np) { >> + dev_err(parent, >> + "otg_dev is NULL or no otg-controller property in DT\n"); >> + return -EINVAL; >> + } >> + >> + pdev = of_find_device_by_node(np); >> + of_node_put(np); >> + if (!pdev) { >> + dev_err(parent, "couldn't get otg-controller device\n"); >> + return -ENODEV; >> + } >> + >> + gadget->otg_dev = &pdev->dev; >> + } else { >> + gadget->otg_dev = otg_dev; >> + } > > The above the code is duplicated with hcd's. Can we do something common > at usb-otg.c, eg define an API get_usb_otg_dev(struct usb_gadget *gadget, struct > usb_hcd *hcd), in this API we can try to get otg_dev for both gadget and > hcd, and we can call it at controller driver during register otg. > Good point. I'll address it in v7. cheers, -roger