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