From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753327AbbIIHcf (ORCPT ); Wed, 9 Sep 2015 03:32:35 -0400 Received: from mail-bl2on0143.outbound.protection.outlook.com ([65.55.169.143]:9827 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751342AbbIIHc0 (ORCPT ); Wed, 9 Sep 2015 03:32:26 -0400 Authentication-Results: spf=fail (sender IP is 192.88.158.2) smtp.mailfrom=freescale.com; vger.kernel.org; dkim=none (message not signed) header.d=none;vger.kernel.org; dmarc=none action=none header.from=freescale.com; Date: Wed, 9 Sep 2015 14:20:07 +0800 From: Li Jun To: Roger Quadros CC: , , , , , , , , , , , , Subject: Re: [PATCH v4 07/13] usb: otg: add OTG core Message-ID: <20150909062006.GB812@shlinux2> References: <1440422484-4737-1-git-send-email-rogerq@ti.com> <1440422484-4737-8-git-send-email-rogerq@ti.com> <20150907074002.GA30578@shlinux2> <55ED6C9F.2000502@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <55ED6C9F.2000502@ti.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11FD002;1:70Gb2Geec9NfhUVrv8r8JgLtP9YoiV9o1EuM00g/jjJ73cr0VR7thzWFtXp8WSmZNoqenMb+Osup+BsFpLb+U7dyi+pe8F855YLIro16wv1VWkNtrkTrwtVTv9OSy0P84U+SOQoYsJ02nnLZ7fnaSguMAOfNVoHI/b9fNSE/CfEXdSIPNd3SL3C7LtjkF0lohPVCbpF4Sq8raT82PxsUqb7q8ggn8l7UaLai57pEo4VNQDptl50rrGu346918TxLwBJo2NVNGnXc05nLXqWLzngw3hp3EIVU+O4799ai/ZDDGr8sHu5Kj2qCaeTK+cc8YsEfxMMkCQf+8Nyslb6HWCYQQ0oMzcnHlMh6iBRbhyADT7DixI7LrNDQYO4pwPe8euI3ng5Tmp6Ygif253ATgeHZFVLoekMyFtSj2sCwDcU= X-Forefront-Antispam-Report: CIP:192.88.158.2;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(2980300002)(1109001)(1110001)(339900001)(24454002)(479174004)(189002)(57704003)(199003)(46102003)(5001860100001)(110136002)(50986999)(87936001)(5001960100002)(62966003)(85426001)(189998001)(76176999)(5001830100001)(54356999)(33656002)(68736005)(93886004)(97756001)(77096005)(106466001)(77156002)(23726002)(105606002)(92566002)(2950100001)(46406003)(19580395003)(5007970100001)(104016003)(64706001)(19580405001)(4001540100001)(11100500001)(6806004)(33716001)(50466002)(47776003)(4001350100001)(97736004)(81156007)(83506001)(69596002);DIR:OUT;SFP:1102;SCL:1;SRVR:CY1PR0301MB0857;H:az84smr01.freescale.net;FPR:;SPF:Fail;PTR:InfoDomainNonexistent;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;CY1PR0301MB0857;2:LF2iarD1oM6i9+3oggud/CpZt8m9bnbh6pI37zKoDE/bw121IKszuBAJelfgng8QD1pV5mwQi/sY90mA894FeTBX25Qy5AYVzD0fqcWxnc7lTpUWCG1Kad5f2YUVpNHt7lzlsF5XTcuHpxlp/h3hA1YveGH1Jq0Xtxnhf3EvMnc=;3:kuR4MyFYGXAGrG41KyTUfQAwHmcKUc7qph3ov6KEXf8sl4tB1L46CVXgWNNDIoN94MFWdY8KVTGqFbxEpt1JxayWNRh4kn87mvLZyAUkKzYSBMGNG/hkUgj1rsWuFwExJodi7/SqxG/wAwapQrNmB4OkYc+b4mIksRJzlkOQ0rFE8OaDKr0Tf023RF7XspN3kUO1D5GEi/kg0S/ewvPdcxF4oPj8hbGJsQqcQrX6N+k=;25:TpeXY8W1KGUnwL2lOfApMMXnKNUrkD43+DqoFuRGYOCO2Bl2+dkD4uC+OZc1QwmyPBjppUq1XLK3wIPiSImGMhf/pSvJyMpLok10n4eisvc8eqRLwQENF4CfJyXZxTumiy3P4AmPz0i/LSiU4atlCji+Z3g3cjvCmz0Fi705vEasFD1vBDBiXpQ6gE3infJW9l9nAjf8icmx5Jtpv4/RbSfc0RE9R8Eml2WA8/MIgWIWfW1mpv1Jfgj6eHn0AEGNwM2ZFwfEjhjmU1O28PW9Mg== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CY1PR0301MB0857; X-Microsoft-Exchange-Diagnostics: 1;CY1PR0301MB0857;20:kK09nVN998RHUQRO68dToES+qSf6g5f2Ja4UMFSwWWeGqzJRDTUbW9f/3a16xs+zUO9WX+Wp+NsnnjsfDyMXb/Qz4UVFJKfddMOCPwREgV/m5mxiyUC6meBXOna1Q69P7BZpFM0ZdFGRdCsevr+eQsZHK4cgfm6JL7FOBdN6SLd3ZJpFXhEECmGU6HSkx+teaj+Cx9ma/JjtoCvfsQTyAenxnwRD424WywuJGeToH4cx03SLuAwZ8MeAZMxEN8ls2UdcKZATzzAySCX4YGkXUwXI1vGkodmyiw427AYHu8P71HJBidmsvXy6n0CDmmynqXJoCp9gSNdp5qR0yNormYeDckIpebuPVcITXi19DtU=;4:kc0zTKF6XU0gYpQ9Q6I0WEOcNP8EOR3Y8F0yGSQYxVOIM3qK+D2wEn5d35DC1QF6Pr73RrJRMJrMMHq3CsyXzSFyz2NTXivR0OIKhw72MwH8Nk9iGv9hX5I9goekbq5+EoXbHqaVLHsGZgP+QW3aL0N8kezV4eTf+7niTjWt+zlXzcfxyQRuC23eW6VriVEgxpLfruDQyYA4Npi7GGreo/BAxXbQddkruogkSOqmQIUiNbGl1JLWn9vpzN7v1fZ4ELaxsgunktzGkRH1r38gCqSmYop+oP9CO5hkcfqADSHWzliN/k8g3qOaEfLWgV7r X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5005006)(8121501046)(3002001);SRVR:CY1PR0301MB0857;BCL:0;PCL:0;RULEID:;SRVR:CY1PR0301MB0857; X-Forefront-PRVS: 0694C54398 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;CY1PR0301MB0857;23:dCPkGdbAXQuPTvFtQnonaeissxadk17aqjiU5q4?= =?us-ascii?Q?9nCe9JRxjCS/ol1/oAe7no1uvUHdSYJjzKaWeHna+gD7I0TuhxQ1B1QQQ8RN?= =?us-ascii?Q?bHcfyMNsIMuGAEdCCVizLjY7Za7yooCpIoAeEB41tsgdyT9aWP9NrI5rO+0Z?= =?us-ascii?Q?BsKU3iSa7XzitmnK28eGXQSrrcZmtHyBVV+VOOJv42IJBwuY3Iq3gsAgdabc?= =?us-ascii?Q?T+KoTO9AO7xAZxoxTxJdGJjE2lQHqJLKZLsIOKeGQ03tYSxFFvGE9qL71F48?= =?us-ascii?Q?C0ullIKnMyqO0Pe6FiaqkTZ6wWsTZ9Sfo0xuGRNXYW+cuLND1A6CHrY/AFnL?= =?us-ascii?Q?BGrNEyTMQFAnSi06vkVfm9zC1ABdGbfg38W6ZsSHTLYWLZ0xoK8mZlvUPbNb?= =?us-ascii?Q?rzKTj5ahPMk4QYL58RnACg0t6DgyL6pEdxb4oxzBm+WoxbnvwMZSOmNzUP+h?= =?us-ascii?Q?zDkHtu+XI8guKK5Mos39pNbJ8dnKnjHGUG8Aarmvv7hRwmw1a9VXJNhWLkmz?= =?us-ascii?Q?yqYq9BEhYuoHmKFjVgdmfSyDevEllun2xxhvPigwVKOedea6c6LvqEWY4MA+?= =?us-ascii?Q?Y1egCIdDvrqbFaO0qFmo1EdKKBUzKWYotQHvMVIemLJT85w+6tVHAymM1yPi?= =?us-ascii?Q?seUmAfrX4oniGFeH0D5he4//sUG3Aub7luomfvvBETAS3P3UqhGpATCKlEo0?= =?us-ascii?Q?ne6iZStEybd5/S85H3FflmrPVUFZoHo/PvhdeeH+QYsIkG2fvAOmcOPoUULK?= =?us-ascii?Q?LflaEuf0aGRm0fImtWp1+5N3dqt4GZ6det8Tc0RYTpMdxHMF8OxIVmRWVJfH?= =?us-ascii?Q?4n1O43ORFK+40zyerZBHg3Iv6fPcgUMffq1XBbDUyW+61kBQn4KwVjymMyg9?= =?us-ascii?Q?N0CCHPiq+Sh/WHT6DYQg7J8pli6Q2Eg5+LAGwGK/vkUy3uJAT32HT1AiTC/n?= =?us-ascii?Q?II5WKz7bp0i4FxGlQZ2hZ1Y7pS/K/KpN5npAL5ZXCXo9tOvmXOhkjQIfdLiz?= =?us-ascii?Q?HLHOXMezAp+OIlc4XzxxtzKKrcUuGK9sHKVSpkWImCeyH99U85Trsd76ms3A?= =?us-ascii?Q?RyZbPjU8v3ig4FTVMh1k14Ffw2ZuWL7/k2ZdcJmgICrym7iTF/Dvrfr8HJgk?= =?us-ascii?Q?Pt9jBUVFf0CV1E5I3qq0JqcB49i2mLGCHpGXKtmKnEZI+Z8fwiuTlOjREMLg?= =?us-ascii?Q?arFhzxj0kuPOeR839CjfSlQP+vqy5ydl61oYlgAZcTDdOIoHCmNdavw6Rob7?= =?us-ascii?Q?Ajq0AXjtCULaSVqun4cN6hjnTdrVRD5u0pcHLMfsBWtMzZMtXjozee8Y/GVS?= =?us-ascii?Q?wSWS3NzeCKg4QYysV5+6Y25g=3D?= X-Microsoft-Exchange-Diagnostics: 1;CY1PR0301MB0857;5:PRfKaarqcZnZkYV6tsFPGX9u9l6QpPjHJDz6HDCDaA96fK26Ha5NNMHtYR+FpBGxV+NPipY4XZ4fznE9jQz39uOFqrwUNoyPHRvLyTD6smk1HwBn9jLtI3OTtwoB9e4QjvD59K7Jm4gi+6/x0n/bZg==;24:fTUe2Zct72+xTh6Xct4ZWZFADsI1dO4RI9ojaKIOpHAmgn+jmg3SBPGIgYGNga7HDmjoZMdFJe2PJDpBf+1o2k2LKrzLGQ+G6EcDm59zp30=;20:myYieOC+PNUInImtz0vGE/P8tebWiNxLOQEnfB7Ns/OY4dKx8lo0E1OE69m6z2jdXX99J8D7d52z4Y8dGM87bw== X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Sep 2015 07:32:21.5180 (UTC) X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.158.2];Helo=[az84smr01.freescale.net] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0301MB0857 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 07, 2015 at 01:53:19PM +0300, Roger Quadros wrote: > On 07/09/15 10:40, Li Jun wrote: > > On Mon, Aug 24, 2015 at 04:21:18PM +0300, Roger Quadros wrote: > >> The OTG core instantiates the OTG Finite State Machine > >> per OTG controller and manages starting/stopping the > >> host and gadget controllers based on the bus state. > >> > >> It provides APIs for the following tasks > >> > >> - Registering an OTG capable controller > >> - Registering Host and Gadget controllers to OTG core > >> - Providing inputs to and kicking the OTG state machine > >> > >> Signed-off-by: Roger Quadros > >> --- > >> MAINTAINERS | 4 +- > >> drivers/usb/Kconfig | 2 +- > >> drivers/usb/Makefile | 1 + > >> drivers/usb/common/Makefile | 3 +- > >> drivers/usb/common/usb-otg.c | 1061 ++++++++++++++++++++++++++++++++++++++++++ > >> drivers/usb/common/usb-otg.h | 71 +++ > >> drivers/usb/core/Kconfig | 11 +- > >> include/linux/usb/otg.h | 189 +++++++- > >> 8 files changed, 1321 insertions(+), 21 deletions(-) > >> create mode 100644 drivers/usb/common/usb-otg.c > >> create mode 100644 drivers/usb/common/usb-otg.h > >> > > > > ... ... > > > >> + > >> +/** > >> + * Get OTG device from host or gadget device. > >> + * > >> + * For non device tree boot, the OTG controller is assumed to be > >> + * the parent of the host/gadget device. > > > > This assumption/restriction maybe a problem, as I pointed in your previous > > version, usb_create_hcd() use the passed dev as its dev, but, > > usb_add_gadget_udc() use the passed dev as its parent dev, so often the > > host and gadget don't share the same parent device, at least it doesn't > > apply to chipidea case. > > Let's provide a way for OTG driver to provide the OTG core exactly which is > the related host/gadget device. > > > > >> + * For device tree boot, the OTG controller is derived from the > >> + * "otg-controller" property. > >> + */ > >> +static struct device *usb_otg_get_device(struct device *hcd_gcd_dev) > >> +{ > >> + struct device *otg_dev; > >> + > >> + if (!hcd_gcd_dev) > >> + return NULL; > >> + > >> + if (hcd_gcd_dev->of_node) { > >> + struct device_node *np; > >> + struct platform_device *pdev; > >> + > >> + np = of_parse_phandle(hcd_gcd_dev->of_node, "otg-controller", > >> + 0); > >> + if (!np) > >> + goto legacy; /* continue legacy way */ > >> + > >> + pdev = of_find_device_by_node(np); > >> + of_node_put(np); > >> + if (!pdev) { > >> + dev_err(&pdev->dev, "couldn't get otg-controller device\n"); > >> + return NULL; > >> + } > >> + > >> + otg_dev = &pdev->dev; > >> + return otg_dev; > >> + } > >> + > >> +legacy: > >> + /* otg device is parent and must be registered */ > >> + otg_dev = hcd_gcd_dev->parent; > >> + if (!usb_otg_get_data(otg_dev)) > >> + return NULL; > >> + > >> + return otg_dev; > >> +} > >> + > > > > ... ... > > > >> +static void usb_otg_init_timers(struct usb_otg *otgd, unsigned *timeouts) > >> +{ > >> + struct otg_fsm *fsm = &otgd->fsm; > >> + unsigned long tmouts[NUM_OTG_FSM_TIMERS]; > >> + int i; > >> + > >> + /* set default timeouts */ > >> + tmouts[A_WAIT_VRISE] = TA_WAIT_VRISE; > >> + tmouts[A_WAIT_VFALL] = TA_WAIT_VFALL; > >> + tmouts[A_WAIT_BCON] = TA_WAIT_BCON; > >> + tmouts[A_AIDL_BDIS] = TA_AIDL_BDIS; > >> + tmouts[A_BIDL_ADIS] = TA_BIDL_ADIS; > >> + tmouts[B_ASE0_BRST] = TB_ASE0_BRST; > >> + tmouts[B_SE0_SRP] = TB_SE0_SRP; > >> + tmouts[B_SRP_FAIL] = TB_SRP_FAIL; > >> + > >> + /* set controller provided timeouts */ > >> + if (timeouts) { > >> + for (i = 0; i < NUM_OTG_FSM_TIMERS; i++) { > >> + if (timeouts[i]) > >> + tmouts[i] = timeouts[i]; > >> + } > >> + } > >> + > >> + otg_timer_init(A_WAIT_VRISE, otgd, set_tmout, TA_WAIT_VRISE, > >> + &fsm->a_wait_vrise_tmout); > >> + otg_timer_init(A_WAIT_VFALL, otgd, set_tmout, TA_WAIT_VFALL, > >> + &fsm->a_wait_vfall_tmout); > >> + otg_timer_init(A_WAIT_BCON, otgd, set_tmout, TA_WAIT_BCON, > >> + &fsm->a_wait_bcon_tmout); > >> + otg_timer_init(A_AIDL_BDIS, otgd, set_tmout, TA_AIDL_BDIS, > >> + &fsm->a_aidl_bdis_tmout); > >> + otg_timer_init(A_BIDL_ADIS, otgd, set_tmout, TA_BIDL_ADIS, > >> + &fsm->a_bidl_adis_tmout); > >> + otg_timer_init(B_ASE0_BRST, otgd, set_tmout, TB_ASE0_BRST, > >> + &fsm->b_ase0_brst_tmout); > >> + > >> + otg_timer_init(B_SE0_SRP, otgd, set_tmout, TB_SE0_SRP, > >> + &fsm->b_se0_srp); > >> + otg_timer_init(B_SRP_FAIL, otgd, set_tmout, TB_SRP_FAIL, > >> + &fsm->b_srp_done); > >> + > >> + /* FIXME: what about A_WAIT_ENUM? */ > > > > Either you init it as other timers, or you remove all of it, otherwise > > there will be NULL pointer crash. > > I want to initialize it but was not sure about the timeout value. > What timeout value I must use? > It's not defined in OTG spec, I don't know either. or you filter it out when add/del timer in below 2 functions. if (id == A_WAIT_ENUM) return; > > > >> +} > >> + > >> +/** > >> + * OTG FSM ops function to add timer > >> + */ > >> +static void usb_otg_add_timer(struct otg_fsm *fsm, enum otg_fsm_timer id) > >> +{ > >> + struct usb_otg *otgd = container_of(fsm, struct usb_otg, fsm); > >> + struct otg_timer *otgtimer = &otgd->timers[id]; > >> + struct hrtimer *timer = &otgtimer->timer; > >> + > >> + if (!otgd->fsm_running) > >> + return; > >> + > >> + /* if timer is already active, exit */ > >> + if (hrtimer_active(timer)) { > >> + dev_err(otgd->dev, "otg: timer %d is already running\n", id); > >> + return; > >> + } > >> + > >> + hrtimer_start(timer, otgtimer->timeout, HRTIMER_MODE_REL); > >> +} > >> + > >> +/** > >> + * OTG FSM ops function to delete timer > >> + */ > >> +static void usb_otg_del_timer(struct otg_fsm *fsm, enum otg_fsm_timer id) > >> +{ > >> + struct usb_otg *otgd = container_of(fsm, struct usb_otg, fsm); > >> + struct hrtimer *timer = &otgd->timers[id].timer; > >> + > >> + hrtimer_cancel(timer); > >> +} > >> + ... ... > >> + } > >> + > >> + otgd->dev = dev; > >> + otgd->caps = &config->otg_caps; > > > > How about define otgd->caps as a pointer, then don't need copy it. > > otgd->caps is a pointer. > okay, you are right. > > > >> + INIT_WORK(&otgd->work, usb_otg_work); > >> + otgd->wq = create_singlethread_workqueue("usb_otg"); > >> + if (!otgd->wq) { > >> + dev_err(dev, "otg: %s: can't create workqueue\n", > >> + __func__); > >> + ret = -ENOMEM; > >> + goto err_wq; > >> + } > >> + > >> + usb_otg_init_timers(otgd, config->otg_timeouts); > >> + > >> + /* create copy of original ops */ > >> + otgd->fsm_ops = *config->fsm_ops; > > > > The same, use a pointer is enough? > > We are creating a copy because we are overriding timer ops. > okay. > > > >> + /* FIXME: we ignore caller's timer ops */ > >> + otgd->fsm_ops.add_timer = usb_otg_add_timer; > >> + otgd->fsm_ops.del_timer = usb_otg_del_timer; > >> + /* set otg ops */ > >> + otgd->fsm.ops = &otgd->fsm_ops; > >> + otgd->fsm.otg = otgd; > >> + * ... ... > >> + * This is used by the USB Host stack to register the Host controller > >> + * to the OTG core. Host controller must not be started by the > >> + * caller as it is left upto the OTG state machine to do so. > > > > I am confused on how to use this function. > > - This function should be called before start fsm per usb_otg_start_fsm(). > > yes. > > > - Called by usb_add_hcd(), so we need call usb_add_hcd() before start fsm. > > yes. > > > - If I want to add hcd when switch to host role, and remove hcd when switch > > to peripheral, with this design, I cannot use this function? > > You add hcd only once during the life of the OTG device. If it is linked to the > OTG controller the OTG fsm manages the start/stop of hcd using the otg_hcd_ops. > > "usb/core/hcd.c" > static struct otg_hcd_ops otg_hcd_intf = { > .add = usb_otg_add_hcd, > .remove = usb_otg_remove_hcd, > }; > > Your otg driver must use teh usb_otg_add/remove_hcd to start/stop the controller. > Using usb_remove_hcd() means the hcd resource is no longer available and the > otg fsm will be stopped. > > > - How about split it out of usb_add_hcd()? > > Adding the HCD and starting/stopping the hcd is split into > usb_add/remove_hcd() and usb_otg_add/remove_hcd() for OTG case. > Catch your point now. Do usb_add_hcd to register hcd before start fsm, and use usb_otg_start_host() to start/stop host role for otg fsm. > > > >> + * > >> + * Returns: 0 on success, error value otherwise. > >> + */ > >> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum, > >> + unsigned long irqflags, struct otg_hcd_ops *ops) > >> +{ > >> + struct usb_otg *otgd; > >> + struct device *hcd_dev = hcd->self.controller; > >> + struct device *otg_dev = usb_otg_get_device(hcd_dev); > >> + > >> + if (!otg_dev) > >> + return -EINVAL; /* we're definitely not OTG */ > >> + > >> + /* we're otg but otg controller might not yet be registered */ > >> + mutex_lock(&otg_list_mutex); > >> + otgd = usb_otg_get_data(otg_dev); > >> + mutex_unlock(&otg_list_mutex); > >> + if (!otgd) { > >> + dev_dbg(hcd_dev, > >> + "otg: controller not yet registered. waiting..\n"); > >> + /* > >> + * otg controller might register later. Put the hcd in > >> + * wait list and call us back when ready > >> + */ > >> + if (usb_otg_hcd_wait_add(otg_dev, hcd, irqnum, irqflags, ops)) { > >> + dev_dbg(hcd_dev, "otg: failed to add to wait list\n"); > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> + } > >> + > >> + /* HCD will be started by OTG fsm when needed */ > >> + mutex_lock(&otgd->fsm.lock); > > > > If call usb_add_hcd() when start host role, deadlock. > > No. You must call usb_otg_add_hcd() to start host role. > > > > >> + if (otgd->primary_hcd.hcd) { > >> + /* probably a shared HCD ? */ > >> + if (usb_otg_hcd_is_primary_hcd(hcd)) { > >> + dev_err(otg_dev, "otg: primary host already registered\n"); > >> + goto err; > >> + } > >> + > >> + if (hcd->shared_hcd == otgd->primary_hcd.hcd) { > >> + if (otgd->shared_hcd.hcd) { > >> + dev_err(otg_dev, "otg: shared host already registered\n"); > >> + goto err; > >> + } > >> + > >> + otgd->shared_hcd.hcd = hcd; > >> + otgd->shared_hcd.irqnum = irqnum; > >> + otgd->shared_hcd.irqflags = irqflags; > >> + otgd->shared_hcd.ops = ops; > >> + dev_info(otg_dev, "otg: shared host %s registered\n", > >> + dev_name(hcd->self.controller)); > >> + } else { > >> + dev_err(otg_dev, "otg: invalid shared host %s\n", > >> + dev_name(hcd->self.controller)); > >> + goto err; > >> + } > >> + } else { > >> + if (!usb_otg_hcd_is_primary_hcd(hcd)) { > >> + dev_err(otg_dev, "otg: primary host must be registered first\n"); > >> + goto err; > >> + } > >> + > >> + otgd->primary_hcd.hcd = hcd; > >> + otgd->primary_hcd.irqnum = irqnum; > >> + otgd->primary_hcd.irqflags = irqflags; > >> + otgd->primary_hcd.ops = ops; > >> + dev_info(otg_dev, "otg: primary host %s registered\n", > >> + dev_name(hcd->self.controller)); > >> + } > >> + > >> + /* > >> + * we're ready only if we have shared HCD > >> + * or we don't need shared HCD. > >> + */ > >> + if (otgd->shared_hcd.hcd || !otgd->primary_hcd.hcd->shared_hcd) { > >> + otgd->fsm.otg->host = hcd_to_bus(hcd); > > > > otgd->host = hcd_to_bus(hcd); > > ok. So we set host at both places. struct usb_otg in struct otg_fsm starts to > feel redundant now. I think we should get rid of it and get the usb_otg struct > using container_of() instead. > Then you may come to force existing otg fsm code to use your OTG core. > > > >> + /* FIXME: set bus->otg_port if this is true OTG port with HNP */ > >> + > >> + /* start FSM */ > >> + usb_otg_start_fsm(&otgd->fsm); > >> + } else { > >> + dev_dbg(otg_dev, "otg: can't start till shared host registers\n"); > >> + } > >> + > >> + mutex_unlock(&otgd->fsm.lock); > >> + > >> + return 0; > >> + > >> +err: > >> + mutex_unlock(&otgd->fsm.lock); > >> + return -EINVAL; > > > > Return non-zero, then if err, do we need call usb_otg_add_hcd() after > > usb_otg_register_hcd() fails? > > You should not call usb_otg_register_hcd() but usb_add_hcd(). > If that fails then you fail as ususal. My point is if we use usb_add_hcd(), but failed in usb_otg_register_hcd(), then usb_otg_add_hcd() will be called in *all* error case, is this your expectation? if (usb_otg_register_hcd(hcd, irqnum, irqflags, &otg_hcd_intf)) return usb_otg_add_hcd(hcd, irqnum, irqflags); > >> + * @fsm_running: state machine running/stopped indicator > >> + */ > >> struct usb_otg { > >> u8 default_a; > >> > >> struct phy *phy; > >> /* old usb_phy interface */ > >> struct usb_phy *usb_phy; > >> + > > > > add a blank line? > > You missed this. > >> struct usb_bus *host; > >> struct usb_gadget *gadget; > >> > >> enum usb_otg_state state; > >> > >> + struct device *dev; > >> + struct usb_otg_caps *caps; > >> + struct otg_fsm fsm; > >> + struct otg_fsm_ops fsm_ops; > >> + > >> + /* internal use only */ > >> + struct otg_hcd primary_hcd; > >> + struct otg_hcd shared_hcd; > >> + struct otg_gadget_ops *gadget_ops; > >> + struct otg_timer timers[NUM_OTG_FSM_TIMERS]; > >> + struct list_head list; > >> + struct work_struct work; > >> -- > >> 2.1.4