Re: [PATCH v2 1/6] usb: otg-fsm: add HNP polling operation function.

2014-02-19 Thread Felipe Balbi 
Hi,

On Wed, Feb 19, 2014 at 05:48:49AM +, Peter Chen wrote:
  On Mon, Jan 20, 2014 at 10:00:15AM +0800, Li Jun wrote:
   This patch adds HNP polling operation function for OTG fsm.
  
   Signed-off-by: Li Jun b47...@freescale.com
   ---
drivers/usb/phy/phy-fsm-usb.c |2 ++
include/linux/usb/otg-fsm.h   |9 +
2 files changed, 11 insertions(+), 0 deletions(-)
  
   diff --git a/drivers/usb/phy/phy-fsm-usb.c
   b/drivers/usb/phy/phy-fsm-usb.c index c47e5a6..ef91961 100644
   --- a/drivers/usb/phy/phy-fsm-usb.c
   +++ b/drivers/usb/phy/phy-fsm-usb.c
   @@ -169,6 +169,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum
  usb_otg_state new_state)
 otg_set_protocol(fsm, PROTO_HOST);
 usb_bus_start_enum(fsm-otg-host,
 fsm-otg-host-otg_port);
   + otg_start_hnp_polling(fsm);
 break;
 case OTG_STATE_A_IDLE:
 otg_drv_vbus(fsm, 0);
   @@ -203,6 +204,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum
  usb_otg_state new_state)
  */
 if (!fsm-a_bus_req || fsm-a_suspend_req_inf)
 otg_add_timer(fsm, A_WAIT_ENUM);
   + otg_start_hnp_polling(fsm);
 break;
 case OTG_STATE_A_SUSPEND:
 otg_drv_vbus(fsm, 1);
   diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
   index b6ba1bf..79c6ee8 100644
   --- a/include/linux/usb/otg-fsm.h
   +++ b/include/linux/usb/otg-fsm.h
   @@ -127,6 +127,7 @@ struct otg_fsm_ops {
 void(*start_pulse)(struct otg_fsm *fsm);
 void(*start_adp_prb)(struct otg_fsm *fsm);
 void(*start_adp_sns)(struct otg_fsm *fsm);
   + void(*start_hnp_polling)(struct otg_fsm *fsm);
  
  why ? HNP polling is a generic operation, is it not ? Which means you
  shouldn't need to add this function pointer here, just implement a
  generic helper function, ideally in usb-common.c
  
 
 Only OTG capable device will use hnp polling, why it is a generic
 operation?

Is this a serious question ?

Look at what you're patching here. You're already patching the OTG layer
so you know that *all* users are OTG capable.

What I meant, however, is that the implementation of start_hnp_polling()
will be the same for every HNP-capable controller, since that's just a
generic USB GetStatus request.

Place that in drivers/usb/usb-common.c wrapped around CONFIG_OTG ifdef
and you're good to go.

-- 
balbi


signature.asc
Description: Digital signature

RE: [PATCH v2 1/6] usb: otg-fsm: add HNP polling operation function.

2014-02-19 Thread Peter Chen 
 
 On Wed, Feb 19, 2014 at 05:48:49AM +, Peter Chen wrote:
   On Mon, Jan 20, 2014 at 10:00:15AM +0800, Li Jun wrote:
This patch adds HNP polling operation function for OTG fsm.
   
Signed-off-by: Li Jun b47...@freescale.com
---
 drivers/usb/phy/phy-fsm-usb.c |2 ++
 include/linux/usb/otg-fsm.h   |9 +
 2 files changed, 11 insertions(+), 0 deletions(-)
   
diff --git a/drivers/usb/phy/phy-fsm-usb.c
b/drivers/usb/phy/phy-fsm-usb.c index c47e5a6..ef91961 100644
--- a/drivers/usb/phy/phy-fsm-usb.c
+++ b/drivers/usb/phy/phy-fsm-usb.c
@@ -169,6 +169,7 @@ static int otg_set_state(struct otg_fsm *fsm,
enum
   usb_otg_state new_state)
otg_set_protocol(fsm, PROTO_HOST);
usb_bus_start_enum(fsm-otg-host,
fsm-otg-host-otg_port);
+   otg_start_hnp_polling(fsm);
break;
case OTG_STATE_A_IDLE:
otg_drv_vbus(fsm, 0);
@@ -203,6 +204,7 @@ static int otg_set_state(struct otg_fsm *fsm,
enum
   usb_otg_state new_state)
 */
if (!fsm-a_bus_req || fsm-a_suspend_req_inf)
otg_add_timer(fsm, A_WAIT_ENUM);
+   otg_start_hnp_polling(fsm);
break;
case OTG_STATE_A_SUSPEND:
otg_drv_vbus(fsm, 1);
diff --git a/include/linux/usb/otg-fsm.h
b/include/linux/usb/otg-fsm.h index b6ba1bf..79c6ee8 100644
--- a/include/linux/usb/otg-fsm.h
+++ b/include/linux/usb/otg-fsm.h
@@ -127,6 +127,7 @@ struct otg_fsm_ops {
void(*start_pulse)(struct otg_fsm *fsm);
void(*start_adp_prb)(struct otg_fsm *fsm);
void(*start_adp_sns)(struct otg_fsm *fsm);
+   void(*start_hnp_polling)(struct otg_fsm *fsm);
  
   why ? HNP polling is a generic operation, is it not ? Which means
   you shouldn't need to add this function pointer here, just implement
   a generic helper function, ideally in usb-common.c
  
 
  Only OTG capable device will use hnp polling, why it is a generic
  operation?
 
 Is this a serious question ?
 

Nothing seriously, we just want to understand more about your points,
thanks.

Peter

 Look at what you're patching here. You're already patching the OTG layer
 so you know that *all* users are OTG capable.
 
 What I meant, however, is that the implementation of start_hnp_polling()
 will be the same for every HNP-capable controller, since that's just a
 generic USB GetStatus request.
 
 Place that in drivers/usb/usb-common.c wrapped around CONFIG_OTG ifdef
 and you're good to go.
 
 --
 balbi
--
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 v2 1/6] usb: otg-fsm: add HNP polling operation function.

2014-02-18 Thread Felipe Balbi 
On Mon, Jan 20, 2014 at 10:00:15AM +0800, Li Jun wrote:
 This patch adds HNP polling operation function for OTG fsm.
 
 Signed-off-by: Li Jun b47...@freescale.com
 ---
  drivers/usb/phy/phy-fsm-usb.c |2 ++
  include/linux/usb/otg-fsm.h   |9 +
  2 files changed, 11 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/usb/phy/phy-fsm-usb.c b/drivers/usb/phy/phy-fsm-usb.c
 index c47e5a6..ef91961 100644
 --- a/drivers/usb/phy/phy-fsm-usb.c
 +++ b/drivers/usb/phy/phy-fsm-usb.c
 @@ -169,6 +169,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum 
 usb_otg_state new_state)
   otg_set_protocol(fsm, PROTO_HOST);
   usb_bus_start_enum(fsm-otg-host,
   fsm-otg-host-otg_port);
 + otg_start_hnp_polling(fsm);
   break;
   case OTG_STATE_A_IDLE:
   otg_drv_vbus(fsm, 0);
 @@ -203,6 +204,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum 
 usb_otg_state new_state)
