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

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

[Li Jun]

Hi Baolin,

On Fri, Aug 14, 2015 at 05:47:43PM +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.
> 

Why not add power supply class support into this?  

Li Jun

[PATCH v2 0/3] 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.

Now introduce one user with wm831x_power to support and test the usb charger,
which is pending testing. Moreover there may be other potential users will use
it in future.

Changes since v1:
 - Rebase my branch and re-create the patch series.
 - Remove the extcon wrappers and gadget wrappers for registering a notifier.
 - Remove the user count in usb charger.
 - Use idr instead of my own logic.
 - Replace the spinlock with mutex.
 - Use the list of driver core to manage the usb charger device.
 - Add the Kconfig and Makefile changes.
 - Add some attribute files to show or store the current limitation.
 - Introduce one user of usb charger by Mark Brown.
 - Other modifications.

Baolin Wang (3):
  gadget: Support for the usb charger framework
  gadget: Introduce the usb charger framework
  power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c      |   69 +++++
 drivers/usb/gadget/Kconfig        |    7 +
 drivers/usb/gadget/Makefile       |    1 +
 drivers/usb/gadget/charger.c      |  561 +++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/udc-core.c |   38 +++
 include/linux/mfd/wm831x/pdata.h  |    3 +
 include/linux/usb/gadget.h        |   20 ++
 include/linux/usb/usb_charger.h   |  145 ++++++++++
 8 files changed, 844 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

-- 
1.7.9.5

[PATCH v2 1/3] 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 |   38 +++++++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h        |   20 +++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index f660afb..47b231c 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
@@ -129,6 +130,32 @@ 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)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_register(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
+
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+				 struct notifier_block *nb)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
+
 /* ------------------------------------------------------------------------- */
 
 /**
@@ -226,6 +253,10 @@ static void usb_gadget_state_work(struct work_struct *work)
 	struct usb_gadget *gadget = work_to_gadget(work);
 	struct usb_udc *udc = gadget->udc;
 
+	mutex_lock(&gadget->lock);
+	raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
+	mutex_unlock(&gadget->lock);
+
 	if (udc)
 		sysfs_notify(&udc->dev.kobj, NULL, "state");
 }
@@ -364,6 +395,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);
+	mutex_init(&gadget->lock);
 	gadget->dev.parent = parent;
 
 #ifdef	CONFIG_HAS_DMA
@@ -405,6 +438,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:
@@ -481,6 +518,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 c14a69b..78cc862 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -537,6 +537,7 @@ struct usb_gadget_ops {
 	struct usb_ep *(*match_ep)(struct usb_gadget *,
 			struct usb_endpoint_descriptor *,
 			struct usb_ss_ep_comp_descriptor *);
+	enum usb_charger_type	(*get_charger_type)(struct usb_gadget *);
 };
 
 /**
@@ -609,6 +610,9 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	struct raw_notifier_head	nh;
+	struct usb_charger		*uchger;
+	struct mutex			lock;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -1183,6 +1187,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

[PATCH v2 2/3] 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/Kconfig      |    7 +
 drivers/usb/gadget/Makefile     |    1 +
 drivers/usb/gadget/charger.c    |  561 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb/usb_charger.h |  145 ++++++++++
 4 files changed, 714 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index bcf83c0..3d2b959 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -127,6 +127,13 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
 	   a module parameter as well.
 	   If unsure, say 2.
 
+config USB_CHARGER
+	bool "USB charger support"
+	help
+	  The usb charger driver based on the usb gadget that makes an
+	  enhancement to a power driver which can set the current limitation
+	  when the usb charger is added or removed.
+
 source "drivers/usb/gadget/udc/Kconfig"
 
 #
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 598a67d..1e421c1 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -10,3 +10,4 @@ libcomposite-y			:= usbstring.o config.o epautoconf.o
 libcomposite-y			+= composite.o functions.o configfs.o u_f.o
 
 obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
+obj-$(CONFIG_USB_CHARGER)	+= charger.o
diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
new file mode 100644
index 0000000..f24f7b7
--- /dev/null
+++ b/drivers/usb/gadget/charger.c
@@ -0,0 +1,561 @@
+/*
+ * 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 DEFINE_IDA(usb_charger_ida);
+static struct bus_type usb_charger_subsys = {
+	.name           = "usb-charger",
+	.dev_name       = "usb-charger",
+};
+
+static struct usb_charger *dev_to_uchger(struct device *udev)
+{
+	return container_of(udev, struct usb_charger, dev);
+}
+
+static ssize_t usb_charger_cur_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d %d %d %d\n",
+			 uchger->cur_limit.sdp_cur_limit,
+			 uchger->cur_limit.dcp_cur_limit,
+			 uchger->cur_limit.cdp_cur_limit,
+			 uchger->cur_limit.aca_cur_limit);
+}
+
+static ssize_t usb_charger_cur_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	struct usb_charger_cur_limit cur;
+	int ret;
+
+	ret = sscanf(buf, "%d %d %d %d",
+		     &cur.sdp_cur_limit, &cur.dcp_cur_limit,
+		     &cur.cdp_cur_limit, &cur.aca_cur_limit);
+	if (ret == 0)
+		return -EINVAL;
+
+	ret = usb_charger_set_cur_limit(uchger, &cur);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR(cur_limit, 0644, usb_charger_cur_show, usb_charger_cur_store);
+
+static struct attribute *uchger_attributes[] = {
+	&dev_attr_cur_limit.attr,
+	NULL
+};
+
+static const struct attribute_group uchger_attr_group = {
+	.attrs = uchger_attributes,
+};
+
+/*
+ * usb_charger_find_by_name - Get the usb charger device by name.
+ * @name - usb charger device name.
+ *
+ * return the instance of usb charger device, the device must be
+ * released with usb_charger_put().
+ */
+struct usb_charger *usb_charger_find_by_name(const char *name)
+{
+	struct device *udev;
+
+	if (!name)
+		return ERR_PTR(-EINVAL);
+
+	udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name);
+	if (!udev)
+		return ERR_PTR(-ENODEV);
+
+	return dev_to_uchger(udev);
+}
+
+/*
+ * usb_charger_get() - Reference a usb charger.
+ * @uchger - usb charger
+ */
+struct usb_charger *usb_charger_get(struct usb_charger *uchger)
+{
+	return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
+}
+
+/*
+ * usb_charger_put() - Dereference a usb charger.
+ * @uchger - charger to release
+ */
+void usb_charger_put(struct usb_charger *uchger)
+{
+	if (uchger)
+		put_device(&uchger->dev);
+}
+
+/*
+ * usb_charger_register_notify() - Register a notifiee to get notified by
+ * any attach status changes from the usb charger detection.
+ * @uchger - the usb charger device which is monitored.
+ * @nb - a notifier block to be registered.
+ */
+int usb_charger_register_notify(struct usb_charger *uchger,
+				struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
+
+	/* Generate an initial notify so users start in the right state */
+	if (!ret) {
+		usb_charger_detect_type(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+					usb_charger_get_cur_limit(uchger),
+					uchger);
+	}
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+
+/*
+ * 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.
+ */
+int usb_charger_unregister_notify(struct usb_charger *uchger,
+				  struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+
+/*
+ * usb_charger_detect_type() - Get the usb charger type by the callback
+ * which is implemented by gadget operations.
+ * @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_set_cur_limit() - Set the current limitation.
+ * @uchger - the usb charger device.
+ * @cur_limit_set - the current limitation.
+ */
+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.
+ *
+ * return the current limitation to set.
+ */
+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_notifier_others() - It will notify other device registered
+ * on usb charger when the usb charger state is changed.
+ * @uchger - the usb charger device.
+ * @state - the state of the usb charger.
+ */
+static void
+usb_charger_notify_others(struct usb_charger *uchger,
+			  enum usb_charger_state state)
+{
+	mutex_lock(&uchger->lock);
+	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;
+	}
+	mutex_unlock(&uchger->lock);
+}
+
+/*
+ * usb_charger_plug_by_extcon() - The notifier call function which is registered
+ * on the extcon device.
+ * @nb - the notifier block that notified by extcon device.
+ * @state - the extcon device state.
+ * @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.
+ * @nb - the notifier block that notified by usb gadget device.
+ * @state - the usb gadget state.
+ * @data - here specify a usb gadget 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);
+
+	kfree(uchger->name);
+	kfree(uchger);
+}
+
+/*
+ * usb_charger_unregister() - Unregister a usb charger device.
+ * @uchger - the usb charger to be unregistered.
+ */
+static int usb_charger_unregister(struct usb_charger *uchger)
+{
+	if (!uchger)
+		return -EINVAL;
+
+	sysfs_remove_group(&uchger->dev.kobj, &uchger_attr_group);
+	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
+ * which is created by the usb charger framework.
+ * @uchger - the new usb charger device.
+ */
+static int usb_charger_register(struct device *dev, struct usb_charger *uchger)
+{
+	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;
+	uchger->dev.bus = &usb_charger_subsys;
+
+	ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0) {
+		dev_err(dev, "get usb charger id failed\n");
+		return ret;
+	}
+
+	uchger->id = ret;
+	dev_set_name(&uchger->dev, "usb-charger.%d", uchger->id);
+	dev_set_drvdata(&uchger->dev, uchger);
+
+	ret = device_register(&uchger->dev);
+	if (ret)
+		goto fail_register;
+
+	ret = sysfs_create_group(&uchger->dev.kobj, &uchger_attr_group);
+	if (ret)
+		goto fail_create_files;
+
+	return 0;
+
+fail_create_files:
+	device_unregister(&uchger->dev);
+fail_register:
+	put_device(&uchger->dev);
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+	uchger->id = -1;
+	dev_err(dev, "Failed to register usb charger (%s)\n",
+		uchger->name);
+	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->id = -1;
+	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;
+
+	mutex_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;
+		extcon_register_notifier(edev, EXTCON_USB, &uchger->extcon_nb.nb);
+	}
+
+	/* 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_gadget_register_notify(ugadget, &uchger->gadget_nb);
+
+	/* register a new usb charger */
+	ret = usb_charger_register(&ugadget->dev, uchger);
+	if (ret)
+		goto fail;
+
+	return 0;
+
+fail:
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB, &uchger->extcon_nb.nb);
+
+	usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
+	return ret;
+}
+
+int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger = ugadget->uchger;
+
+	if (!uchger)
+		return -EINVAL;
+
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB, &uchger->extcon_nb.nb);
+
+	usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+
+	return usb_charger_unregister(uchger);
+}
+
+static int __init usb_charger_sysfs_init(void)
+{
+	return subsys_system_register(&usb_charger_subsys, NULL);
+}
+core_initcall(usb_charger_sysfs_init);
+
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
+MODULE_DESCRIPTION("USB charger driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
new file mode 100644
index 0000000..8cbfaae
--- /dev/null
+++ b/include/linux/usb/usb_charger.h
@@ -0,0 +1,145 @@
+#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;
+	struct mutex		lock;
+	int			id;
+	enum usb_charger_type	type;
+	enum usb_charger_state	state;
+
+	/* 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;
+};
+
+#ifdef CONFIG_USB_CHARGER
+extern struct usb_charger *usb_charger_find_by_name(const char *name);
+
+extern struct usb_charger *usb_charger_get(struct usb_charger *uchger);
+extern void usb_charger_put(struct usb_charger *uchger);
+
+extern int usb_charger_register_notify(struct usb_charger *uchger,
+					struct notifier_block *nb);
+extern int usb_charger_unregister_notify(struct usb_charger *uchger,
+					  struct notifier_block *nb);
+
+extern int usb_charger_set_cur_limit(struct usb_charger *uchger,
+			      struct usb_charger_cur_limit *cur_limit_set);
+extern unsigned int usb_charger_get_cur_limit(struct usb_charger *uchger);
+
+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);
+#else
+static inline struct usb_charger *usb_charger_find_by_name(const char *name)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline struct usb_charger *usb_charger_get(struct usb_charger *uchger)
+{
+	return NULL;
+}
+
+static inline void usb_charger_put(struct usb_charger *uchger)
+{
+}
+
+static inline int
+usb_charger_register_notify(struct usb_charger *uchger,
+			    struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_unregister_notify(struct usb_charger *uchger,
+			      struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_cur_limit(struct usb_charger *uchger,
+			  struct usb_charger_cur_limit *cur_limit_set)
+{
+	return 0;
+}
+
+static inline unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	return 0;
+}
+
+static inline enum usb_charger_type
+usb_charger_detect_type(struct usb_charger *uchger)
+{
+	return UNKNOWN_TYPE;
+}
+
+static inline int usb_charger_init(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+
+static inline int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+#endif
+
+#endif /* __LINUX_USB_CHARGER_H__ */
-- 
1.7.9.5

[PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

Integrate with the newly added USB charger interface to limit the current
we draw from the USB input based on the input device configuration
identified by the USB stack, allowing us to charge more quickly from high
current inputs without drawing more current than specified from others.

Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/power/wm831x_power.c     |   69 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/wm831x/pdata.h |    3 ++
 2 files changed, 72 insertions(+)

diff --git a/drivers/power/wm831x_power.c b/drivers/power/wm831x_power.c
index db11ae6..72c661f 100644
--- a/drivers/power/wm831x_power.c
+++ b/drivers/power/wm831x_power.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
+#include <linux/usb/usb_charger.h>
 
 #include <linux/mfd/wm831x/core.h>
 #include <linux/mfd/wm831x/auxadc.h>
@@ -31,6 +32,8 @@ struct wm831x_power {
 	char usb_name[20];
 	char battery_name[20];
 	bool have_battery;
+	struct usb_charger *usb_charger;
+	struct notifier_block usb_notify;
 };
 
 static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
@@ -125,6 +128,43 @@ static enum power_supply_property wm831x_usb_props[] = {
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 };
 
+/* In miliamps */
+static unsigned int wm831x_usb_limits[] = {
+	0,
+	2,
+	100,
+	500,
+	900,
+	1500,
+	1800,
+	550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+				   unsigned long limit, void *data)
+{
+	struct wm831x_power *wm831x_power = container_of(nb,
+							 struct wm831x_power,
+							 usb_notify);
+	int i, best;
+
+	/* Find the highest supported limit */
+	best = 0;
+	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+		if (limit < wm831x_usb_limits[i] &&
+		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
+			best = i;
+	}
+
+	dev_dbg(wm831x_power->wm831x->dev,
+		"Limiting USB current to %dmA", wm831x_usb_limits[best]);
+
+	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+		        WM831X_USB_ILIM_MASK, best);
+
+	return 0;
+}
+
 /*********************************************************************
  *		Battery properties
  *********************************************************************/
@@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+		power->usb_charger =
+			usb_charger_find_by_name(wm831x_pdata->usb_gadget);
+		if (IS_ERR(power->usb_charger)) {
+			ret = PTR_ERR(power->usb_charger);
+			dev_err(&pdev->dev,
+				"Failed to find USB gadget: %d\n", ret);
+			goto err_bat_irq;
+		}
+
+		power->usb_notify.notifier_call = wm831x_usb_limit_change;
+
+		ret = usb_charger_register_notify(power->usb_charger,
+						  &power->usb_notify);
+		if (ret != 0) {
+			dev_err(&pdev->dev,
+				"Failed to register notifier: %d\n", ret);
+			goto err_usb_charger;
+		}
+	}
+
 	return ret;
 
