[PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

Currently the Linux kernel does not provide any standard integration of this
feature that integrates the USB subsystem with the system power regulation
provided by PMICs meaning that either vendors must add this in their kernels
or USB gadget devices based on Linux (such as mobile phones) may not behave
as they should.

Providing a standard framework for doing this in the kernel.

Baolin Wang (2):
  gadget: Introduce the usb charger framework
  gadget: Support for the usb charger framework

 drivers/usb/gadget/charger.c      |  547 +++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/udc-core.c |   41 +++
 include/linux/usb/gadget.h        |   20 ++
 include/linux/usb/usb_charger.h   |  101 +++++++
 4 files changed, 709 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

-- 
1.7.9.5

[PATCH 1/2] gadget: Introduce the usb charger framework

[Baolin Wang]

This patch introduces the usb charger driver based on usb gadget that
makes an enhancement to a power driver. It works well in practice but
that requires a system with suitable hardware.

The basic conception of the usb charger is that, when one usb charger
is added or removed by reporting from the usb gadget state change or
the extcon device state change, the usb charger will report to power
user to set the current limitation.

The usb charger will register notifiees on the usb gadget or the extcon
device to get notified the usb charger state.

Power user will register a notifiee on the usb charger to get notified
by status changes from the usb charger. It will report to power user
to set the current limitation when detecting the usb charger is added
or removed from extcon device state or usb gadget state.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/gadget/charger.c    |  547 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb/usb_charger.h |  101 ++++++++
 2 files changed, 648 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
new file mode 100644
index 0000000..3ca0180
--- /dev/null
+++ b/drivers/usb/gadget/charger.c
@@ -0,0 +1,547 @@
+/*
+ * usb charger.c -- USB charger driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/extcon.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/usb/ch9.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/usb_charger.h>
+
+#define DEFAULT_CUR_PROTECT	(50)
+#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_DCP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_CDP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_ACA_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
+
+static LIST_HEAD(usb_charger_list);
+static DEFINE_MUTEX(usb_charger_list_lock);
+
+/*
+ * usb_charger_find_by_name - Get the usb charger device by name.
+ * @name - usb charger device name.
+ *
+ * notes: when this function walks the list and returns a charger
+ * it's dropped the lock which means that something else could come
+ * along and delete the charger before we dereference the pointer.
+ * It's very unlikely but it's a possibility so you should take care
+ * of it.
+ * Thus when you get the usb charger by name, you should call
+ * put_usb_charger() to derease the reference count of the usb charger.
+ *
+ * return the instance of usb charger device.
+ */
+struct usb_charger *usb_charger_find_by_name(char *name)
+{
+	struct usb_charger *uchger;
+
+	if (!name)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&usb_charger_list_lock);
+	list_for_each_entry(uchger, &usb_charger_list, entry) {
+		if (!strcmp(uchger->name, name)) {
+			get_usb_charger(uchger);
+			mutex_unlock(&usb_charger_list_lock);
+			return uchger;
+		}
+	}
+	mutex_unlock(&usb_charger_list_lock);
+
+	return NULL;
+}
+
+/*
+ * usb_charger_register_notify() - Register a notifiee to get notified by
+ * 		any attach status changes from the usb charger type detection.
+ * @uchger - the usb charger device which is monitored.
+ * @nb - a notifier block to be registered.
+ */
+void usb_charger_register_notify(struct usb_charger *uchger,
+				 struct notifier_block *nb)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&uchger->lock, flags);
+	raw_notifier_chain_register(&uchger->uchger_nh, nb);
+	spin_unlock_irqrestore(&uchger->lock, flags);
+}
+
+/*
+ * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
+ * @uchger - the usb charger device which is monitored.
+ * @nb - a notifier block to be unregistered.
+ */
+void usb_charger_unregister_notify(struct usb_charger *uchger,
+				   struct notifier_block *nb)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&uchger->lock, flags);
+	raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
+	spin_unlock_irqrestore(&uchger->lock, flags);
+}
+
+/*
+ * usb_charger_register_extcon_notifier() - Register a notifiee of the usb
+ * 		charger to get notified by any attach status changes from
+ * 		the extcon device.
+ * @uchger - the usb charger device.
+ * @edev - the extcon device.
+ * @extcon_id - extcon id.
+ */
+int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
+					 struct extcon_dev *edev,
+					 unsigned int extcon_id)
+{
+	if (!uchger || !edev)
+		return -EINVAL;
+
+	return extcon_register_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
+}
+
+/*
+ * usb_charger_unregister_extcon_notifier() - Unregister a notifiee of the
+ * 		 usb charger from the extcon device.
+ * @uchger - the usb charger device.
+ * @edev - the extcon device.
+ * @extcon_id - extcon id.
+ */
+int usb_charger_unregister_extcon_notifier(struct usb_charger *uchger,
+					   struct extcon_dev *edev,
+					   unsigned int extcon_id)
+{
+	if (!uchger || !edev)
+		return -EINVAL;
+
+	return extcon_unregister_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
+}
+
+/*
+ * usb_charger_register_gadget_notifier() - Register a notifiee of the usb
+ * 		charger to get notified by any attach status changes from
+ * 		the usb gadget device.
+ * @uchger - the usb charger device.
+ */
+int usb_charger_register_gadget_notifier(struct usb_charger *uchger)
+{
+	struct usb_gadget *ugadget = uchger->gadget;
+
+	return usb_gadget_register_notify(ugadget, &uchger->gadget_nb);
+}
+
+/*
+ * usb_charger_unregister_gadget_notifier() - Unregister a notifiee of the usb
+ * 		charger from the usb gadget device.
+ * @uchger - the usb charger device.
+ */
+int usb_charger_unregister_gadget_notifier(struct usb_charger *uchger)
+{
+	struct usb_gadget *ugadget = uchger->gadget;
+
+	return usb_gadget_unregister_notify(ugadget, &uchger->gadget_nb);
+}
+
+/*
+ * usb_charger_set_cur_limit() - Set the current limitation.
+ * @uchger - the usb charger device.
+ * @xxx_cur - the current limit by different charger type.
+ *
+ */
+int usb_charger_set_cur_limit(struct usb_charger *uchger,
+			      struct usb_charger_cur_limit *cur_limit_set)
+{
+	if (!uchger || !cur_limit_set)
+		return -EINVAL;
+
+	uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
+	uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
+	uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
+	uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
+	return 0;
+}
+
+/*
+ * usb_charger_get_cur_limit() - Get the current limitation by different usb
+ * 		charger type.
+ * @uchger - the usb charger device.
+ * @type - the usb charger type.
+ *
+ * return the current limitation to set.
+ */
+static unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	enum usb_charger_type uchger_type = uchger->type;
+	unsigned int cur_limit;
+
+	switch (uchger_type) {
+	case SDP_TYPE:
+		cur_limit = uchger->cur_limit.sdp_cur_limit;
+		break;
+	case DCP_TYPE:
+		cur_limit = uchger->cur_limit.dcp_cur_limit;
+		break;
+	case CDP_TYPE:
+		cur_limit = uchger->cur_limit.cdp_cur_limit;
+		break;
+	case ACA_TYPE:
+		cur_limit = uchger->cur_limit.aca_cur_limit;
+		break;
+	default:
+		return 0;
+	}
+
+	return cur_limit;
+}
+
+/*
+ * usb_charger_detect_type() - Get the usb charger type by the callback which is
+ * 		implemented by user.
+ * @uchger - the usb charger device.
+ *
+ * return the usb charger type.
+ */
+enum usb_charger_type
+usb_charger_detect_type(struct usb_charger *uchger)
+{
+	if (uchger->gadget && uchger->gadget->ops
+	    && uchger->gadget->ops->get_charger_type)
+		uchger->type =
+			uchger->gadget->ops->get_charger_type(uchger->gadget);
+	else
+		uchger->type = UNKNOWN_TYPE;
+
+	return uchger->type;
+}
+
+/*
+ * usb_charger_notifier_others() - It will notify other device registered on
+ * 		usb charger.
+ * @uchger - the usb charger device.
+ *
+ */
+static void
+usb_charger_notify_others(struct usb_charger *uchger,
+			  enum usb_charger_state state)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&uchger->lock, flags);
+	uchger->state = state;
+
+	switch (state) {
+	case USB_CHARGER_PRESENT:
+		usb_charger_detect_type(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+			usb_charger_get_cur_limit(uchger),
+			uchger);
+		break;
+	case USB_CHARGER_REMOVE:
+		uchger->type = UNKNOWN_TYPE;
+		raw_notifier_call_chain(&uchger->uchger_nh, 0, uchger);
+		break;
+	default:
+		dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
+			 state);
+		break;
+	}
+	spin_unlock_irqrestore(&uchger->lock, flags);
+}
+
+/*
+ * usb_charger_plug_by_extcon() - The notifier call function which is registered
+ * 		on the extcon device.
+ * @nb - thr notifier block that notified by extcon device.
+ * @state - the extcon device state changed.
+ * @data - here specify a extcon device.
+ *
+ * return the notify flag.
+ */
+static int
+usb_charger_plug_by_extcon(struct notifier_block *nb,
+			   unsigned long state, void *data)
+{
+	struct usb_charger_nb *extcon_nb =
+		container_of(nb, struct usb_charger_nb, nb);
+	struct usb_charger *uchger = extcon_nb->uchger;
+	enum usb_charger_state uchger_state;
+
+	if (!uchger)
+		return NOTIFY_BAD;
+
+	/* Report event to power to setting the current limitation
+	 * for this usb charger when one usb charger is added or removed
+	 * with detecting by extcon device.
+	 */
+	if (state)
+		uchger_state = USB_CHARGER_PRESENT;
+	else
+		uchger_state = USB_CHARGER_REMOVE;
+
+	usb_charger_notify_others(uchger, uchger_state);
+
+	return NOTIFY_OK;
+}
+
+/*
+ * usb_charger_plug_by_gadget() - Set the usb charger current limitation
+ * 		according to the usb gadget device state.
+ * @data - here specify a usb charger device.
+ *
+ */
+static int
+usb_charger_plug_by_gadget(struct notifier_block *nb,
+			   unsigned long state, void *data)
+{
+	struct usb_gadget *gadget = (struct usb_gadget *)data;
+	struct usb_charger *uchger = gadget->uchger;
+	enum usb_charger_state uchger_state;
+
+	if (!uchger)
+		return NOTIFY_BAD;
+
+	/* Report event to power to setting the current limitation
+	 * for this usb charger when one usb charger state is changed
+	 * with detecting by usb gadget state.
+	 */
+	if (uchger->old_gadget_state != state) {
+		uchger->old_gadget_state = state;
+
+		if (state >= USB_STATE_ATTACHED)
+			uchger_state = USB_CHARGER_PRESENT;
+		else if (state == USB_STATE_NOTATTACHED)
+			uchger_state = USB_CHARGER_REMOVE;
+		else
+			uchger_state = USB_CHARGER_DEFAULT;
+
+		usb_charger_notify_others(uchger, uchger_state);
+	}
+
+	return NOTIFY_OK;
+}
+
+static int devm_uchger_dev_match(struct device *dev, void *res, void *data)
+{
+	struct usb_charger **r = res;
+
+	if (WARN_ON(!r || !*r))
+		return 0;
+
+	return *r == data;
+}
+
+static void usb_charger_release(struct device *dev)
+{
+	struct usb_charger *uchger = dev_get_drvdata(dev);
+
+	if (!atomic_dec_and_test(&uchger->count)) {
+		dev_err(dev, "The usb charger is still in use\n");
+		return;
+	}
+
+	kfree(uchger->name);
+	kfree(uchger);
+}
+
+/*
+ * usb_charger_unregister() - Unregister a usb charger device.
+ * @uchger - the usb charger device.
+ *
+ */
+int usb_charger_unregister(struct usb_charger *uchger)
+{
+	if (!uchger)
+		return -EINVAL;
+
+	mutex_lock(&usb_charger_list_lock);
+	list_del(&uchger->entry);
+	mutex_unlock(&usb_charger_list_lock);
+
+	device_unregister(&uchger->dev);
+	return 0;
+}
+
+static void devm_uchger_dev_unreg(struct device *dev, void *res)
+{
+	usb_charger_unregister(*(struct usb_charger **)res);
+}
+
+void devm_usb_charger_unregister(struct device *dev,
+				 struct usb_charger *uchger)
+{
+	devres_release(dev, devm_uchger_dev_unreg,
+		       devm_uchger_dev_match, uchger);
+}
+
+/*
+ * usb_charger_register() - Register a new usb charger device.
+ * @uchger - the new usb charger device.
+ *
+ */
+int usb_charger_register(struct device *dev, struct usb_charger *uchger)
+{
+	static atomic_t uchger_no = ATOMIC_INIT(-1);
+	struct usb_charger *tmp;
+	int ret;
+
+	if (!uchger) {
+		dev_err(dev, "no device provided for charger\n");
+		return -EINVAL;
+	}
+
+	uchger->dev.parent = dev;
+	uchger->dev.release = usb_charger_release;
+	dev_set_name(&uchger->dev, "usb-chger%lu",
+		     (unsigned long)atomic_inc_return(&uchger_no));
+
+	ret = device_register(&uchger->dev);
+	if (ret) {
+		put_device(&uchger->dev);
+		return ret;
+	}
+
+	dev_set_drvdata(&uchger->dev, uchger);
+
+	mutex_lock(&usb_charger_list_lock);
+	list_for_each_entry(tmp, &usb_charger_list, entry) {
+		if (!(strcmp(tmp->name, uchger->name))) {
+			mutex_unlock(&usb_charger_list_lock);
+			ret = -EEXIST;
+			goto out;
+		}
+	}
+	list_add_tail(&uchger->entry, &usb_charger_list);
+	mutex_unlock(&usb_charger_list_lock);
+
+	return 0;
+
+out:
+	dev_err(dev, "Failed to register usb charger (%s)\n",
+		uchger->name);
+	device_unregister(&uchger->dev);
+	put_device(&uchger->dev);
+	return ret;
+}
+
+int devm_usb_charger_register(struct device *dev,
+			      struct usb_charger *uchger)
+{
+	struct usb_charger **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = usb_charger_register(dev, uchger);
+	if (ret) {
+		devres_free(ptr);
+		return ret;
+	}
+
+	*ptr = uchger;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+
+int usb_charger_init(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger;
+	struct extcon_dev *edev;
+	char buf[100];
+	char *str;
+	int ret;
+
+	if (!ugadget)
+		return -EINVAL;
+
+	uchger = devm_kzalloc(&ugadget->dev, sizeof(struct usb_charger),
+			      GFP_KERNEL);
+	if (!uchger)
+		return -ENOMEM;
+
+	sprintf(buf, "usb-charger.%s", ugadget->name);
+	str = devm_kzalloc(&ugadget->dev, sizeof(char) * (strlen(buf) + 1),
+			   GFP_KERNEL);
+	if (!str)
+		return -ENOMEM;
+
+	strcpy(str, buf);
+	uchger->name = str;
+	uchger->type = UNKNOWN_TYPE;
+	uchger->state = USB_CHARGER_DEFAULT;
+	uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT;
+	uchger->cur_limit.dcp_cur_limit = DEFAULT_DCP_CUR_LIMIT;
+	uchger->cur_limit.cdp_cur_limit = DEFAULT_CDP_CUR_LIMIT;
+	uchger->cur_limit.aca_cur_limit = DEFAULT_ACA_CUR_LIMIT;
+
+	atomic_set(&uchger->count, 1);
+	spin_lock_init(&uchger->lock);
+	INIT_LIST_HEAD(&uchger->entry);
+	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
+
+	/* register a notifier on a extcon device if it is exsited */
+	edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
+	if (!IS_ERR_OR_NULL(edev)) {
+		uchger->extcon_dev = edev;
+		uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
+		uchger->extcon_nb.uchger = uchger;
+		usb_charger_register_extcon_notifier(uchger, edev, EXTCON_USB);
+	}
+
+	/* register a notifier on a usb gadget device */
+	uchger->gadget = ugadget;
+	ugadget->uchger = uchger;
+	uchger->old_gadget_state = ugadget->state;
+	uchger->gadget_nb.notifier_call = usb_charger_plug_by_gadget;
+	usb_charger_register_gadget_notifier(uchger);
+
+	ret = usb_charger_register(&ugadget->dev, uchger);
+	if (ret)
+		goto reg_fail;
+
+	return 0;
+
+reg_fail:
+	if (uchger->extcon_dev)
+		usb_charger_unregister_extcon_notifier(uchger,
+				uchger->extcon_dev, EXTCON_USB);
+
+	usb_charger_unregister_gadget_notifier(uchger);
+	return ret;
+}
+
+int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger = ugadget->uchger;
+
+	if (!uchger)
+		return -EINVAL;
+
+	if (uchger->extcon_dev)
+		usb_charger_unregister_extcon_notifier(uchger,
+				uchger->extcon_dev, EXTCON_USB);
+
+	usb_charger_unregister_gadget_notifier(uchger);
+	return usb_charger_unregister(uchger);
+}
+
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
+MODULE_DESCRIPTION("USB charger driver");
diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
new file mode 100644
index 0000000..da4d3c9
--- /dev/null
+++ b/include/linux/usb/usb_charger.h
@@ -0,0 +1,101 @@
+#ifndef __LINUX_USB_CHARGER_H__
+#define __LINUX_USB_CHARGER_H__
+
+#include <linux/device.h>
+#include <linux/notifier.h>
+#include <linux/sysfs.h>
+#include <linux/usb/ch9.h>
+
+/* USB charger type:
+ * SDP (Standard Downstream Port)
+ * DCP (Dedicated Charging Port)
+ * CDP (Charging Downstream Port)
+ * ACA (Accessory Charger Adapters)
+ */
+enum usb_charger_type {
+	UNKNOWN_TYPE,
+	SDP_TYPE,
+	DCP_TYPE,
+	CDP_TYPE,
+	ACA_TYPE,
+};
+
+/* USB charger state */
+enum usb_charger_state {
+	USB_CHARGER_DEFAULT,
+	USB_CHARGER_PRESENT,
+	USB_CHARGER_REMOVE,
+};
+
+/* Current limitation by charger type */
+struct usb_charger_cur_limit {
+	unsigned int sdp_cur_limit;
+	unsigned int dcp_cur_limit;
+	unsigned int cdp_cur_limit;
+	unsigned int aca_cur_limit;
+};
+
+struct usb_charger_nb {
+	struct notifier_block	nb;
+	struct usb_charger	*uchger;
+};
+
+struct usb_charger {
+	/* Internal data. Please do not set. */
+	const char		*name;
+	struct device		dev;
+	struct raw_notifier_head	uchger_nh;
+	struct list_head	entry;
+	spinlock_t		lock;
+	enum usb_charger_type	type;
+	enum usb_charger_state	state;
+	atomic_t		count;
+
+	/* For supporting extcon usb gpio */
+	struct extcon_dev	*extcon_dev;
+	struct usb_charger_nb	extcon_nb;
+
+	/* For supporting usb gadget */
+	struct usb_gadget	*gadget;
+	enum usb_device_state	old_gadget_state;
+	struct notifier_block	gadget_nb;
+
+	/* Current limitation */
+	struct usb_charger_cur_limit	cur_limit;
+};
+
+extern struct usb_charger *usb_charger_find_by_name(char *name);
+
+extern void usb_charger_register_notify(struct usb_charger *uchger,
+					struct notifier_block *nb);
+extern void usb_charger_unregister_notify(struct usb_charger *uchger,
+					  struct notifier_block *nb);
+
+extern int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
+						struct extcon_dev *edev,
+						unsigned int extcon_id);
+extern int usb_charger_unregister_extcon_notifier(struct usb_charger *uchger,
+						  struct extcon_dev *edev,
+						  unsigned int extcon_id);
+
+extern int usb_charger_register_gadget_notifier(struct usb_charger *uchger);
+extern int usb_charger_unregister_gadget_notifier(struct usb_charger *uchger);
+
+extern int usb_charger_set_cur_limit(struct usb_charger *uchger,
+			      struct usb_charger_cur_limit *cur_limit_set);
+extern enum usb_charger_type usb_charger_detect_type(struct usb_charger *uchger);
+
+extern int usb_charger_init(struct usb_gadget *ugadget);
+extern int usb_charger_exit(struct usb_gadget *ugadget);
+
+static inline void get_usb_charger(struct usb_charger *uchger)
+{
+	atomic_inc(&uchger->count);
+}
+
+static inline void put_usb_charger(struct usb_charger *uchger)
+{
+	atomic_dec(&uchger->count);
+}
+
+#endif /* __LINUX_USB_CHARGER_H__ */
-- 
1.7.9.5

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

