Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core

[Peter Chen]

On Mon, May 23, 2016 at 01:36:51PM +0300, Roger Quadros wrote:
> On 23/05/16 13:34, Jun Li wrote:
> > Hi
> > 
> >> -----Original Message-----
> >> From: Roger Quadros [mailto:rogerq@xxxxxx]
> >> Sent: Monday, May 23, 2016 6:12 PM
> >> To: Peter Chen <hzpeterchen@xxxxxxxxx>
> >> Cc: Jun Li <jun.li@xxxxxxx>; peter.chen@xxxxxxxxxxxxx; balbi@xxxxxxxxxx;
> >> tony@xxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; dan.j.williams@xxxxxxxxx;
> >> mathias.nyman@xxxxxxxxxxxxxxx; Joao.Pinto@xxxxxxxxxxxx;
> >> sergei.shtylyov@xxxxxxxxxxxxxxxxxx; jun.li@xxxxxxxxxxxxx;
> >> grygorii.strashko@xxxxxx; yoshihiro.shimoda.uh@xxxxxxxxxxx;
> >> robh@xxxxxxxxxx; nsekhar@xxxxxx; b-liu@xxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> >> linux-omap@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> devicetree@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> On 23/05/16 06:21, Peter Chen wrote:
> >>> On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote:
> >>>> On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote:
> >>>>> On 18/05/16 17:46, Jun Li wrote:
> >>>>>>
> >>>>>>
> >>>>>>>>>
> >>>>>>>>> I didn't want to have complex Kconfig so decided to have otg as
> >>>>>>>>> built-in only.
> >>>>>>>>> What do you want me to change in existing code? and why?
> >>>>>>>>
> >>>>>>>> Remove those stuff which only for pass diff driver config Like
> >>>>>>>> every controller driver need a duplicated
> >>>>>>>>
> >>>>>>>> static struct otg_hcd_ops ci_hcd_ops = {
> >>>>>>>>     ...
> >>>>>>>> }
> >>>>>>>
> >>>>>>> This is an exception only. Every controller driver doesn't need to
> >>>>>>> implement hcd_ops. It is implemented in the hcd core.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> And here is another example, for gadget connect, otg driver can
> >>>>>>>> directly call to usb_udc_vbus_handler() in drd state machine, but
> >>>>>>>> you create another interface:
> >>>>>>>>
> >>>>>>>> .connect_control = usb_gadget_connect_control,
> >>>>>>>>
> >>>>>>>> If the symbol is defined in one driver which is 'm', another
> >>>>>>>> driver reference it should be 'm' as well, then there is no this
> >>>>>>>> kind of problem as my understanding.
> >>>>>>>
> >>>>>>> That is fine as long as all are 'm'. but how do you solve the case
> >>>>>>> when Gadget is built in and host is 'm'? OTG has to be built-in
> >>>>>>> and you will need an hcd to gadget interface.
> >>>>>>
> >>>>>> Hcd to gadget interface? Or you want to say otg to host interface?
> >>>>>
> >>>>> Sorry, I meant to say host to otg interface.
> >>>>>
> >>>>>>
> >>>>>> I think hcd and gadget are independent each other, now
> >>>>>>
> >>>>>> Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol)
> >>>>>
> >>>>> It is actually a circular dependency for both.
> >>>>>  hcd <--> otg and gadget <--> otg
> >>>>>
> >>>>> hcd -> otg for usb_otg_register/unregister_hcd otg -> hcd for
> >>>>> usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg,
> >>>>> usb_hub_find_child
> >>>>>
> >>>>> gadget -> otg for usb_otg_register/unregister_gadget
> >>>>> otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler
> >>>>>
> >>>>> Now consider what will happen if I get rid of the otg_hcd and
> >> otg_gadget interfaces.
> >>>>> 'y' means built-in, 'm' means module.
> >>>>>
> >>>>> 1) hcd 'y', gadget 'y'
> >>>>> otg has to be 'y' for proper build.
> >>>>>
> >>>>> 2) hcd 'm', gadget 'm'
> >>>>> otg has to be 'm' for proper build.
> >>>>>
> >>>>> 3) hcd 'y', gadget 'm'
> >>>>> Build will fail always.
> >>>>> If otg is 'y', otg build will fail due to dependency on gadget.
> >>>>> If otg is 'm', hcd build will fail due to dependency on otg.
> >>>>>
> >>>>> 4) hcd 'm', gadget 'y'
> >>>>> Build will fail always.
> >>>>> If otg is 'y', otg build will fail due to dependency on hcd.
> >>>>> If otg is 'm', gadget build will fails due to dependency on otg.
> >>>>>
> >>>>> So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops
> >>>>> to remove otg->hcd and otg->gadget dependency.
> >>>>>
> >>>>> Now we can address 3) and 4) like so
> >>>>>
> >>>>> 3) hcd 'y', gadget 'm'
> >>>>> otg has to be 'y' for proper build.
> >>>>>
> >>>>> 4) hcd 'm', gadget 'y'
> >>>>> otg has to be 'y' for proper build.
> >>>>>
> >>>>
> >>>> How about this:
> >>>> Moving usb_otg_register/unregister_hcd to host driver to remove
> >>>> dependency hcd->otg. And moving usb_otg_get_data to common.c.
> >>>>
> >>>> Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data
> >>>> returns NULL, the host/device driver's probe return -EPROBE_DEFER.
> >>>> When the otg driver is probed successfully, the host/device will be
> >>>> re-probed again, and usb_otg_register_hcd will be called again.
> >>>>
> >>>> And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and
> >>>> otg_gadget_ops. Below build dependency issues can be fixed.
> >>>> What do you think?
> >>>>
> >>>>> 1) hcd 'y', gadget 'y'
> >>>>> otg has to be 'y' for proper build.
> >>>>>
> >>>>> 2) hcd 'm', gadget 'm'
> >>>>> otg has to be 'm' for proper build.
> >>>>>
> >>>>> 3) hcd 'y', gadget 'm'
> >>>>> Build will fail always.
> >>>>> If otg is 'y', otg build will fail due to dependency on gadget.
> >>>>> If otg is 'm', hcd build will fail due to dependency on otg.
> >>>>>
> >>>>> 4) hcd 'm', gadget 'y'
> >>>>> Build will fail always.
> >>>>> If otg is 'y', otg build will fail due to dependency on hcd.
> >>>>> If otg is 'm', gadget build will fails due to dependency on otg.
> >>>> --
> >>>>
> >>>
> >>> After thinking more, my suggestion can't work. How about moving
> >>> CONFIG_USB_OTG out of CONFIG_USB, in that case, CONFIG_USB_OTG can
> >>> only be built in.
> >>>
> >> I tried this but it still doesn't resolve 3 and 4. I.e.
> >> it can't be automatically set to 'y' when either of hcd/gadget is 'y'
> >> and the other is 'm'.
> > 
> > USB_OTG only can be selected by *user*, can't be automatically set to
> > 'y' like you said, no matter what's hcd/gadget config, USB_OTG can be
> > disabled in any cases.
> 
> Yes. I am on the same page with that.
> > 
> > I think Peter's intention is to make USB_OTG to be 'bool', either disabled;
> > or built-in(if enabled), something like your original idea?
> 
> I'm fine with my original idea if you guys can confirm :).
> 

Please do it.

-- 

Best Regards,
Peter Chen