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