[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.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);
+	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,
-- 
1.7.9.5

Re: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

Hi,

On Thu, Aug 06, 2015 at 03:03:47PM +0800, Baolin Wang wrote:
> Currently the Linux kernel does not provide any standard integration of this
> feature that integrates the USB subsystem with the system power regulation
> provided by PMICs meaning that either vendors must add this in their kernels
> or USB gadget devices based on Linux (such as mobile phones) may not behave
> as they should.
> 
> Providing a standard framework for doing this in the kernel.

it's too late in this cycle to even start discussing this. i'll drop
from my queue, please resend rebase on v4.3-rc1 once that's out.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] gadget: Introduce the usb charger framework

[Greg KH]

On Thu, Aug 06, 2015 at 03:03:48PM +0800, Baolin Wang wrote:
> This patch introduces the usb charger driver based on usb gadget that
> makes an enhancement to a power driver. It works well in practice but
> that requires a system with suitable hardware.
> 
> The basic conception of the usb charger is that, when one usb charger
> is added or removed by reporting from the usb gadget state change or
> the extcon device state change, the usb charger will report to power
> user to set the current limitation.
> 
> The usb charger will register notifiees on the usb gadget or the extcon
> device to get notified the usb charger state.
> 
> Power user will register a notifiee on the usb charger to get notified
> by status changes from the usb charger. It will report to power user
> to set the current limitation when detecting the usb charger is added
> or removed from extcon device state or usb gadget state.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/gadget/charger.c    |  547 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/usb_charger.h |  101 ++++++++
>  2 files changed, 648 insertions(+)
>  create mode 100644 drivers/usb/gadget/charger.c
>  create mode 100644 include/linux/usb/usb_charger.h
> 
> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
> new file mode 100644
> index 0000000..3ca0180
> --- /dev/null
> +++ b/drivers/usb/gadget/charger.c
> @@ -0,0 +1,547 @@
> +/*
> + * usb charger.c -- USB charger driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

I have to ask, do you really mean "any later version"?

> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/extcon.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/usb_charger.h>
> +
> +#define DEFAULT_CUR_PROTECT	(50)
> +#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_DCP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_CDP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_ACA_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +
> +static LIST_HEAD(usb_charger_list);
> +static DEFINE_MUTEX(usb_charger_list_lock);
> +
> +/*
> + * usb_charger_find_by_name - Get the usb charger device by name.
> + * @name - usb charger device name.
> + *
> + * notes: when this function walks the list and returns a charger
> + * it's dropped the lock which means that something else could come
> + * along and delete the charger before we dereference the pointer.
> + * It's very unlikely but it's a possibility so you should take care
> + * of it.
> + * Thus when you get the usb charger by name, you should call
> + * put_usb_charger() to derease the reference count of the usb charger.
> + *
> + * return the instance of usb charger device.
> + */
> +struct usb_charger *usb_charger_find_by_name(char *name)
> +{
> +	struct usb_charger *uchger;
> +
> +	if (!name)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&usb_charger_list_lock);
> +	list_for_each_entry(uchger, &usb_charger_list, entry) {
> +		if (!strcmp(uchger->name, name)) {
> +			get_usb_charger(uchger);
> +			mutex_unlock(&usb_charger_list_lock);
> +			return uchger;
> +		}
> +	}
> +	mutex_unlock(&usb_charger_list_lock);
> +
> +	return NULL;
> +}
> +
> +/*
> + * usb_charger_register_notify() - Register a notifiee to get notified by
> + * 		any attach status changes from the usb charger type detection.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be registered.
> + */
> +void usb_charger_register_notify(struct usb_charger *uchger,
> +				 struct notifier_block *nb)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&uchger->lock, flags);
> +	raw_notifier_chain_register(&uchger->uchger_nh, nb);
> +	spin_unlock_irqrestore(&uchger->lock, flags);
> +}
> +
> +/*
> + * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be unregistered.
> + */
> +void usb_charger_unregister_notify(struct usb_charger *uchger,
> +				   struct notifier_block *nb)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&uchger->lock, flags);
> +	raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
> +	spin_unlock_irqrestore(&uchger->lock, flags);
> +}
> +
> +/*
> + * usb_charger_register_extcon_notifier() - Register a notifiee of the usb
> + * 		charger to get notified by any attach status changes from
> + * 		the extcon device.
> + * @uchger - the usb charger device.
> + * @edev - the extcon device.
> + * @extcon_id - extcon id.
> + */
> +int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
> +					 struct extcon_dev *edev,
> +					 unsigned int extcon_id)
> +{
> +	if (!uchger || !edev)
> +		return -EINVAL;
> +
> +	return extcon_register_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
> +}

