Re: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-08 Thread Baolin Wang 
On 8 August 2015 at 01:53, Greg KH gre...@linuxfoundation.org wrote:
 On Fri, Aug 07, 2015 at 05:22:47PM +0800, Baolin Wang wrote:
 On 7 August 2015 at 17:07, Peter Chen peter.c...@freescale.com wrote:
 
/**
 * struct usb_udc - describes one usb device controller @@ -127,12
   +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
   EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
  
   +int usb_gadget_register_notify(struct usb_gadget *gadget,
   +struct notifier_block *nb) {
   + unsigned long flags;
   + int ret;
   +
   + spin_lock_irqsave(gadget-lock, flags);
  
   I find you use so many spin_lock_irqsave, any reasons for that?
   Why mutex_lock can't be used?
  
 
  The spin_lock_irqsave() can make it as a atomic notifier, that can make 
  sure the
  gadget state event can be quickly reported to the user who register a 
  notifier
  on the gadget device. Is it OK?
 
 
  I don't think it is a good reason, spin_lock_irqsave is usually used for 
  protecting
  data which is accessed at atomic environment.
 

 Yes, we want the notify process is a atomic environment which do not
 want to be interrupted by irq or other things to make the notice can
 be quickly reported to the user.

 No, this is a slow event, you don't need to notify anyone under atomic
 context, that's crazy.

 Also i think the notify process is less cost, thus i use the spinlock. 
 Thanks.

 No, use a mutex please, that's the safe thing.  This is not
 time-critical code at all.


OK, Thanks for your comments and will fix the lock thing.

 thanks,

 greg k-h



-- 
Baolin.wang
Best Regards
--
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 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Peter Chen 
 
   /**
* struct usb_udc - describes one usb device controller @@ -127,12
  +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
  +int usb_gadget_register_notify(struct usb_gadget *gadget,
  +struct notifier_block *nb) {
  + unsigned long flags;
  + int ret;
  +
  + spin_lock_irqsave(gadget-lock, flags);
 
  I find you use so many spin_lock_irqsave, any reasons for that?
  Why mutex_lock can't be used?
 
 
 The spin_lock_irqsave() can make it as a atomic notifier, that can make sure 
 the
 gadget state event can be quickly reported to the user who register a notifier
 on the gadget device. Is it OK?
 

I don't think it is a good reason, spin_lock_irqsave is usually used for 
protecting
data which is accessed at atomic environment. 

Peter

Re: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Peter Chen 
On Thu, Aug 06, 2015 at 03:03:49PM +0800, Baolin Wang wrote:
 The usb charger framework is based on usb gadget, and each usb gadget
 can be one usb charger to set the current limitation.
 
 This patch adds a notifier mechanism for usb charger to report to usb
 charger when the usb gadget state is changed.
 
 Also we introduce a callback 'get_charger_type' which will implemented
 by user for usb gadget operations to get the usb charger type.
 
 Signed-off-by: Baolin Wang baolin.w...@linaro.org
 ---
  drivers/usb/gadget/udc/udc-core.c |   41 
 +
  include/linux/usb/gadget.h|   20 ++
  2 files changed, 61 insertions(+)
 
 diff --git a/drivers/usb/gadget/udc/udc-core.c 
 b/drivers/usb/gadget/udc/udc-core.c
 index d69c355..d5368088 100644
 --- a/drivers/usb/gadget/udc/udc-core.c
 +++ b/drivers/usb/gadget/udc/udc-core.c
 @@ -28,6 +28,7 @@
  #include linux/usb/ch9.h
  #include linux/usb/gadget.h
  #include linux/usb.h
 +#include linux/usb/usb_charger.h
  
  /**
   * struct usb_udc - describes one usb device controller
 @@ -127,12 +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
  
 +int usb_gadget_register_notify(struct usb_gadget *gadget,
 +struct notifier_block *nb)
 +{
 + unsigned long flags;
 + int ret;
 +
 + spin_lock_irqsave(gadget-lock, flags);

I find you use so many spin_lock_irqsave, any reasons for that?
Why mutex_lock can't be used?

-- 

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 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Baolin Wang 
On 7 August 2015 at 13:45, Peter Chen peter.c...@freescale.com wrote:
 On Thu, Aug 06, 2015 at 03:03:49PM +0800, Baolin Wang wrote:
 The usb charger framework is based on usb gadget, and each usb gadget
 can be one usb charger to set the current limitation.

 This patch adds a notifier mechanism for usb charger to report to usb
 charger when the usb gadget state is changed.

 Also we introduce a callback 'get_charger_type' which will implemented
 by user for usb gadget operations to get the usb charger type.

 Signed-off-by: Baolin Wang baolin.w...@linaro.org
 ---
  drivers/usb/gadget/udc/udc-core.c |   41 
 +
  include/linux/usb/gadget.h|   20 ++
  2 files changed, 61 insertions(+)

 diff --git a/drivers/usb/gadget/udc/udc-core.c 
 b/drivers/usb/gadget/udc/udc-core.c
 index d69c355..d5368088 100644
 --- a/drivers/usb/gadget/udc/udc-core.c
 +++ b/drivers/usb/gadget/udc/udc-core.c
 @@ -28,6 +28,7 @@
  #include linux/usb/ch9.h
  #include linux/usb/gadget.h
  #include linux/usb.h
 +#include linux/usb/usb_charger.h

  /**
   * struct usb_udc - describes one usb device controller
 @@ -127,12 +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);

 +int usb_gadget_register_notify(struct usb_gadget *gadget,
 +struct notifier_block *nb)
 +{
 + unsigned long flags;
 + int ret;
 +
 + spin_lock_irqsave(gadget-lock, flags);

 I find you use so many spin_lock_irqsave, any reasons for that?
 Why mutex_lock can't be used?


The spin_lock_irqsave() can make it as a atomic notifier, that can
make sure the gadget state event can be quickly reported to the user
who register a notifier on the gadget device. Is it OK?

 --

 Best Regards,
 Peter Chen



-- 
Baolin.wang
Best Regards
--
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 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Baolin Wang 
On 7 August 2015 at 17:07, Peter Chen peter.c...@freescale.com wrote:

   /**
* struct usb_udc - describes one usb device controller @@ -127,12
  +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
  +int usb_gadget_register_notify(struct usb_gadget *gadget,
  +struct notifier_block *nb) {
  + unsigned long flags;
  + int ret;
  +
  + spin_lock_irqsave(gadget-lock, flags);
 
  I find you use so many spin_lock_irqsave, any reasons for that?
  Why mutex_lock can't be used?
 

 The spin_lock_irqsave() can make it as a atomic notifier, that can make sure 
 the
 gadget state event can be quickly reported to the user who register a 
 notifier
 on the gadget device. Is it OK?


 I don't think it is a good reason, spin_lock_irqsave is usually used for 
 protecting
 data which is accessed at atomic environment.


Yes, we want the notify process is a atomic environment which do not
want to be interrupted by irq or other things to make the notice can
be quickly reported to the user.

Also i think the notify process is less cost, thus i use the spinlock. Thanks.

 Peter



-- 
Baolin.wang
Best Regards
--
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 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Greg KH 
On Fri, Aug 07, 2015 at 05:22:47PM +0800, Baolin Wang wrote:
 On 7 August 2015 at 17:07, Peter Chen peter.c...@freescale.com wrote:
 
/**
 * struct usb_udc - describes one usb device controller @@ -127,12
   +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
   EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
  
   +int usb_gadget_register_notify(struct usb_gadget *gadget,
   +struct notifier_block *nb) {
   + unsigned long flags;
   + int ret;
   +
   + spin_lock_irqsave(gadget-lock, flags);
  
   I find you use so many spin_lock_irqsave, any reasons for that?
   Why mutex_lock can't be used?
  
 
  The spin_lock_irqsave() can make it as a atomic notifier, that can make 
  sure the
  gadget state event can be quickly reported to the user who register a 
  notifier
  on the gadget device. Is it OK?
 
 
  I don't think it is a good reason, spin_lock_irqsave is usually used for 
  protecting
  data which is accessed at atomic environment.
 
 
 Yes, we want the notify process is a atomic environment which do not
 want to be interrupted by irq or other things to make the notice can
 be quickly reported to the user.

No, this is a slow event, you don't need to notify anyone under atomic
context, that's crazy.

 Also i think the notify process is less cost, thus i use the spinlock. Thanks.

No, use a mutex please, that's the safe thing.  This is not
time-critical code at all.

thanks,

greg k-h
--
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 2/2] gadget: Support for the usb charger framework

2015-08-06 Thread Baolin Wang 
The usb charger framework is based on usb gadget, and each usb gadget
can be one usb charger to set the current limitation.

This patch adds a notifier mechanism for usb charger to report to usb
charger when the usb gadget state is changed.

Also we introduce a callback 'get_charger_type' which will implemented
by user for usb gadget operations to get the usb charger type.

Signed-off-by: Baolin Wang baolin.w...@linaro.org
---
 drivers/usb/gadget/udc/udc-core.c |   41 +
 include/linux/usb/gadget.h|   20 ++
 2 files changed, 61 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c 
b/drivers/usb/gadget/udc/udc-core.c
index d69c355..d5368088 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include linux/usb/ch9.h
 #include linux/usb/gadget.h
 #include linux/usb.h
+#include linux/usb/usb_charger.h
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -127,12 +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
 }
 EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+  struct notifier_block *nb)
+{
+   unsigned long flags;
+   int ret;
+
+   spin_lock_irqsave(gadget-lock, flags);
+   ret = raw_notifier_chain_register(gadget-nh, nb);
+   spin_unlock_irqrestore(gadget-lock, flags);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
+
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+struct notifier_block *nb)
+{
+   unsigned long flags;
+   int ret;
+
+   spin_lock_irqsave(gadget-lock, flags);
+   ret = raw_notifier_chain_unregister(gadget-nh, nb);
+   spin_unlock_irqrestore(gadget-lock, flags);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
+
 /* - */
 
 static void usb_gadget_state_work(struct work_struct *work)
 {
struct usb_gadget *gadget = work_to_gadget(work);
struct usb_udc *udc = gadget-udc;
+   unsigned long flags;
+
+   spin_lock_irqsave(gadget-lock, flags);
+   raw_notifier_call_chain(gadget-nh, gadget-state, gadget);
+   spin_unlock_irqrestore(gadget-lock, flags);
 
if (udc)
sysfs_notify(udc-dev.kobj, NULL, state);
@@ -272,6 +306,8 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
 
dev_set_name(gadget-dev, gadget);
INIT_WORK(gadget-work, usb_gadget_state_work);
+   RAW_INIT_NOTIFIER_HEAD(gadget-nh);
+   spin_lock_init(gadget-lock);
gadget-dev.parent = parent;
 
 #ifdef CONFIG_HAS_DMA
@@ -313,6 +349,10 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
 
mutex_unlock(udc_lock);
 
+   ret = usb_charger_init(gadget);
+   if (ret)
+   goto err4;
+
return 0;
 
 err4:
@@ -388,6 +428,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
flush_work(gadget-work);
device_unregister(udc-dev);
+   usb_charger_exit(gadget);
device_unregister(gadget-dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 4f3dfb7..f24d6ac 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -492,6 +492,7 @@ struct usb_gadget_ops {
int (*udc_start)(struct usb_gadget *,
struct usb_gadget_driver *);
int (*udc_stop)(struct usb_gadget *);
+   enum usb_charger_type   (*get_charger_type)(struct usb_gadget *);
 };
 
 /**
@@ -559,6 +560,9 @@ struct usb_gadget {
struct device   dev;
unsignedout_epnum;
unsignedin_epnum;
+   struct raw_notifier_headnh;
+   struct usb_charger  *uchger;
+   spinlock_t  lock;
 
unsignedsg_supported:1;
unsignedis_otg:1;
@@ -1014,6 +1018,22 @@ extern void usb_gadget_unmap_request(struct usb_gadget 
*gadget,
 
 /*-*/
 
+/**
+ * Register a notifiee to get notified by any attach status changes from
+ * the usb gadget
+ */
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+  struct notifier_block *nb);
+
+/*-*/
+
+
+/* Unregister a notifiee from the usb gadget */
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+struct notifier_block *nb);
+
+/*-*/
+
 /* utility to set gadget state