| Message ID | 8fcb9ff7a8e01bec53813f0702ff606bf4982943.1438844454.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 7 August 2015 at 13:45, Peter Chen <peter.chen@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.wang@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
On 7 August 2015 at 17:07, Peter Chen <Peter.Chen@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
On 8 August 2015 at 01:53, Greg KH <gregkh@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.Chen@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
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; unsigned out_epnum; unsigned in_epnum; + struct raw_notifier_head nh; + struct usb_charger *uchger; + spinlock_t lock; unsigned sg_supported:1; unsigned is_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 properly */ extern void usb_gadget_set_state(struct usb_gadget *gadget,
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.wang@linaro.org> --- drivers/usb/gadget/udc/udc-core.c | 41 +++++++++++++++++++++++++++++++++++++ include/linux/usb/gadget.h | 20 ++++++++++++++++++ 2 files changed, 61 insertions(+)