Why do we need wrappers around extcon?  I thought extcon was supposed to
do all of this for us, why are we putting another layer on top of it?

> +static void usb_charger_release(struct device *dev)
> +{
> +	struct usb_charger *uchger = dev_get_drvdata(dev);
> +
> +	if (!atomic_dec_and_test(&uchger->count)) {
> +		dev_err(dev, "The usb charger is still in use\n");

Why is the "count" different from the reference count?  You shouldn't be
in this function if the reference count is not 0, so tie your "user"
count to this one.  Having two different reference counts is a nightmare
and almost impossible to get right.  And a huge red flag that the design
is incorrect.

> +		return;

You can't "fail" a release call, so you just leaked memory all over the
floor here :(

> +/*
> + * usb_charger_register() - Register a new usb charger device.
> + * @uchger - the new usb charger device.

No, you should create the new charger device, as this subsystem now owns
the life cycle.  Don't rely on someone else to pass you an already
created structure.

> + *
> + */
> +int usb_charger_register(struct device *dev, struct usb_charger *uchger)
> +{
> +	static atomic_t uchger_no = ATOMIC_INIT(-1);

Use an idr/ida structure, don't try to roll your own logic here for
stuff that was long done for you.


> +	struct usb_charger *tmp;
> +	int ret;
> +
> +	if (!uchger) {
> +		dev_err(dev, "no device provided for charger\n");
> +		return -EINVAL;
> +	}
> +
> +	uchger->dev.parent = dev;
> +	uchger->dev.release = usb_charger_release;
> +	dev_set_name(&uchger->dev, "usb-chger%lu",
> +		     (unsigned long)atomic_inc_return(&uchger_no));
> +
> +	ret = device_register(&uchger->dev);
> +	if (ret) {
> +		put_device(&uchger->dev);
> +		return ret;
> +	}
> +
> +	dev_set_drvdata(&uchger->dev, uchger);
> +
> +	mutex_lock(&usb_charger_list_lock);
> +	list_for_each_entry(tmp, &usb_charger_list, entry) {
> +		if (!(strcmp(tmp->name, uchger->name))) {
> +			mutex_unlock(&usb_charger_list_lock);
> +			ret = -EEXIST;
> +			goto out;
> +		}
> +	}
> +	list_add_tail(&uchger->entry, &usb_charger_list);

Why do you need a separate list?  This subsystem's bus structure should
own that list of devices, no need for a separate one (again, a huge red
flag that the design is not correct.)

I stopped here.  Please rebase on linux-next and resend.

thanks,

greg k-h

Re: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Greg KH]

On Thu, Aug 06, 2015 at 11:21:22AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Aug 06, 2015 at 03:03:47PM +0800, Baolin Wang wrote:
> > Currently the Linux kernel does not provide any standard integration of this
> > feature that integrates the USB subsystem with the system power regulation
> > provided by PMICs meaning that either vendors must add this in their kernels
> > or USB gadget devices based on Linux (such as mobile phones) may not behave
> > as they should.
> > 
> > Providing a standard framework for doing this in the kernel.
> 
> it's too late in this cycle to even start discussing this. i'll drop
> from my queue, please resend rebase on v4.3-rc1 once that's out.

Why should that matter?  Can't they just rebase on linux-next and we can
work it out from there?  the merge cycle is just for us maintainers to
worry about, people can submit code whenever they want to, it's up to us
as to what tree we merge it to (now or next).

thanks,

greg k-h

Re: [PATCH 1/2] gadget: Introduce the usb charger framework

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

On Thu, Aug 06, 2015 at 09:39:05AM -0700, Greg KH wrote:
> On Thu, Aug 06, 2015 at 03:03:48PM +0800, Baolin Wang wrote:

> > +static void usb_charger_release(struct device *dev)
> > +{
> > +	struct usb_charger *uchger = dev_get_drvdata(dev);

> > +	if (!atomic_dec_and_test(&uchger->count)) {
> > +		dev_err(dev, "The usb charger is still in use\n");

> Why is the "count" different from the reference count?  You shouldn't be
> in this function if the reference count is not 0, so tie your "user"
> count to this one.  Having two different reference counts is a nightmare
> and almost impossible to get right.  And a huge red flag that the design
> is incorrect.

> > +		return;

> You can't "fail" a release call, so you just leaked memory all over the
> floor here :(

Indeed.  I did discuss this with Baolin off list but I'd missed the
dynamic allocation of devices for some reason.

> > +	mutex_lock(&usb_charger_list_lock);
> > +	list_for_each_entry(tmp, &usb_charger_list, entry) {
> > +		if (!(strcmp(tmp->name, uchger->name))) {
> > +			mutex_unlock(&usb_charger_list_lock);
> > +			ret = -EEXIST;
> > +			goto out;
> > +		}
> > +	}
> > +	list_add_tail(&uchger->entry, &usb_charger_list);

> Why do you need a separate list?  This subsystem's bus structure should
> own that list of devices, no need for a separate one (again, a huge red
> flag that the design is not correct.)

Right, if we dynamically allocate a device per charger then the lifetime
issues should go away and we get a list for free.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Peter Chen]

On Thu, Aug 06, 2015 at 03:03:47PM +0800, Baolin Wang wrote:
> Currently the Linux kernel does not provide any standard integration of this
> feature that integrates the USB subsystem with the system power regulation
> provided by PMICs meaning that either vendors must add this in their kernels
> or USB gadget devices based on Linux (such as mobile phones) may not behave
> as they should.
> 
> Providing a standard framework for doing this in the kernel.

Baolin, thanks for introducing a framework for doing it, we do support
USB Charger for chipidea driver at internal tree, but it is specific 
for imx, and still have some problems to upstream due to need to
change some common code.

One suggestion, would you add your user next time? In that case, we can
know better for this framework.

> 
> Baolin Wang (2):
>   gadget: Introduce the usb charger framework
>   gadget: Support for the usb charger framework
> 
>  drivers/usb/gadget/charger.c      |  547 +++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/udc/udc-core.c |   41 +++
>  include/linux/usb/gadget.h        |   20 ++
>  include/linux/usb/usb_charger.h   |  101 +++++++
>  4 files changed, 709 insertions(+)
>  create mode 100644 drivers/usb/gadget/charger.c
>  create mode 100644 include/linux/usb/usb_charger.h
> 
> -- 
> 1.7.9.5
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/2] gadget: Introduce the usb charger framework

[Peter Chen]

On Thu, Aug 06, 2015 at 03:03:48PM +0800, Baolin Wang wrote:
> This patch introduces the usb charger driver based on usb gadget that
> makes an enhancement to a power driver. It works well in practice but
> that requires a system with suitable hardware.
> 
> The basic conception of the usb charger is that, when one usb charger
> is added or removed by reporting from the usb gadget state change or
> the extcon device state change, the usb charger will report to power
> user to set the current limitation.
> 
> The usb charger will register notifiees on the usb gadget or the extcon
> device to get notified the usb charger state.
> 
> Power user will register a notifiee on the usb charger to get notified
> by status changes from the usb charger. It will report to power user
> to set the current limitation when detecting the usb charger is added
> or removed from extcon device state or usb gadget state.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/gadget/charger.c    |  547 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/usb_charger.h |  101 ++++++++
>  2 files changed, 648 insertions(+)
>  create mode 100644 drivers/usb/gadget/charger.c
>  create mode 100644 include/linux/usb/usb_charger.h
> 
> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
> new file mode 100644
> index 0000000..3ca0180
> --- /dev/null
> +++ b/drivers/usb/gadget/charger.c
> @@ -0,0 +1,547 @@
> +/*
> + * usb charger.c -- USB charger driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/extcon.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/usb_charger.h>
> +
> +#define DEFAULT_CUR_PROTECT	(50)
> +#define DEFAULT_SDP_CUR_LIMIT	(500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_DCP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_CDP_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +#define DEFAULT_ACA_CUR_LIMIT	(1500 - DEFAULT_CUR_PROTECT)
> +
> +static LIST_HEAD(usb_charger_list);
> +static DEFINE_MUTEX(usb_charger_list_lock);
> +
> +/*
> + * usb_charger_find_by_name - Get the usb charger device by name.
> + * @name - usb charger device name.
> + *
> + * notes: when this function walks the list and returns a charger
> + * it's dropped the lock which means that something else could come
> + * along and delete the charger before we dereference the pointer.
> + * It's very unlikely but it's a possibility so you should take care
> + * of it.
> + * Thus when you get the usb charger by name, you should call
> + * put_usb_charger() to derease the reference count of the usb charger.
> + *
> + * return the instance of usb charger device.
> + */
> +struct usb_charger *usb_charger_find_by_name(char *name)
> +{
> +	struct usb_charger *uchger;
> +
> +	if (!name)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&usb_charger_list_lock);
> +	list_for_each_entry(uchger, &usb_charger_list, entry) {
> +		if (!strcmp(uchger->name, name)) {
> +			get_usb_charger(uchger);
> +			mutex_unlock(&usb_charger_list_lock);
> +			return uchger;
> +		}
> +	}
> +	mutex_unlock(&usb_charger_list_lock);
> +
> +	return NULL;
> +}
> +
> +/*
> + * usb_charger_register_notify() - Register a notifiee to get notified by
> + * 		any attach status changes from the usb charger type detection.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be registered.
> + */
> +void usb_charger_register_notify(struct usb_charger *uchger,
> +				 struct notifier_block *nb)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&uchger->lock, flags);
> +	raw_notifier_chain_register(&uchger->uchger_nh, nb);
> +	spin_unlock_irqrestore(&uchger->lock, flags);
> +}
> +
> +/*
> + * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be unregistered.
> + */
> +void usb_charger_unregister_notify(struct usb_charger *uchger,
> +				   struct notifier_block *nb)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&uchger->lock, flags);
> +	raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
> +	spin_unlock_irqrestore(&uchger->lock, flags);
> +}
> +
> +/*
> + * usb_charger_register_extcon_notifier() - Register a notifiee of the usb
> + * 		charger to get notified by any attach status changes from
> + * 		the extcon device.
> + * @uchger - the usb charger device.
> + * @edev - the extcon device.
> + * @extcon_id - extcon id.
> + */
> +int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
> +					 struct extcon_dev *edev,
> +					 unsigned int extcon_id)
> +{
> +	if (!uchger || !edev)
> +		return -EINVAL;
> +
> +	return extcon_register_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
> +}
> +
> +/*
> + * usb_charger_unregister_extcon_notifier() - Unregister a notifiee of the
> + * 		 usb charger from the extcon device.
> + * @uchger - the usb charger device.
> + * @edev - the extcon device.
> + * @extcon_id - extcon id.
> + */
> +int usb_charger_unregister_extcon_notifier(struct usb_charger *uchger,
> +					   struct extcon_dev *edev,
> +					   unsigned int extcon_id)
> +{
> +	if (!uchger || !edev)
> +		return -EINVAL;
> +
> +	return extcon_unregister_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
> +}
> +
> +/*
> + * usb_charger_register_gadget_notifier() - Register a notifiee of the usb
> + * 		charger to get notified by any attach status changes from
> + * 		the usb gadget device.
> + * @uchger - the usb charger device.
> + */
> +int usb_charger_register_gadget_notifier(struct usb_charger *uchger)
> +{
> +	struct usb_gadget *ugadget = uchger->gadget;
> +
> +	return usb_gadget_register_notify(ugadget, &uchger->gadget_nb);
> +}

