Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-13 Thread Roger Quadros 
On 12/06/16 14:21, Peter Chen wrote:
> On Fri, Jun 10, 2016 at 04:07:17PM +0300, Roger Quadros wrote:
>> index dca7856..03f7204 100644
>> --- a/drivers/usb/Makefile
>> +++ b/drivers/usb/Makefile
>> @@ -59,5 +59,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)+= renesas_usbhs/
>>  obj-$(CONFIG_USB_GADGET)+= gadget/
>>  
>>  obj-$(CONFIG_USB_COMMON)+= common/
>> +obj-$(CONFIG_USB_OTG_CORE)  += common/
> 
> I don't think you need to make above change, why you do it?

You are right. I no longer get undefined symbol errors during build.
I'll remove this change.
> 
>> +
>> +/**
>> + * usb_otg_get_data() - get usb_otg data structa
> 
> %s/structa/structure

OK.
> 
>> +
>> +/**
>> + * usb_otg_kick_fsm() - Kick the OTG state machine
>> + * @otg_dev:OTG controller device
>> + *
>> + * Used by USB host/gadget stack to sync OTG related
>> + * events to the OTG state machine.
>> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
>> + *
>> + * Return: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_kick_fsm(struct device *otg_dev)
>> +{
>> +struct usb_otg *otg;
>> +
>> +mutex_lock(_list_mutex);
>> +otg = usb_otg_get_data(otg_dev);
>> +mutex_unlock(_list_mutex);
>> +if (!otg) {
>> +dev_dbg(otg_dev, "otg: %s: invalid otg device\n",
>> +__func__);
>> +return -ENODEV;
>> +}
>> +
>> +usb_otg_sync_inputs(otg);
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
> 
> Do you have any users for this API? If no, please delete it in this
> version.

I'll remove it.
> 
>> +
>> +/**
>> + * usb_otg_register_hcd() - Register the host controller to OTG core
>> + * @hcd:host controller
>> + * @irqnum: interrupt number
>> + * @irqflags:   interrupt flags
>> + * @ops:HCD ops to interface with the HCD
>> + *
>> + * This is used by the USB Host stack to register the host controller
>> + * to the OTG core. Host controller must not be started by the
>> + * caller as it is left upto the OTG state machine to do so.
> 
> %s/upto/up to
> 
>> +
>> +/**
>> + * usb_otg_register_gadget() - Register the gadget controller to OTG core
>> + * @gadget: gadget controller instance
>> + * @ops:gadget interface ops
>> + *
>> + * This is used by the USB gadget stack to register the gadget controller
>> + * to the OTG core. Gadget controller must not be started by the
>> + * caller as it is left upto the OTG state machine to do so.
>> + *
> 
> %s/upto/up to
> 

OK for both.

--
cheers,
-roger
--
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

Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-12 Thread Peter Chen 
On Fri, Jun 10, 2016 at 04:07:17PM +0300, Roger Quadros wrote:
> index dca7856..03f7204 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -59,5 +59,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
>  obj-$(CONFIG_USB_GADGET) += gadget/
>  
>  obj-$(CONFIG_USB_COMMON) += common/
> +obj-$(CONFIG_USB_OTG_CORE)   += common/

I don't think you need to make above change, why you do it?

> +
> +/**
> + * usb_otg_get_data() - get usb_otg data structa

%s/structa/structure

> +
> +/**
> + * usb_otg_kick_fsm() - Kick the OTG state machine
> + * @otg_dev: OTG controller device
> + *
> + * Used by USB host/gadget stack to sync OTG related
> + * events to the OTG state machine.
> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_kick_fsm(struct device *otg_dev)
> +{
> + struct usb_otg *otg;
> +
> + mutex_lock(_list_mutex);
> + otg = usb_otg_get_data(otg_dev);
> + mutex_unlock(_list_mutex);
> + if (!otg) {
> + dev_dbg(otg_dev, "otg: %s: invalid otg device\n",
> + __func__);
> + return -ENODEV;
> + }
> +
> + usb_otg_sync_inputs(otg);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);

Do you have any users for this API? If no, please delete it in this
version.

> +
> +/**
> + * usb_otg_register_hcd() - Register the host controller to OTG core
> + * @hcd: host controller
> + * @irqnum:  interrupt number
> + * @irqflags:interrupt flags
> + * @ops: HCD ops to interface with the HCD
> + *
> + * This is used by the USB Host stack to register the host controller
> + * to the OTG core. Host controller must not be started by the
> + * caller as it is left upto the OTG state machine to do so.

%s/upto/up to

> +
> +/**
> + * usb_otg_register_gadget() - Register the gadget controller to OTG core
> + * @gadget:  gadget controller instance
> + * @ops: gadget interface ops
> + *
> + * This is used by the USB gadget stack to register the gadget controller
> + * to the OTG core. Gadget controller must not be started by the
> + * caller as it is left upto the OTG state machine to do so.
> + *

%s/upto/up to

-- 

Best Regards,
Peter Chen
--
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

Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-10 Thread Felipe Balbi 

Hi,

Roger Quadros  writes:
>>> What is wrong with calling it gadget controller?
>> 
>>It's not a controller, it's a piece of software AFAIU. Or is my English 
>> just too weak? :-)
>
> Everything at this point is a piece of software :).
>
> struct usb_gadget, represents the gadget controller device not the
> driver (or software).
>
> struct usb_gadget_driver represents the gadget function driver.
> struct usb_gadget_ops represents the UDC driver ops.

right, and usb_udc was added just to have a way of linking usb_gadget
with usb_gadget_driver so we could have more than one peripheral port in
a system.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-10 Thread Roger Quadros 
On 10/06/16 13:44, Sergei Shtylyov wrote:
> On 6/10/2016 1:19 PM, Roger Quadros wrote:
> 
 It provides APIs for the following tasks

 - Registering an OTG/dual-role capable controller
 - Registering Host and Gadget controllers to OTG core
 - Providing inputs to and kicking the OTG state machine

 Provide a dual-role device (DRD) state machine.
 DRD mode is a reduced functionality OTG mode. In this mode
 we don't support SRP, HNP and dynamic role-swap.

 In DRD operation, the controller mode (Host or Peripheral)
 is decided based on the ID pin status. Once a cable plug (Type-A
 or Type-B) is attached the controller selects the state
 and doesn't change till the cable in unplugged and a different
 cable type is inserted.

 As we don't need most of the complex OTG states and OTG timers
 we implement a lean DRD state machine in usb-otg.c.
 The DRD state machine is only interested in 2 hardware inputs
 'id' and 'b_sess_vld'.

 Signed-off-by: Roger Quadros 
>>>
>>
>> 
>>
>>> [...]
 +/**
 + * usb_otg_register_gadget - Register the gadget controller to OTG core
 + * @gadget:gadget controller
>>>
>>>We call that USB device controller (UDC). I'm not sure what you meant 
>>> here...
>>>And what about the 2nd arg, 'ops'?
>>
>> There are 2 data structures representing the Device controller.
>>
>> struct usb_gadget - represents a usb slave device
>> struct usb_udc -struct usb_udc - describes one usb device controller
>>
>> usb_udc is for private use only. usb_otg_register_gadget() takes struct 
>> usb_gadget
>> as argument.
>>
>> Do you want me to refer to struct usb_gadget as UDC?
> 
>No.
> 
>> What is wrong with calling it gadget controller?
> 
>It's not a controller, it's a piece of software AFAIU. Or is my English 
> just too weak? :-)

Everything at this point is a piece of software :).

struct usb_gadget, represents the gadget controller device not the driver (or 
software).

struct usb_gadget_driver represents the gadget function driver.
struct usb_gadget_ops represents the UDC driver ops.

--
cheers,
-roger
--
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

Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-10 Thread Sergei Shtylyov 

On 6/10/2016 1:19 PM, Roger Quadros wrote:


It provides APIs for the following tasks

- Registering an OTG/dual-role capable controller
- Registering Host and Gadget controllers to OTG core
- Providing inputs to and kicking the OTG state machine

Provide a dual-role device (DRD) state machine.
DRD mode is a reduced functionality OTG mode. In this mode
we don't support SRP, HNP and dynamic role-swap.

In DRD operation, the controller mode (Host or Peripheral)
is decided based on the ID pin status. Once a cable plug (Type-A
or Type-B) is attached the controller selects the state
and doesn't change till the cable in unplugged and a different
cable type is inserted.

As we don't need most of the complex OTG states and OTG timers
we implement a lean DRD state machine in usb-otg.c.
The DRD state machine is only interested in 2 hardware inputs
'id' and 'b_sess_vld'.

Signed-off-by: Roger Quadros 







[...]

+/**
+ * usb_otg_register_gadget - Register the gadget controller to OTG core
+ * @gadget:gadget controller


   We call that USB device controller (UDC). I'm not sure what you meant here...
   And what about the 2nd arg, 'ops'?


There are 2 data structures representing the Device controller.

struct usb_gadget - represents a usb slave device
struct usb_udc -struct usb_udc - describes one usb device controller

usb_udc is for private use only. usb_otg_register_gadget() takes struct 
usb_gadget
as argument.

Do you want me to refer to struct usb_gadget as UDC?


   No.


What is wrong with calling it gadget controller?


   It's not a controller, it's a piece of software AFAIU. Or is my English 
just too weak? :-)



--
cheers,
-roger


MBR, Sergei

--
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

Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-10 Thread Roger Quadros 
Hi,

On 09/06/16 15:34, Sergei Shtylyov wrote:
> On 6/9/2016 10:53 AM, Roger Quadros wrote:
> 
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> Provide a dual-role device (DRD) state machine.
>> DRD mode is a reduced functionality OTG mode. In this mode
>> we don't support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral)
>> is decided based on the ID pin status. Once a cable plug (Type-A
>> or Type-B) is attached the controller selects the state
>> and doesn't change till the cable in unplugged and a different
>> cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers
>> we implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs
>> 'id' and 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros 
> 



> [...]
>> +/**
>> + * usb_otg_register_gadget - Register the gadget controller to OTG core
>> + * @gadget:gadget controller
> 
>We call that USB device controller (UDC). I'm not sure what you meant 
> here...
>And what about the 2nd arg, 'ops'?

There are 2 data structures representing the Device controller.

struct usb_gadget - represents a usb slave device
struct usb_udc -struct usb_udc - describes one usb device controller

usb_udc is for private use only. usb_otg_register_gadget() takes struct 
usb_gadget
as argument.

Do you want me to refer to struct usb_gadget as UDC?

What is wrong with calling it gadget controller?

--
cheers,
-roger
--
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

Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-10 Thread Roger Quadros 
Hi Sergei,

On 09/06/16 15:34, Sergei Shtylyov wrote:
> On 6/9/2016 10:53 AM, Roger Quadros wrote:
> 
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> Provide a dual-role device (DRD) state machine.
>> DRD mode is a reduced functionality OTG mode. In this mode
>> we don't support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral)
>> is decided based on the ID pin status. Once a cable plug (Type-A
>> or Type-B) is attached the controller selects the state
>> and doesn't change till the cable in unplugged and a different
>> cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers
>> we implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs
>> 'id' and 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros 
> 
> [...]
> 
>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>> new file mode 100644
>> index 000..baebe5c
>> --- /dev/null
>> +++ b/drivers/usb/common/usb-otg.c
>> @@ -0,0 +1,833 @@
> [...]
>> +/**
>> + * Change USB protocol when there is a protocol change.
>> + * fsm->lock must be held.
>> + */
> 
>If you're using the kernel-doc comment, please follow the rules.
> 