*/
   if (!fsm-a_bus_req || fsm-a_suspend_req_inf)
   otg_add_timer(fsm, A_WAIT_ENUM);
 + otg_start_hnp_polling(fsm);
   break;
   case OTG_STATE_A_SUSPEND:
   otg_drv_vbus(fsm, 1);
 diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
 index b6ba1bf..79c6ee8 100644
 --- a/include/linux/usb/otg-fsm.h
 +++ b/include/linux/usb/otg-fsm.h
 @@ -127,6 +127,7 @@ struct otg_fsm_ops {
   void(*start_pulse)(struct otg_fsm *fsm);
   void(*start_adp_prb)(struct otg_fsm *fsm);
   void(*start_adp_sns)(struct otg_fsm *fsm);
 + void(*start_hnp_polling)(struct otg_fsm *fsm);

why ? HNP polling is a generic operation, is it not ? Which means you
shouldn't need to add this function pointer here, just implement a
generic helper function, ideally in usb-common.c

Also, I need to see a patch adding proper kernel doc to those
structures.

cheers

-- 
balbi


signature.asc
Description: Digital signature

RE: [PATCH v2 1/6] usb: otg-fsm: add HNP polling operation function.

2014-02-18 Thread Peter Chen 

 
 On Mon, Jan 20, 2014 at 10:00:15AM +0800, Li Jun wrote:
  This patch adds HNP polling operation function for OTG fsm.
 
  Signed-off-by: Li Jun b47...@freescale.com
  ---
   drivers/usb/phy/phy-fsm-usb.c |2 ++
   include/linux/usb/otg-fsm.h   |9 +
   2 files changed, 11 insertions(+), 0 deletions(-)
 
  diff --git a/drivers/usb/phy/phy-fsm-usb.c
  b/drivers/usb/phy/phy-fsm-usb.c index c47e5a6..ef91961 100644
  --- a/drivers/usb/phy/phy-fsm-usb.c
  +++ b/drivers/usb/phy/phy-fsm-usb.c
  @@ -169,6 +169,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum
 usb_otg_state new_state)
  otg_set_protocol(fsm, PROTO_HOST);
  usb_bus_start_enum(fsm-otg-host,
  fsm-otg-host-otg_port);
  +   otg_start_hnp_polling(fsm);
  break;
  case OTG_STATE_A_IDLE:
  otg_drv_vbus(fsm, 0);
  @@ -203,6 +204,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum
 usb_otg_state new_state)
   */
  if (!fsm-a_bus_req || fsm-a_suspend_req_inf)
  otg_add_timer(fsm, A_WAIT_ENUM);
  +   otg_start_hnp_polling(fsm);
  break;
  case OTG_STATE_A_SUSPEND:
  otg_drv_vbus(fsm, 1);
  diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
  index b6ba1bf..79c6ee8 100644
  --- a/include/linux/usb/otg-fsm.h
  +++ b/include/linux/usb/otg-fsm.h
  @@ -127,6 +127,7 @@ struct otg_fsm_ops {
  void(*start_pulse)(struct otg_fsm *fsm);
  void(*start_adp_prb)(struct otg_fsm *fsm);
  void(*start_adp_sns)(struct otg_fsm *fsm);
  +   void(*start_hnp_polling)(struct otg_fsm *fsm);
 
 why ? HNP polling is a generic operation, is it not ? Which means you
 shouldn't need to add this function pointer here, just implement a
 generic helper function, ideally in usb-common.c
 

Only OTG capable device will use hnp polling, why it is a generic operation?

Peter

 Also, I need to see a patch adding proper kernel doc to those structures.
 
 cheers
 
 --
 balbi
--
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

[PATCH v2 1/6] usb: otg-fsm: add HNP polling operation function.

2014-01-19 Thread Li Jun 
This patch adds HNP polling operation function for OTG fsm.

Signed-off-by: Li Jun b47...@freescale.com
---
 drivers/usb/phy/phy-fsm-usb.c |2 ++
 include/linux/usb/otg-fsm.h   |9 +
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/phy/phy-fsm-usb.c b/drivers/usb/phy/phy-fsm-usb.c
index c47e5a6..ef91961 100644
--- a/drivers/usb/phy/phy-fsm-usb.c
+++ b/drivers/usb/phy/phy-fsm-usb.c
@@ -169,6 +169,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum 
usb_otg_state new_state)
otg_set_protocol(fsm, PROTO_HOST);
usb_bus_start_enum(fsm-otg-host,
fsm-otg-host-otg_port);
+   otg_start_hnp_polling(fsm);
break;
case OTG_STATE_A_IDLE:
otg_drv_vbus(fsm, 0);
@@ -203,6 +204,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum 
usb_otg_state new_state)
 */