usb_gadget_register_notify is defined at your 2nd patch, it will
cause 'git bisect' problem.

> +
> +/*
> + * usb_charger_unregister_gadget_notifier() - Unregister a notifiee of the usb
> + * 		charger from the usb gadget device.
> + * @uchger - the usb charger device.
> + */
> +int usb_charger_unregister_gadget_notifier(struct usb_charger *uchger)
> +{
> +	struct usb_gadget *ugadget = uchger->gadget;
> +
> +	return usb_gadget_unregister_notify(ugadget, &uchger->gadget_nb);
> +}
> +
> +/*
> + * usb_charger_set_cur_limit() - Set the current limitation.
> + * @uchger - the usb charger device.
> + * @xxx_cur - the current limit by different charger type.
> + *
> + */
> +int usb_charger_set_cur_limit(struct usb_charger *uchger,
> +			      struct usb_charger_cur_limit *cur_limit_set)
> +{
> +	if (!uchger || !cur_limit_set)
> +		return -EINVAL;
> +
> +	uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
> +	uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
> +	uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
> +	uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
> +	return 0;
> +}
> +
> +/*
> + * usb_charger_get_cur_limit() - Get the current limitation by different usb
> + * 		charger type.
> + * @uchger - the usb charger device.
> + * @type - the usb charger type.
> + *
> + * return the current limitation to set.
> + */
> +static unsigned int
> +usb_charger_get_cur_limit(struct usb_charger *uchger)
> +{
> +	enum usb_charger_type uchger_type = uchger->type;
> +	unsigned int cur_limit;
> +
> +	switch (uchger_type) {
> +	case SDP_TYPE:
> +		cur_limit = uchger->cur_limit.sdp_cur_limit;
> +		break;
> +	case DCP_TYPE:
> +		cur_limit = uchger->cur_limit.dcp_cur_limit;
> +		break;
> +	case CDP_TYPE:
> +		cur_limit = uchger->cur_limit.cdp_cur_limit;
> +		break;
> +	case ACA_TYPE:
> +		cur_limit = uchger->cur_limit.aca_cur_limit;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return cur_limit;
> +}
> +
> +/*
> + * usb_charger_detect_type() - Get the usb charger type by the callback which is
> + * 		implemented by user.
> + * @uchger - the usb charger device.
> + *
> + * return the usb charger type.
> + */
> +enum usb_charger_type
> +usb_charger_detect_type(struct usb_charger *uchger)
> +{
> +	if (uchger->gadget && uchger->gadget->ops
> +	    && uchger->gadget->ops->get_charger_type)
> +		uchger->type =
> +			uchger->gadget->ops->get_charger_type(uchger->gadget);
> +	else
> +		uchger->type = UNKNOWN_TYPE;
> +
> +	return uchger->type;
> +}
> +
> +/*
> + * usb_charger_notifier_others() - It will notify other device registered on
> + * 		usb charger.
> + * @uchger - the usb charger device.
> + *
> + */
> +static void
> +usb_charger_notify_others(struct usb_charger *uchger,
> +			  enum usb_charger_state state)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&uchger->lock, flags);

it is better to use mutex_lock instead of disabling interrupt since
checking charger type may cause sleeping.