All your comments are valid and I'll fix the issues.



> [...]
> 
>Phew, that was a long patch... normally I don't review the patches that 
> are such big. :-)

Thanks for the patience and review :)

--
cheers,
-roger
--
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

Re: [PATCH v10 08/14] usb: otg: add OTG/dual-role core

2016-06-09 Thread Sergei Shtylyov 

On 6/9/2016 10:53 AM, Roger Quadros wrote:


It provides APIs for the following tasks

- Registering an OTG/dual-role capable controller
- Registering Host and Gadget controllers to OTG core
- Providing inputs to and kicking the OTG state machine

Provide a dual-role device (DRD) state machine.
DRD mode is a reduced functionality OTG mode. In this mode
we don't support SRP, HNP and dynamic role-swap.

In DRD operation, the controller mode (Host or Peripheral)
is decided based on the ID pin status. Once a cable plug (Type-A
or Type-B) is attached the controller selects the state
and doesn't change till the cable in unplugged and a different
cable type is inserted.

As we don't need most of the complex OTG states and OTG timers
we implement a lean DRD state machine in usb-otg.c.
The DRD state machine is only interested in 2 hardware inputs
'id' and 'b_sess_vld'.

Signed-off-by: Roger Quadros 


[...]


diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
new file mode 100644
index 000..baebe5c
--- /dev/null
+++ b/drivers/usb/common/usb-otg.c
@@ -0,0 +1,833 @@