+err_usb_charger:
+	usb_charger_put(power->usb_charger);
 err_bat_irq:
 	--i;
 	for (; i >= 0; i--) {
@@ -637,6 +700,12 @@ static int wm831x_power_remove(struct platform_device *pdev)
 	struct wm831x *wm831x = wm831x_power->wm831x;
 	int irq, i;
 
+	if (wm831x_power->usb_charger) {
+		usb_charger_unregister_notify(wm831x_power->usb_charger,
+					      &wm831x_power->usb_notify);
+		usb_charger_put(wm831x_power->usb_charger);
+	}
+
 	for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
 		irq = wm831x_irq(wm831x, 
 				 platform_get_irq_byname(pdev,
diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
index dcc9631..5af8399 100644
--- a/include/linux/mfd/wm831x/pdata.h
+++ b/include/linux/mfd/wm831x/pdata.h
@@ -126,6 +126,9 @@ struct wm831x_pdata {
 	/** The driver should initiate a power off sequence during shutdown */
 	bool soft_shutdown;
 
+	/** dev_name of USB charger gadget to integrate with */
+	const char *usb_gadget;
+
 	int irq_base;
 	int gpio_base;
 	int gpio_defaults[WM831X_GPIO_NUM];
-- 
1.7.9.5

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Lee Jones]

On Fri, 14 Aug 2015, Baolin Wang wrote:

> Integrate with the newly added USB charger interface to limit the current
> we draw from the USB input based on the input device configuration
> identified by the USB stack, allowing us to charge more quickly from high
> current inputs without drawing more current than specified from others.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/power/wm831x_power.c     |   69 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/wm831x/pdata.h |    3 ++

Acked-by: Lee Jones <lee.jones@linaro.org>

>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/power/wm831x_power.c b/drivers/power/wm831x_power.c
> index db11ae6..72c661f 100644
> --- a/drivers/power/wm831x_power.c
> +++ b/drivers/power/wm831x_power.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/slab.h>
> +#include <linux/usb/usb_charger.h>
>  
>  #include <linux/mfd/wm831x/core.h>
>  #include <linux/mfd/wm831x/auxadc.h>
> @@ -31,6 +32,8 @@ struct wm831x_power {
>  	char usb_name[20];
>  	char battery_name[20];
>  	bool have_battery;
> +	struct usb_charger *usb_charger;
> +	struct notifier_block usb_notify;
>  };
>  
>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
> @@ -125,6 +128,43 @@ static enum power_supply_property wm831x_usb_props[] = {
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  };
>  
> +/* In miliamps */
> +static unsigned int wm831x_usb_limits[] = {
> +	0,
> +	2,
> +	100,
> +	500,
> +	900,
> +	1500,
> +	1800,
> +	550,
> +};
> +
> +static int wm831x_usb_limit_change(struct notifier_block *nb,
> +				   unsigned long limit, void *data)
> +{
> +	struct wm831x_power *wm831x_power = container_of(nb,
> +							 struct wm831x_power,
> +							 usb_notify);
> +	int i, best;
> +
> +	/* Find the highest supported limit */
> +	best = 0;
> +	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
> +		if (limit < wm831x_usb_limits[i] &&
> +		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
> +			best = i;
> +	}
> +
> +	dev_dbg(wm831x_power->wm831x->dev,
> +		"Limiting USB current to %dmA", wm831x_usb_limits[best]);
> +
> +	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
> +		        WM831X_USB_ILIM_MASK, best);
> +
> +	return 0;
> +}
> +
>  /*********************************************************************
>   *		Battery properties
>   *********************************************************************/
> @@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
> +		power->usb_charger =
> +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);
> +		if (IS_ERR(power->usb_charger)) {
> +			ret = PTR_ERR(power->usb_charger);
> +			dev_err(&pdev->dev,
> +				"Failed to find USB gadget: %d\n", ret);
> +			goto err_bat_irq;
> +		}
> +
> +		power->usb_notify.notifier_call = wm831x_usb_limit_change;
> +
> +		ret = usb_charger_register_notify(power->usb_charger,
> +						  &power->usb_notify);
> +		if (ret != 0) {
> +			dev_err(&pdev->dev,
> +				"Failed to register notifier: %d\n", ret);
> +			goto err_usb_charger;
> +		}
> +	}
> +
>  	return ret;
>  
> +err_usb_charger:
> +	usb_charger_put(power->usb_charger);
>  err_bat_irq:
>  	--i;
>  	for (; i >= 0; i--) {
> @@ -637,6 +700,12 @@ static int wm831x_power_remove(struct platform_device *pdev)
>  	struct wm831x *wm831x = wm831x_power->wm831x;
>  	int irq, i;
>  
> +	if (wm831x_power->usb_charger) {
> +		usb_charger_unregister_notify(wm831x_power->usb_charger,
> +					      &wm831x_power->usb_notify);
> +		usb_charger_put(wm831x_power->usb_charger);
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
>  		irq = wm831x_irq(wm831x, 
>  				 platform_get_irq_byname(pdev,
> diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
> index dcc9631..5af8399 100644
> --- a/include/linux/mfd/wm831x/pdata.h
> +++ b/include/linux/mfd/wm831x/pdata.h
> @@ -126,6 +126,9 @@ struct wm831x_pdata {
>  	/** The driver should initiate a power off sequence during shutdown */
>  	bool soft_shutdown;
>  
> +	/** dev_name of USB charger gadget to integrate with */
> +	const char *usb_gadget;
> +
>  	int irq_base;
>  	int gpio_base;
>  	int gpio_defaults[WM831X_GPIO_NUM];

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Baolin Wang]

On 14 August 2015 at 16:55, Li Jun <b47624@freescale.com> wrote:
> Hi Baolin,
>
> On Fri, Aug 14, 2015 at 05:47:43PM +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.
>>
>
> Why not add power supply class support into this?
>

Hi Jun,

We don't need the power supply class support into the usb charger,
just introduce the notify mechanism for power to set the current limit
when notifying some events from usb charger. Maybe I misunderstand
your meanings, please describe it detailedly. Thanks for your
comments.

> Li Jun
>



-- 
Baolin.wang
Best Regards

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

[Greg KH]

On Fri, Aug 14, 2015 at 05:47:45PM +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/Kconfig      |    7 +
>  drivers/usb/gadget/Makefile     |    1 +
>  drivers/usb/gadget/charger.c    |  561 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/usb_charger.h |  145 ++++++++++
>  4 files changed, 714 insertions(+)
>  create mode 100644 drivers/usb/gadget/charger.c
>  create mode 100644 include/linux/usb/usb_charger.h
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index bcf83c0..3d2b959 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -127,6 +127,13 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
>  	   a module parameter as well.
>  	   If unsure, say 2.
>  
> +config USB_CHARGER
> +	bool "USB charger support"
> +	help
> +	  The usb charger driver based on the usb gadget that makes an
> +	  enhancement to a power driver which can set the current limitation
> +	  when the usb charger is added or removed.
> +
>  source "drivers/usb/gadget/udc/Kconfig"
>  
>  #
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 598a67d..1e421c1 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -10,3 +10,4 @@ libcomposite-y			:= usbstring.o config.o epautoconf.o
>  libcomposite-y			+= composite.o functions.o configfs.o u_f.o
>  
>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
> +obj-$(CONFIG_USB_CHARGER)	+= charger.o
> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
> new file mode 100644
> index 0000000..f24f7b7
> --- /dev/null
> +++ b/drivers/usb/gadget/charger.c
> @@ -0,0 +1,561 @@
> +/*
> + * usb charger.c -- USB charger driver

I think your company also wants a copyright line here.  Not that it
means anything at all, but some lawyers love cargo-cult text blobs.


> + *
> + * 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 DEFINE_IDA(usb_charger_ida);
> +static struct bus_type usb_charger_subsys = {
> +	.name           = "usb-charger",
> +	.dev_name       = "usb-charger",
> +};
> +
> +static struct usb_charger *dev_to_uchger(struct device *udev)
> +{
> +	return container_of(udev, struct usb_charger, dev);
> +}
> +
> +static ssize_t usb_charger_cur_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d %d %d %d\n",
> +			 uchger->cur_limit.sdp_cur_limit,
> +			 uchger->cur_limit.dcp_cur_limit,
> +			 uchger->cur_limit.cdp_cur_limit,
> +			 uchger->cur_limit.aca_cur_limit);
> +}
> +
> +static ssize_t usb_charger_cur_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	struct usb_charger_cur_limit cur;
> +	int ret;
> +
> +	ret = sscanf(buf, "%d %d %d %d",
> +		     &cur.sdp_cur_limit, &cur.dcp_cur_limit,
> +		     &cur.cdp_cur_limit, &cur.aca_cur_limit);
> +	if (ret == 0)
> +		return -EINVAL;
> +
> +	ret = usb_charger_set_cur_limit(uchger, &cur);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR(cur_limit, 0644, usb_charger_cur_show, usb_charger_cur_store);

DEVICE_ATTR_RW()?

> +
> +static struct attribute *uchger_attributes[] = {
> +	&dev_attr_cur_limit.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group uchger_attr_group = {
> +	.attrs = uchger_attributes,
> +};

ATTRIBUTE_GROUPS()?

> +
> +/*
> + * usb_charger_find_by_name - Get the usb charger device by name.
> + * @name - usb charger device name.
> + *
> + * return the instance of usb charger device, the device must be
> + * released with usb_charger_put().
> + */
> +struct usb_charger *usb_charger_find_by_name(const char *name)
> +{
> +	struct device *udev;
> +
> +	if (!name)
> +		return ERR_PTR(-EINVAL);
> +
> +	udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name);
> +	if (!udev)
> +		return ERR_PTR(-ENODEV);
> +
> +	return dev_to_uchger(udev);
> +}
> +
> +/*
> + * usb_charger_get() - Reference a usb charger.
> + * @uchger - usb charger
> + */
> +struct usb_charger *usb_charger_get(struct usb_charger *uchger)
> +{
> +	return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
> +}
> +
> +/*
> + * usb_charger_put() - Dereference a usb charger.
> + * @uchger - charger to release
> + */
> +void usb_charger_put(struct usb_charger *uchger)
> +{
> +	if (uchger)
> +		put_device(&uchger->dev);
> +}
> +
> +/*
> + * usb_charger_register_notify() - Register a notifiee to get notified by
> + * any attach status changes from the usb charger detection.
> + * @uchger - the usb charger device which is monitored.
> + * @nb - a notifier block to be registered.
> + */
> +int usb_charger_register_notify(struct usb_charger *uchger,
> +				struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!uchger || !nb)
> +		return -EINVAL;
> +
> +	mutex_lock(&uchger->lock);
> +	ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
> +
> +	/* Generate an initial notify so users start in the right state */
> +	if (!ret) {
> +		usb_charger_detect_type(uchger);
> +		raw_notifier_call_chain(&uchger->uchger_nh,
> +					usb_charger_get_cur_limit(uchger),
> +					uchger);
> +	}
> +	mutex_unlock(&uchger->lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * 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.
> + */
> +int usb_charger_unregister_notify(struct usb_charger *uchger,
> +				  struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!uchger || !nb)
> +		return -EINVAL;
> +
> +	mutex_lock(&uchger->lock);
> +	ret = raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
> +	mutex_unlock(&uchger->lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * usb_charger_detect_type() - Get the usb charger type by the callback
> + * which is implemented by gadget operations.
> + * @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_set_cur_limit() - Set the current limitation.
> + * @uchger - the usb charger device.
> + * @cur_limit_set - the current limitation.
> + */
> +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.
> + *
> + * return the current limitation to set.
> + */
> +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_notifier_others() - It will notify other device registered
> + * on usb charger when the usb charger state is changed.
> + * @uchger - the usb charger device.
> + * @state - the state of the usb charger.
> + */
> +static void
> +usb_charger_notify_others(struct usb_charger *uchger,
> +			  enum usb_charger_state state)
> +{
> +	mutex_lock(&uchger->lock);
> +	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;
> +	}
> +	mutex_unlock(&uchger->lock);
> +}
> +
> +/*
> + * usb_charger_plug_by_extcon() - The notifier call function which is registered
> + * on the extcon device.
> + * @nb - the notifier block that notified by extcon device.
> + * @state - the extcon device state.
> + * @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.
> + * @nb - the notifier block that notified by usb gadget device.
> + * @state - the usb gadget state.
> + * @data - here specify a usb gadget 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);
> +
> +	kfree(uchger->name);
> +	kfree(uchger);
> +}
> +
> +/*
> + * usb_charger_unregister() - Unregister a usb charger device.
> + * @uchger - the usb charger to be unregistered.
> + */
> +static int usb_charger_unregister(struct usb_charger *uchger)
> +{
> +	if (!uchger)
> +		return -EINVAL;
> +
> +	sysfs_remove_group(&uchger->dev.kobj, &uchger_attr_group);
> +	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
> + * which is created by the usb charger framework.
> + * @uchger - the new usb charger device.
> + */
> +static int usb_charger_register(struct device *dev, struct usb_charger *uchger)
> +{
> +	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;
> +	uchger->dev.bus = &usb_charger_subsys;
> +
> +	ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0) {
> +		dev_err(dev, "get usb charger id failed\n");
> +		return ret;
> +	}
> +
> +	uchger->id = ret;
> +	dev_set_name(&uchger->dev, "usb-charger.%d", uchger->id);
> +	dev_set_drvdata(&uchger->dev, uchger);
> +
> +	ret = device_register(&uchger->dev);
> +	if (ret)
> +		goto fail_register;
> +
> +	ret = sysfs_create_group(&uchger->dev.kobj, &uchger_attr_group);

And you just raced with userspace, and lost :(

Assign the group to the device _before_ you tell userspace about it.

The big hint here that is wrong is that you have to call a sysfs_*
function for a struct device.  There is no device_create_group() call
for a reason.



> +	if (ret)
> +		goto fail_create_files;
> +
> +	return 0;
> +
> +fail_create_files:
> +	device_unregister(&uchger->dev);
> +fail_register:
> +	put_device(&uchger->dev);
> +	ida_simple_remove(&usb_charger_ida, uchger->id);
> +	uchger->id = -1;
> +	dev_err(dev, "Failed to register usb charger (%s)\n",
> +		uchger->name);
> +	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];

That's a lot of stack, why?

> +	char *str;
> +	int ret;
> +
> +	if (!ugadget)
> +		return -EINVAL;
> +
> +	uchger = devm_kzalloc(&ugadget->dev, sizeof(struct usb_charger),
> +			      GFP_KERNEL);

No, you are creating a new struct device, you can't assign its resources
to another struct device, that breaks all of the lifetime rules.
kzalloc() only please.

> +	if (!uchger)
> +		return -ENOMEM;
> +
> +	sprintf(buf, "usb-charger.%s", ugadget->name);
> +	str = devm_kzalloc(&ugadget->dev, sizeof(char) * (strlen(buf) + 1),
> +			   GFP_KERNEL);

Why have a string when you already have a name for the device, in the
device itself?  What is this all for?


> +	if (!str)
> +		return -ENOMEM;
> +
> +	strcpy(str, buf);
> +	uchger->name = str;
> +	uchger->type = UNKNOWN_TYPE;
> +	uchger->state = USB_CHARGER_DEFAULT;
> +	uchger->id = -1;
> +	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;
> +
> +	mutex_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;
> +		extcon_register_notifier(edev, EXTCON_USB, &uchger->extcon_nb.nb);
> +	}
> +
> +	/* 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_gadget_register_notify(ugadget, &uchger->gadget_nb);
> +
> +	/* register a new usb charger */
> +	ret = usb_charger_register(&ugadget->dev, uchger);
> +	if (ret)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	if (uchger->extcon_dev)
> +		extcon_unregister_notifier(uchger->extcon_dev,
> +					   EXTCON_USB, &uchger->extcon_nb.nb);
> +
> +	usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
> +	return ret;
> +}
> +
> +int usb_charger_exit(struct usb_gadget *ugadget)
> +{
> +	struct usb_charger *uchger = ugadget->uchger;
> +
> +	if (!uchger)
> +		return -EINVAL;
> +
> +	if (uchger->extcon_dev)
> +		extcon_unregister_notifier(uchger->extcon_dev,
> +					   EXTCON_USB, &uchger->extcon_nb.nb);
> +
> +	usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
> +	ida_simple_remove(&usb_charger_ida, uchger->id);
> +
> +	return usb_charger_unregister(uchger);
> +}
> +
> +static int __init usb_charger_sysfs_init(void)
> +{
> +	return subsys_system_register(&usb_charger_subsys, NULL);
> +}
> +core_initcall(usb_charger_sysfs_init);
> +
> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
> +MODULE_DESCRIPTION("USB charger driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
> new file mode 100644
> index 0000000..8cbfaae
> --- /dev/null
> +++ b/include/linux/usb/usb_charger.h
> @@ -0,0 +1,145 @@
> +#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. */

Heh, as if people read comments :(

> +	const char		*name;

This isn't needed, use the one in your struct device.

> +	struct device		dev;
> +	struct raw_notifier_head	uchger_nh;
> +	struct list_head	entry;

Why do you need another list?  The device already is on a list.

> +	struct mutex		lock;

What is this protecting, document it.

thanks,

greg k-h

Re: [PATCH v2 1/3] gadget: Support for the usb charger framework

[Peter Chen]

On Fri, Aug 14, 2015 at 05:47:44PM +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 |   38 +++++++++++++++++++++++++++++++++++++
>  include/linux/usb/gadget.h        |   20 +++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index f660afb..47b231c 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
> @@ -129,6 +130,32 @@ 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)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->lock);
> +	ret = raw_notifier_chain_register(&gadget->nh, nb);
> +	mutex_unlock(&gadget->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> +
> +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> +				 struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->lock);
> +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> +	mutex_unlock(&gadget->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
> +
>  /* ------------------------------------------------------------------------- */
>  
>  /**
> @@ -226,6 +253,10 @@ static void usb_gadget_state_work(struct work_struct *work)
>  	struct usb_gadget *gadget = work_to_gadget(work);
>  	struct usb_udc *udc = gadget->udc;
>  
> +	mutex_lock(&gadget->lock);
> +	raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
> +	mutex_unlock(&gadget->lock);
> +
>  	if (udc)
>  		sysfs_notify(&udc->dev.kobj, NULL, "state");
>  }
> @@ -364,6 +395,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);
> +	mutex_init(&gadget->lock);
>  	gadget->dev.parent = parent;
>  
>  #ifdef	CONFIG_HAS_DMA
> @@ -405,6 +438,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;
> +

If the charger's initialization fails, you may need to
call device_del(&udc->dev).

Peter

>  	return 0;
>  
>  err4:
> @@ -481,6 +518,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 c14a69b..78cc862 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -537,6 +537,7 @@ struct usb_gadget_ops {
>  	struct usb_ep *(*match_ep)(struct usb_gadget *,
>  			struct usb_endpoint_descriptor *,
>  			struct usb_ss_ep_comp_descriptor *);
> +	enum usb_charger_type	(*get_charger_type)(struct usb_gadget *);
>  };
>  
>  /**
> @@ -609,6 +610,9 @@ struct usb_gadget {
>  	unsigned			out_epnum;
>  	unsigned			in_epnum;
>  	struct usb_otg_caps		*otg_caps;
> +	struct raw_notifier_head	nh;
> +	struct usb_charger		*uchger;
> +	struct mutex			lock;
>  
>  	unsigned			sg_supported:1;
>  	unsigned			is_otg:1;
> @@ -1183,6 +1187,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
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Peter Chen]

On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:
> Integrate with the newly added USB charger interface to limit the current
> we draw from the USB input based on the input device configuration
> identified by the USB stack, allowing us to charge more quickly from high
> current inputs without drawing more current than specified from others.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/power/wm831x_power.c     |   69 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/wm831x/pdata.h |    3 ++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/power/wm831x_power.c b/drivers/power/wm831x_power.c
> index db11ae6..72c661f 100644
> --- a/drivers/power/wm831x_power.c
> +++ b/drivers/power/wm831x_power.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/slab.h>
> +#include <linux/usb/usb_charger.h>
>  
>  #include <linux/mfd/wm831x/core.h>
>  #include <linux/mfd/wm831x/auxadc.h>
> @@ -31,6 +32,8 @@ struct wm831x_power {
>  	char usb_name[20];
>  	char battery_name[20];
>  	bool have_battery;
> +	struct usb_charger *usb_charger;
> +	struct notifier_block usb_notify;
>  };
>  
>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
> @@ -125,6 +128,43 @@ static enum power_supply_property wm831x_usb_props[] = {
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  };
>  
> +/* In miliamps */
> +static unsigned int wm831x_usb_limits[] = {
> +	0,
> +	2,
> +	100,
> +	500,
> +	900,
> +	1500,
> +	1800,
> +	550,
> +};

Why 550 is the last, but not 1800?

> +
> +static int wm831x_usb_limit_change(struct notifier_block *nb,
> +				   unsigned long limit, void *data)
> +{
> +	struct wm831x_power *wm831x_power = container_of(nb,
> +							 struct wm831x_power,
> +							 usb_notify);
> +	int i, best;
> +
> +	/* Find the highest supported limit */
> +	best = 0;
> +	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
> +		if (limit < wm831x_usb_limits[i] &&
> +		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
> +			best = i;
> +	}
> +
> +	dev_dbg(wm831x_power->wm831x->dev,
> +		"Limiting USB current to %dmA", wm831x_usb_limits[best]);
> +
> +	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
> +		        WM831X_USB_ILIM_MASK, best);
> +
> +	return 0;
> +}
> +
>  /*********************************************************************
>   *		Battery properties
>   *********************************************************************/
> @@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (wm831x_pdata && wm831x_pdata->usb_gadget) {

Where the wm831x_pdata->usb_gadget is initialized?

> +		power->usb_charger =
> +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);
> +		if (IS_ERR(power->usb_charger)) {
> +			ret = PTR_ERR(power->usb_charger);
> +			dev_err(&pdev->dev,
> +				"Failed to find USB gadget: %d\n", ret);
> +			goto err_bat_irq;
> +		}
> +
> +		power->usb_notify.notifier_call = wm831x_usb_limit_change;
> +
> +		ret = usb_charger_register_notify(power->usb_charger,
> +						  &power->usb_notify);
> +		if (ret != 0) {
> +			dev_err(&pdev->dev,
> +				"Failed to register notifier: %d\n", ret);
> +			goto err_usb_charger;
> +		}
> +	}
> +
>  	return ret;
>  
> +err_usb_charger:
> +	usb_charger_put(power->usb_charger);
>  err_bat_irq:
>  	--i;
>  	for (; i >= 0; i--) {
> @@ -637,6 +700,12 @@ static int wm831x_power_remove(struct platform_device *pdev)
>  	struct wm831x *wm831x = wm831x_power->wm831x;
>  	int irq, i;
>  
> +	if (wm831x_power->usb_charger) {
> +		usb_charger_unregister_notify(wm831x_power->usb_charger,
> +					      &wm831x_power->usb_notify);
> +		usb_charger_put(wm831x_power->usb_charger);
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
>  		irq = wm831x_irq(wm831x, 
>  				 platform_get_irq_byname(pdev,
> diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
> index dcc9631..5af8399 100644
> --- a/include/linux/mfd/wm831x/pdata.h
> +++ b/include/linux/mfd/wm831x/pdata.h
> @@ -126,6 +126,9 @@ struct wm831x_pdata {
>  	/** The driver should initiate a power off sequence during shutdown */
>  	bool soft_shutdown;
>  
> +	/** dev_name of USB charger gadget to integrate with */
> +	const char *usb_gadget;
> +
>  	int irq_base;
>  	int gpio_base;
>  	int gpio_defaults[WM831X_GPIO_NUM];
> -- 
> 1.7.9.5
> 

-- 

Best Regards,
Peter Chen

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

[Li Jun]

On Fri, Aug 14, 2015 at 07:04:56PM +0800, Baolin Wang wrote:
> On 14 August 2015 at 16:55, Li Jun <b47624@freescale.com> wrote:
> > Hi Baolin,
> >
> > On Fri, Aug 14, 2015 at 05:47:43PM +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.
> >>
> >
> > Why not add power supply class support into this?
> >
> 
> Hi Jun,
> 
> We don't need the power supply class support into the usb charger,
I suppose usb charger is also a power supply for the system, we can use power
supply class framework for notify mechanism and get/set many attributes(maybe
also the current limit), I see those usb charger drivers under ./driver/power/
are designed with power supply supported.

Li Jun
> just introduce the notify mechanism for power to set the current limit
> when notifying some events from usb charger. Maybe I misunderstand
> your meanings, please describe it detailedly. Thanks for your
> comments.
> 
> > Li Jun
> >
> 
> 
> 
> -- 
> Baolin.wang
> Best Regards

Re: [PATCH v2 1/3] gadget: Support for the usb charger framework

[Baolin Wang]

On 17 August 2015 at 08:40, Peter Chen <peter.chen@freescale.com> wrote:
> On Fri, Aug 14, 2015 at 05:47:44PM +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 |   38 +++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/gadget.h        |   20 +++++++++++++++++++
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>> index f660afb..47b231c 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
>> @@ -129,6 +130,32 @@ 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)
>> +{
>> +     int ret;
>> +
>> +     mutex_lock(&gadget->lock);
>> +     ret = raw_notifier_chain_register(&gadget->nh, nb);
>> +     mutex_unlock(&gadget->lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
>> +
>> +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
>> +                              struct notifier_block *nb)
>> +{
>> +     int ret;
>> +
>> +     mutex_lock(&gadget->lock);
>> +     ret = raw_notifier_chain_unregister(&gadget->nh, nb);
>> +     mutex_unlock(&gadget->lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
>> +
>>  /* ------------------------------------------------------------------------- */
>>
>>  /**
>> @@ -226,6 +253,10 @@ static void usb_gadget_state_work(struct work_struct *work)
>>       struct usb_gadget *gadget = work_to_gadget(work);
>>       struct usb_udc *udc = gadget->udc;
>>
>> +     mutex_lock(&gadget->lock);
>> +     raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
>> +     mutex_unlock(&gadget->lock);
>> +
>>       if (udc)
>>               sysfs_notify(&udc->dev.kobj, NULL, "state");
>>  }
>> @@ -364,6 +395,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);
>> +     mutex_init(&gadget->lock);
>>       gadget->dev.parent = parent;
>>
>>  #ifdef       CONFIG_HAS_DMA
>> @@ -405,6 +438,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;
>> +
>
> If the charger's initialization fails, you may need to
> call device_del(&udc->dev).
>

Yes, I missed that. Thanks for your comments.

> Peter
>
>>       return 0;
>>
>>  err4:
>> @@ -481,6 +518,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 c14a69b..78cc862 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -537,6 +537,7 @@ struct usb_gadget_ops {
>>       struct usb_ep *(*match_ep)(struct usb_gadget *,
>>                       struct usb_endpoint_descriptor *,
>>                       struct usb_ss_ep_comp_descriptor *);
>> +     enum usb_charger_type   (*get_charger_type)(struct usb_gadget *);
>>  };
>>
>>  /**
>> @@ -609,6 +610,9 @@ struct usb_gadget {
>>       unsigned                        out_epnum;
>>       unsigned                        in_epnum;
>>       struct usb_otg_caps             *otg_caps;
>> +     struct raw_notifier_head        nh;
>> +     struct usb_charger              *uchger;
>> +     struct mutex                    lock;
>>
>>       unsigned                        sg_supported:1;
>>       unsigned                        is_otg:1;
>> @@ -1183,6 +1187,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
>>
>
> --
>
> Best Regards,
> Peter Chen



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 14 August 2015 at 23:27, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Aug 14, 2015 at 05:47:45PM +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/Kconfig      |    7 +
>>  drivers/usb/gadget/Makefile     |    1 +
>>  drivers/usb/gadget/charger.c    |  561 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/usb_charger.h |  145 ++++++++++
>>  4 files changed, 714 insertions(+)
>>  create mode 100644 drivers/usb/gadget/charger.c
>>  create mode 100644 include/linux/usb/usb_charger.h
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index bcf83c0..3d2b959 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -127,6 +127,13 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
>>          a module parameter as well.
>>          If unsure, say 2.
>>
>> +config USB_CHARGER
>> +     bool "USB charger support"
>> +     help
>> +       The usb charger driver based on the usb gadget that makes an
>> +       enhancement to a power driver which can set the current limitation
>> +       when the usb charger is added or removed.
>> +
>>  source "drivers/usb/gadget/udc/Kconfig"
>>
>>  #
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 598a67d..1e421c1 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -10,3 +10,4 @@ libcomposite-y                      := usbstring.o config.o epautoconf.o
>>  libcomposite-y                       += composite.o functions.o configfs.o u_f.o
>>
>>  obj-$(CONFIG_USB_GADGET)     += udc/ function/ legacy/
>> +obj-$(CONFIG_USB_CHARGER)    += charger.o
>> diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
>> new file mode 100644
>> index 0000000..f24f7b7
>> --- /dev/null
>> +++ b/drivers/usb/gadget/charger.c
>> @@ -0,0 +1,561 @@
>> +/*
>> + * usb charger.c -- USB charger driver
>
> I think your company also wants a copyright line here.  Not that it
> means anything at all, but some lawyers love cargo-cult text blobs.
>

Hi Greg,

Thanks for your reviewing and comments. I'll add it in next patch.

>
>> + *
>> + * 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 think I did not get your point, could you explain it detailedly?

>> + */
>> +
>> +#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 DEFINE_IDA(usb_charger_ida);
>> +static struct bus_type usb_charger_subsys = {
>> +     .name           = "usb-charger",
>> +     .dev_name       = "usb-charger",
>> +};
>> +
>> +static struct usb_charger *dev_to_uchger(struct device *udev)
>> +{
>> +     return container_of(udev, struct usb_charger, dev);
>> +}
>> +
>> +static ssize_t usb_charger_cur_show(struct device *dev,
>> +                                 struct device_attribute *attr,
>> +                                 char *buf)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +
>> +     return scnprintf(buf, PAGE_SIZE, "%d %d %d %d\n",
>> +                      uchger->cur_limit.sdp_cur_limit,
>> +                      uchger->cur_limit.dcp_cur_limit,
>> +                      uchger->cur_limit.cdp_cur_limit,
>> +                      uchger->cur_limit.aca_cur_limit);
>> +}
>> +
>> +static ssize_t usb_charger_cur_store(struct device *dev,
>> +                                  struct device_attribute *attr,
>> +                                  const char *buf, size_t count)
>> +{
>> +     struct usb_charger *uchger = dev_to_uchger(dev);
>> +     struct usb_charger_cur_limit cur;
>> +     int ret;
>> +
>> +     ret = sscanf(buf, "%d %d %d %d",
>> +                  &cur.sdp_cur_limit, &cur.dcp_cur_limit,
>> +                  &cur.cdp_cur_limit, &cur.aca_cur_limit);
>> +     if (ret == 0)
>> +             return -EINVAL;
>> +
>> +     ret = usb_charger_set_cur_limit(uchger, &cur);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +static DEVICE_ATTR(cur_limit, 0644, usb_charger_cur_show, usb_charger_cur_store);
>
> DEVICE_ATTR_RW()?
>

OK, I'll replace it with DEVICE_ATTR_RW().

>> +
>> +static struct attribute *uchger_attributes[] = {
>> +     &dev_attr_cur_limit.attr,
>> +     NULL
>> +};
>> +
>> +static const struct attribute_group uchger_attr_group = {
>> +     .attrs = uchger_attributes,
>> +};
>
> ATTRIBUTE_GROUPS()?
>

OK, I'll replace it with ATTRIBUTE_GROUPS().

>> +
>> +/*
>> + * usb_charger_find_by_name - Get the usb charger device by name.
>> + * @name - usb charger device name.
>> + *
>> + * return the instance of usb charger device, the device must be
>> + * released with usb_charger_put().
>> + */
>> +struct usb_charger *usb_charger_find_by_name(const char *name)
>> +{
>> +     struct device *udev;
>> +
>> +     if (!name)
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     udev = bus_find_device_by_name(&usb_charger_subsys, NULL, name);
>> +     if (!udev)
>> +             return ERR_PTR(-ENODEV);
>> +
>> +     return dev_to_uchger(udev);
>> +}
>> +
>> +/*
>> + * usb_charger_get() - Reference a usb charger.
>> + * @uchger - usb charger
>> + */
>> +struct usb_charger *usb_charger_get(struct usb_charger *uchger)
>> +{
>> +     return (uchger && get_device(&uchger->dev)) ? uchger : NULL;
>> +}
>> +
>> +/*
>> + * usb_charger_put() - Dereference a usb charger.
>> + * @uchger - charger to release
>> + */
>> +void usb_charger_put(struct usb_charger *uchger)
>> +{
>> +     if (uchger)
>> +             put_device(&uchger->dev);
>> +}
>> +
>> +/*
>> + * usb_charger_register_notify() - Register a notifiee to get notified by
>> + * any attach status changes from the usb charger detection.
>> + * @uchger - the usb charger device which is monitored.
>> + * @nb - a notifier block to be registered.
>> + */
>> +int usb_charger_register_notify(struct usb_charger *uchger,
>> +                             struct notifier_block *nb)
>> +{
>> +     int ret;
>> +
>> +     if (!uchger || !nb)
>> +             return -EINVAL;
>> +
>> +     mutex_lock(&uchger->lock);
>> +     ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
>> +
>> +     /* Generate an initial notify so users start in the right state */
>> +     if (!ret) {
>> +             usb_charger_detect_type(uchger);
>> +             raw_notifier_call_chain(&uchger->uchger_nh,
>> +                                     usb_charger_get_cur_limit(uchger),
>> +                                     uchger);
>> +     }
>> +     mutex_unlock(&uchger->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +/*
>> + * 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.
>> + */
>> +int usb_charger_unregister_notify(struct usb_charger *uchger,
>> +                               struct notifier_block *nb)
>> +{
>> +     int ret;
>> +
>> +     if (!uchger || !nb)
>> +             return -EINVAL;
>> +
>> +     mutex_lock(&uchger->lock);
>> +     ret = raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
>> +     mutex_unlock(&uchger->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +/*
>> + * usb_charger_detect_type() - Get the usb charger type by the callback
>> + * which is implemented by gadget operations.
>> + * @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_set_cur_limit() - Set the current limitation.
>> + * @uchger - the usb charger device.
>> + * @cur_limit_set - the current limitation.
>> + */
>> +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.
>> + *
>> + * return the current limitation to set.
>> + */
>> +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_notifier_others() - It will notify other device registered
>> + * on usb charger when the usb charger state is changed.
>> + * @uchger - the usb charger device.
>> + * @state - the state of the usb charger.
>> + */
>> +static void
>> +usb_charger_notify_others(struct usb_charger *uchger,
>> +                       enum usb_charger_state state)
>> +{
>> +     mutex_lock(&uchger->lock);
>> +     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;
>> +     }
>> +     mutex_unlock(&uchger->lock);
>> +}
>> +
>> +/*
>> + * usb_charger_plug_by_extcon() - The notifier call function which is registered
>> + * on the extcon device.
>> + * @nb - the notifier block that notified by extcon device.
>> + * @state - the extcon device state.
>> + * @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.
>> + * @nb - the notifier block that notified by usb gadget device.
>> + * @state - the usb gadget state.
>> + * @data - here specify a usb gadget 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);
>> +
>> +     kfree(uchger->name);
>> +     kfree(uchger);
>> +}
>> +
>> +/*
>> + * usb_charger_unregister() - Unregister a usb charger device.
>> + * @uchger - the usb charger to be unregistered.
>> + */
>> +static int usb_charger_unregister(struct usb_charger *uchger)
>> +{
>> +     if (!uchger)
>> +             return -EINVAL;
>> +
>> +     sysfs_remove_group(&uchger->dev.kobj, &uchger_attr_group);
>> +     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
>> + * which is created by the usb charger framework.
>> + * @uchger - the new usb charger device.
>> + */
>> +static int usb_charger_register(struct device *dev, struct usb_charger *uchger)
>> +{
>> +     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;
>> +     uchger->dev.bus = &usb_charger_subsys;
>> +
>> +     ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
>> +     if (ret < 0) {
>> +             dev_err(dev, "get usb charger id failed\n");
>> +             return ret;
>> +     }
>> +
>> +     uchger->id = ret;
>> +     dev_set_name(&uchger->dev, "usb-charger.%d", uchger->id);
>> +     dev_set_drvdata(&uchger->dev, uchger);
>> +
>> +     ret = device_register(&uchger->dev);
>> +     if (ret)
>> +             goto fail_register;
>> +
>> +     ret = sysfs_create_group(&uchger->dev.kobj, &uchger_attr_group);
>
> And you just raced with userspace, and lost :(
>
> Assign the group to the device _before_ you tell userspace about it.
>
> The big hint here that is wrong is that you have to call a sysfs_*
> function for a struct device.  There is no device_create_group() call
> for a reason.
>

I think your suggestion is device_add_groups() and not
device_create_group() which is not found in mainline. Is it right?

>
>
>> +     if (ret)
>> +             goto fail_create_files;
>> +
>> +     return 0;
>> +
>> +fail_create_files:
>> +     device_unregister(&uchger->dev);
>> +fail_register:
>> +     put_device(&uchger->dev);
>> +     ida_simple_remove(&usb_charger_ida, uchger->id);
>> +     uchger->id = -1;
>> +     dev_err(dev, "Failed to register usb charger (%s)\n",
>> +             uchger->name);
>> +     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];
>
> That's a lot of stack, why?
>

Consider storing a new usb charger name firstly with buf[], but now
will remove the 'name' memeber in struct usb_charger.

>> +     char *str;
>> +     int ret;
>> +
>> +     if (!ugadget)
>> +             return -EINVAL;
>> +
>> +     uchger = devm_kzalloc(&ugadget->dev, sizeof(struct usb_charger),
>> +                           GFP_KERNEL);
>
> No, you are creating a new struct device, you can't assign its resources
> to another struct device, that breaks all of the lifetime rules.
> kzalloc() only please.
>

Make sense of it.

>> +     if (!uchger)
>> +             return -ENOMEM;
>> +
>> +     sprintf(buf, "usb-charger.%s", ugadget->name);
>> +     str = devm_kzalloc(&ugadget->dev, sizeof(char) * (strlen(buf) + 1),
>> +                        GFP_KERNEL);
>
> Why have a string when you already have a name for the device, in the
> device itself?  What is this all for?
>

Consider expanding to other uses with member 'name' in future, but I
think it is reasonable to remove these with your suggestion.

>
>> +     if (!str)
>> +             return -ENOMEM;
>> +
>> +     strcpy(str, buf);
>> +     uchger->name = str;
>> +     uchger->type = UNKNOWN_TYPE;
>> +     uchger->state = USB_CHARGER_DEFAULT;
>> +     uchger->id = -1;
>> +     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;
>> +
>> +     mutex_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;
>> +             extcon_register_notifier(edev, EXTCON_USB, &uchger->extcon_nb.nb);
>> +     }
>> +
>> +     /* 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_gadget_register_notify(ugadget, &uchger->gadget_nb);
>> +
>> +     /* register a new usb charger */
>> +     ret = usb_charger_register(&ugadget->dev, uchger);
>> +     if (ret)
>> +             goto fail;
>> +
>> +     return 0;
>> +
>> +fail:
>> +     if (uchger->extcon_dev)
>> +             extcon_unregister_notifier(uchger->extcon_dev,
>> +                                        EXTCON_USB, &uchger->extcon_nb.nb);
>> +
>> +     usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
>> +     return ret;
>> +}
>> +
>> +int usb_charger_exit(struct usb_gadget *ugadget)
>> +{
>> +     struct usb_charger *uchger = ugadget->uchger;
>> +
>> +     if (!uchger)
>> +             return -EINVAL;
>> +
>> +     if (uchger->extcon_dev)
>> +             extcon_unregister_notifier(uchger->extcon_dev,
>> +                                        EXTCON_USB, &uchger->extcon_nb.nb);
>> +
>> +     usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
>> +     ida_simple_remove(&usb_charger_ida, uchger->id);
>> +
>> +     return usb_charger_unregister(uchger);
>> +}
>> +
>> +static int __init usb_charger_sysfs_init(void)
>> +{
>> +     return subsys_system_register(&usb_charger_subsys, NULL);
>> +}
>> +core_initcall(usb_charger_sysfs_init);
>> +
>> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
>> +MODULE_DESCRIPTION("USB charger driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/usb/usb_charger.h b/include/linux/usb/usb_charger.h
>> new file mode 100644
>> index 0000000..8cbfaae
>> --- /dev/null
>> +++ b/include/linux/usb/usb_charger.h
>> @@ -0,0 +1,145 @@
>> +#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. */
>
> Heh, as if people read comments :(
>

OK, remove it.

>> +     const char              *name;
>
> This isn't needed, use the one in your struct device.
>

OK, remove it.

>> +     struct device           dev;
>> +     struct raw_notifier_head        uchger_nh;
>> +     struct list_head        entry;
>
> Why do you need another list?  The device already is on a list.
>

Sorry, I did not remember to remove the 'entry' member in the structure

>> +     struct mutex            lock;
>
> What is this protecting, document it.
>

OK, I'll  add some comments for the lock.

> thanks,
>
> greg k-h



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 17 August 2015 at 09:15, Li Jun <b47624@freescale.com> wrote:
> On Fri, Aug 14, 2015 at 07:04:56PM +0800, Baolin Wang wrote:
>> On 14 August 2015 at 16:55, Li Jun <b47624@freescale.com> wrote:
>> > Hi Baolin,
>> >
>> > On Fri, Aug 14, 2015 at 05:47:43PM +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.
>> >>
>> >
>> > Why not add power supply class support into this?
>> >
>>
>> Hi Jun,
>>
>> We don't need the power supply class support into the usb charger,
> I suppose usb charger is also a power supply for the system, we can use power
> supply class framework for notify mechanism and get/set many attributes(maybe
> also the current limit), I see those usb charger drivers under ./driver/power/
> are designed with power supply supported.
>

I don't think so. The usb charger is rely on the usb gadget, which is
not a complete power supply device and it combines the usb and the
power supply. Thus we make it into usb gadget system. Thanks.

> Li Jun
>> just introduce the notify mechanism for power to set the current limit
>> when notifying some events from usb charger. Maybe I misunderstand
>> your meanings, please describe it detailedly. Thanks for your
>> comments.
>>
>> > Li Jun
>> >
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards

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

[Li Jun]

On Mon, Aug 17, 2015 at 02:02:08PM +0800, Baolin Wang wrote:
> On 17 August 2015 at 09:15, Li Jun <b47624@freescale.com> wrote:
> > On Fri, Aug 14, 2015 at 07:04:56PM +0800, Baolin Wang wrote:
> >> On 14 August 2015 at 16:55, Li Jun <b47624@freescale.com> wrote:
> >> > Hi Baolin,
> >> >
> >> > On Fri, Aug 14, 2015 at 05:47:43PM +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.
> >> >>
> >> >
> >> > Why not add power supply class support into this?
> >> >
> >>
> >> Hi Jun,
> >>
> >> We don't need the power supply class support into the usb charger,
> > I suppose usb charger is also a power supply for the system, we can use power
> > supply class framework for notify mechanism and get/set many attributes(maybe
> > also the current limit), I see those usb charger drivers under ./driver/power/
> > are designed with power supply supported.
> >
> 
> I don't think so. The usb charger is rely on the usb gadget, which is
> not a complete power supply device and it combines the usb and the
> power supply. Thus we make it into usb gadget system. Thanks.
> 
Why it cannot be a complete power supply device? I was thinking this
framework can cover it, I have no doubt on putting this framework
into gadget system, but still can't understand why we don't need
power supply class at all for a usb charger, or you think introduce
power supply into usb gadget is not a right direction from code structure
point view?

Li Jun
> > Li Jun
> >> just introduce the notify mechanism for power to set the current limit
> >> when notifying some events from usb charger. Maybe I misunderstand
> >> your meanings, please describe it detailedly. Thanks for your
> >> comments.
> >>
> >> > Li Jun
> >> >
> >>
> >>
> >>
> >> --
> >> Baolin.wang
> >> Best Regards
> 
> 
> 
> -- 
> Baolin.wang
> Best Regards

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

[Baolin Wang]

On 17 August 2015 at 16:20, Li Jun <b47624@freescale.com> wrote:
> On Mon, Aug 17, 2015 at 02:02:08PM +0800, Baolin Wang wrote:
>> On 17 August 2015 at 09:15, Li Jun <b47624@freescale.com> wrote:
>> > On Fri, Aug 14, 2015 at 07:04:56PM +0800, Baolin Wang wrote:
>> >> On 14 August 2015 at 16:55, Li Jun <b47624@freescale.com> wrote:
>> >> > Hi Baolin,
>> >> >
>> >> > On Fri, Aug 14, 2015 at 05:47:43PM +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.
>> >> >>
>> >> >
>> >> > Why not add power supply class support into this?
>> >> >
>> >>
>> >> Hi Jun,
>> >>
>> >> We don't need the power supply class support into the usb charger,
>> > I suppose usb charger is also a power supply for the system, we can use power
>> > supply class framework for notify mechanism and get/set many attributes(maybe
>> > also the current limit), I see those usb charger drivers under ./driver/power/
>> > are designed with power supply supported.
>> >
>>
>> I don't think so. The usb charger is rely on the usb gadget, which is
>> not a complete power supply device and it combines the usb and the
>> power supply. Thus we make it into usb gadget system. Thanks.
>>
> Why it cannot be a complete power supply device? I was thinking this
> framework can cover it, I have no doubt on putting this framework
> into gadget system, but still can't understand why we don't need
> power supply class at all for a usb charger, or you think introduce
> power supply into usb gadget is not a right direction from code structure
> point view?
>

We just do not think the usb charger as a real device, which is only
used to set the current limitation when the usb charger state is
changed detecting by extcon device or usb gadget. So we just need one
message to notify the power user to set the current limitation when
uab charge is added or removed. I also agree with the power supply
framework can cover it, but we don't need to implement it to be
another power supply, cause there is a real device as the power supply
to deal with the power issue in the system. Thanks.

> Li Jun
>> > Li Jun
>> >> just introduce the notify mechanism for power to set the current limit
>> >> when notifying some events from usb charger. Maybe I misunderstand
>> >> your meanings, please describe it detailedly. Thanks for your
>> >> comments.
>> >>
>> >> > Li Jun
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> Baolin.wang
>> >> Best Regards
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards

Re: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

[Tim Bird]

On 08/16/2015 08:03 PM, Baolin Wang via device-mainlining wrote:
> On 14 August 2015 at 23:27, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:
>>> + *
>>> + * 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 think I did not get your point, could you explain it detailedly?

The full kernel is licensed under v2 of the GPL only, not "any later version".
See the second paragraph at the top of the COPYING file in the root directory
of the kernel source tree. There are differences on individual files, but
having this file allow "any later version" makes it different from much of
rest of the kernel.

Unless you have a specific reason to allow greater-than-V2 GPL licensing on this
file, you should change the licensing clause.  The following wording appears
to be pretty popular:

* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.

  -- Tim

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

[Mark Brown]

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

On Mon, Aug 17, 2015 at 11:03:26AM +0800, Baolin Wang wrote:
> On 14 August 2015 at 23:27, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:

> >> + * 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 think I did not get your point, could you explain it detailedly?

Unlike a lot of userspace projects the kernel is normally just licensed
under GPLv2 without the option to relicense with a later version so the
"...or any later version" is a bit unusual.  There is some controversy
about some of the changes in GPLv3.

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

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

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

On Mon, Aug 17, 2015 at 09:07:08AM +0800, Peter Chen wrote:
> On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:

> > +	1500,
> > +	1800,
> > +	550,
> > +};

> Why 550 is the last, but not 1800?

You'd have to ask the hardware engineers who designed the chip.  I
suspect it's because 550 was added at a late stage in the design process
but I'm basically just guessing there.

> >   *********************************************************************/
> > @@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
> >  		}
> >  	}

> > +	if (wm831x_pdata && wm831x_pdata->usb_gadget) {

> Where the wm831x_pdata->usb_gadget is initialized?

It's platform data, it will be initialised by whatever registers the
platform data.

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

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

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

On Mon, Aug 17, 2015 at 10:26:23AM -0700, Mark Brown wrote:
> On Mon, Aug 17, 2015 at 09:07:08AM +0800, Peter Chen wrote:
> > On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:
> 
> > > +	1500,
> > > +	1800,
> > > +	550,
> > > +};
> 
> > Why 550 is the last, but not 1800?
> 
> You'd have to ask the hardware engineers who designed the chip.  I
> suspect it's because 550 was added at a late stage in the design process
> but I'm basically just guessing there.
> 
> > >   *********************************************************************/
> > > @@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
> > >  		}
> > >  	}
> 
> > > +	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
> 
> > Where the wm831x_pdata->usb_gadget is initialized?
> 
> It's platform data, it will be initialised by whatever registers the
> platform data.

passing pointers through pdata ?

-- 
balbi

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

Re: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

[Baolin Wang]

On 17 August 2015 at 23:25, Tim Bird <tim.bird@sonymobile.com> wrote:
>
>
> On 08/16/2015 08:03 PM, Baolin Wang via device-mainlining wrote:
>> On 14 August 2015 at 23:27, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:
>>>> + *
>>>> + * 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 think I did not get your point, could you explain it detailedly?
>
> The full kernel is licensed under v2 of the GPL only, not "any later version".
> See the second paragraph at the top of the COPYING file in the root directory
> of the kernel source tree. There are differences on individual files, but
> having this file allow "any later version" makes it different from much of
> rest of the kernel.
>
> Unless you have a specific reason to allow greater-than-V2 GPL licensing on this
> file, you should change the licensing clause.  The following wording appears
> to be pretty popular:
>
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
>

Hi Tim,

Make sense of it. Thanks for your explanation.

>   -- Tim



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 18 August 2015 at 01:24, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Aug 17, 2015 at 11:03:26AM +0800, Baolin Wang wrote:
>> On 14 August 2015 at 23:27, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:
>
>> >> + * 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 think I did not get your point, could you explain it detailedly?
>
> Unlike a lot of userspace projects the kernel is normally just licensed
> under GPLv2 without the option to relicense with a later version so the
> "...or any later version" is a bit unusual.  There is some controversy
> about some of the changes in GPLv3.

Make sense. Thanks a lot.


-- 
Baolin.wang
Best Regards

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

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

On Mon, Aug 17, 2015 at 06:58:16PM -0500, Felipe Balbi wrote:
> On Mon, Aug 17, 2015 at 10:26:23AM -0700, Mark Brown wrote:
> > On Mon, Aug 17, 2015 at 09:07:08AM +0800, Peter Chen wrote:
> > > On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:

> > > > +	if (wm831x_pdata && wm831x_pdata->usb_gadget) {

> > > Where the wm831x_pdata->usb_gadget is initialized?

> > It's platform data, it will be initialised by whatever registers the
> > platform data.

> passing pointers through pdata ?

It's a char * to a name, not a pointer to struct.

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

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Peter Chen]

On Mon, Aug 17, 2015 at 10:26:23AM -0700, Mark Brown wrote:
> On Mon, Aug 17, 2015 at 09:07:08AM +0800, Peter Chen wrote:
> > On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:
> 
> > > +	1500,
> > > +	1800,
> > > +	550,
> > > +};
> 
> > Why 550 is the last, but not 1800?
> 
> You'd have to ask the hardware engineers who designed the chip.  I
> suspect it's because 550 was added at a late stage in the design process
> but I'm basically just guessing there.

ok, I just had suspected below function's correctness, after looking
it again, it always set 1800 as charging limit, does it be expected?

+/* In miliamps */
+static unsigned int wm831x_usb_limits[] = {
+       0,
+       2,
+       100,
+       500,
+       900,
+       1500,
+       1800,
+       550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+                                  unsigned long limit, void *data)
+{
+       struct wm831x_power *wm831x_power = container_of(nb,
+                                                        struct wm831x_power,
+                                                        usb_notify);
+       int i, best;
+
+       /* Find the highest supported limit */
+       best = 0;
+       for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+               if (limit < wm831x_usb_limits[i] &&
+                   wm831x_usb_limits[best] < wm831x_usb_limits[i])
+                       best = i;
+       }
+
+       dev_dbg(wm831x_power->wm831x->dev,
+               "Limiting USB current to %dmA", wm831x_usb_limits[best]);
+
+       wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+                       WM831X_USB_ILIM_MASK, best);
+
+       return 0;
+}
+

> 
> > >   *********************************************************************/
> > > @@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
> > >  		}
> > >  	}
> 
> > > +	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
> 
> > Where the wm831x_pdata->usb_gadget is initialized?
> 
> It's platform data, it will be initialised by whatever registers the
> platform data.

Get it, I just do not find in this patch set, thanks.

-- 

Best Regards,
Peter Chen

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

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

On Mon, Aug 17, 2015 at 10:04:05PM -0700, Mark Brown wrote:
> On Mon, Aug 17, 2015 at 06:58:16PM -0500, Felipe Balbi wrote:
> > On Mon, Aug 17, 2015 at 10:26:23AM -0700, Mark Brown wrote:
> > > On Mon, Aug 17, 2015 at 09:07:08AM +0800, Peter Chen wrote:
> > > > On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:
> 
> > > > > +	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
> 
> > > > Where the wm831x_pdata->usb_gadget is initialized?
> 
> > > It's platform data, it will be initialised by whatever registers the
> > > platform data.
> 
> > passing pointers through pdata ?
> 
> It's a char * to a name, not a pointer to struct.

aha, fair enough.

-- 
balbi

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

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

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

On Tue, Aug 18, 2015 at 01:20:12PM +0800, Peter Chen wrote:

> ok, I just had suspected below function's correctness, after looking
> it again, it always set 1800 as charging limit, does it be expected?

> +       /* Find the highest supported limit */
> +       best = 0;
> +       for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
> +               if (limit < wm831x_usb_limits[i] &&

The above check is intended to ensure that we don't go over the limit
that was passed in in the callback.  The goal is to select the highest
option that is less than the limit passed in.

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

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Peter Chen]

On Tue, Aug 18, 2015 at 09:12:00AM -0700, Mark Brown wrote:
> On Tue, Aug 18, 2015 at 01:20:12PM +0800, Peter Chen wrote:
> 
> > ok, I just had suspected below function's correctness, after looking
> > it again, it always set 1800 as charging limit, does it be expected?
> 
> > +       /* Find the highest supported limit */
> > +       best = 0;
> > +       for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
> > +               if (limit < wm831x_usb_limits[i] &&
> 

+/* In miliamps */
+static unsigned int wm831x_usb_limits[] = {
+       0,
+       2,
+       100,
+       500,
+       900,
+       1500,
+       1800,
+       550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+                                  unsigned long limit, void *data)
+{
+       struct wm831x_power *wm831x_power = container_of(nb,
+                                                        struct wm831x_power,
+                                                        usb_notify);
+       int i, best;
+
+       /* Find the highest supported limit */
+       best = 0;
+       for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+               if (limit < wm831x_usb_limits[i] &&
+                   wm831x_usb_limits[best] < wm831x_usb_limits[i])
+                       best = i;
+       }
+
+       dev_dbg(wm831x_power->wm831x->dev,
+               "Limiting USB current to %dmA", wm831x_usb_limits[best]);
+
+       wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+                       WM831X_USB_ILIM_MASK, best);
+
+       return 0;
+}
+

> The above check is intended to ensure that we don't go over the limit
> that was passed in in the callback.  The goal is to select the highest
> option that is less than the limit passed in.

Please correct me if I was wrong, If there is SDP (PC), the current
limit is 500mA, then when the i = 4, (500 < 900) && (0 < 900) is true,
the best is 4. When the i goes to 5, (500 < 1500) && (900 < 1500) is
true, the best is 5. At last, the best will be 1800, which is not our
expectation.

Below code may be correct for the goal you expressed.

       for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
               if (limit >= wm831x_usb_limits[i] &&
                   wm831x_usb_limits[best] < wm831x_usb_limits[i])
                       best = i;
       }

-- 

Best Regards,
Peter Chen

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

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

On Wed, Aug 19, 2015 at 08:02:37AM +0800, Peter Chen wrote:

> Below code may be correct for the goal you expressed.

>        for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
>                if (limit >= wm831x_usb_limits[i] &&
>                    wm831x_usb_limits[best] < wm831x_usb_limits[i])
>                        best = i;
>        }

Yes, that's right.

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

RE: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[David Laight]

From: Baolin Wang
> Sent: 14 August 2015 10:48
> +/* In miliamps */

Spelling police: "milliamps"

> +static unsigned int wm831x_usb_limits[] = {
> +	0,
> +	2,
> +	100,
> +	500,
> +	900,
> +	1500,
> +	1800,
> +	550,
> +};

	David

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

On 20 August 2015 at 17:02, David Laight <David.Laight@aculab.com> wrote:
> From: Baolin Wang
>> Sent: 14 August 2015 10:48
>> +/* In miliamps */
>
> Spelling police: "milliamps"
>

Hi David,

I'll correct it in next patch series. Thanks for your comments.

>> +static unsigned int wm831x_usb_limits[] = {
>> +     0,
>> +     2,
>> +     100,
>> +     500,
>> +     900,
>> +     1500,
>> +     1800,
>> +     550,
>> +};
>
>         David
>



-- 
Baolin.wang
Best Regards

Re: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

[Pavel Machek]

Hi!

> On 08/16/2015 08:03 PM, Baolin Wang via device-mainlining wrote:
> > On 14 August 2015 at 23:27, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:
> >>> + *
> >>> + * 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 think I did not get your point, could you explain it detailedly?
> 
> The full kernel is licensed under v2 of the GPL only, not "any later version".
> See the second paragraph at the top of the COPYING file in the root directory
> of the kernel source tree. There are differences on individual files, but
> having this file allow "any later version" makes it different from much of
> rest of the kernel.
> 
> Unless you have a specific reason to allow greater-than-V2 GPL licensing on this
> file, you should change the licensing clause.  The following wording appears
> to be pretty popular:
> 
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.

Please, keep it V2 or later, if you can. It makes sharing code with U-Boot (for
example) easier.

There's long tradition of "V2 or later" code in the kernel.

Best regards,
									Pavel

Re: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

[Greg KH]

On Thu, Oct 08, 2015 at 05:50:31PM +0200, Pavel Machek wrote:
> Hi!
> 
> > On 08/16/2015 08:03 PM, Baolin Wang via device-mainlining wrote:
> > > On 14 August 2015 at 23:27, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >> On Fri, Aug 14, 2015 at 05:47:45PM +0800, Baolin Wang wrote:
> > >>> + *
> > >>> + * 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 think I did not get your point, could you explain it detailedly?
> > 
> > The full kernel is licensed under v2 of the GPL only, not "any later version".
> > See the second paragraph at the top of the COPYING file in the root directory
> > of the kernel source tree. There are differences on individual files, but
> > having this file allow "any later version" makes it different from much of
> > rest of the kernel.
> > 
> > Unless you have a specific reason to allow greater-than-V2 GPL licensing on this
> > file, you should change the licensing clause.  The following wording appears
> > to be pretty popular:
> > 
> > * This program is free software; you can redistribute it and/or modify
> > * it under the terms of the GNU General Public License version 2 as
> > * published by the Free Software Foundation.
> 
> Please, keep it V2 or later, if you can. It makes sharing code with U-Boot (for
> example) easier.
> 
> There's long tradition of "V2 or later" code in the kernel.

And there's even longer "v2 only" tradition as well.  It's up to the
person / company submitting the code to pick the license for it, let
them make the decision please.

thanks,

greg k-h

Re: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

[Pavel Machek]

Hi!

> > > * This program is free software; you can redistribute it and/or modify
> > > * it under the terms of the GNU General Public License version 2 as
> > > * published by the Free Software Foundation.
> > 
> > Please, keep it V2 or later, if you can. It makes sharing code with U-Boot (for
> > example) easier.
> > 
> > There's long tradition of "V2 or later" code in the kernel.
> 
> And there's even longer "v2 only" tradition as well.  It's up to the
> person / company submitting the code to pick the license for it, let
> them make the decision please.

You wrote:

>> + *
>> + * 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"?

Given that you are quite high-level maintainer, that does not sound exactly
like "let them make the decision". It sounds like a pressure to change the
license to the one you liked. WHy did you have to ask?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Device-mainlining] [PATCH v2 2/3] gadget: Introduce the usb charger framework