> +	uchger->state = state;
> +
> +	switch (state) {
> +	case USB_CHARGER_PRESENT:
> +		usb_charger_detect_type(uchger);
> +		raw_notifier_call_chain(&uchger->uchger_nh,
> +			usb_charger_get_cur_limit(uchger),
> +			uchger);
> +		break;
> +	case USB_CHARGER_REMOVE:
> +		uchger->type = UNKNOWN_TYPE;
> +		raw_notifier_call_chain(&uchger->uchger_nh, 0, uchger);
> +		break;
> +	default:
> +		dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
> +			 state);
> +		break;
> +	}
> +	spin_unlock_irqrestore(&uchger->lock, flags);
> +}
> +
> +/*
> + * usb_charger_plug_by_extcon() - The notifier call function which is registered
> + * 		on the extcon device.
> + * @nb - thr notifier block that notified by extcon device.
> + * @state - the extcon device state changed.
> + * @data - here specify a extcon device.
> + *
> + * return the notify flag.
> + */
> +static int
> +usb_charger_plug_by_extcon(struct notifier_block *nb,
> +			   unsigned long state, void *data)
> +{
> +	struct usb_charger_nb *extcon_nb =
> +		container_of(nb, struct usb_charger_nb, nb);
> +	struct usb_charger *uchger = extcon_nb->uchger;
> +	enum usb_charger_state uchger_state;
> +
> +	if (!uchger)
> +		return NOTIFY_BAD;
> +
> +	/* Report event to power to setting the current limitation
> +	 * for this usb charger when one usb charger is added or removed
> +	 * with detecting by extcon device.
> +	 */
> +	if (state)
> +		uchger_state = USB_CHARGER_PRESENT;
> +	else
> +		uchger_state = USB_CHARGER_REMOVE;
> +
> +	usb_charger_notify_others(uchger, uchger_state);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/*
> + * usb_charger_plug_by_gadget() - Set the usb charger current limitation
> + * 		according to the usb gadget device state.
> + * @data - here specify a usb charger device.
> + *
> + */
> +static int
> +usb_charger_plug_by_gadget(struct notifier_block *nb,
> +			   unsigned long state, void *data)
> +{
> +	struct usb_gadget *gadget = (struct usb_gadget *)data;
> +	struct usb_charger *uchger = gadget->uchger;
> +	enum usb_charger_state uchger_state;
> +
> +	if (!uchger)
> +		return NOTIFY_BAD;
> +
> +	/* Report event to power to setting the current limitation
> +	 * for this usb charger when one usb charger state is changed
> +	 * with detecting by usb gadget state.
> +	 */
> +	if (uchger->old_gadget_state != state) {
> +		uchger->old_gadget_state = state;
> +
> +		if (state >= USB_STATE_ATTACHED)
> +			uchger_state = USB_CHARGER_PRESENT;
> +		else if (state == USB_STATE_NOTATTACHED)
> +			uchger_state = USB_CHARGER_REMOVE;
> +		else
> +			uchger_state = USB_CHARGER_DEFAULT;
> +
> +		usb_charger_notify_others(uchger, uchger_state);
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int devm_uchger_dev_match(struct device *dev, void *res, void *data)
> +{
> +	struct usb_charger **r = res;
> +
> +	if (WARN_ON(!r || !*r))
> +		return 0;
> +
> +	return *r == data;
> +}
> +
> +static void usb_charger_release(struct device *dev)
> +{
> +	struct usb_charger *uchger = dev_get_drvdata(dev);
> +
> +	if (!atomic_dec_and_test(&uchger->count)) {
> +		dev_err(dev, "The usb charger is still in use\n");
> +		return;
> +	}
> +
> +	kfree(uchger->name);
> +	kfree(uchger);
> +}
> +
> +/*
> + * usb_charger_unregister() - Unregister a usb charger device.
> + * @uchger - the usb charger device.
> + *
> + */
> +int usb_charger_unregister(struct usb_charger *uchger)
> +{
> +	if (!uchger)
> +		return -EINVAL;
> +
> +	mutex_lock(&usb_charger_list_lock);
> +	list_del(&uchger->entry);
> +	mutex_unlock(&usb_charger_list_lock);
> +
> +	device_unregister(&uchger->dev);
> +	return 0;
> +}
> +
> +static void devm_uchger_dev_unreg(struct device *dev, void *res)
> +{
> +	usb_charger_unregister(*(struct usb_charger **)res);
> +}
> +
> +void devm_usb_charger_unregister(struct device *dev,
> +				 struct usb_charger *uchger)
> +{
> +	devres_release(dev, devm_uchger_dev_unreg,
> +		       devm_uchger_dev_match, uchger);
> +}
> +
> +/*
> + * usb_charger_register() - Register a new usb charger device.
> + * @uchger - the new usb charger device.
> + *
> + */
> +int usb_charger_register(struct device *dev, struct usb_charger *uchger)
> +{
> +	static atomic_t uchger_no = ATOMIC_INIT(-1);
> +	struct usb_charger *tmp;
> +	int ret;
> +
> +	if (!uchger) {
> +		dev_err(dev, "no device provided for charger\n");
> +		return -EINVAL;
> +	}
> +
> +	uchger->dev.parent = dev;
> +	uchger->dev.release = usb_charger_release;
> +	dev_set_name(&uchger->dev, "usb-chger%lu",
> +		     (unsigned long)atomic_inc_return(&uchger_no));

For the name of usb-charger, which not using "usb-charger.x" directly?
> +
> +	ret = device_register(&uchger->dev);
> +	if (ret) {
> +		put_device(&uchger->dev);
> +		return ret;
> +	}
> +
> +	dev_set_drvdata(&uchger->dev, uchger);
> +
> +	mutex_lock(&usb_charger_list_lock);
> +	list_for_each_entry(tmp, &usb_charger_list, entry) {
> +		if (!(strcmp(tmp->name, uchger->name))) {
> +			mutex_unlock(&usb_charger_list_lock);
> +			ret = -EEXIST;
> +			goto out;
> +		}
> +	}
> +	list_add_tail(&uchger->entry, &usb_charger_list);
> +	mutex_unlock(&usb_charger_list_lock);
> +
> +	return 0;
> +
> +out:
> +	dev_err(dev, "Failed to register usb charger (%s)\n",
> +		uchger->name);
> +	device_unregister(&uchger->dev);
> +	put_device(&uchger->dev);
> +	return ret;
> +}
> +
> +int devm_usb_charger_register(struct device *dev,
> +			      struct usb_charger *uchger)
> +{
> +	struct usb_charger **ptr;
> +	int ret;
> +
> +	ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = usb_charger_register(dev, uchger);
> +	if (ret) {
> +		devres_free(ptr);
> +		return ret;
> +	}
> +
> +	*ptr = uchger;
> +	devres_add(dev, ptr);
> +
> +	return 0;
> +}
> +
> +int usb_charger_init(struct usb_gadget *ugadget)
> +{
> +	struct usb_charger *uchger;
> +	struct extcon_dev *edev;
> +	char buf[100];
> +	char *str;
> +	int ret;
> +
> +	if (!ugadget)
> +		return -EINVAL;
> +
> +	uchger = devm_kzalloc(&ugadget->dev, sizeof(struct usb_charger),
> +			      GFP_KERNEL);
> +	if (!uchger)
> +		return -ENOMEM;
> +
> +	sprintf(buf, "usb-charger.%s", ugadget->name);
> +	str = devm_kzalloc(&ugadget->dev, sizeof(char) * (strlen(buf) + 1),
> +			   GFP_KERNEL);
> +	if (!str)
> +		return -ENOMEM;
> +
> +	strcpy(str, buf);
> +	uchger->name = str;
> +	uchger->type = UNKNOWN_TYPE;
> +	uchger->state = USB_CHARGER_DEFAULT;
> +	uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT;
> +	uchger->cur_limit.dcp_cur_limit = DEFAULT_DCP_CUR_LIMIT;
> +	uchger->cur_limit.cdp_cur_limit = DEFAULT_CDP_CUR_LIMIT;
> +	uchger->cur_limit.aca_cur_limit = DEFAULT_ACA_CUR_LIMIT;
> +
> +	atomic_set(&uchger->count, 1);
> +	spin_lock_init(&uchger->lock);
> +	INIT_LIST_HEAD(&uchger->entry);
> +	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
> +
> +	/* register a notifier on a extcon device if it is exsited */
> +	edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
> +	if (!IS_ERR_OR_NULL(edev)) {
> +		uchger->extcon_dev = edev;
> +		uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
> +		uchger->extcon_nb.uchger = uchger;
> +		usb_charger_register_extcon_notifier(uchger, edev, EXTCON_USB);
> +	}
> +
> +	/* register a notifier on a usb gadget device */
> +	uchger->gadget = ugadget;
> +	ugadget->uchger = uchger;
> +	uchger->old_gadget_state = ugadget->state;
> +	uchger->gadget_nb.notifier_call = usb_charger_plug_by_gadget;
> +	usb_charger_register_gadget_notifier(uchger);
> +
> +	ret = usb_charger_register(&ugadget->dev, uchger);
> +	if (ret)
> +		goto reg_fail;
> +
> +	return 0;
> +
> +reg_fail:
> +	if (uchger->extcon_dev)
> +		usb_charger_unregister_extcon_notifier(uchger,
> +				uchger->extcon_dev, EXTCON_USB);
> +
> +	usb_charger_unregister_gadget_notifier(uchger);
> +	return ret;
> +}
> +
> +int usb_charger_exit(struct usb_gadget *ugadget)
> +{
> +	struct usb_charger *uchger = ugadget->uchger;
> +
> +	if (!uchger)
> +		return -EINVAL;
> +
> +	if (uchger->extcon_dev)
> +		usb_charger_unregister_extcon_notifier(uchger,
> +				uchger->extcon_dev, EXTCON_USB);
> +
> +	usb_charger_unregister_gadget_notifier(uchger);
> +	return usb_charger_unregister(uchger);
> +}
> +
> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
> +MODULE_DESCRIPTION("USB charger driver");
> diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
> new file mode 100644
> index 0000000..da4d3c9
> --- /dev/null
> +++ b/include/linux/usb/usb_charger.h
> @@ -0,0 +1,101 @@
> +#ifndef __LINUX_USB_CHARGER_H__
> +#define __LINUX_USB_CHARGER_H__
> +
> +#include <linux/device.h>
> +#include <linux/notifier.h>
> +#include <linux/sysfs.h>
> +#include <linux/usb/ch9.h>
> +
> +/* USB charger type:
> + * SDP (Standard Downstream Port)
> + * DCP (Dedicated Charging Port)
> + * CDP (Charging Downstream Port)
> + * ACA (Accessory Charger Adapters)
> + */
> +enum usb_charger_type {
> +	UNKNOWN_TYPE,
> +	SDP_TYPE,
> +	DCP_TYPE,
> +	CDP_TYPE,
> +	ACA_TYPE,
> +};
> +
> +/* USB charger state */
> +enum usb_charger_state {
> +	USB_CHARGER_DEFAULT,
> +	USB_CHARGER_PRESENT,
> +	USB_CHARGER_REMOVE,
> +};
> +
> +/* Current limitation by charger type */
> +struct usb_charger_cur_limit {
> +	unsigned int sdp_cur_limit;
> +	unsigned int dcp_cur_limit;
> +	unsigned int cdp_cur_limit;
> +	unsigned int aca_cur_limit;
> +};
> +
> +struct usb_charger_nb {
> +	struct notifier_block	nb;
> +	struct usb_charger	*uchger;
> +};
> +
> +struct usb_charger {
> +	/* Internal data. Please do not set. */
> +	const char		*name;
> +	struct device		dev;
> +	struct raw_notifier_head	uchger_nh;
> +	struct list_head	entry;
> +	spinlock_t		lock;
> +	enum usb_charger_type	type;
> +	enum usb_charger_state	state;
> +	atomic_t		count;
> +
> +	/* For supporting extcon usb gpio */
> +	struct extcon_dev	*extcon_dev;
> +	struct usb_charger_nb	extcon_nb;
> +
> +	/* For supporting usb gadget */
> +	struct usb_gadget	*gadget;
> +	enum usb_device_state	old_gadget_state;
> +	struct notifier_block	gadget_nb;
> +
> +	/* Current limitation */
> +	struct usb_charger_cur_limit	cur_limit;
> +};
> +
> +extern struct usb_charger *usb_charger_find_by_name(char *name);
> +
> +extern void usb_charger_register_notify(struct usb_charger *uchger,
> +					struct notifier_block *nb);
> +extern void usb_charger_unregister_notify(struct usb_charger *uchger,
> +					  struct notifier_block *nb);
> +
> +extern int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
> +						struct extcon_dev *edev,
> +						unsigned int extcon_id);
> +extern int usb_charger_unregister_extcon_notifier(struct usb_charger *uchger,
> +						  struct extcon_dev *edev,
> +						  unsigned int extcon_id);
> +
> +extern int usb_charger_register_gadget_notifier(struct usb_charger *uchger);
> +extern int usb_charger_unregister_gadget_notifier(struct usb_charger *uchger);
> +
> +extern int usb_charger_set_cur_limit(struct usb_charger *uchger,
> +			      struct usb_charger_cur_limit *cur_limit_set);
> +extern enum usb_charger_type usb_charger_detect_type(struct usb_charger *uchger);
> +
> +extern int usb_charger_init(struct usb_gadget *ugadget);
> +extern int usb_charger_exit(struct usb_gadget *ugadget);
> +
> +static inline void get_usb_charger(struct usb_charger *uchger)
> +{
> +	atomic_inc(&uchger->count);
> +}
> +
> +static inline void put_usb_charger(struct usb_charger *uchger)
> +{
> +	atomic_dec(&uchger->count);
> +}
> +
> +#endif /* __LINUX_USB_CHARGER_H__ */
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

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

[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.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?

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/2] gadget: Introduce the usb charger framework

[Baolin Wang]

On 7 August 2015 at 00:39, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Aug 06, 2015 at 03:03:48PM +0800, Baolin Wang wrote:
>> This patch introduces the usb charger driver based on usb gadget that
>> makes an enhancement to a power driver. It works well in practice but
>> that requires a system with suitable hardware.
>>
>> The basic conception of the usb charger is that, when one usb charger
>> is added or removed by reporting from the usb gadget state change or
>> the extcon device state change, the usb charger will report to power
>> user to set the current limitation.
>>
>> The usb charger will register notifiees on the usb gadget or the extcon
>> device to get notified the usb charger state.
>>
>> Power user will register a notifiee on the usb charger to get notified
>> by status changes from the usb charger. It will report to power user
>> to set the current limitation when detecting the usb charger is added
>> or removed from extcon device state or usb gadget state.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/gadget/charger.c    |  547 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/usb_charger.h |  101 ++++++++
>>  2 files changed, 648 insertions(+)
>>  create mode 100644 drivers/usb/gadget/charger.c
>>  create mode 100644 include/linux/usb/usb_charger.h
>>
>> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
>> new file mode 100644
>> index 0000000..3ca0180
>> --- /dev/null
>> +++ b/drivers/usb/gadget/charger.c
>> @@ -0,0 +1,547 @@
>> +/*
>> + * usb charger.c -- USB charger driver
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
> I have to ask, do you really mean "any later version"?
>

I'll think about this and modify it.

>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/extcon.h>
>> +#include <linux/export.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/ch9.h>
>> +#include <linux/usb/gadget.h>
>> +#include <linux/usb/usb_charger.h>
>> +
>> +#define DEFAULT_CUR_PROTECT  (50)
>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_DCP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_CDP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_ACA_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +
>> +static LIST_HEAD(usb_charger_list);
>> +static DEFINE_MUTEX(usb_charger_list_lock);
>> +
>> +/*
>> + * usb_charger_find_by_name - Get the usb charger device by name.
>> + * @name - usb charger device name.
>> + *
>> + * notes: when this function walks the list and returns a charger
>> + * it's dropped the lock which means that something else could come
>> + * along and delete the charger before we dereference the pointer.
>> + * It's very unlikely but it's a possibility so you should take care
>> + * of it.
>> + * Thus when you get the usb charger by name, you should call
>> + * put_usb_charger() to derease the reference count of the usb charger.
>> + *
>> + * return the instance of usb charger device.
>> + */
>> +struct usb_charger *usb_charger_find_by_name(char *name)
>> +{
>> +     struct usb_charger *uchger;
>> +
>> +     if (!name)
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     mutex_lock(&usb_charger_list_lock);
>> +     list_for_each_entry(uchger, &usb_charger_list, entry) {
>> +             if (!strcmp(uchger->name, name)) {
>> +                     get_usb_charger(uchger);
>> +                     mutex_unlock(&usb_charger_list_lock);
>> +                     return uchger;
>> +             }
>> +     }
>> +     mutex_unlock(&usb_charger_list_lock);
>> +
>> +     return NULL;
>> +}
>> +
>> +/*
>> + * usb_charger_register_notify() - Register a notifiee to get notified by
>> + *           any attach status changes from the usb charger type detection.
>> + * @uchger - the usb charger device which is monitored.
>> + * @nb - a notifier block to be registered.
>> + */
>> +void usb_charger_register_notify(struct usb_charger *uchger,
>> +                              struct notifier_block *nb)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&uchger->lock, flags);
>> +     raw_notifier_chain_register(&uchger->uchger_nh, nb);
>> +     spin_unlock_irqrestore(&uchger->lock, flags);
>> +}
>> +
>> +/*
>> + * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
>> + * @uchger - the usb charger device which is monitored.
>> + * @nb - a notifier block to be unregistered.
>> + */
>> +void usb_charger_unregister_notify(struct usb_charger *uchger,
>> +                                struct notifier_block *nb)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&uchger->lock, flags);
>> +     raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
>> +     spin_unlock_irqrestore(&uchger->lock, flags);
>> +}
>> +
>> +/*
>> + * usb_charger_register_extcon_notifier() - Register a notifiee of the usb
>> + *           charger to get notified by any attach status changes from
>> + *           the extcon device.
>> + * @uchger - the usb charger device.
>> + * @edev - the extcon device.
>> + * @extcon_id - extcon id.
>> + */
>> +int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
>> +                                      struct extcon_dev *edev,
>> +                                      unsigned int extcon_id)
>> +{
>> +     if (!uchger || !edev)
>> +             return -EINVAL;
>> +
>> +     return extcon_register_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
>> +}
>
> Why do we need wrappers around extcon?  I thought extcon was supposed to
> do all of this for us, why are we putting another layer on top of it?
>

