From: Roger Quadros <rogerq@ti.com> To: Peter Chen <hzpeterchen@gmail.com> Cc: <peter.chen@freescale.com>, <balbi@kernel.org>, <tony@atomide.com>, <gregkh@linuxfoundation.org>, <dan.j.williams@intel.com>, <mathias.nyman@linux.intel.com>, <Joao.Pinto@synopsys.com>, <sergei.shtylyov@cogentembedded.com>, <jun.li@freescale.com>, <grygorii.strashko@ti.com>, <yoshihiro.shimoda.uh@renesas.com>, <robh@kernel.org>, <nsekhar@ti.com>, <b-liu@ti.com>, <linux-usb@vger.kernel.org>, <linux-omap@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core Date: Fri, 20 May 2016 10:26:03 +0300 [thread overview] Message-ID: <573EBC0B.7030204@ti.com> (raw) In-Reply-To: <20160520013931.GA10896@shlinux2> Peter, On 20/05/16 04:39, Peter Chen wrote: > On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote: >> On 18/05/16 06:18, Peter Chen wrote: >>> On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote: >>>> On 16/05/16 12:23, Peter Chen wrote: >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >>>>>> Hi, >>>>>> >>>>>> On 16/05/16 10:02, Peter Chen wrote: >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>>>>>> + >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) >>>>>>>> +{ >>>>>>>> + struct usb_udc *udc; >>>>>>>> + >>>>>>>> + mutex_lock(&udc_lock); >>>>>>>> + udc = usb_gadget_to_udc(gadget); >>>>>>>> + if (!udc) { >>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>>>>>> + __func__); >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (connect) { >>>>>>>> + if (!gadget->connected) >>>>>>>> + usb_gadget_connect(udc->gadget); >>>>>>>> + } else { >>>>>>>> + if (gadget->connected) { >>>>>>>> + usb_gadget_disconnect(udc->gadget); >>>>>>>> + udc->driver->disconnect(udc->gadget); >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>> >>>>>>> Since this is called for vbus interrupt, why not using >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect >>>>>>> at usb_gadget_stop. >>>>>> >>>>>> We can't assume that this is always called for vbus interrupt so >>>>>> I decided not to call usb_udc_vbus_handler. >>>>>> >>>>>> udc->vbus is really pointless for us. We keep vbus states in our >>>>>> state machine and leave udc->vbus as ture always. >>>>>> >>>>>> Why do you want to move udc->driver->disconnect() to stop? >>>>>> If USB controller disconnected from bus then the gadget driver >>>>>> must be notified about the disconnect immediately. The controller >>>>>> may or may not be stopped by the core. >>>>>> >>>>> >>>>> Then, would you give some comments when this API will be used? >>>>> I was assumed it is only used for drd state machine. >>>> >>>> drd_state machine didn't even need this API in the first place :). >>>> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. >>>> Won't full OTG state machine want to use this API? If not what would it use? >>>> >>> >>> Oh, I meant only drd and fully otg state machine needs it. I am >>> wondering if we need have a new API to do it. Two questions: >> >> OK. >>> >>> - Except for vbus interrupt, any chances this API will be used at >>> current logic? >> >> I don't think so. But we can't assume caller behaviour for any API. >> >>> - When this API is called but without a coming gadget->stop? >>> >> Never for DRD case. But we want to catch wrong users. >> > > In future, otg_start_gadget will be used for both DRD and fully OTG FSM. > There is no otg_loc_conn at current DRD FSM, but there is > otg_loc_conn at current OTG FSM, see below. > > DRD FSM: > case OTG_STATE_B_IDLE: > drd_set_protocol(fsm, PROTO_UNDEF); > otg_drv_vbus(otg, 0); > break; > case OTG_STATE_B_PERIPHERAL: > drd_set_protocol(fsm, PROTO_GADGET); > otg_drv_vbus(otg, 0); > break; > > OTG FSM: > case OTG_STATE_B_IDLE: > otg_drv_vbus(otg, 0); > otg_chrg_vbus(otg, 0); > otg_loc_conn(otg, 0); > otg_loc_sof(otg, 0); > /* > * Driver is responsible for starting ADP probing > * if ADP sensing times out. > */ > otg_start_adp_sns(otg); > otg_set_protocol(fsm, PROTO_UNDEF); > otg_add_timer(otg, B_SE0_SRP); > break; > case OTG_STATE_B_PERIPHERAL: > otg_chrg_vbus(otg, 0); > otg_loc_sof(otg, 0); > otg_set_protocol(fsm, PROTO_GADGET); > otg_loc_conn(otg, 1); > break; > > My original suggestion is to have an API to do pull dp and this API > will be used at both DRD and OTG FSM, and called at otg_loc_conn. The API is usb_gadget_connect_control(); > The (de)initialize is the same for both two FSMs, it both includes > init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET) > otg_loc_conn(otg, 1); > > What do you think? > I think loc_conn is a bit confusing to drd users. Another issue I see is that DRD controller drivers will need to explicitly pass .loc_conn ops via the otg_fsm_ops. This is an additional step and totally unnecessary as it can be automatically done via direct DRD -> UDC-core call. cheers, -roger
WARNING: multiple messages have this Message-ID (diff)
From: Roger Quadros <rogerq@ti.com> To: Peter Chen <hzpeterchen@gmail.com> Cc: peter.chen@freescale.com, balbi@kernel.org, tony@atomide.com, gregkh@linuxfoundation.org, dan.j.williams@intel.com, mathias.nyman@linux.intel.com, Joao.Pinto@synopsys.com, sergei.shtylyov@cogentembedded.com, jun.li@freescale.com, grygorii.strashko@ti.com, yoshihiro.shimoda.uh@renesas.com, robh@kernel.org, nsekhar@ti.com, b-liu@ti.com, linux-usb@vger.kernel.org, linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core Date: Fri, 20 May 2016 10:26:03 +0300 [thread overview] Message-ID: <573EBC0B.7030204@ti.com> (raw) In-Reply-To: <20160520013931.GA10896@shlinux2> Peter, On 20/05/16 04:39, Peter Chen wrote: > On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote: >> On 18/05/16 06:18, Peter Chen wrote: >>> On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote: >>>> On 16/05/16 12:23, Peter Chen wrote: >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >>>>>> Hi, >>>>>> >>>>>> On 16/05/16 10:02, Peter Chen wrote: >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>>>>>> + >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) >>>>>>>> +{ >>>>>>>> + struct usb_udc *udc; >>>>>>>> + >>>>>>>> + mutex_lock(&udc_lock); >>>>>>>> + udc = usb_gadget_to_udc(gadget); >>>>>>>> + if (!udc) { >>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>>>>>> + __func__); >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (connect) { >>>>>>>> + if (!gadget->connected) >>>>>>>> + usb_gadget_connect(udc->gadget); >>>>>>>> + } else { >>>>>>>> + if (gadget->connected) { >>>>>>>> + usb_gadget_disconnect(udc->gadget); >>>>>>>> + udc->driver->disconnect(udc->gadget); >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>> >>>>>>> Since this is called for vbus interrupt, why not using >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect >>>>>>> at usb_gadget_stop. >>>>>> >>>>>> We can't assume that this is always called for vbus interrupt so >>>>>> I decided not to call usb_udc_vbus_handler. >>>>>> >>>>>> udc->vbus is really pointless for us. We keep vbus states in our >>>>>> state machine and leave udc->vbus as ture always. >>>>>> >>>>>> Why do you want to move udc->driver->disconnect() to stop? >>>>>> If USB controller disconnected from bus then the gadget driver >>>>>> must be notified about the disconnect immediately. The controller >>>>>> may or may not be stopped by the core. >>>>>> >>>>> >>>>> Then, would you give some comments when this API will be used? >>>>> I was assumed it is only used for drd state machine. >>>> >>>> drd_state machine didn't even need this API in the first place :). >>>> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. >>>> Won't full OTG state machine want to use this API? If not what would it use? >>>> >>> >>> Oh, I meant only drd and fully otg state machine needs it. I am >>> wondering if we need have a new API to do it. Two questions: >> >> OK. >>> >>> - Except for vbus interrupt, any chances this API will be used at >>> current logic? >> >> I don't think so. But we can't assume caller behaviour for any API. >> >>> - When this API is called but without a coming gadget->stop? >>> >> Never for DRD case. But we want to catch wrong users. >> > > In future, otg_start_gadget will be used for both DRD and fully OTG FSM. > There is no otg_loc_conn at current DRD FSM, but there is > otg_loc_conn at current OTG FSM, see below. > > DRD FSM: > case OTG_STATE_B_IDLE: > drd_set_protocol(fsm, PROTO_UNDEF); > otg_drv_vbus(otg, 0); > break; > case OTG_STATE_B_PERIPHERAL: > drd_set_protocol(fsm, PROTO_GADGET); > otg_drv_vbus(otg, 0); > break; > > OTG FSM: > case OTG_STATE_B_IDLE: > otg_drv_vbus(otg, 0); > otg_chrg_vbus(otg, 0); > otg_loc_conn(otg, 0); > otg_loc_sof(otg, 0); > /* > * Driver is responsible for starting ADP probing > * if ADP sensing times out. > */ > otg_start_adp_sns(otg); > otg_set_protocol(fsm, PROTO_UNDEF); > otg_add_timer(otg, B_SE0_SRP); > break; > case OTG_STATE_B_PERIPHERAL: > otg_chrg_vbus(otg, 0); > otg_loc_sof(otg, 0); > otg_set_protocol(fsm, PROTO_GADGET); > otg_loc_conn(otg, 1); > break; > > My original suggestion is to have an API to do pull dp and this API > will be used at both DRD and OTG FSM, and called at otg_loc_conn. The API is usb_gadget_connect_control(); > The (de)initialize is the same for both two FSMs, it both includes > init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET) > otg_loc_conn(otg, 1); > > What do you think? > I think loc_conn is a bit confusing to drd users. Another issue I see is that DRD controller drivers will need to explicitly pass .loc_conn ops via the otg_fsm_ops. This is an additional step and totally unnecessary as it can be automatically done via direct DRD -> UDC-core call. cheers, -roger
next prev parent reply other threads:[~2016-05-20 7:26 UTC|newest] Thread overview: 121+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-05-13 10:03 [PATCH v8 00/14] USB OTG/dual-role framework Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 01/14] usb: hcd: Initialize hcd->flags to 0 Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 02/14] usb: otg-fsm: Prevent build warning "VDBG" redefined Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 03/14] usb: hcd.h: Add OTG to HCD interface Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 04/14] usb: otg-fsm: use usb_otg wherever possible Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 05/14] usb: otg-fsm: move host controller operations into usb_otg->hcd_ops Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 06/14] usb: gadget.h: Add OTG to gadget interface Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 07/14] usb: otg: get rid of CONFIG_USB_OTG_FSM in favour of CONFIG_USB_OTG Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 08/14] usb: otg: add OTG/dual-role core Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-16 9:00 ` Roger Quadros 2016-05-16 9:00 ` Roger Quadros 2016-05-18 7:45 ` Peter Chen 2016-05-18 12:59 ` Roger Quadros 2016-05-18 12:59 ` Roger Quadros 2016-05-20 8:31 ` Roger Quadros 2016-05-20 8:31 ` Roger Quadros 2016-05-20 9:19 ` Roger Quadros 2016-05-20 9:19 ` Roger Quadros 2016-05-20 9:53 ` Peter Chen 2016-05-20 9:53 ` Peter Chen 2016-05-23 10:06 ` Roger Quadros 2016-05-23 10:06 ` Roger Quadros 2016-05-24 9:45 ` Roger Quadros 2016-05-24 9:45 ` Roger Quadros 2016-05-25 2:44 ` Peter Chen 2016-05-25 3:19 ` Jun Li 2016-05-25 3:19 ` Jun Li 2016-05-25 12:26 ` Roger Quadros 2016-05-25 12:26 ` Roger Quadros 2016-05-25 12:21 ` Roger Quadros 2016-05-25 12:21 ` Roger Quadros 2016-05-25 14:44 ` Jun Li 2016-05-25 14:44 ` Jun Li 2016-05-27 8:03 ` Peter Chen 2016-05-27 8:12 ` Peter Chen 2016-05-13 10:03 ` [PATCH v8 09/14] usb: of: add an API to get OTG device from USB controller node Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-20 9:29 ` [PATCH v9 " Roger Quadros 2016-05-20 9:29 ` Roger Quadros 2016-05-23 21:06 ` Rob Herring 2016-05-13 10:03 ` [PATCH v8 10/14] usb: otg: add hcd companion support Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 18:13 ` Rob Herring 2016-05-13 18:13 ` Rob Herring 2016-05-16 8:12 ` Roger Quadros 2016-05-16 8:12 ` Roger Quadros 2016-05-20 9:32 ` [PATCH v9 " Roger Quadros 2016-05-20 9:32 ` Roger Quadros 2016-05-23 21:07 ` Rob Herring 2016-05-13 10:03 ` [PATCH v8 11/14] usb: otg: use dev_dbg() instead of VDBG() Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 12/14] usb: hcd: Adapt to OTG core Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 13/14] usb: gadget: udc: adapt " Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-16 7:02 ` Peter Chen 2016-05-16 8:26 ` Roger Quadros 2016-05-16 8:26 ` Roger Quadros 2016-05-16 9:23 ` Peter Chen 2016-05-16 9:23 ` Peter Chen 2016-05-16 9:51 ` Roger Quadros 2016-05-16 9:51 ` Roger Quadros 2016-05-17 7:38 ` Jun Li 2016-05-17 7:38 ` Jun Li 2016-05-17 8:08 ` Roger Quadros 2016-05-17 8:08 ` Roger Quadros 2016-05-17 8:28 ` Jun Li 2016-05-17 8:28 ` Jun Li 2016-05-18 12:42 ` Roger Quadros 2016-05-18 12:42 ` Roger Quadros 2016-05-18 13:12 ` Jun Li 2016-05-18 13:12 ` Jun Li 2016-05-18 13:43 ` Roger Quadros 2016-05-18 13:43 ` Roger Quadros 2016-05-18 14:46 ` Jun Li 2016-05-18 14:46 ` Jun Li 2016-05-19 7:32 ` Roger Quadros 2016-05-19 7:32 ` Roger Quadros 2016-05-21 2:29 ` Peter Chen 2016-05-21 2:29 ` Peter Chen 2016-05-23 3:21 ` Peter Chen 2016-05-23 3:21 ` Peter Chen 2016-05-23 10:11 ` Roger Quadros 2016-05-23 10:11 ` Roger Quadros 2016-05-23 10:34 ` Jun Li 2016-05-23 10:34 ` Jun Li 2016-05-23 10:36 ` Roger Quadros 2016-05-23 10:36 ` Roger Quadros 2016-05-24 2:53 ` Peter Chen 2016-05-24 2:53 ` Peter Chen 2016-06-08 7:32 ` Roger Quadros 2016-06-08 7:32 ` Roger Quadros 2016-06-08 9:05 ` Peter Chen 2016-06-08 9:05 ` Peter Chen 2016-05-18 3:18 ` Peter Chen 2016-05-18 12:45 ` Roger Quadros 2016-05-18 12:45 ` Roger Quadros 2016-05-20 1:39 ` Peter Chen 2016-05-20 1:39 ` Peter Chen 2016-05-20 7:26 ` Roger Quadros [this message] 2016-05-20 7:26 ` Roger Quadros 2016-05-21 2:44 ` Peter Chen 2016-05-21 2:44 ` Peter Chen 2016-06-01 7:38 ` Peter Chen 2016-06-02 11:07 ` Roger Quadros 2016-06-02 11:07 ` Roger Quadros 2016-05-13 10:03 ` [PATCH v8 14/14] usb: host: xhci-plat: Add otg device to platform data Roger Quadros 2016-05-13 10:03 ` Roger Quadros 2016-05-30 9:29 ` [PATCH v8 00/14] USB OTG/dual-role framework Peter Chen 2016-05-30 9:29 ` Peter Chen 2016-05-30 14:04 ` Roger Quadros 2016-05-30 14:04 ` Roger Quadros
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=573EBC0B.7030204@ti.com \ --to=rogerq@ti.com \ --cc=Joao.Pinto@synopsys.com \ --cc=b-liu@ti.com \ --cc=balbi@kernel.org \ --cc=dan.j.williams@intel.com \ --cc=devicetree@vger.kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=grygorii.strashko@ti.com \ --cc=hzpeterchen@gmail.com \ --cc=jun.li@freescale.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=mathias.nyman@linux.intel.com \ --cc=nsekhar@ti.com \ --cc=peter.chen@freescale.com \ --cc=robh@kernel.org \ --cc=sergei.shtylyov@cogentembedded.com \ --cc=tony@atomide.com \ --cc=yoshihiro.shimoda.uh@renesas.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.