[...]

+/**
+ * Change USB protocol when there is a protocol change.
+ * fsm->lock must be held.
+ */


   If you're using the kernel-doc comment, please follow the rules.


+static int drd_set_protocol(struct otg_fsm *fsm, int protocol)

[...]

+/**
+ * Called when entering a DRD state.
+ * fsm->lock must be held.
+ */


   Same here.


+static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)

[...]

+/**
+ * DRD state change judgement
+ *
+ * For DRD we're only interested in some of the OTG states
+ * i.e. OTG_STATE_B_IDLE: both peripheral and host are stopped
+ * OTG_STATE_B_PERIPHERAL: peripheral active
+ * OTG_STATE_A_HOST: host active
+ * we're only interested in the following inputs
+ * fsm->id, fsm->b_sess_vld
+ */


   And here.


+/**
+ * Dual-role device (DRD) work function
+ */


   And here.


+static void usb_drd_work(struct work_struct *work)
+{
+   struct usb_otg *otg = container_of(work, struct usb_otg, work);
+
+   pm_runtime_get_sync(otg->dev);
+   while (drd_statemachine(otg))
+   ;


   Indent it more please.

[...]

+/**
+ * start/kick the OTG FSM if we can
+ * fsm->lock must be held
+ */


   Please follow the kernel-doc rules.

[...]

+/**
+ * stop the OTG FSM. Stops Host & Gadget controllers as well.
+ * fsm->lock must be held
+ */


   Likewise.

[...]

+/**
+ * usb_otg_sync_inputs - Sync OTG inputs with the OTG state machine
+ * @fsm:   OTG FSM instance


   Doesn't match the prototype.


+ *
+ * Used by the OTG driver to update the inputs to the OTG
+ * state machine.
+ *
+ * Can be called in IRQ context.
+ */
+void usb_otg_sync_inputs(struct usb_otg *otg)

[...]

+/**
+ * usb_otg_register_gadget - Register the gadget controller to OTG core
+ * @gadget:gadget controller


   We call that USB device controller (UDC). I'm not sure what you meant here...
   And what about the 2nd arg, 'ops'?

[...]

diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index 85b8fb5..5d4850a 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -10,10 +10,68 @@
 #define __LINUX_USB_OTG_H

 #include 
-#include 
-#include 
+#include 
+#include 
 #include 
+#include 
+#include 
+#include 
+
+/**
+ * struct otg_hcd - host controller state and interface
+ *
+ * @hcd: host controller
+ * @irqnum: irq number
+ * @irqflags: irq flags


   IRQ?


+ * @ops: otg to host controller interface
+ * @ops: otg to host controller interface


   Once is enough. :-)


+ * @otg_dev: otg controller device


   OTG?


+/**
+ * struct usb_otg - usb otg controller state
+ *
+ * @default_a: Indicates we are an A device. i.e. Host.
+ * @phy: USB phy interface


   PHY?


+ * @usb_phy: old usb_phy interface
+ * @host: host controller bus
+ * @gadget: gadget device
+ * @state: current otg state
+ * @dev: otg controller device
+ * @caps: otg capabilities revision, hnp, srp, etc
+ * @fsm: otg finite state machine


   OTG?


+ * @hcd_ops: host controller interface
+ * --- internal use only ---
+ * @primary_hcd: primary host state and interface
+ * @shared_hcd: shared host state and interface
+ * @gadget_ops: gadget controller interface
+ * @list: list of otg controllers
+ * @work: otg state machine work
+ * @wq: otg state machine work queue
+ * @flags: to track if host/gadget is running
+ */
 struct usb_otg {
u8  default_a;


[...]

@@ -42,26 +116,101 @@ struct usb_otg {

/* start or continue HNP role switch */
int (*start_hnp)(struct usb_otg *otg);
-
+/*---*/
 };

 /**
- * struct usb_otg_caps - describes the otg capabilities of the device
- * @otg_rev: The OTG revision number the device is compliant with, it's
- * in binary-coded decimal (i.e. 2.0 is 0200H).
- * @hnp_support: Indicates if the device supports HNP.
- *