OK, I'll remove the wrapper.

>> +static void usb_charger_release(struct device *dev)
>> +{
>> +     struct usb_charger *uchger = dev_get_drvdata(dev);
>> +
>> +     if (!atomic_dec_and_test(&uchger->count)) {
>> +             dev_err(dev, "The usb charger is still in use\n");
>
> Why is the "count" different from the reference count?  You shouldn't be
> in this function if the reference count is not 0, so tie your "user"
> count to this one.  Having two different reference counts is a nightmare
> and almost impossible to get right.  And a huge red flag that the design
> is incorrect.
>

Make sense of it. I'll fix that.

>> +             return;
>
> You can't "fail" a release call, so you just leaked memory all over the
> floor here :(
>

Sorry, I'll think about here with the reference count.

>> +/*
>> + * usb_charger_register() - Register a new usb charger device.
>> + * @uchger - the new usb charger device.
>
> No, you should create the new charger device, as this subsystem now owns
> the life cycle.  Don't rely on someone else to pass you an already
> created structure.
>

Make sense.

>> + *
>> + */
>> +int usb_charger_register(struct device *dev, struct usb_charger *uchger)
>> +{
>> +     static atomic_t uchger_no = ATOMIC_INIT(-1);
>
> Use an idr/ida structure, don't try to roll your own logic here for
> stuff that was long done for you.
>

OK.

>
>> +     struct usb_charger *tmp;
>> +     int ret;
>> +
>> +     if (!uchger) {
>> +             dev_err(dev, "no device provided for charger\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     uchger->dev.parent = dev;
>> +     uchger->dev.release = usb_charger_release;
>> +     dev_set_name(&uchger->dev, "usb-chger%lu",
>> +                  (unsigned long)atomic_inc_return(&uchger_no));
>> +
>> +     ret = device_register(&uchger->dev);
>> +     if (ret) {
>> +             put_device(&uchger->dev);
>> +             return ret;
>> +     }
>> +
>> +     dev_set_drvdata(&uchger->dev, uchger);
>> +
>> +     mutex_lock(&usb_charger_list_lock);
>> +     list_for_each_entry(tmp, &usb_charger_list, entry) {
>> +             if (!(strcmp(tmp->name, uchger->name))) {
>> +                     mutex_unlock(&usb_charger_list_lock);
>> +                     ret = -EEXIST;
>> +                     goto out;
>> +             }
>> +     }
>> +     list_add_tail(&uchger->entry, &usb_charger_list);
>
> Why do you need a separate list?  This subsystem's bus structure should
> own that list of devices, no need for a separate one (again, a huge red
> flag that the design is not correct.)
>
> I stopped here.  Please rebase on linux-next and resend.
>

OK, Very sorry for the email reply before. I'll modify the problems
you point out. Very thanks for your comments.

> thanks,
>
> greg k-h



-- 
Baolin.wang
Best Regards

Re: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

On 7 August 2015 at 13:34, Peter Chen <peter.chen@freescale.com> wrote:
> On Thu, Aug 06, 2015 at 03:03:47PM +0800, Baolin Wang wrote:
>> Currently the Linux kernel does not provide any standard integration of this
>> feature that integrates the USB subsystem with the system power regulation
>> provided by PMICs meaning that either vendors must add this in their kernels
>> or USB gadget devices based on Linux (such as mobile phones) may not behave
>> as they should.
>>
>> Providing a standard framework for doing this in the kernel.
>
> Baolin, thanks for introducing a framework for doing it, we do support
> USB Charger for chipidea driver at internal tree, but it is specific
> for imx, and still have some problems to upstream due to need to
> change some common code.
>
> One suggestion, would you add your user next time? In that case, we can
> know better for this framework.
>

Peter, Thanks for your reviewing and comments. Now I just introduce
the framework to review for more feedbacks and do not have a useful
user to use it. I just can show you some example code to show how to
use it. Thanks.

>>
>> Baolin Wang (2):
>>   gadget: Introduce the usb charger framework
>>   gadget: Support for the usb charger framework
>>
>>  drivers/usb/gadget/charger.c      |  547 +++++++++++++++++++++++++++++++++++++
>>  drivers/usb/gadget/udc/udc-core.c |   41 +++
>>  include/linux/usb/gadget.h        |   20 ++
>>  include/linux/usb/usb_charger.h   |  101 +++++++
>>  4 files changed, 709 insertions(+)
>>  create mode 100644 drivers/usb/gadget/charger.c
>>  create mode 100644 include/linux/usb/usb_charger.h
>>
>> --
>> 1.7.9.5
>>
>
> --
>
> Best Regards,
> Peter Chen



-- 
Baolin.wang
Best Regards

RE: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Peter Chen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 533 bytes --]

 
> 
> Peter, Thanks for your reviewing and comments. Now I just introduce the
> framework to review for more feedbacks and do not have a useful user to use
> it. I just can show you some example code to show how to use it. Thanks.
> 

Felipe may not accept the code which are no user on it, I remember he said it before.
Besides, if no user on it, how you test it?

Peter
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] gadget: Introduce the usb charger framework

[Baolin Wang]

On 7 August 2015 at 13:41, Peter Chen <peter.chen@freescale.com> wrote:
> On Thu, Aug 06, 2015 at 03:03:48PM +0800, Baolin Wang wrote:
>> This patch introduces the usb charger driver based on usb gadget that
>> makes an enhancement to a power driver. It works well in practice but
>> that requires a system with suitable hardware.
>>
>> The basic conception of the usb charger is that, when one usb charger
>> is added or removed by reporting from the usb gadget state change or
>> the extcon device state change, the usb charger will report to power
>> user to set the current limitation.
>>
>> The usb charger will register notifiees on the usb gadget or the extcon
>> device to get notified the usb charger state.
>>
>> Power user will register a notifiee on the usb charger to get notified
>> by status changes from the usb charger. It will report to power user
>> to set the current limitation when detecting the usb charger is added
>> or removed from extcon device state or usb gadget state.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/gadget/charger.c    |  547 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/usb_charger.h |  101 ++++++++
>>  2 files changed, 648 insertions(+)
>>  create mode 100644 drivers/usb/gadget/charger.c
>>  create mode 100644 include/linux/usb/usb_charger.h
>>
>> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
>> new file mode 100644
>> index 0000000..3ca0180
>> --- /dev/null
>> +++ b/drivers/usb/gadget/charger.c
>> @@ -0,0 +1,547 @@
>> +/*
>> + * usb charger.c -- USB charger driver
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/extcon.h>
>> +#include <linux/export.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/ch9.h>
>> +#include <linux/usb/gadget.h>
>> +#include <linux/usb/usb_charger.h>
>> +
>> +#define DEFAULT_CUR_PROTECT  (50)
>> +#define DEFAULT_SDP_CUR_LIMIT        (500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_DCP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_CDP_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +#define DEFAULT_ACA_CUR_LIMIT        (1500 - DEFAULT_CUR_PROTECT)
>> +
>> +static LIST_HEAD(usb_charger_list);
>> +static DEFINE_MUTEX(usb_charger_list_lock);
>> +
>> +/*
>> + * usb_charger_find_by_name - Get the usb charger device by name.
>> + * @name - usb charger device name.
>> + *
>> + * notes: when this function walks the list and returns a charger
>> + * it's dropped the lock which means that something else could come
>> + * along and delete the charger before we dereference the pointer.
>> + * It's very unlikely but it's a possibility so you should take care
>> + * of it.
>> + * Thus when you get the usb charger by name, you should call
>> + * put_usb_charger() to derease the reference count of the usb charger.
>> + *
>> + * return the instance of usb charger device.
>> + */
>> +struct usb_charger *usb_charger_find_by_name(char *name)
>> +{
>> +     struct usb_charger *uchger;
>> +
>> +     if (!name)
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     mutex_lock(&usb_charger_list_lock);
>> +     list_for_each_entry(uchger, &usb_charger_list, entry) {
>> +             if (!strcmp(uchger->name, name)) {
>> +                     get_usb_charger(uchger);
>> +                     mutex_unlock(&usb_charger_list_lock);
>> +                     return uchger;
>> +             }
>> +     }
>> +     mutex_unlock(&usb_charger_list_lock);
>> +
>> +     return NULL;
>> +}
>> +
>> +/*
>> + * usb_charger_register_notify() - Register a notifiee to get notified by
>> + *           any attach status changes from the usb charger type detection.
>> + * @uchger - the usb charger device which is monitored.
>> + * @nb - a notifier block to be registered.
>> + */
>> +void usb_charger_register_notify(struct usb_charger *uchger,
>> +                              struct notifier_block *nb)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&uchger->lock, flags);
>> +     raw_notifier_chain_register(&uchger->uchger_nh, nb);
>> +     spin_unlock_irqrestore(&uchger->lock, flags);
>> +}
>> +
>> +/*
>> + * usb_charger_unregister_notify() - Unregister a notifiee from the usb charger.
>> + * @uchger - the usb charger device which is monitored.
>> + * @nb - a notifier block to be unregistered.
>> + */
>> +void usb_charger_unregister_notify(struct usb_charger *uchger,
>> +                                struct notifier_block *nb)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&uchger->lock, flags);
>> +     raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
>> +     spin_unlock_irqrestore(&uchger->lock, flags);
>> +}
>> +
>> +/*
>> + * usb_charger_register_extcon_notifier() - Register a notifiee of the usb
>> + *           charger to get notified by any attach status changes from
>> + *           the extcon device.
>> + * @uchger - the usb charger device.
>> + * @edev - the extcon device.
>> + * @extcon_id - extcon id.
>> + */
>> +int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
>> +                                      struct extcon_dev *edev,
>> +                                      unsigned int extcon_id)
>> +{
>> +     if (!uchger || !edev)
>> +             return -EINVAL;
>> +
>> +     return extcon_register_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
>> +}
>> +
>> +/*
>> + * usb_charger_unregister_extcon_notifier() - Unregister a notifiee of the
>> + *            usb charger from the extcon device.
>> + * @uchger - the usb charger device.
>> + * @edev - the extcon device.
>> + * @extcon_id - extcon id.
>> + */
>> +int usb_charger_unregister_extcon_notifier(struct usb_charger *uchger,
>> +                                        struct extcon_dev *edev,
>> +                                        unsigned int extcon_id)
>> +{
>> +     if (!uchger || !edev)
>> +             return -EINVAL;
>> +
>> +     return extcon_unregister_notifier(edev, extcon_id, &uchger->extcon_nb.nb);
>> +}
>> +
>> +/*
>> + * usb_charger_register_gadget_notifier() - Register a notifiee of the usb
>> + *           charger to get notified by any attach status changes from
>> + *           the usb gadget device.
>> + * @uchger - the usb charger device.
>> + */
>> +int usb_charger_register_gadget_notifier(struct usb_charger *uchger)
>> +{
>> +     struct usb_gadget *ugadget = uchger->gadget;
>> +
>> +     return usb_gadget_register_notify(ugadget, &uchger->gadget_nb);
>> +}
>
> usb_gadget_register_notify is defined at your 2nd patch, it will
> cause 'git bisect' problem.
>