if (!fsm-a_bus_req || fsm-a_suspend_req_inf)
otg_add_timer(fsm, A_WAIT_ENUM);
+   otg_start_hnp_polling(fsm);
break;
case OTG_STATE_A_SUSPEND:
otg_drv_vbus(fsm, 1);
diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
index b6ba1bf..79c6ee8 100644
--- a/include/linux/usb/otg-fsm.h
+++ b/include/linux/usb/otg-fsm.h
@@ -127,6 +127,7 @@ struct otg_fsm_ops {
void(*start_pulse)(struct otg_fsm *fsm);
void(*start_adp_prb)(struct otg_fsm *fsm);
void(*start_adp_sns)(struct otg_fsm *fsm);
+   void(*start_hnp_polling)(struct otg_fsm *fsm);
void(*add_timer)(struct otg_fsm *fsm, enum otg_fsm_timer timer);
void(*del_timer)(struct otg_fsm *fsm, enum otg_fsm_timer timer);
int (*start_host)(struct otg_fsm *fsm, int on);
@@ -209,6 +210,14 @@ static inline int otg_start_adp_sns(struct otg_fsm *fsm)
return 0;
 }
 
+static inline int otg_start_hnp_polling(struct otg_fsm *fsm)
+{
+   if (!fsm-ops-start_hnp_polling)
+   return -EOPNOTSUPP;
+   fsm-ops-start_hnp_polling(fsm);
+   return 0;
+}
+
 static inline int otg_add_timer(struct otg_fsm *fsm, enum otg_fsm_timer timer)
 {
if (!fsm-ops-add_timer)
-- 
1.7.8


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