[Greg KH]

On Thu, Oct 08, 2015 at 07:04:05PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > * This program is free software; you can redistribute it and/or modify
> > > > * it under the terms of the GNU General Public License version 2 as
> > > > * published by the Free Software Foundation.
> > > 
> > > Please, keep it V2 or later, if you can. It makes sharing code with U-Boot (for
> > > example) easier.
> > > 
> > > There's long tradition of "V2 or later" code in the kernel.
> > 
> > And there's even longer "v2 only" tradition as well.  It's up to the
> > person / company submitting the code to pick the license for it, let
> > them make the decision please.
> 
> You wrote:
> 
> >> + *
> >> + * 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"?
> 
> Given that you are quite high-level maintainer, that does not sound exactly
> like "let them make the decision". It sounds like a pressure to change the
> license to the one you liked. WHy did you have to ask?

I have to "ask" because I need to ensure that people put thought into
the license they chose instead of blindly cut-and-pasting it from a
random web site.  Companies all have policies on what the license of the
kernel code they produce are, and by nicely asking new developers this
question, it ensures that they are picking the license in a thoughtful
manner.

If they pick v2 vs. v2+ is of no opinion of mine, either is just
wonderful.

thanks,

greg k-h

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

Hi,

On 3 July 2017 at 16:53, Charles Keepax <ckeepax@opensource.cirrus.com> wrote:
> On Mon, Jul 03, 2017 at 02:07:15PM +0800, Baolin Wang wrote:
>> Integrate with the newly added USB charger interface to limit the current
>> we draw from the USB input based on the input device configuration
>> identified by the USB stack, allowing us to charge more quickly from high
>> current inputs without drawing more current than specified from others.
>>
>> Signed-off-by: Mark Brown <broonie@kernel.org>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
> <snip>
>>  /*********************************************************************
>>   *           Battery properties
>>   *********************************************************************/
>> @@ -607,6 +647,22 @@ static int wm831x_power_probe(struct platform_device *pdev)
>>               }
>>       }
>>
>> +     power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
>> +                                                  "usb-phy", 0);
>> +     if (IS_ERR(power->usb_phy)) {
>> +             ret = PTR_ERR(power->usb_phy);
>> +             dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
>> +             goto err_bat_irq;
>> +     }
>> +
>
> We should probably update the binding documentation for this as
> well: mfd/wm831x.txt