Oh, I miss that and will fix that.

>> +
>> +/*
>> + * usb_charger_unregister_gadget_notifier() - Unregister a notifiee of the usb
>> + *           charger from the usb gadget device.
>> + * @uchger - the usb charger device.
>> + */
>> +int usb_charger_unregister_gadget_notifier(struct usb_charger *uchger)
>> +{
>> +     struct usb_gadget *ugadget = uchger->gadget;
>> +
>> +     return usb_gadget_unregister_notify(ugadget, &uchger->gadget_nb);
>> +}
>> +
>> +/*
>> + * usb_charger_set_cur_limit() - Set the current limitation.
>> + * @uchger - the usb charger device.
>> + * @xxx_cur - the current limit by different charger type.
>> + *
>> + */
>> +int usb_charger_set_cur_limit(struct usb_charger *uchger,
>> +                           struct usb_charger_cur_limit *cur_limit_set)
>> +{
>> +     if (!uchger || !cur_limit_set)
>> +             return -EINVAL;
>> +
>> +     uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
>> +     uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
>> +     uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
>> +     uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
>> +     return 0;
>> +}
>> +
>> +/*
>> + * usb_charger_get_cur_limit() - Get the current limitation by different usb
>> + *           charger type.
>> + * @uchger - the usb charger device.
>> + * @type - the usb charger type.
>> + *
>> + * return the current limitation to set.
>> + */
>> +static unsigned int
>> +usb_charger_get_cur_limit(struct usb_charger *uchger)
>> +{
>> +     enum usb_charger_type uchger_type = uchger->type;
>> +     unsigned int cur_limit;
>> +
>> +     switch (uchger_type) {
>> +     case SDP_TYPE:
>> +             cur_limit = uchger->cur_limit.sdp_cur_limit;
>> +             break;
>> +     case DCP_TYPE:
>> +             cur_limit = uchger->cur_limit.dcp_cur_limit;
>> +             break;
>> +     case CDP_TYPE:
>> +             cur_limit = uchger->cur_limit.cdp_cur_limit;
>> +             break;
>> +     case ACA_TYPE:
>> +             cur_limit = uchger->cur_limit.aca_cur_limit;
>> +             break;
>> +     default:
>> +             return 0;
>> +     }
>> +
>> +     return cur_limit;
>> +}
>> +
>> +/*
>> + * usb_charger_detect_type() - Get the usb charger type by the callback which is
>> + *           implemented by user.
>> + * @uchger - the usb charger device.
>> + *
>> + * return the usb charger type.
>> + */
>> +enum usb_charger_type
>> +usb_charger_detect_type(struct usb_charger *uchger)
>> +{
>> +     if (uchger->gadget && uchger->gadget->ops
>> +         && uchger->gadget->ops->get_charger_type)
>> +             uchger->type =
>> +                     uchger->gadget->ops->get_charger_type(uchger->gadget);
>> +     else
>> +             uchger->type = UNKNOWN_TYPE;
>> +
>> +     return uchger->type;
>> +}
>> +
>> +/*
>> + * usb_charger_notifier_others() - It will notify other device registered on
>> + *           usb charger.
>> + * @uchger - the usb charger device.
>> + *
>> + */
>> +static void
>> +usb_charger_notify_others(struct usb_charger *uchger,
>> +                       enum usb_charger_state state)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&uchger->lock, flags);
>
> it is better to use mutex_lock instead of disabling interrupt since
> checking charger type may cause sleeping.
>

Yes, I'll think about the lock again.

