Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On Wed, Jun 08, 2016 at 10:32:19AM +0300, Roger Quadros wrote:
> Hi,
>
> On 24/05/16 05:53, Peter Chen wrote:
> > 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:rog...@ti.com]
> >>>> Sent: Monday, May 23, 2016 6:12 PM
> >>>> To: Peter Chen <hzpeterc...@gmail.com>
> >>>> Cc: Jun Li <jun...@nxp.com>; peter.c...@freescale.com; ba...@kernel.org;
> >>>> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> >>>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> >>>> devicet...@vger.kernel.org
> >>>> 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_hu
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On Wed, Jun 08, 2016 at 10:32:19AM +0300, Roger Quadros wrote:
> Hi,
>
> On 24/05/16 05:53, Peter Chen wrote:
> > 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:rog...@ti.com]
> >>>> Sent: Monday, May 23, 2016 6:12 PM
> >>>> To: Peter Chen
> >>>> Cc: Jun Li ; peter.c...@freescale.com; ba...@kernel.org;
> >>>> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> >>>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> >>>> devicet...@vger.kernel.org
> >>>> 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
> >>>>>>>
> >>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi,
On 24/05/16 05:53, Peter Chen wrote:
> 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:rog...@ti.com]
>>>> Sent: Monday, May 23, 2016 6:12 PM
>>>> To: Peter Chen <hzpeterc...@gmail.com>
>>>> Cc: Jun Li <jun...@nxp.com>; peter.c...@freescale.com; ba...@kernel.org;
>>>> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>>>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>> devicet...@vger.kernel.org
>>>> 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,
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi,
On 24/05/16 05:53, Peter Chen wrote:
> 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:rog...@ti.com]
>>>> Sent: Monday, May 23, 2016 6:12 PM
>>>> To: Peter Chen
>>>> Cc: Jun Li ; peter.c...@freescale.com; ba...@kernel.org;
>>>> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>>>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>> devicet...@vger.kernel.org
>>>> 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.
>>>>>>>
>>>>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 01/06/16 10:38, Peter Chen wrote:
> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>> @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> }
>> mutex_unlock(_lock);
>>
>> +mutex_unlock(_lock);
>> +
>
> Here, you have one more mutex_unlock.
Will fix it. Thanks.
>> kobject_uevent(>dev.kobj, KOBJ_REMOVE);
>> flush_work(>work);
>> device_unregister(>dev);
>> @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>>
>> /*
>> - */
>>
>> +struct otg_gadget_ops otg_gadget_intf = {
>> +.start = usb_gadget_start,
>> +.stop = usb_gadget_stop,
>> +.connect_control = usb_gadget_connect_control,
>> +};
>> +
>> +/* udc_lock must be held */
>> static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver
>> *driver)
>> {
>> int ret;
>> @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc,
>> struct usb_gadget_driver *dri
>> ret = driver->bind(udc->gadget, driver);
>> if (ret)
>> goto err1;
>> -ret = usb_gadget_udc_start(udc);
>> -if (ret) {
>> -driver->unbind(udc->gadget);
>> -goto err1;
>> +
>> +/* If OTG, the otg core starts the UDC when needed */
>> +if (udc->gadget->otg_dev) {
>> +mutex_unlock(_lock);
>> +usb_otg_register_gadget(udc->gadget, _gadget_intf);
>> +mutex_lock(_lock);
>> +} else {
>> +ret = usb_gadget_udc_start(udc);
>> +if (ret) {
>> +driver->unbind(udc->gadget);
>> +goto err1;
>> +}
>> +usb_udc_connect_control(udc);
>> }
>> -usb_udc_connect_control(udc);
>>
>> kobject_uevent(>dev.kobj, KOBJ_CHANGE);
>> return 0;
>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
>> *dev,
>> return -EOPNOTSUPP;
>> }
>>
>> +/* In OTG mode we don't support softconnect, but b_bus_req */
>> +if (udc->gadget->otg_dev) {
>> +dev_err(dev, "soft-connect not supported in OTG mode\n");
>> +return -EOPNOTSUPP;
>> +}
>> +
>> if (sysfs_streq(buf, "connect")) {
>> usb_gadget_udc_start(udc);
>> -usb_gadget_connect(udc->gadget);
>> +usb_udc_connect_control(udc);
>> } else if (sysfs_streq(buf, "disconnect")) {
>> usb_gadget_disconnect(udc->gadget);
>> udc->driver->disconnect(udc->gadget);
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 3ecfddd..79d654f 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent,
>> struct usb_gadget *gadget);
>> extern void usb_del_gadget_udc(struct usb_gadget *gadget);
>> extern char *usb_get_gadget_udc_name(void);
>>
>> +extern int usb_otg_add_gadget_udc(struct device *parent,
>> + struct usb_gadget *gadget,
>> + struct device *otg_dev);
>> +
>>
>> /*-*/
>>
>> /* utility to simplify dealing with string descriptors */
>> --
>> 2.7.4
>>
--
cheers,
-roger
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 01/06/16 10:38, Peter Chen wrote:
> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>> @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> }
>> mutex_unlock(_lock);
>>
>> +mutex_unlock(_lock);
>> +
>
> Here, you have one more mutex_unlock.
Will fix it. Thanks.
>> kobject_uevent(>dev.kobj, KOBJ_REMOVE);
>> flush_work(>work);
>> device_unregister(>dev);
>> @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>>
>> /*
>> - */
>>
>> +struct otg_gadget_ops otg_gadget_intf = {
>> +.start = usb_gadget_start,
>> +.stop = usb_gadget_stop,
>> +.connect_control = usb_gadget_connect_control,
>> +};
>> +
>> +/* udc_lock must be held */
>> static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver
>> *driver)
>> {
>> int ret;
>> @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc,
>> struct usb_gadget_driver *dri
>> ret = driver->bind(udc->gadget, driver);
>> if (ret)
>> goto err1;
>> -ret = usb_gadget_udc_start(udc);
>> -if (ret) {
>> -driver->unbind(udc->gadget);
>> -goto err1;
>> +
>> +/* If OTG, the otg core starts the UDC when needed */
>> +if (udc->gadget->otg_dev) {
>> +mutex_unlock(_lock);
>> +usb_otg_register_gadget(udc->gadget, _gadget_intf);
>> +mutex_lock(_lock);
>> +} else {
>> +ret = usb_gadget_udc_start(udc);
>> +if (ret) {
>> +driver->unbind(udc->gadget);
>> +goto err1;
>> +}
>> +usb_udc_connect_control(udc);
>> }
>> -usb_udc_connect_control(udc);
>>
>> kobject_uevent(>dev.kobj, KOBJ_CHANGE);
>> return 0;
>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
>> *dev,
>> return -EOPNOTSUPP;
>> }
>>
>> +/* In OTG mode we don't support softconnect, but b_bus_req */
>> +if (udc->gadget->otg_dev) {
>> +dev_err(dev, "soft-connect not supported in OTG mode\n");
>> +return -EOPNOTSUPP;
>> +}
>> +
>> if (sysfs_streq(buf, "connect")) {
>> usb_gadget_udc_start(udc);
>> -usb_gadget_connect(udc->gadget);
>> +usb_udc_connect_control(udc);
>> } else if (sysfs_streq(buf, "disconnect")) {
>> usb_gadget_disconnect(udc->gadget);
>> udc->driver->disconnect(udc->gadget);
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 3ecfddd..79d654f 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent,
>> struct usb_gadget *gadget);
>> extern void usb_del_gadget_udc(struct usb_gadget *gadget);
>> extern char *usb_get_gadget_udc_name(void);
>>
>> +extern int usb_otg_add_gadget_udc(struct device *parent,
>> + struct usb_gadget *gadget,
>> + struct device *otg_dev);
>> +
>>
>> /*-*/
>>
>> /* utility to simplify dealing with string descriptors */
>> --
>> 2.7.4
>>
--
cheers,
-roger
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> }
> mutex_unlock(_lock);
>
> + mutex_unlock(_lock);
> +
Here, you have one more mutex_unlock.
Peter
> kobject_uevent(>dev.kobj, KOBJ_REMOVE);
> flush_work(>work);
> device_unregister(>dev);
> @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>
> /* -
> */
>
> +struct otg_gadget_ops otg_gadget_intf = {
> + .start = usb_gadget_start,
> + .stop = usb_gadget_stop,
> + .connect_control = usb_gadget_connect_control,
> +};
> +
> +/* udc_lock must be held */
> static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver
> *driver)
> {
> int ret;
> @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc,
> struct usb_gadget_driver *dri
> ret = driver->bind(udc->gadget, driver);
> if (ret)
> goto err1;
> - ret = usb_gadget_udc_start(udc);
> - if (ret) {
> - driver->unbind(udc->gadget);
> - goto err1;
> +
> + /* If OTG, the otg core starts the UDC when needed */
> + if (udc->gadget->otg_dev) {
> + mutex_unlock(_lock);
> + usb_otg_register_gadget(udc->gadget, _gadget_intf);
> + mutex_lock(_lock);
> + } else {
> + ret = usb_gadget_udc_start(udc);
> + if (ret) {
> + driver->unbind(udc->gadget);
> + goto err1;
> + }
> + usb_udc_connect_control(udc);
> }
> - usb_udc_connect_control(udc);
>
> kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> return 0;
> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> + /* In OTG mode we don't support softconnect, but b_bus_req */
> + if (udc->gadget->otg_dev) {
> + dev_err(dev, "soft-connect not supported in OTG mode\n");
> + return -EOPNOTSUPP;
> + }
> +
> if (sysfs_streq(buf, "connect")) {
> usb_gadget_udc_start(udc);
> - usb_gadget_connect(udc->gadget);
> + usb_udc_connect_control(udc);
> } else if (sysfs_streq(buf, "disconnect")) {
> usb_gadget_disconnect(udc->gadget);
> udc->driver->disconnect(udc->gadget);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3ecfddd..79d654f 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent,
> struct usb_gadget *gadget);
> extern void usb_del_gadget_udc(struct usb_gadget *gadget);
> extern char *usb_get_gadget_udc_name(void);
>
> +extern int usb_otg_add_gadget_udc(struct device *parent,
> + struct usb_gadget *gadget,
> + struct device *otg_dev);
> +
> /*-*/
>
> /* utility to simplify dealing with string descriptors */
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> }
> mutex_unlock(_lock);
>
> + mutex_unlock(_lock);
> +
Here, you have one more mutex_unlock.
Peter
> kobject_uevent(>dev.kobj, KOBJ_REMOVE);
> flush_work(>work);
> device_unregister(>dev);
> @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>
> /* -
> */
>
> +struct otg_gadget_ops otg_gadget_intf = {
> + .start = usb_gadget_start,
> + .stop = usb_gadget_stop,
> + .connect_control = usb_gadget_connect_control,
> +};
> +
> +/* udc_lock must be held */
> static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver
> *driver)
> {
> int ret;
> @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc,
> struct usb_gadget_driver *dri
> ret = driver->bind(udc->gadget, driver);
> if (ret)
> goto err1;
> - ret = usb_gadget_udc_start(udc);
> - if (ret) {
> - driver->unbind(udc->gadget);
> - goto err1;
> +
> + /* If OTG, the otg core starts the UDC when needed */
> + if (udc->gadget->otg_dev) {
> + mutex_unlock(_lock);
> + usb_otg_register_gadget(udc->gadget, _gadget_intf);
> + mutex_lock(_lock);
> + } else {
> + ret = usb_gadget_udc_start(udc);
> + if (ret) {
> + driver->unbind(udc->gadget);
> + goto err1;
> + }
> + usb_udc_connect_control(udc);
> }
> - usb_udc_connect_control(udc);
>
> kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> return 0;
> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> + /* In OTG mode we don't support softconnect, but b_bus_req */
> + if (udc->gadget->otg_dev) {
> + dev_err(dev, "soft-connect not supported in OTG mode\n");
> + return -EOPNOTSUPP;
> + }
> +
> if (sysfs_streq(buf, "connect")) {
> usb_gadget_udc_start(udc);
> - usb_gadget_connect(udc->gadget);
> + usb_udc_connect_control(udc);
> } else if (sysfs_streq(buf, "disconnect")) {
> usb_gadget_disconnect(udc->gadget);
> udc->driver->disconnect(udc->gadget);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3ecfddd..79d654f 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent,
> struct usb_gadget *gadget);
> extern void usb_del_gadget_udc(struct usb_gadget *gadget);
> extern char *usb_get_gadget_udc_name(void);
>
> +extern int usb_otg_add_gadget_udc(struct device *parent,
> + struct usb_gadget *gadget,
> + struct device *otg_dev);
> +
> /*-*/
>
> /* utility to simplify dealing with string descriptors */
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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:rog...@ti.com]
> >> Sent: Monday, May 23, 2016 6:12 PM
> >> To: Peter Chen <hzpeterc...@gmail.com>
> >> Cc: Jun Li <jun...@nxp.com>; peter.c...@freescale.com; ba...@kernel.org;
> >> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> >> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> devicet...@vger.kernel.org
> >> 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 prope
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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:rog...@ti.com]
> >> Sent: Monday, May 23, 2016 6:12 PM
> >> To: Peter Chen
> >> Cc: Jun Li ; peter.c...@freescale.com; ba...@kernel.org;
> >> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> >> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> devicet...@vger.kernel.org
> >> 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.
> >>>>>
>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 23/05/16 13:34, Jun Li wrote:
> Hi
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Monday, May 23, 2016 6:12 PM
>> To: Peter Chen <hzpeterc...@gmail.com>
>> Cc: Jun Li <jun...@nxp.com>; peter.c...@freescale.com; ba...@kernel.org;
>> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> 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
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 23/05/16 13:34, Jun Li wrote:
> Hi
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Monday, May 23, 2016 6:12 PM
>> To: Peter Chen
>> Cc: Jun Li ; peter.c...@freescale.com; ba...@kernel.org;
>> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> 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',
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Monday, May 23, 2016 6:12 PM
> To: Peter Chen <hzpeterc...@gmail.com>
> Cc: Jun Li <jun...@nxp.com>; peter.c...@freescale.com; ba...@kernel.org;
> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> 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 addre
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Monday, May 23, 2016 6:12 PM
> To: Peter Chen
> Cc: Jun Li ; peter.c...@freescale.com; ba...@kernel.org;
> t...@atomide.com; gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> 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
> >>>
> >
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'.
I think some kconfig trickery can be done to get what we want.
cheers,
-roger
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'.
I think some kconfig trickery can be done to get what we want.
cheers,
-roger
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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.
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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.
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On Fri, May 20, 2016 at 10:26:03AM +0300, Roger Quadros wrote:
> 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(_lock);
> +udc = usb_gadget_to_udc(gadget);
> +if (!udc) {
> +dev_err(gadget->dev.parent, "%s: gadget not
> registered.\n",
> +__func__);
> +mutex_unlock(_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(_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
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On Fri, May 20, 2016 at 10:26:03AM +0300, Roger Quadros wrote:
> 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(_lock);
> +udc = usb_gadget_to_udc(gadget);
> +if (!udc) {
> +dev_err(gadget->dev.parent, "%s: gadget not
> registered.\n",
> +__func__);
> +mutex_unlock(_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(_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
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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.
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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.
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
+ udc = usb_gadget_to_udc(gadget);
+ if (!udc) {
+ dev_err(gadget->dev.parent, "%s: gadget not
registered.\n",
+ __func__);
+ mutex_unlock(_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(_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
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
+ udc = usb_gadget_to_udc(gadget);
+ if (!udc) {
+ dev_err(gadget->dev.parent, "%s: gadget not
registered.\n",
+ __func__);
+ mutex_unlock(_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(_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
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> >> + udc = usb_gadget_to_udc(gadget);
> >> + if (!udc) {
> >> + dev_err(gadget->dev.parent, "%s: gadget not
> >> registered.\n",
> >> + __func__);
> >> + mutex_unlock(_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(_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 (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?
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> >> + udc = usb_gadget_to_udc(gadget);
> >> + if (!udc) {
> >> + dev_err(gadget->dev.parent, "%s: gadget not
> >> registered.\n",
> >> + __func__);
> >> + mutex_unlock(_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(_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 (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?
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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.
>
> If you directly call to usb_udc_vbus_handler() in drd state machine
>
> Then:
>
> Hcd --> otg; and gadget <--> otg, (gadget and otg will refer to symbol of
> each other)
It's not so easy. We are again creating a circular dependency and all
build configurations don't work like I pointed above.
The only optimization I could do is make CONFIG_OTG tristate and
allow it to be built as 'm' if both hcd and gadget are 'm'.
i.e. case (2).
cheers,
-roger
>>
>> Do you have any ideas to solve that case?
>>
>> cheers,
>> -roger
>>
>
>>>
return 0;
@@ -660,9 +830,15 @@ static ssize_t
usb_udc_softconn_store(struct
device *dev,
return -EOPNOTSUPP;
}
+ /* In OTG mode we don't support softconnect, but
>> b_bus_req */
+ if (udc->gadget->otg_dev) {
+ dev_err(dev, "soft-connect not supported in OTG
>> mode\n");
+ return -EOPNOTSUPP;
+ }
+
>>>
>>> The soft-connect can be supported at dual-role mode currently,
>>> we can use b_bus_req entry once it is implemented later.
>>
>> Soft-connect should be done via sysfs handling within the OTG
>> core.
>> This can be added later. I don't want anything outside the OTG
>> core to handle soft-connect behaviour as it will be hard to
>> keep things in sync.
>>
>> I can update the comment to something like this.
>>
>> /* In OTG/dual-role mode, soft-connect should be handled by OTG
>> core */
>
> Ok, let's Felipe decide it.
>
>>
>>>
if (sysfs_streq(buf, "connect")) {
usb_gadget_udc_start(udc);
- usb_gadget_connect(udc->gadget);
+ usb_udc_connect_control(udc);
>>>
>>> This line seems to be not related with this patch.
>>>
>> Right. I'll remove it.
>>
>> cheers,
>> -roger
>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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.
>
> If you directly call to usb_udc_vbus_handler() in drd state machine
>
> Then:
>
> Hcd --> otg; and gadget <--> otg, (gadget and otg will refer to symbol of
> each other)
It's not so easy. We are again creating a circular dependency and all
build configurations don't work like I pointed above.
The only optimization I could do is make CONFIG_OTG tristate and
allow it to be built as 'm' if both hcd and gadget are 'm'.
i.e. case (2).
cheers,
-roger
>>
>> Do you have any ideas to solve that case?
>>
>> cheers,
>> -roger
>>
>
>>>
return 0;
@@ -660,9 +830,15 @@ static ssize_t
usb_udc_softconn_store(struct
device *dev,
return -EOPNOTSUPP;
}
+ /* In OTG mode we don't support softconnect, but
>> b_bus_req */
+ if (udc->gadget->otg_dev) {
+ dev_err(dev, "soft-connect not supported in OTG
>> mode\n");
+ return -EOPNOTSUPP;
+ }
+
>>>
>>> The soft-connect can be supported at dual-role mode currently,
>>> we can use b_bus_req entry once it is implemented later.
>>
>> Soft-connect should be done via sysfs handling within the OTG
>> core.
>> This can be added later. I don't want anything outside the OTG
>> core to handle soft-connect behaviour as it will be hard to
>> keep things in sync.
>>
>> I can update the comment to something like this.
>>
>> /* In OTG/dual-role mode, soft-connect should be handled by OTG
>> core */
>
> Ok, let's Felipe decide it.
>
>>
>>>
if (sysfs_streq(buf, "connect")) {
usb_gadget_udc_start(udc);
- usb_gadget_connect(udc->gadget);
+ usb_udc_connect_control(udc);
>>>
>>> This line seems to be not related with this patch.
>>>
>> Right. I'll remove it.
>>
>> cheers,
>> -roger
>
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> 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?
I think hcd and gadget are independent each other, now
Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol)
If you directly call to usb_udc_vbus_handler() in drd state machine
Then:
Hcd --> otg; and gadget <--> otg, (gadget and otg will refer to symbol of each
other)
Li Jun
>
> Do you have any ideas to solve that case?
>
> cheers,
> -roger
>
> >>
> >>>
> >
> >>return 0;
> >> @@ -660,9 +830,15 @@ static ssize_t
> >> usb_udc_softconn_store(struct
> >> device *dev,
> >>return -EOPNOTSUPP;
> >>}
> >>
> >> + /* In OTG mode we don't support softconnect, but
> b_bus_req */
> >> + if (udc->gadget->otg_dev) {
> >> + dev_err(dev, "soft-connect not supported in OTG
> mode\n");
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >
> > The soft-connect can be supported at dual-role mode currently,
> > we can use b_bus_req entry once it is implemented later.
>
> Soft-connect should be done via sysfs handling within the OTG
> core.
> This can be added later. I don't want anything outside the OTG
> core to handle soft-connect behaviour as it will be hard to
> keep things in sync.
>
> I can update the comment to something like this.
>
> /* In OTG/dual-role mode, soft-connect should be handled by OTG
> core */
> >>>
> >>> Ok, let's Felipe decide it.
> >>>
>
> >
> >>if (sysfs_streq(buf, "connect")) {
> >>usb_gadget_udc_start(udc);
> >> - usb_gadget_connect(udc->gadget);
> >> + usb_udc_connect_control(udc);
> >
> > This line seems to be not related with this patch.
> >
> Right. I'll remove it.
>
> cheers,
> -roger
> >>>
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> 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?
I think hcd and gadget are independent each other, now
Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol)
If you directly call to usb_udc_vbus_handler() in drd state machine
Then:
Hcd --> otg; and gadget <--> otg, (gadget and otg will refer to symbol of each
other)
Li Jun
>
> Do you have any ideas to solve that case?
>
> cheers,
> -roger
>
> >>
> >>>
> >
> >>return 0;
> >> @@ -660,9 +830,15 @@ static ssize_t
> >> usb_udc_softconn_store(struct
> >> device *dev,
> >>return -EOPNOTSUPP;
> >>}
> >>
> >> + /* In OTG mode we don't support softconnect, but
> b_bus_req */
> >> + if (udc->gadget->otg_dev) {
> >> + dev_err(dev, "soft-connect not supported in OTG
> mode\n");
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >
> > The soft-connect can be supported at dual-role mode currently,
> > we can use b_bus_req entry once it is implemented later.
>
> Soft-connect should be done via sysfs handling within the OTG
> core.
> This can be added later. I don't want anything outside the OTG
> core to handle soft-connect behaviour as it will be hard to
> keep things in sync.
>
> I can update the comment to something like this.
>
> /* In OTG/dual-role mode, soft-connect should be handled by OTG
> core */
> >>>
> >>> Ok, let's Felipe decide it.
> >>>
>
> >
> >>if (sysfs_streq(buf, "connect")) {
> >>usb_gadget_udc_start(udc);
> >> - usb_gadget_connect(udc->gadget);
> >> + usb_udc_connect_control(udc);
> >
> > This line seems to be not related with this patch.
> >
> Right. I'll remove it.
>
> cheers,
> -roger
> >>>
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Wednesday, May 18, 2016 8:43 PM
> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>
> On 17/05/16 11:28, Jun Li wrote:
> > Hi Roger,
> >
> >> -Original Message-
> >> From: Roger Quadros [mailto:rog...@ti.com]
> >> Sent: Tuesday, May 17, 2016 4:09 PM
> >> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
> >> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> >> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
> >> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
> >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> On 17/05/16 10:38, Jun Li wrote:
> >>> Hi
> >>>
> >>>> -Original Message-
> >>>> From: Roger Quadros [mailto:rog...@ti.com]
> >>>> Sent: Monday, May 16, 2016 5:52 PM
> >>>> To: Peter Chen <hzpeterc...@gmail.com>
> >>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> >>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
> >>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
> >>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
> >>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>>>
> >>>> 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(_lock);
> >>>>>>>> +udc = usb_gadget_to_udc(gadget);
> >>>>>>>> +if (!udc) {
> >>>>>>>> +dev_err(gadget->dev.parent, "%s: gadget not
> >>>> registered.\n",
> >>>>>>>> +__func__);
> >>>>>>>> +mutex_unlock(_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(_lock);
> >>>>>>>> +
> >>>>>>>> +return 0;
> >>&g
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Wednesday, May 18, 2016 8:43 PM
> To: Jun Li ; Peter Chen
> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>
> On 17/05/16 11:28, Jun Li wrote:
> > Hi Roger,
> >
> >> -Original Message-
> >> From: Roger Quadros [mailto:rog...@ti.com]
> >> Sent: Tuesday, May 17, 2016 4:09 PM
> >> To: Jun Li ; Peter Chen
> >> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> >> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
> >> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
> >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> On 17/05/16 10:38, Jun Li wrote:
> >>> Hi
> >>>
> >>>> -Original Message-
> >>>> From: Roger Quadros [mailto:rog...@ti.com]
> >>>> Sent: Monday, May 16, 2016 5:52 PM
> >>>> To: Peter Chen
> >>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> >>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
> >>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
> >>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
> >>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>>>
> >>>> 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(_lock);
> >>>>>>>> +udc = usb_gadget_to_udc(gadget);
> >>>>>>>> +if (!udc) {
> >>>>>>>> +dev_err(gadget->dev.parent, "%s: gadget not
> >>>> registered.\n",
> >>>>>>>> +__func__);
> >>>>>>>> +mutex_unlock(_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(_lock);
> >>>>>>>> +
> >>>>>>>> +return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>
> >>>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 18/05/16 16:12, Jun Li wrote:
> Hi
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Wednesday, May 18, 2016 8:43 PM
>> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 17/05/16 11:28, Jun Li wrote:
>>> Hi Roger,
>>>
>>>> -Original Message-
>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>> Sent: Tuesday, May 17, 2016 4:09 PM
>>>> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> On 17/05/16 10:38, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -Original Message-
>>>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>>>> Sent: Monday, May 16, 2016 5:52 PM
>>>>>> To: Peter Chen <hzpeterc...@gmail.com>
>>>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
>>>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>>>
>>>>>> 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(_lock);
>>>>>>>>>> +udc = usb_gadget_to_udc(gadget);
>>>>>>>>>> +if (!udc) {
>>>>>>>>>> +dev_err(gadget->dev.parent, "%s: gadget not
>>>>>> registered.\n",
>>>>>>>>>> +__func__);
>>>>>>>>>> +mutex_unlock(_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);
>>>>>>&
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 18/05/16 16:12, Jun Li wrote:
> Hi
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Wednesday, May 18, 2016 8:43 PM
>> To: Jun Li ; Peter Chen
>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 17/05/16 11:28, Jun Li wrote:
>>> Hi Roger,
>>>
>>>> -Original Message-
>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>> Sent: Tuesday, May 17, 2016 4:09 PM
>>>> To: Jun Li ; Peter Chen
>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> On 17/05/16 10:38, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -Original Message-
>>>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>>>> Sent: Monday, May 16, 2016 5:52 PM
>>>>>> To: Peter Chen
>>>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
>>>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>>>
>>>>>> 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(_lock);
>>>>>>>>>> +udc = usb_gadget_to_udc(gadget);
>>>>>>>>>> +if (!udc) {
>>>>>>>>>> +dev_err(gadget->dev.parent, "%s: gadget not
>>>>>> registered.\n",
>>>>>>>>>> +__func__);
>>>>>>>>>> +mutex_unlock(_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);
>>>>>>>>>> +}
>>>>>>>>>> +}
>>>>>>>>>> +
>>>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
>> +udc = usb_gadget_to_udc(gadget);
>> +if (!udc) {
>> +dev_err(gadget->dev.parent, "%s: gadget not
>> registered.\n",
>> +__func__);
>> +mutex_unlock(_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(_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.
cheers,
-roger
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
>> +udc = usb_gadget_to_udc(gadget);
>> +if (!udc) {
>> +dev_err(gadget->dev.parent, "%s: gadget not
>> registered.\n",
>> +__func__);
>> +mutex_unlock(_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(_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.
cheers,
-roger
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 17/05/16 11:28, Jun Li wrote:
> Hi Roger,
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Tuesday, May 17, 2016 4:09 PM
>> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 17/05/16 10:38, Jun Li wrote:
>>> Hi
>>>
>>>> -Original Message-
>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>> Sent: Monday, May 16, 2016 5:52 PM
>>>> To: Peter Chen <hzpeterc...@gmail.com>
>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> 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(_lock);
>>>>>>>> + udc = usb_gadget_to_udc(gadget);
>>>>>>>> + if (!udc) {
>>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not
>>>> registered.\n",
>>>>>>>> + __func__);
>>>>>>>> + mutex_unlock(_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(_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.
>>>>&
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 17/05/16 11:28, Jun Li wrote:
> Hi Roger,
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Tuesday, May 17, 2016 4:09 PM
>> To: Jun Li ; Peter Chen
>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 17/05/16 10:38, Jun Li wrote:
>>> Hi
>>>
>>>> -Original Message-
>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>> Sent: Monday, May 16, 2016 5:52 PM
>>>> To: Peter Chen
>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> 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(_lock);
>>>>>>>> + udc = usb_gadget_to_udc(gadget);
>>>>>>>> + if (!udc) {
>>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not
>>>> registered.\n",
>>>>>>>> + __func__);
>>>>>>>> + mutex_unlock(_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(_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 whe
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> +udc = usb_gadget_to_udc(gadget);
> +if (!udc) {
> +dev_err(gadget->dev.parent, "%s: gadget not
> registered.\n",
> +__func__);
> +mutex_unlock(_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(_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:
- Except for vbus interrupt, any chances this API will be used at
current logic?
- When this API is called but without a coming gadget->stop?
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> +udc = usb_gadget_to_udc(gadget);
> +if (!udc) {
> +dev_err(gadget->dev.parent, "%s: gadget not
> registered.\n",
> +__func__);
> +mutex_unlock(_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(_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:
- Except for vbus interrupt, any chances this API will be used at
current logic?
- When this API is called but without a coming gadget->stop?
--
Best Regards,
Peter Chen
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi Roger,
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Tuesday, May 17, 2016 4:09 PM
> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>
> On 17/05/16 10:38, Jun Li wrote:
> > Hi
> >
> >> -Original Message-
> >> From: Roger Quadros [mailto:rog...@ti.com]
> >> Sent: Monday, May 16, 2016 5:52 PM
> >> To: Peter Chen <hzpeterc...@gmail.com>
> >> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> >> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
> >> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
> >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> 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(_lock);
> >>>>>> + udc = usb_gadget_to_udc(gadget);
> >>>>>> + if (!udc) {
> >>>>>> + dev_err(gadget->dev.parent, "%s: gadget not
> >> registered.\n",
> >>>>>> + __func__);
> >>>>>> + mutex_unlock(_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(_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 se
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi Roger,
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Tuesday, May 17, 2016 4:09 PM
> To: Jun Li ; Peter Chen
> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>
> On 17/05/16 10:38, Jun Li wrote:
> > Hi
> >
> >> -Original Message-
> >> From: Roger Quadros [mailto:rog...@ti.com]
> >> Sent: Monday, May 16, 2016 5:52 PM
> >> To: Peter Chen
> >> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> >> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> >> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> >> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> >> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> >> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
> >> linux-...@vger.kernel.org; linux-o...@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; devicet...@vger.kernel.org
> >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> 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(_lock);
> >>>>>> + udc = usb_gadget_to_udc(gadget);
> >>>>>> + if (!udc) {
> >>>>>> + dev_err(gadget->dev.parent, "%s: gadget not
> >> registered.\n",
> >>>>>> + __func__);
> >>>>>> + mutex_unlock(_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(_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
> >>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 17/05/16 10:38, Jun Li wrote:
> Hi
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Monday, May 16, 2016 5:52 PM
>> To: Peter Chen <hzpeterc...@gmail.com>
>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> 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(_lock);
>>>>>> +udc = usb_gadget_to_udc(gadget);
>>>>>> +if (!udc) {
>>>>>> +dev_err(gadget->dev.parent, "%s: gadget not
>> registered.\n",
>>>>>> +__func__);
>>>>>> +mutex_unlock(_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(_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?
>
> Instead create those new interfaces/symbol here and there just aim to
> address build problems in diff configures, Could we only allow meaningful
> combination of those 3 drivers configures?
>
> Hcd=y, gadget=y, otg=y or
> Hcd=m, gadget=m, otg=m
This is still a limitation.
It is perfectly fine to have
hcd=m, gadget=y
or
hcd=y, gadget=m
cheers,
-roger
>
> Li Jun
>
>>
>> cheers,
>> -roger
>>
>>>
>>>>>
>>>>>> return 0;
>>>>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct
>> device *dev,
>>>>>> return -EOPNOTSUPP;
>>>>>> }
>>>>>>
>>>>>> +/* In OTG mode we don't support softconnect, but b_bus_req */
>>>>>> +if (udc->gadget->otg_dev) {
>>>>>> +dev_err(dev, "soft-connect not supported in OTG
>>>>>> mode\n");
>>>>>> +return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> The soft-connect can be supported at dual-role mode currently, we
>>>>> can use b_bus_req entry once it is implemented later.
>>>>
>>>> Soft-connect should be done via sysfs handling within the OTG core.
>>>> This can be added later. I don't want anything outside the OTG core
>>>> to handle soft-connect behaviour as it will be hard to keep things in
>>>> sync.
>>>>
>>>> I can update the comment to something like this.
>>>>
>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG core
>>>> */
>>>
>>> Ok, let's Felipe decide it.
>>>
>>>>
>>>>>
>>>>>> if (sysfs_streq(buf, "connect")) {
>>>>>> usb_gadget_udc_start(udc);
>>>>>> -usb_gadget_connect(udc->gadget);
>>>>>> +usb_udc_connect_control(udc);
>>>>>
>>>>> This line seems to be not related with this patch.
>>>>>
>>>> Right. I'll remove it.
>>>>
>>>> cheers,
>>>> -roger
>>>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
On 17/05/16 10:38, Jun Li wrote:
> Hi
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Monday, May 16, 2016 5:52 PM
>> To: Peter Chen
>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicet...@vger.kernel.org
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> 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(_lock);
>>>>>> +udc = usb_gadget_to_udc(gadget);
>>>>>> +if (!udc) {
>>>>>> +dev_err(gadget->dev.parent, "%s: gadget not
>> registered.\n",
>>>>>> +__func__);
>>>>>> +mutex_unlock(_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(_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?
>
> Instead create those new interfaces/symbol here and there just aim to
> address build problems in diff configures, Could we only allow meaningful
> combination of those 3 drivers configures?
>
> Hcd=y, gadget=y, otg=y or
> Hcd=m, gadget=m, otg=m
This is still a limitation.
It is perfectly fine to have
hcd=m, gadget=y
or
hcd=y, gadget=m
cheers,
-roger
>
> Li Jun
>
>>
>> cheers,
>> -roger
>>
>>>
>>>>>
>>>>>> return 0;
>>>>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct
>> device *dev,
>>>>>> return -EOPNOTSUPP;
>>>>>> }
>>>>>>
>>>>>> +/* In OTG mode we don't support softconnect, but b_bus_req */
>>>>>> +if (udc->gadget->otg_dev) {
>>>>>> +dev_err(dev, "soft-connect not supported in OTG
>>>>>> mode\n");
>>>>>> +return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> The soft-connect can be supported at dual-role mode currently, we
>>>>> can use b_bus_req entry once it is implemented later.
>>>>
>>>> Soft-connect should be done via sysfs handling within the OTG core.
>>>> This can be added later. I don't want anything outside the OTG core
>>>> to handle soft-connect behaviour as it will be hard to keep things in
>>>> sync.
>>>>
>>>> I can update the comment to something like this.
>>>>
>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG core
>>>> */
>>>
>>> Ok, let's Felipe decide it.
>>>
>>>>
>>>>>
>>>>>> if (sysfs_streq(buf, "connect")) {
>>>>>> usb_gadget_udc_start(udc);
>>>>>> -usb_gadget_connect(udc->gadget);
>>>>>> +usb_udc_connect_control(udc);
>>>>>
>>>>> This line seems to be not related with this patch.
>>>>>
>>>> Right. I'll remove it.
>>>>
>>>> cheers,
>>>> -roger
>>>
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Monday, May 16, 2016 5:52 PM
> To: Peter Chen <hzpeterc...@gmail.com>
> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>
> 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(_lock);
> >>>> +udc = usb_gadget_to_udc(gadget);
> >>>> +if (!udc) {
> >>>> +dev_err(gadget->dev.parent, "%s: gadget not
> registered.\n",
> >>>> +__func__);
> >>>> +mutex_unlock(_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(_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?
Instead create those new interfaces/symbol here and there just aim to
address build problems in diff configures, Could we only allow meaningful
combination of those 3 drivers configures?
Hcd=y, gadget=y, otg=y or
Hcd=m, gadget=m, otg=m
Li Jun
>
> cheers,
> -roger
>
> >
> >>>
> >>>> return 0;
> >>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct
> device *dev,
> >>>> return -EOPNOTSUPP;
> >>>> }
> >>>>
> >>>> +/* In OTG mode we don't support softconnect, but b_bus_req */
> >>>> +if (udc->gadget->otg_dev) {
> >>>> +dev_err(dev, "soft-connect not supported in OTG
> >>>> mode\n");
> >>>> +return -EOPNOTSUPP;
> >>>> +}
> >>>> +
> >>>
> >>> The soft-connect can be supported at dual-role mode currently, we
> >>> can use b_bus_req entry once it is implemented later.
> >>
> >> Soft-connect should be done via sysfs handling within the OTG core.
> >> This can be added later. I don't want anything outside the OTG core
> >> to handle soft-connect behaviour as it will be hard to keep things in
> >> sync.
> >>
> >> I can update the comment to something like this.
> >>
> >> /* In OTG/dual-role mode, soft-connect should be handled by OTG core
> >> */
> >
> > Ok, let's Felipe decide it.
> >
> >>
> >>>
> >>>> if (sysfs_streq(buf, "connect")) {
> >>>> usb_gadget_udc_start(udc);
> >>>> -usb_gadget_connect(udc->gadget);
> >>>> +usb_udc_connect_control(udc);
> >>>
> >>> This line seems to be not related with this patch.
> >>>
> >> Right. I'll remove it.
> >>
> >> cheers,
> >> -roger
> >
RE: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
Hi
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Monday, May 16, 2016 5:52 PM
> To: Peter Chen
> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>
> 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(_lock);
> >>>> +udc = usb_gadget_to_udc(gadget);
> >>>> +if (!udc) {
> >>>> +dev_err(gadget->dev.parent, "%s: gadget not
> registered.\n",
> >>>> +__func__);
> >>>> +mutex_unlock(_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(_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?
Instead create those new interfaces/symbol here and there just aim to
address build problems in diff configures, Could we only allow meaningful
combination of those 3 drivers configures?
Hcd=y, gadget=y, otg=y or
Hcd=m, gadget=m, otg=m
Li Jun
>
> cheers,
> -roger
>
> >
> >>>
> >>>> return 0;
> >>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct
> device *dev,
> >>>> return -EOPNOTSUPP;
> >>>> }
> >>>>
> >>>> +/* In OTG mode we don't support softconnect, but b_bus_req */
> >>>> +if (udc->gadget->otg_dev) {
> >>>> +dev_err(dev, "soft-connect not supported in OTG
> >>>> mode\n");
> >>>> +return -EOPNOTSUPP;
> >>>> +}
> >>>> +
> >>>
> >>> The soft-connect can be supported at dual-role mode currently, we
> >>> can use b_bus_req entry once it is implemented later.
> >>
> >> Soft-connect should be done via sysfs handling within the OTG core.
> >> This can be added later. I don't want anything outside the OTG core
> >> to handle soft-connect behaviour as it will be hard to keep things in
> >> sync.
> >>
> >> I can update the comment to something like this.
> >>
> >> /* In OTG/dual-role mode, soft-connect should be handled by OTG core
> >> */
> >
> > Ok, let's Felipe decide it.
> >
> >>
> >>>
> >>>> if (sysfs_streq(buf, "connect")) {
> >>>> usb_gadget_udc_start(udc);
> >>>> -usb_gadget_connect(udc->gadget);
> >>>> +usb_udc_connect_control(udc);
> >>>
> >>> This line seems to be not related with this patch.
> >>>
> >> Right. I'll remove it.
> >>
> >> cheers,
> >> -roger
> >
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
+ udc = usb_gadget_to_udc(gadget);
+ if (!udc) {
+ dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+ __func__);
+ mutex_unlock(_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(_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?
cheers,
-roger
>
>>>
return 0;
@@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
*dev,
return -EOPNOTSUPP;
}
+ /* In OTG mode we don't support softconnect, but b_bus_req */
+ if (udc->gadget->otg_dev) {
+ dev_err(dev, "soft-connect not supported in OTG mode\n");
+ return -EOPNOTSUPP;
+ }
+
>>>
>>> The soft-connect can be supported at dual-role mode currently, we can
>>> use b_bus_req entry once it is implemented later.
>>
>> Soft-connect should be done via sysfs handling within the OTG core.
>> This can be added later. I don't want anything outside the OTG core
>> to handle soft-connect behaviour as it will be hard to keep things
>> in sync.
>>
>> I can update the comment to something like this.
>>
>> /* In OTG/dual-role mode, soft-connect should be handled by OTG core */
>
> Ok, let's Felipe decide it.
>
>>
>>>
if (sysfs_streq(buf, "connect")) {
usb_gadget_udc_start(udc);
- usb_gadget_connect(udc->gadget);
+ usb_udc_connect_control(udc);
>>>
>>> This line seems to be not related with this patch.
>>>
>> Right. I'll remove it.
>>
>> cheers,
>> -roger
>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
+ udc = usb_gadget_to_udc(gadget);
+ if (!udc) {
+ dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+ __func__);
+ mutex_unlock(_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(_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?
cheers,
-roger
>
>>>
return 0;
@@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
*dev,
return -EOPNOTSUPP;
}
+ /* In OTG mode we don't support softconnect, but b_bus_req */
+ if (udc->gadget->otg_dev) {
+ dev_err(dev, "soft-connect not supported in OTG mode\n");
+ return -EOPNOTSUPP;
+ }
+
>>>
>>> The soft-connect can be supported at dual-role mode currently, we can
>>> use b_bus_req entry once it is implemented later.
>>
>> Soft-connect should be done via sysfs handling within the OTG core.
>> This can be added later. I don't want anything outside the OTG core
>> to handle soft-connect behaviour as it will be hard to keep things
>> in sync.
>>
>> I can update the comment to something like this.
>>
>> /* In OTG/dual-role mode, soft-connect should be handled by OTG core */
>
> Ok, let's Felipe decide it.
>
>>
>>>
if (sysfs_streq(buf, "connect")) {
usb_gadget_udc_start(udc);
- usb_gadget_connect(udc->gadget);
+ usb_udc_connect_control(udc);
>>>
>>> This line seems to be not related with this patch.
>>>
>> Right. I'll remove it.
>>
>> cheers,
>> -roger
>
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> >> + udc = usb_gadget_to_udc(gadget);
> >> + if (!udc) {
> >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >> + __func__);
> >> + mutex_unlock(_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(_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.
> >
> >>return 0;
> >> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
> >> *dev,
> >>return -EOPNOTSUPP;
> >>}
> >>
> >> + /* In OTG mode we don't support softconnect, but b_bus_req */
> >> + if (udc->gadget->otg_dev) {
> >> + dev_err(dev, "soft-connect not supported in OTG mode\n");
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >
> > The soft-connect can be supported at dual-role mode currently, we can
> > use b_bus_req entry once it is implemented later.
>
> Soft-connect should be done via sysfs handling within the OTG core.
> This can be added later. I don't want anything outside the OTG core
> to handle soft-connect behaviour as it will be hard to keep things
> in sync.
>
> I can update the comment to something like this.
>
> /* In OTG/dual-role mode, soft-connect should be handled by OTG core */
Ok, let's Felipe decide it.
>
> >
> >>if (sysfs_streq(buf, "connect")) {
> >>usb_gadget_udc_start(udc);
> >> - usb_gadget_connect(udc->gadget);
> >> + usb_udc_connect_control(udc);
> >
> > This line seems to be not related with this patch.
> >
> Right. I'll remove it.
>
> cheers,
> -roger
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> >> + udc = usb_gadget_to_udc(gadget);
> >> + if (!udc) {
> >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >> + __func__);
> >> + mutex_unlock(_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(_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.
> >
> >>return 0;
> >> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
> >> *dev,
> >>return -EOPNOTSUPP;
> >>}
> >>
> >> + /* In OTG mode we don't support softconnect, but b_bus_req */
> >> + if (udc->gadget->otg_dev) {
> >> + dev_err(dev, "soft-connect not supported in OTG mode\n");
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >
> > The soft-connect can be supported at dual-role mode currently, we can
> > use b_bus_req entry once it is implemented later.
>
> Soft-connect should be done via sysfs handling within the OTG core.
> This can be added later. I don't want anything outside the OTG core
> to handle soft-connect behaviour as it will be hard to keep things
> in sync.
>
> I can update the comment to something like this.
>
> /* In OTG/dual-role mode, soft-connect should be handled by OTG core */
Ok, let's Felipe decide it.
>
> >
> >>if (sysfs_streq(buf, "connect")) {
> >>usb_gadget_udc_start(udc);
> >> - usb_gadget_connect(udc->gadget);
> >> + usb_udc_connect_control(udc);
> >
> > This line seems to be not related with this patch.
> >
> Right. I'll remove it.
>
> cheers,
> -roger
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
>> +udc = usb_gadget_to_udc(gadget);
>> +if (!udc) {
>> +dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>> +__func__);
>> +mutex_unlock(_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(_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.
>
>> return 0;
>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
>> *dev,
>> return -EOPNOTSUPP;
>> }
>>
>> +/* In OTG mode we don't support softconnect, but b_bus_req */
>> +if (udc->gadget->otg_dev) {
>> +dev_err(dev, "soft-connect not supported in OTG mode\n");
>> +return -EOPNOTSUPP;
>> +}
>> +
>
> The soft-connect can be supported at dual-role mode currently, we can
> use b_bus_req entry once it is implemented later.
Soft-connect should be done via sysfs handling within the OTG core.
This can be added later. I don't want anything outside the OTG core
to handle soft-connect behaviour as it will be hard to keep things
in sync.
I can update the comment to something like this.
/* In OTG/dual-role mode, soft-connect should be handled by OTG core */
>
>> if (sysfs_streq(buf, "connect")) {
>> usb_gadget_udc_start(udc);
>> -usb_gadget_connect(udc->gadget);
>> +usb_udc_connect_control(udc);
>
> This line seems to be not related with this patch.
>
Right. I'll remove it.
cheers,
-roger
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
>> +udc = usb_gadget_to_udc(gadget);
>> +if (!udc) {
>> +dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>> +__func__);
>> +mutex_unlock(_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(_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.
>
>> return 0;
>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device
>> *dev,
>> return -EOPNOTSUPP;
>> }
>>
>> +/* In OTG mode we don't support softconnect, but b_bus_req */
>> +if (udc->gadget->otg_dev) {
>> +dev_err(dev, "soft-connect not supported in OTG mode\n");
>> +return -EOPNOTSUPP;
>> +}
>> +
>
> The soft-connect can be supported at dual-role mode currently, we can
> use b_bus_req entry once it is implemented later.
Soft-connect should be done via sysfs handling within the OTG core.
This can be added later. I don't want anything outside the OTG core
to handle soft-connect behaviour as it will be hard to keep things
in sync.
I can update the comment to something like this.
/* In OTG/dual-role mode, soft-connect should be handled by OTG core */
>
>> if (sysfs_streq(buf, "connect")) {
>> usb_gadget_udc_start(udc);
>> -usb_gadget_connect(udc->gadget);
>> +usb_udc_connect_control(udc);
>
> This line seems to be not related with this patch.
>
Right. I'll remove it.
cheers,
-roger
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> + udc = usb_gadget_to_udc(gadget);
> + if (!udc) {
> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> + __func__);
> + mutex_unlock(_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(_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.
> return 0;
> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> + /* In OTG mode we don't support softconnect, but b_bus_req */
> + if (udc->gadget->otg_dev) {
> + dev_err(dev, "soft-connect not supported in OTG mode\n");
> + return -EOPNOTSUPP;
> + }
> +
The soft-connect can be supported at dual-role mode currently, we can
use b_bus_req entry once it is implemented later.
> if (sysfs_streq(buf, "connect")) {
> usb_gadget_udc_start(udc);
> - usb_gadget_connect(udc->gadget);
> + usb_udc_connect_control(udc);
This line seems to be not related with this patch.
--
Best Regards,
Peter Chen
Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
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(_lock);
> + udc = usb_gadget_to_udc(gadget);
> + if (!udc) {
> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> + __func__);
> + mutex_unlock(_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(_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.
> return 0;
> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
> return -EOPNOTSUPP;
> }
>
> + /* In OTG mode we don't support softconnect, but b_bus_req */
> + if (udc->gadget->otg_dev) {
> + dev_err(dev, "soft-connect not supported in OTG mode\n");
> + return -EOPNOTSUPP;
> + }
> +
The soft-connect can be supported at dual-role mode currently, we can
use b_bus_req entry once it is implemented later.
> if (sysfs_streq(buf, "connect")) {
> usb_gadget_udc_start(udc);
> - usb_gadget_connect(udc->gadget);
> + usb_udc_connect_control(udc);
This line seems to be not related with this patch.
--
Best Regards,
Peter Chen