Yes, I will update the binding documentation.

>
> Also I am not sure this needs to be implemented now, but what is
> the plan regarding pdata systems? Generally the driver supports
> both and it would be nice to know there is a way forward for that
> even if we don't implement it yet.

OK, I'll modify this in next version. Thanks.

-- 
Baolin.wang
Best Regards

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

Hi,

On 3 July 2017 at 23:50, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
> Hi,
>
> On Mon, Jul 03, 2017 at 02:07:15PM +0800, Baolin Wang wrote:
>> Integrate with the newly added USB charger interface to limit the current
>> we draw from the USB input based on the input device configuration
>> identified by the USB stack, allowing us to charge more quickly from high
>> current inputs without drawing more current than specified from others.
>>
>> Signed-off-by: Mark Brown <broonie@kernel.org>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>
> Missing DT binding documentation, otherwise fine with me.
> Would be nice to convert existing drivers:
>
> $ git grep -l extcon -- drivers/power/supply
> drivers/power/supply/axp288_charger.c
> drivers/power/supply/bq24190_charger.c
> drivers/power/supply/charger-manager.c
> drivers/power/supply/qcom_smbb.c

I'll create new patches with adding the DT binding documentation if
there are no objections for USB phy modification, moreover I will help
to convert these drivers you listed. Thanks.

