From patchwork Fri Jan 25 12:12:20 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Shishkin X-Patchwork-Id: 2044661 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork1.kernel.org (Postfix) with ESMTP id E3F3A3FDD1 for ; Fri, 25 Jan 2013 12:13:58 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1Tyi7a-0006zM-S5; Fri, 25 Jan 2013 12:11:02 +0000 Received: from mga09.intel.com ([134.134.136.24]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1Tyi7X-0006yI-22 for linux-arm-kernel@lists.infradead.org; Fri, 25 Jan 2013 12:11:00 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 25 Jan 2013 04:09:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.84,538,1355126400"; d="scan'208";a="252340523" Received: from um.fi.intel.com (HELO localhost) ([10.237.72.164]) by orsmga001.jf.intel.com with ESMTP; 25 Jan 2013 04:10:52 -0800 From: Alexander Shishkin To: Peter Chen Subject: Re: [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles In-Reply-To: <20130125033013.GB29795@nchen-desktop> References: <1358733418-17969-1-git-send-email-peter.chen@freescale.com> <1358733418-17969-5-git-send-email-peter.chen@freescale.com> <8738xqodhp.fsf@ashishki-desk.ger.corp.intel.com> <20130125033013.GB29795@nchen-desktop> User-Agent: Notmuch/0.12+187~ga2502b0 (http://notmuchmail.org) Emacs/23.4.1 (x86_64-pc-linux-gnu) Date: Fri, 25 Jan 2013 14:12:20 +0200 Message-ID: <87sj5pbgwr.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130125_071059_370836_A7D37725 X-CRM114-Status: GOOD ( 31.46 ) X-Spam-Score: -4.6 (----) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-4.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- 3.0 KHOP_BIG_TO_CC Sent to 10+ recipients instaed of Bcc or a list -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [134.134.136.24 listed in list.dnswl.org] -0.7 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: marex@denx.de, m.grzeschik@pengutronix.de, gregkh@linuxfoundation.org, pkondeti@codeaurora.org, linux-usb@vger.kernel.org, balbi@ti.com, mkl@pengutronix.de, kernel@pengutronix.de, matt@genesi-usa.com, maxime.ripard@free-electrons.com, shawn.guo@linaro.org, festevam@gmail.com, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org Peter Chen writes: > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote: >> Peter Chen writes: >> >> > - Create init/destroy API for probe and remove >> > - start/stop API are only used otg id switch process >> > - Create the gadget at ci_hdrc_probe if the gadget is supported >> > at that port, the main purpose for this is to avoid gadget module >> > load fail at init.rc >> >> I don't think it's necessary to mention android-specific init scripts >> here in our patches. > > Ok, will just mention init script. >> >> > >> > +static inline void ci_role_destroy(struct ci13xxx *ci) >> > +{ >> > + enum ci_role role = ci->role; >> > + >> > + if (role == CI_ROLE_END) >> > + return; >> > + >> > + ci->role = CI_ROLE_END; >> > + >> > + ci->roles[role]->destroy(ci); >> > +} >> >> What does this do? It should take role an a parameter, at least. Or it >> can be called ci_roles_destroy() and, well, destroy all the roles. Now >> we're in a situation where we destroy one. > Yes, this function has some problems, I suggest just call two lines at > ci_role_destory, do you think so? > > ci->roles[role]->destroy(ci); > ci->role = CI_ROLE_END; I just don't see why it's needed at all. See below. >> >> The idea is that, with this api, we can (and should) have both roles >> allocated all the time, but only one role start()'ed. >> >> What we can do here is move the udc's .init() code to >> ci_hdrc_gadget_init() and add a complementary ci_hdrc_gadget_destroy(), >> which we call in ci_hdrc_remove() and probe's error path. And we can >> probably do something similar for the host, or just leave it as it is >> now. Seems simpler to me. > Yes, current code has bug that it should call ci_role_destroy at probe's > error path. > For your comments, it still needs to add destory function for udc which will > be called at core.c. I still prefer current way due to below reasons: > > - It can keep ci_hdrc_gadget_init() clean > - .init and .remove are different logical function with .start and .stop. > The .init and .remove are used for create and destroy, .start and .stop > are used at id role switch. True, and we can keep it that way: (again, untested) ---cut--- ---cut--- Which is shorter (32 insertions(+), 53 deletions(-)) and makes the main probe easier on the eyes. What do you think? Regards, --- Alex diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h index 342b430..36f495a 100644 --- a/drivers/usb/chipidea/ci.h +++ b/drivers/usb/chipidea/ci.h @@ -67,18 +67,14 @@ enum ci_role { /** * struct ci_role_driver - host/gadget role driver - * init: init this role (used at module probe) * start: start this role (used at id switch) * stop: stop this role (used at id switch) - * destroy: destroy this role (used at module remove) * irq: irq handler for this role * name: role name string (host/gadget) */ struct ci_role_driver { - int (*init)(struct ci13xxx *); int (*start)(struct ci13xxx *); void (*stop)(struct ci13xxx *); - void (*destroy)(struct ci13xxx *); irqreturn_t (*irq)(struct ci13xxx *); const char *name; }; @@ -212,17 +208,6 @@ static inline void ci_role_stop(struct ci13xxx *ci) ci->roles[role]->stop(ci); } -static inline void ci_role_destroy(struct ci13xxx *ci) -{ - enum ci_role role = ci->role; - - if (role == CI_ROLE_END) - return; - - ci->role = CI_ROLE_END; - - ci->roles[role]->destroy(ci); -} /****************************************************************************** * REGISTERS *****************************************************************************/ diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index a61e759..83f35fa 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, if (ret) return ret; + /* + * there won't be an interrupt in case of manual switching, + * so we need to check vbus session manually + */ + ci_handle_vbus_change(ci); + return count; } @@ -592,30 +598,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) otgsc = hw_read(ci, OP_OTGSC, ~0); - /* - * If the gadget is supported, call its init unconditionally, - * We need to support load gadget module at init.rc. - */ - if (ci->roles[CI_ROLE_GADGET]) { - ret = ci->roles[CI_ROLE_GADGET]->init(ci); - if (ret) { - dev_err(dev, "can't init %s role, ret=%d\n", - ci_role(ci)->name, ret); - ret = -ENODEV; - goto rm_wq; - } - } - - if (ci->role == CI_ROLE_HOST) { - ret = ci->roles[CI_ROLE_HOST]->init(ci); - if (ret) { - dev_err(dev, "can't init %s role, ret=%d\n", - ci_role(ci)->name, ret); - ret = -ENODEV; - goto rm_wq; - } - } - platform_set_drvdata(pdev, ci); ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name, ci); @@ -626,6 +608,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) if (ret) goto rm_attr; + ci_role_start(ci, ci->role); + /* Handle the situation that usb device at the MicroB to A cable */ if (ci->is_otg && !(otgsc & OTGSC_ID)) queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500)); @@ -651,7 +635,8 @@ static int ci_hdrc_remove(struct platform_device *pdev) destroy_workqueue(ci->wq); device_remove_file(ci->dev, &dev_attr_role); free_irq(ci->irq, ci); - ci_role_destroy(ci); + ci_hdrc_gadget_destroy(ci); + ci_hdrc_host_destroy(ci); return 0; } diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 7f4538c..ead3158 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -95,10 +95,8 @@ int ci_hdrc_host_init(struct ci13xxx *ci) if (!rdrv) return -ENOMEM; - rdrv->init = host_start; rdrv->start = host_start; rdrv->stop = host_stop; - rdrv->destroy = host_stop; rdrv->irq = host_irq; rdrv->name = "host"; ci->roles[CI_ROLE_HOST] = rdrv; @@ -107,3 +105,9 @@ int ci_hdrc_host_init(struct ci13xxx *ci) return 0; } + +void ci_hdrc_host_destroy(struct ci13xxx *ci) +{ + if (ci->role == CI_ROLE_HOST) + host_stop(ci); +} diff --git a/drivers/usb/chipidea/host.h b/drivers/usb/chipidea/host.h index 761fb1f..2e529be 100644 --- a/drivers/usb/chipidea/host.h +++ b/drivers/usb/chipidea/host.h @@ -4,6 +4,7 @@ #ifdef CONFIG_USB_CHIPIDEA_HOST int ci_hdrc_host_init(struct ci13xxx *ci); +void ci_hdrc_host_destroy(struct ci13xxx *ci); #else @@ -12,6 +13,10 @@ static inline int ci_hdrc_host_init(struct ci13xxx *ci) return -ENXIO; } +static void ci_hdrc_host_destroy(struct ci13xxx *ci) +{ +} + #endif #endif /* __DRIVERS_USB_CHIPIDEA_HOST_H */ diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 38446de..38b06ac 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -1750,10 +1750,10 @@ static int udc_start(struct ci13xxx *ci) * - Enable vbus detect * - If it has already connected to host, notify udc */ - if (ci->role == CI_ROLE_GADGET) { + //if (ci->role == CI_ROLE_GADGET) { ci_enable_otg_interrupt(ci, OTGSC_BSVIE); ci_handle_vbus_change(ci); - } + //} return retval; @@ -1798,11 +1798,10 @@ static void udc_id_switch_for_host(struct ci13xxx *ci) } /** - * udc_remove: parent remove must call this to remove UDC - * - * No interrupts active, the IRQ has been released + * ci_hdrc_gadget_init - initialize device related bits + * ci: the controller */ -static void udc_stop(struct ci13xxx *ci) +void ci_hdrc_gadget_destroy(struct ci13xxx *ci) { if (ci == NULL) return; @@ -1821,8 +1820,6 @@ static void udc_stop(struct ci13xxx *ci) } dbg_remove_files(&ci->gadget.dev); device_unregister(&ci->gadget.dev); - /* my kobject is dynamic, I swear! */ - memset(&ci->gadget, 0, sizeof(ci->gadget)); } /** @@ -1842,13 +1839,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci) if (!rdrv) return -ENOMEM; - rdrv->init = udc_start; rdrv->start = udc_id_switch_for_device; rdrv->stop = udc_id_switch_for_host; - rdrv->destroy = udc_stop; rdrv->irq = udc_irq; rdrv->name = "gadget"; ci->roles[CI_ROLE_GADGET] = rdrv; - return 0; + return udc_start(ci); } diff --git a/drivers/usb/chipidea/udc.h b/drivers/usb/chipidea/udc.h index 4ff2384d..12fd7cf 100644 --- a/drivers/usb/chipidea/udc.h +++ b/drivers/usb/chipidea/udc.h @@ -80,6 +80,7 @@ struct ci13xxx_req { #ifdef CONFIG_USB_CHIPIDEA_UDC int ci_hdrc_gadget_init(struct ci13xxx *ci); +void ci_hdrc_gadget_destroy(struct ci13xxx *ci); #else @@ -88,6 +89,10 @@ static inline int ci_hdrc_gadget_init(struct ci13xxx *ci) return -ENXIO; } +static void ci_hdrc_gadget_destroy(struct ci13xxx *ci) +{ +} + #endif #endif /* __DRIVERS_USB_CHIPIDEA_UDC_H */