>> +     uchger->state = state;
>> +
>> +     switch (state) {
>> +     case USB_CHARGER_PRESENT:
>> +             usb_charger_detect_type(uchger);
>> +             raw_notifier_call_chain(&uchger->uchger_nh,
>> +                     usb_charger_get_cur_limit(uchger),
>> +                     uchger);
>> +             break;
>> +     case USB_CHARGER_REMOVE:
>> +             uchger->type = UNKNOWN_TYPE;
>> +             raw_notifier_call_chain(&uchger->uchger_nh, 0, uchger);
>> +             break;
>> +     default:
>> +             dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
>> +                      state);
>> +             break;
>> +     }
>> +     spin_unlock_irqrestore(&uchger->lock, flags);
>> +}
>> +
>> +/*
>> + * usb_charger_plug_by_extcon() - The notifier call function which is registered
>> + *           on the extcon device.
>> + * @nb - thr notifier block that notified by extcon device.
>> + * @state - the extcon device state changed.
>> + * @data - here specify a extcon device.
>> + *
>> + * return the notify flag.
>> + */
>> +static int
>> +usb_charger_plug_by_extcon(struct notifier_block *nb,
>> +                        unsigned long state, void *data)
>> +{
>> +     struct usb_charger_nb *extcon_nb =
>> +             container_of(nb, struct usb_charger_nb, nb);
>> +     struct usb_charger *uchger = extcon_nb->uchger;
>> +     enum usb_charger_state uchger_state;
>> +
>> +     if (!uchger)
>> +             return NOTIFY_BAD;
>> +
>> +     /* Report event to power to setting the current limitation
>> +      * for this usb charger when one usb charger is added or removed
>> +      * with detecting by extcon device.
>> +      */
>> +     if (state)
>> +             uchger_state = USB_CHARGER_PRESENT;
>> +     else
>> +             uchger_state = USB_CHARGER_REMOVE;
>> +
>> +     usb_charger_notify_others(uchger, uchger_state);
>> +
>> +     return NOTIFY_OK;
>> +}
>> +
>> +/*
>> + * usb_charger_plug_by_gadget() - Set the usb charger current limitation
>> + *           according to the usb gadget device state.
>> + * @data - here specify a usb charger device.
>> + *
>> + */
>> +static int
>> +usb_charger_plug_by_gadget(struct notifier_block *nb,
>> +                        unsigned long state, void *data)
>> +{
>> +     struct usb_gadget *gadget = (struct usb_gadget *)data;
>> +     struct usb_charger *uchger = gadget->uchger;
>> +     enum usb_charger_state uchger_state;
>> +
>> +     if (!uchger)
>> +             return NOTIFY_BAD;
>> +
>> +     /* Report event to power to setting the current limitation
>> +      * for this usb charger when one usb charger state is changed
>> +      * with detecting by usb gadget state.
>> +      */
>> +     if (uchger->old_gadget_state != state) {
>> +             uchger->old_gadget_state = state;
>> +
>> +             if (state >= USB_STATE_ATTACHED)
>> +                     uchger_state = USB_CHARGER_PRESENT;
>> +             else if (state == USB_STATE_NOTATTACHED)
>> +                     uchger_state = USB_CHARGER_REMOVE;
>> +             else
>> +                     uchger_state = USB_CHARGER_DEFAULT;
>> +
>> +             usb_charger_notify_others(uchger, uchger_state);
>> +     }
>> +
>> +     return NOTIFY_OK;
>> +}
>> +
>> +static int devm_uchger_dev_match(struct device *dev, void *res, void *data)
>> +{
>> +     struct usb_charger **r = res;
>> +
>> +     if (WARN_ON(!r || !*r))
>> +             return 0;
>> +
>> +     return *r == data;
>> +}
>> +
>> +static void usb_charger_release(struct device *dev)
>> +{
>> +     struct usb_charger *uchger = dev_get_drvdata(dev);
>> +
>> +     if (!atomic_dec_and_test(&uchger->count)) {
>> +             dev_err(dev, "The usb charger is still in use\n");
>> +             return;
>> +     }
>> +
>> +     kfree(uchger->name);
>> +     kfree(uchger);
>> +}
>> +
>> +/*
>> + * usb_charger_unregister() - Unregister a usb charger device.
>> + * @uchger - the usb charger device.
>> + *
>> + */
>> +int usb_charger_unregister(struct usb_charger *uchger)
>> +{
>> +     if (!uchger)
>> +             return -EINVAL;
>> +
>> +     mutex_lock(&usb_charger_list_lock);
>> +     list_del(&uchger->entry);
>> +     mutex_unlock(&usb_charger_list_lock);
>> +
>> +     device_unregister(&uchger->dev);
>> +     return 0;
>> +}
>> +
>> +static void devm_uchger_dev_unreg(struct device *dev, void *res)
>> +{
>> +     usb_charger_unregister(*(struct usb_charger **)res);
>> +}
>> +
>> +void devm_usb_charger_unregister(struct device *dev,
>> +                              struct usb_charger *uchger)
>> +{
>> +     devres_release(dev, devm_uchger_dev_unreg,
>> +                    devm_uchger_dev_match, uchger);
>> +}
>> +
>> +/*
>> + * usb_charger_register() - Register a new usb charger device.
>> + * @uchger - the new usb charger device.
>> + *
>> + */
>> +int usb_charger_register(struct device *dev, struct usb_charger *uchger)
>> +{
>> +     static atomic_t uchger_no = ATOMIC_INIT(-1);
>> +     struct usb_charger *tmp;
>> +     int ret;
>> +
>> +     if (!uchger) {
>> +             dev_err(dev, "no device provided for charger\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     uchger->dev.parent = dev;
>> +     uchger->dev.release = usb_charger_release;
>> +     dev_set_name(&uchger->dev, "usb-chger%lu",
>> +                  (unsigned long)atomic_inc_return(&uchger_no));
>
> For the name of usb-charger, which not using "usb-charger.x" directly?

OK.

>> +
>> +     ret = device_register(&uchger->dev);
>> +     if (ret) {
>> +             put_device(&uchger->dev);
>> +             return ret;
>> +     }
>> +
>> +     dev_set_drvdata(&uchger->dev, uchger);
>> +
>> +     mutex_lock(&usb_charger_list_lock);
>> +     list_for_each_entry(tmp, &usb_charger_list, entry) {
>> +             if (!(strcmp(tmp->name, uchger->name))) {
>> +                     mutex_unlock(&usb_charger_list_lock);
>> +                     ret = -EEXIST;
>> +                     goto out;
>> +             }
>> +     }
>> +     list_add_tail(&uchger->entry, &usb_charger_list);
>> +     mutex_unlock(&usb_charger_list_lock);
>> +
>> +     return 0;
>> +
>> +out:
>> +     dev_err(dev, "Failed to register usb charger (%s)\n",
>> +             uchger->name);
>> +     device_unregister(&uchger->dev);
>> +     put_device(&uchger->dev);
>> +     return ret;
>> +}
>> +
>> +int devm_usb_charger_register(struct device *dev,
>> +                           struct usb_charger *uchger)
>> +{
>> +     struct usb_charger **ptr;
>> +     int ret;
>> +
>> +     ptr = devres_alloc(devm_uchger_dev_unreg, sizeof(*ptr), GFP_KERNEL);
>> +     if (!ptr)
>> +             return -ENOMEM;
>> +
>> +     ret = usb_charger_register(dev, uchger);
>> +     if (ret) {
>> +             devres_free(ptr);
>> +             return ret;
>> +     }
>> +
>> +     *ptr = uchger;
>> +     devres_add(dev, ptr);
>> +
>> +     return 0;
>> +}
>> +
>> +int usb_charger_init(struct usb_gadget *ugadget)
>> +{
>> +     struct usb_charger *uchger;
>> +     struct extcon_dev *edev;
>> +     char buf[100];
>> +     char *str;
>> +     int ret;
>> +
>> +     if (!ugadget)
>> +             return -EINVAL;
>> +
>> +     uchger = devm_kzalloc(&ugadget->dev, sizeof(struct usb_charger),
>> +                           GFP_KERNEL);
>> +     if (!uchger)
>> +             return -ENOMEM;
>> +
>> +     sprintf(buf, "usb-charger.%s", ugadget->name);
>> +     str = devm_kzalloc(&ugadget->dev, sizeof(char) * (strlen(buf) + 1),
>> +                        GFP_KERNEL);
>> +     if (!str)
>> +             return -ENOMEM;
>> +
>> +     strcpy(str, buf);
>> +     uchger->name = str;
>> +     uchger->type = UNKNOWN_TYPE;
>> +     uchger->state = USB_CHARGER_DEFAULT;
>> +     uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT;
>> +     uchger->cur_limit.dcp_cur_limit = DEFAULT_DCP_CUR_LIMIT;
>> +     uchger->cur_limit.cdp_cur_limit = DEFAULT_CDP_CUR_LIMIT;
>> +     uchger->cur_limit.aca_cur_limit = DEFAULT_ACA_CUR_LIMIT;
>> +
>> +     atomic_set(&uchger->count, 1);
>> +     spin_lock_init(&uchger->lock);
>> +     INIT_LIST_HEAD(&uchger->entry);
>> +     RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
>> +
>> +     /* register a notifier on a extcon device if it is exsited */
>> +     edev = extcon_get_edev_by_phandle(ugadget->dev.parent, 0);
>> +     if (!IS_ERR_OR_NULL(edev)) {
>> +             uchger->extcon_dev = edev;
>> +             uchger->extcon_nb.nb.notifier_call = usb_charger_plug_by_extcon;
>> +             uchger->extcon_nb.uchger = uchger;
>> +             usb_charger_register_extcon_notifier(uchger, edev, EXTCON_USB);
>> +     }
>> +
>> +     /* register a notifier on a usb gadget device */
>> +     uchger->gadget = ugadget;
>> +     ugadget->uchger = uchger;
>> +     uchger->old_gadget_state = ugadget->state;
>> +     uchger->gadget_nb.notifier_call = usb_charger_plug_by_gadget;
>> +     usb_charger_register_gadget_notifier(uchger);
>> +
>> +     ret = usb_charger_register(&ugadget->dev, uchger);
>> +     if (ret)
>> +             goto reg_fail;
>> +
>> +     return 0;
>> +
>> +reg_fail:
>> +     if (uchger->extcon_dev)
>> +             usb_charger_unregister_extcon_notifier(uchger,
>> +                             uchger->extcon_dev, EXTCON_USB);
>> +
>> +     usb_charger_unregister_gadget_notifier(uchger);
>> +     return ret;
>> +}
>> +
>> +int usb_charger_exit(struct usb_gadget *ugadget)
>> +{
>> +     struct usb_charger *uchger = ugadget->uchger;
>> +
>> +     if (!uchger)
>> +             return -EINVAL;
>> +
>> +     if (uchger->extcon_dev)
>> +             usb_charger_unregister_extcon_notifier(uchger,
>> +                             uchger->extcon_dev, EXTCON_USB);
>> +
>> +     usb_charger_unregister_gadget_notifier(uchger);
>> +     return usb_charger_unregister(uchger);
>> +}
>> +
>> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
>> +MODULE_DESCRIPTION("USB charger driver");
>> diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
>> new file mode 100644
>> index 0000000..da4d3c9
>> --- /dev/null
>> +++ b/include/linux/usb/usb_charger.h
>> @@ -0,0 +1,101 @@
>> +#ifndef __LINUX_USB_CHARGER_H__
>> +#define __LINUX_USB_CHARGER_H__
>> +
>> +#include <linux/device.h>
>> +#include <linux/notifier.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/usb/ch9.h>
>> +
>> +/* USB charger type:
>> + * SDP (Standard Downstream Port)
>> + * DCP (Dedicated Charging Port)
>> + * CDP (Charging Downstream Port)
>> + * ACA (Accessory Charger Adapters)
>> + */
>> +enum usb_charger_type {
>> +     UNKNOWN_TYPE,
>> +     SDP_TYPE,
>> +     DCP_TYPE,
>> +     CDP_TYPE,
>> +     ACA_TYPE,
>> +};
>> +
>> +/* USB charger state */
>> +enum usb_charger_state {
>> +     USB_CHARGER_DEFAULT,
>> +     USB_CHARGER_PRESENT,
>> +     USB_CHARGER_REMOVE,
>> +};
>> +
>> +/* Current limitation by charger type */
>> +struct usb_charger_cur_limit {
>> +     unsigned int sdp_cur_limit;
>> +     unsigned int dcp_cur_limit;
>> +     unsigned int cdp_cur_limit;
>> +     unsigned int aca_cur_limit;
>> +};
>> +
>> +struct usb_charger_nb {
>> +     struct notifier_block   nb;
>> +     struct usb_charger      *uchger;
>> +};
>> +
>> +struct usb_charger {
>> +     /* Internal data. Please do not set. */
>> +     const char              *name;
>> +     struct device           dev;
>> +     struct raw_notifier_head        uchger_nh;
>> +     struct list_head        entry;
>> +     spinlock_t              lock;
>> +     enum usb_charger_type   type;
>> +     enum usb_charger_state  state;
>> +     atomic_t                count;
>> +
>> +     /* For supporting extcon usb gpio */
>> +     struct extcon_dev       *extcon_dev;
>> +     struct usb_charger_nb   extcon_nb;
>> +
>> +     /* For supporting usb gadget */
>> +     struct usb_gadget       *gadget;
>> +     enum usb_device_state   old_gadget_state;
>> +     struct notifier_block   gadget_nb;
>> +
>> +     /* Current limitation */
>> +     struct usb_charger_cur_limit    cur_limit;
>> +};
>> +
>> +extern struct usb_charger *usb_charger_find_by_name(char *name);
>> +
>> +extern void usb_charger_register_notify(struct usb_charger *uchger,
>> +                                     struct notifier_block *nb);
>> +extern void usb_charger_unregister_notify(struct usb_charger *uchger,
>> +                                       struct notifier_block *nb);
>> +
>> +extern int usb_charger_register_extcon_notifier(struct usb_charger *uchger,
>> +                                             struct extcon_dev *edev,
>> +                                             unsigned int extcon_id);
>> +extern int usb_charger_unregister_extcon_notifier(struct usb_charger *uchger,
>> +                                               struct extcon_dev *edev,
>> +                                               unsigned int extcon_id);
>> +
>> +extern int usb_charger_register_gadget_notifier(struct usb_charger *uchger);
>> +extern int usb_charger_unregister_gadget_notifier(struct usb_charger *uchger);
>> +
>> +extern int usb_charger_set_cur_limit(struct usb_charger *uchger,
>> +                           struct usb_charger_cur_limit *cur_limit_set);
>> +extern enum usb_charger_type usb_charger_detect_type(struct usb_charger *uchger);
>> +
>> +extern int usb_charger_init(struct usb_gadget *ugadget);
>> +extern int usb_charger_exit(struct usb_gadget *ugadget);
>> +
>> +static inline void get_usb_charger(struct usb_charger *uchger)
>> +{
>> +     atomic_inc(&uchger->count);
>> +}
>> +
>> +static inline void put_usb_charger(struct usb_charger *uchger)
>> +{
>> +     atomic_dec(&uchger->count);
>> +}
>> +
>> +#endif /* __LINUX_USB_CHARGER_H__ */
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
>
> Best Regards,
> Peter Chen



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

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



-- 
Baolin.wang
Best Regards

Re: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

On 7 August 2015 at 16:31, Peter Chen <Peter.Chen@freescale.com> wrote:
>
>>
>> Peter, Thanks for your reviewing and comments. Now I just introduce the
>> framework to review for more feedbacks and do not have a useful user to use
>> it. I just can show you some example code to show how to use it. Thanks.
>>
>
> Felipe may not accept the code which are no user on it, I remember he said it before.
> Besides, if no user on it, how you test it?
>

Make sense of it. I'll try to build the user to test it. Thanks for
your reminding.

> Peter



-- 
Baolin.wang
Best Regards

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

[Peter Chen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1065 bytes --]

 
> >>  /**
> >>   * 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
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Baolin Wang]

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



-- 
Baolin.wang
Best Regards

Re: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Greg KH]

On Fri, Aug 07, 2015 at 04:19:40PM +0800, Baolin Wang wrote:
> On 7 August 2015 at 13:34, Peter Chen <peter.chen@freescale.com> wrote:
> > On Thu, Aug 06, 2015 at 03:03:47PM +0800, Baolin Wang wrote:
> >> Currently the Linux kernel does not provide any standard integration of this
> >> feature that integrates the USB subsystem with the system power regulation
> >> provided by PMICs meaning that either vendors must add this in their kernels
> >> or USB gadget devices based on Linux (such as mobile phones) may not behave
> >> as they should.
> >>
> >> Providing a standard framework for doing this in the kernel.
> >
> > Baolin, thanks for introducing a framework for doing it, we do support
> > USB Charger for chipidea driver at internal tree, but it is specific
> > for imx, and still have some problems to upstream due to need to
> > change some common code.
> >
> > One suggestion, would you add your user next time? In that case, we can
> > know better for this framework.
> >
> 
> Peter, Thanks for your reviewing and comments. Now I just introduce
> the framework to review for more feedbacks and do not have a useful
> user to use it. I just can show you some example code to show how to
> use it. Thanks.

Without a real, in-tree user, I can not accept this code.  We don't add
"frameworks" for non-existant things, otherwise it will be instantly
ripped out the next kernel release.

Please come up with at least 2 users, ideally 3, otherwise there's no
real way to know if the framework is sufficient.

thanks,

greg k-h

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

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

thanks,

greg k-h

Re: [PATCH 0/2] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

On 8 August 2015 at 01:52, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Aug 07, 2015 at 04:19:40PM +0800, Baolin Wang wrote:
>> On 7 August 2015 at 13:34, Peter Chen <peter.chen@freescale.com> wrote:
>> > On Thu, Aug 06, 2015 at 03:03:47PM +0800, Baolin Wang wrote:
>> >> Currently the Linux kernel does not provide any standard integration of this
>> >> feature that integrates the USB subsystem with the system power regulation
>> >> provided by PMICs meaning that either vendors must add this in their kernels
>> >> or USB gadget devices based on Linux (such as mobile phones) may not behave
>> >> as they should.
>> >>
>> >> Providing a standard framework for doing this in the kernel.
>> >
>> > Baolin, thanks for introducing a framework for doing it, we do support
>> > USB Charger for chipidea driver at internal tree, but it is specific
>> > for imx, and still have some problems to upstream due to need to
>> > change some common code.
>> >
>> > One suggestion, would you add your user next time? In that case, we can
>> > know better for this framework.
>> >
>>
>> Peter, Thanks for your reviewing and comments. Now I just introduce
>> the framework to review for more feedbacks and do not have a useful
>> user to use it. I just can show you some example code to show how to
>> use it. Thanks.
>
> Without a real, in-tree user, I can not accept this code.  We don't add
> "frameworks" for non-existant things, otherwise it will be instantly
> ripped out the next kernel release.
>
> Please come up with at least 2 users, ideally 3, otherwise there's no
> real way to know if the framework is sufficient.
>

OK, I'll try to come up with 2 or 3 users. Thanks.

> thanks,
>
> greg k-h



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

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



-- 
Baolin.wang
Best Regards