>
> -- Sebastian
>
>> ---
>>  drivers/power/supply/wm831x_power.c |   61 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
>> index 7082301..3e3480708 100644
>> --- a/drivers/power/supply/wm831x_power.c
>> +++ b/drivers/power/supply/wm831x_power.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/power_supply.h>
>>  #include <linux/slab.h>
>> +#include <linux/usb/phy.h>
>>
>>  #include <linux/mfd/wm831x/core.h>
>>  #include <linux/mfd/wm831x/auxadc.h>
>> @@ -31,6 +32,8 @@ struct wm831x_power {
>>       char usb_name[20];
>>       char battery_name[20];
>>       bool have_battery;
>> +     struct usb_phy *usb_phy;
>> +     struct notifier_block usb_notify;
>>  };
>>
>>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
>> @@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
>>       POWER_SUPPLY_PROP_VOLTAGE_NOW,
>>  };
>>
>> +/* In milliamps */
>> +static const unsigned int wm831x_usb_limits[] = {
>> +     0,
>> +     2,
>> +     100,
>> +     500,
>> +     900,
>> +     1500,
>> +     1800,
>> +     550,
>> +};
>> +
>> +static int wm831x_usb_limit_change(struct notifier_block *nb,
>> +                                unsigned long limit, void *data)
>> +{
>> +     struct wm831x_power *wm831x_power = container_of(nb,
>> +                                                      struct wm831x_power,
>> +                                                      usb_notify);
>> +     unsigned int i, best;
>> +
>> +     /* Find the highest supported limit */
>> +     best = 0;
>> +     for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
>> +             if (limit >= wm831x_usb_limits[i] &&
>> +                 wm831x_usb_limits[best] < wm831x_usb_limits[i])
>> +                     best = i;
>> +     }
>> +
>> +     dev_dbg(wm831x_power->wm831x->dev,
>> +             "Limiting USB current to %umA", wm831x_usb_limits[best]);
>> +
>> +     wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
>> +                     WM831X_USB_ILIM_MASK, best);
>> +
>> +     return 0;
>> +}
>> +
>>  /*********************************************************************
>>   *           Battery properties
>>   *********************************************************************/
>> @@ -607,6 +647,22 @@ static int wm831x_power_probe(struct platform_device *pdev)
>>               }
>>       }
>>
>> +     power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
>> +                                                  "usb-phy", 0);
>> +     if (IS_ERR(power->usb_phy)) {
>> +             ret = PTR_ERR(power->usb_phy);
>> +             dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
>> +             goto err_bat_irq;
>> +     }
>> +
>> +     power->usb_notify.notifier_call = wm831x_usb_limit_change;
>> +     ret = usb_register_notifier(power->usb_phy,
>> +                                 &power->usb_notify);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
>> +             goto err_bat_irq;
>> +     }
>> +
>>       return ret;
>>
>>  err_bat_irq:
>> @@ -637,6 +693,11 @@ static int wm831x_power_remove(struct platform_device *pdev)
>>       struct wm831x *wm831x = wm831x_power->wm831x;
>>       int irq, i;
>>
>> +     if (wm831x_power->usb_phy) {
>> +             usb_unregister_notifier(wm831x_power->usb_phy,
>> +                                     &wm831x_power->usb_notify);
>> +     }
>> +
>>       for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
>>               irq = wm831x_irq(wm831x,
>>                                platform_get_irq_byname(pdev,
>> --
>> 1.7.9.5
>>



-- 
Baolin.wang
Best Regards

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Sebastian Reichel]

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

Hi,

On Mon, Jul 03, 2017 at 02:07:15PM +0800, Baolin Wang wrote:
> Integrate with the newly added USB charger interface to limit the current
> we draw from the USB input based on the input device configuration
> identified by the USB stack, allowing us to charge more quickly from high
> current inputs without drawing more current than specified from others.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Missing DT binding documentation, otherwise fine with me.
Would be nice to convert existing drivers:

$ git grep -l extcon -- drivers/power/supply
drivers/power/supply/axp288_charger.c
drivers/power/supply/bq24190_charger.c
drivers/power/supply/charger-manager.c
drivers/power/supply/qcom_smbb.c

-- Sebastian

> ---
>  drivers/power/supply/wm831x_power.c |   61 +++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
> index 7082301..3e3480708 100644
> --- a/drivers/power/supply/wm831x_power.c
> +++ b/drivers/power/supply/wm831x_power.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/slab.h>
> +#include <linux/usb/phy.h>
>  
>  #include <linux/mfd/wm831x/core.h>
>  #include <linux/mfd/wm831x/auxadc.h>
> @@ -31,6 +32,8 @@ struct wm831x_power {
>  	char usb_name[20];
>  	char battery_name[20];
>  	bool have_battery;
> +	struct usb_phy *usb_phy;
> +	struct notifier_block usb_notify;
>  };
>  
>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
> @@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  };
>  
> +/* In milliamps */
> +static const unsigned int wm831x_usb_limits[] = {
> +	0,
> +	2,
> +	100,
> +	500,
> +	900,
> +	1500,
> +	1800,
> +	550,
> +};
> +
> +static int wm831x_usb_limit_change(struct notifier_block *nb,
> +				   unsigned long limit, void *data)
> +{
> +	struct wm831x_power *wm831x_power = container_of(nb,
> +							 struct wm831x_power,
> +							 usb_notify);
> +	unsigned int i, best;
> +
> +	/* Find the highest supported limit */
> +	best = 0;
> +	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
> +		if (limit >= wm831x_usb_limits[i] &&
> +		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
> +			best = i;
> +	}
> +
> +	dev_dbg(wm831x_power->wm831x->dev,
> +		"Limiting USB current to %umA", wm831x_usb_limits[best]);
> +
> +	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
> +		        WM831X_USB_ILIM_MASK, best);
> +
> +	return 0;
> +}
> +
>  /*********************************************************************
>   *		Battery properties
>   *********************************************************************/
> @@ -607,6 +647,22 @@ static int wm831x_power_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> +						     "usb-phy", 0);
> +	if (IS_ERR(power->usb_phy)) {
> +		ret = PTR_ERR(power->usb_phy);
> +		dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
> +		goto err_bat_irq;
> +	}
> +
> +	power->usb_notify.notifier_call = wm831x_usb_limit_change;
> +	ret = usb_register_notifier(power->usb_phy,
> +				    &power->usb_notify);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
> +		goto err_bat_irq;
> +	}
> +
>  	return ret;
>  
>  err_bat_irq:
> @@ -637,6 +693,11 @@ static int wm831x_power_remove(struct platform_device *pdev)
>  	struct wm831x *wm831x = wm831x_power->wm831x;
>  	int irq, i;
>  
> +	if (wm831x_power->usb_phy) {
> +		usb_unregister_notifier(wm831x_power->usb_phy,
> +					&wm831x_power->usb_notify);
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
>  		irq = wm831x_irq(wm831x, 
>  				 platform_get_irq_byname(pdev,
> -- 
> 1.7.9.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Charles Keepax]

On Mon, Jul 03, 2017 at 02:07:15PM +0800, Baolin Wang wrote:
> Integrate with the newly added USB charger interface to limit the current
> we draw from the USB input based on the input device configuration
> identified by the USB stack, allowing us to charge more quickly from high
> current inputs without drawing more current than specified from others.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
<snip>
>  /*********************************************************************
>   *		Battery properties
>   *********************************************************************/
> @@ -607,6 +647,22 @@ static int wm831x_power_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> +						     "usb-phy", 0);
> +	if (IS_ERR(power->usb_phy)) {
> +		ret = PTR_ERR(power->usb_phy);
> +		dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
> +		goto err_bat_irq;
> +	}
> +

We should probably update the binding documentation for this as
well: mfd/wm831x.txt

Also I am not sure this needs to be implemented now, but what is
the plan regarding pdata systems? Generally the driver supports
both and it would be nice to know there is a way forward for that
even if we don't implement it yet.

Thanks,
Charles

[PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

Integrate with the newly added USB charger interface to limit the current
we draw from the USB input based on the input device configuration
identified by the USB stack, allowing us to charge more quickly from high
current inputs without drawing more current than specified from others.

Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/power/supply/wm831x_power.c |   61 +++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
index 7082301..3e3480708 100644
--- a/drivers/power/supply/wm831x_power.c
+++ b/drivers/power/supply/wm831x_power.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
+#include <linux/usb/phy.h>
 
 #include <linux/mfd/wm831x/core.h>
 #include <linux/mfd/wm831x/auxadc.h>
@@ -31,6 +32,8 @@ struct wm831x_power {
 	char usb_name[20];
 	char battery_name[20];
 	bool have_battery;
+	struct usb_phy *usb_phy;
+	struct notifier_block usb_notify;
 };
 
 static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
@@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 };
 
+/* In milliamps */
+static const unsigned int wm831x_usb_limits[] = {
+	0,
+	2,
+	100,
+	500,
+	900,
+	1500,
+	1800,
+	550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+				   unsigned long limit, void *data)
+{
+	struct wm831x_power *wm831x_power = container_of(nb,
+							 struct wm831x_power,
+							 usb_notify);
+	unsigned int i, best;
+
+	/* Find the highest supported limit */
+	best = 0;
+	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+		if (limit >= wm831x_usb_limits[i] &&
+		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
+			best = i;
+	}
+
+	dev_dbg(wm831x_power->wm831x->dev,
+		"Limiting USB current to %umA", wm831x_usb_limits[best]);
+
+	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+		        WM831X_USB_ILIM_MASK, best);
+
+	return 0;
+}
+
 /*********************************************************************
  *		Battery properties
  *********************************************************************/
@@ -607,6 +647,22 @@ static int wm831x_power_probe(struct platform_device *pdev)
 		}
 	}
 
+	power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
+						     "usb-phy", 0);
+	if (IS_ERR(power->usb_phy)) {
+		ret = PTR_ERR(power->usb_phy);
+		dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
+		goto err_bat_irq;
+	}
+
+	power->usb_notify.notifier_call = wm831x_usb_limit_change;
+	ret = usb_register_notifier(power->usb_phy,
+				    &power->usb_notify);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
+		goto err_bat_irq;
+	}
+
 	return ret;
 
 err_bat_irq:
@@ -637,6 +693,11 @@ static int wm831x_power_remove(struct platform_device *pdev)
 	struct wm831x *wm831x = wm831x_power->wm831x;
 	int irq, i;
 
+	if (wm831x_power->usb_phy) {
+		usb_unregister_notifier(wm831x_power->usb_phy,
+					&wm831x_power->usb_notify);
+	}
+
 	for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
 		irq = wm831x_irq(wm831x, 
 				 platform_get_irq_byname(pdev,
-- 
1.7.9.5