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

[PATCH v12 0/4] 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. Thus provide a standard framework for doing this in 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 v11:
 - Reviewed and tested by Li Jun.

Changes since v10:
 - Introduce usb_charger_get_state() function to check charger state.
 - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
 in case will be issued in atomic context.

Baolin Wang (4):
  gadget: Introduce the usb charger framework
  gadget: Support for the usb charger framework
  gadget: Integrate with the usb gadget supporting for usb charger
  power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c      |   69 ++++
 drivers/usb/gadget/Kconfig        |    7 +
 drivers/usb/gadget/udc/Makefile   |    1 +
 drivers/usb/gadget/udc/charger.c  |  807 +++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/udc-core.c |   11 +
 include/linux/mfd/wm831x/pdata.h  |    3 +
 include/linux/usb/charger.h       |  191 +++++++++
 include/linux/usb/gadget.h        |   11 +
 include/uapi/linux/usb/charger.h  |   31 ++
 9 files changed, 1131 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

-- 
1.7.9.5

[PATCH v12 1/4] 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. It also supplies the
notification mechanism to userspace When the usb charger state is changed.

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.

This patch doesn't yet integrate with the gadget code, so some functions
which rely on the 'gadget' are not completed, that will be implemented
in the following patches.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Reviewed-by: Li Jun <jun.li@nxp.com>
Tested-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/gadget/Kconfig       |    7 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  191 ++++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 1000 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 2057add..89f4e9b 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
 	help
 	   It supports the serial gadget can be used as a console.
 
+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/udc/Makefile b/drivers/usb/gadget/udc/Makefile
index dfee534..0e9564c 100644
--- a/drivers/usb/gadget/udc/Makefile
+++ b/drivers/usb/gadget/udc/Makefile
@@ -2,6 +2,7 @@
 # USB peripheral controller drivers
 #
 obj-$(CONFIG_USB_GADGET)	+= udc-core.o
+obj-$(CONFIG_USB_CHARGER)	+= charger.o
 obj-$(CONFIG_USB_DUMMY_HCD)	+= dummy_hcd.o
 obj-$(CONFIG_USB_NET2272)	+= net2272.o
 obj-$(CONFIG_USB_NET2280)	+= net2280.o
diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
new file mode 100644
index 0000000..7be4c76
--- /dev/null
+++ b/drivers/usb/gadget/udc/charger.c
@@ -0,0 +1,770 @@
+/*
+ * charger.c -- USB charger driver
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * 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.
+ */
+
+#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/slab.h>
+#include <linux/usb.h>
+#include <linux/usb/ch9.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/charger.h>
+#include <linux/power_supply.h>
+
+#define DEFAULT_SDP_CUR_LIMIT		500
+#define DEFAULT_SDP_CUR_LIMIT_SS	900
+#define DEFAULT_DCP_CUR_LIMIT		1500
+#define DEFAULT_CDP_CUR_LIMIT		1500
+#define DEFAULT_ACA_CUR_LIMIT		1500
+
+static DEFINE_IDA(usb_charger_ida);
+struct class *usb_charger_class;
+static unsigned int usb_charger_get_cur_limit(struct usb_charger *uchger);
+
+static struct usb_charger *dev_to_uchger(struct device *dev)
+{
+	return container_of(dev, struct usb_charger, dev);
+}
+
+/*
+ * charger_current_show() - Show the charger current limit.
+ */
+static ssize_t charger_current_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return sprintf(buf, "%u\n", usb_charger_get_cur_limit(uchger));
+}
+static DEVICE_ATTR_RO(charger_current);
+
+/*
+ * charger_type_show() - Show the charger type.
+ *
+ * It can be SDP/DCP/CDP/ACA type, else for unknown type.
+ */
+static ssize_t charger_type_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	int cnt;
+
+	switch (uchger->type) {
+	case SDP_TYPE:
+		cnt = sprintf(buf, "%s\n", "SDP");
+		break;
+	case DCP_TYPE:
+		cnt = sprintf(buf, "%s\n", "DCP");
+		break;
+	case CDP_TYPE:
+		cnt = sprintf(buf, "%s\n", "CDP");
+		break;
+	case ACA_TYPE:
+		cnt = sprintf(buf, "%s\n", "ACA");
+		break;
+	default:
+		cnt = sprintf(buf, "%s\n", "UNKNOWN");
+		break;
+	}
+
+	return cnt;
+}
+static DEVICE_ATTR_RO(charger_type);
+
+/*
+ * charger_state_show() - Show the charger state.
+ *
+ * Charger state can be present or removed.
+ */
+static ssize_t charger_state_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	int cnt;
+
+	switch (uchger->state) {
+	case USB_CHARGER_PRESENT:
+		cnt = sprintf(buf, "%s\n", "PRESENT");
+		break;
+	case USB_CHARGER_REMOVE:
+		cnt = sprintf(buf, "%s\n", "REMOVE");
+		break;
+	default:
+		cnt = sprintf(buf, "%s\n", "UNKNOWN");
+		break;
+	}
+
+	return cnt;
+}
+static DEVICE_ATTR_RO(charger_state);
+
+static struct attribute *usb_charger_attrs[] = {
+	&dev_attr_charger_current.attr,
+	&dev_attr_charger_type.attr,
+	&dev_attr_charger_state.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(usb_charger);
+
+/*
+ * 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 class_dev_iter iter;
+	struct device *dev;
+
+	if (WARN(!name, "can't work with NULL name"))
+		return ERR_PTR(-EINVAL);
+
+	/* Iterate all usb charger devices in the class */
+	class_dev_iter_init(&iter, usb_charger_class, NULL, NULL);
+	while ((dev = class_dev_iter_next(&iter))) {
+		if (!strcmp(dev_name(dev), name))
+			break;
+	}
+	class_dev_iter_exit(&iter);
+
+	if (WARN(!dev, "can't find usb charger device"))
+		return ERR_PTR(-ENODEV);
+
+	return dev_to_uchger(dev);
+}
+EXPORT_SYMBOL_GPL(usb_charger_find_by_name);
+
+/*
+ * 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;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get);
+
+/*
+ * 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);
+}
+EXPORT_SYMBOL_GPL(usb_charger_put);
+
+/*
+ * usb_charger_get_type() - get the usb charger type with lock protection.
+ * @uchger - usb charger device
+ *
+ * Users can get the charger type by this safe API, rather than using the
+ * usb_charger structure directly.
+ */
+enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger)
+{
+	enum usb_charger_type type;
+
+	mutex_lock(&uchger->lock);
+	type = uchger->type;
+	mutex_unlock(&uchger->lock);
+
+	return type;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get_type);
+
+/*
+ * usb_charger_get_state() - Get the charger state with lock protection.
+ * @uchger - the usb charger device.
+ *
+ * Users should get the charger state by this safe API.
+ */
+enum usb_charger_state usb_charger_get_state(struct usb_charger *uchger)
+{
+	enum usb_charger_state state;
+
+	mutex_lock(&uchger->lock);
+	state = uchger->state;
+	mutex_unlock(&uchger->lock);
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get_state);
+
+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid affecting gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger)
+{
+	enum usb_charger_type type;
+
+	if (WARN(!uchger->charger_detect,
+		 "charger detection method should not be NULL"))
+		return -EINVAL;
+
+	type = uchger->charger_detect(uchger);
+
+	mutex_lock(&uchger->lock);
+	uchger->type = type;
+	mutex_unlock(&uchger->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);
+
+/*
+ * usb_charger_get_type_by_others() - Get the usb charger type by the callback
+ * which is implemented by users.
+ * @uchger - the usb charger device.
+ *
+ * Note: This function is just used for getting the charger type, not for
+ * detecting charger type which might affect the DP/DM line when gadget is on
+ * enumeration state.
+ */
+static enum usb_charger_type
+usb_charger_get_type_by_others(struct usb_charger *uchger)
+{
+	if (uchger->type != UNKNOWN_TYPE)
+		return uchger->type;
+
+	if (uchger->psy) {
+		union power_supply_propval val;
+
+		power_supply_get_property(uchger->psy,
+					  POWER_SUPPLY_PROP_CHARGE_TYPE,
+					  &val);
+		switch (val.intval) {
+		case POWER_SUPPLY_TYPE_USB:
+			uchger->type = SDP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_DCP:
+			uchger->type = DCP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_CDP:
+			uchger->type = CDP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_ACA:
+			uchger->type = ACA_TYPE;
+			break;
+		default:
+			uchger->type = UNKNOWN_TYPE;
+			break;
+		}
+	} else if (uchger->get_charger_type) {
+		uchger->type = uchger->get_charger_type(uchger);
+	} else {
+		uchger->type = UNKNOWN_TYPE;
+	}
+
+	return uchger->type;
+}
+
+/*
+ * usb_charger_set_cur_limit_by_type() - Set the current limitation
+ * by charger type.
+ * @uchger - the usb charger device.
+ * @type - the usb charger type.
+ * @cur_limit - the current limitation.
+ */
+int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+				      enum usb_charger_type type,
+				      unsigned int cur_limit)
+{
+	if (WARN(!uchger, "charger can not be NULL"))
+		return -EINVAL;
+
+	switch (type) {
+	case SDP_TYPE:
+		uchger->cur_limit.sdp_cur_limit = cur_limit;
+		uchger->sdp_default_cur_change = 1;
+		break;
+	case DCP_TYPE:
+		uchger->cur_limit.dcp_cur_limit = cur_limit;
+		break;
+	case CDP_TYPE:
+		uchger->cur_limit.cdp_cur_limit	= cur_limit;
+		break;
+	case ACA_TYPE:
+		uchger->cur_limit.aca_cur_limit	= cur_limit;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_type);
+
+/*
+ * usb_charger_set_current_by_type_lock() - Set the current limitation
+ * by charger type with lock protection.
+ * @uchger - the usb charger device.
+ * @type - the usb charger type.
+ * @cur_limit - the current limitation.
+ *
+ * Users should set the current limitation by this lock protection API.
+ */
+int usb_charger_set_current_by_type_lock(struct usb_charger *uchger,
+					 enum usb_charger_type type,
+					 unsigned int cur_limit)
+{
+	int ret;
+
+	if (WARN(!uchger, "charger can not be NULL"))
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = usb_charger_set_cur_limit_by_type(uchger, type, cur_limit);
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_current_by_type_lock);
+
+/*
+ * 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 (WARN(!uchger || !cur_limit_set, "charger or setting can't be NULL"))
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	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;
+	uchger->sdp_default_cur_change = 1;
+	mutex_unlock(&uchger->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit);
+
+/*
+ * 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.
+ */
+static unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	enum usb_charger_type uchger_type =
+		usb_charger_get_type_by_others(uchger);
+	unsigned int cur_limit;
+
+	switch (uchger_type) {
+	case SDP_TYPE:
+		/*
+		 * For super speed gadget, the default charger current should
+		 * be 900 mA.
+		 */
+		if (!uchger->sdp_default_cur_change && uchger->gadget &&
+		    uchger->gadget->speed >= USB_SPEED_SUPER) {
+			uchger->cur_limit.sdp_cur_limit =
+				DEFAULT_SDP_CUR_LIMIT_SS;
+
+			uchger->sdp_default_cur_change = 1;
+		}
+
+		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_get_current() - Get the charger current with lock protection.
+ * @uchger - the usb charger device.
+ *
+ * Users should get the charger current by this safe API.
+ */
+unsigned int usb_charger_get_current(struct usb_charger *uchger)
+{
+	unsigned int cur;
+
+	mutex_lock(&uchger->lock);
+	cur = usb_charger_get_cur_limit(uchger);
+	mutex_unlock(&uchger->lock);
+
+	return cur;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get_current);
+
+/*
+ * 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 (WARN(!uchger || !nb, "charger or nb can not be NULL"))
+		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_get_type_by_others(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+					usb_charger_get_cur_limit(uchger),
+					uchger);
+	}
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_charger_register_notify);
+
+/*
+ * 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 (WARN(!uchger || !nb, "charger or nb can not be NULL"))
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = raw_notifier_chain_unregister(&uchger->uchger_nh, nb);
+	mutex_unlock(&uchger->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_charger_unregister_notify);
+
+/*
+ * 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)
+{
+	char uchger_state[50] = { 0 };
+	char *envp[] = { uchger_state, NULL };
+
+	mutex_lock(&uchger->lock);
+	uchger->state = state;
+
+	switch (state) {
+	case USB_CHARGER_PRESENT:
+		usb_charger_get_type_by_others(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+			usb_charger_get_cur_limit(uchger),
+			uchger);
+		snprintf(uchger_state, ARRAY_SIZE(uchger_state),
+			 "USB_CHARGER_STATE=%s", "USB_CHARGER_PRESENT");
+		break;
+	case USB_CHARGER_REMOVE:
+		uchger->type = UNKNOWN_TYPE;
+		raw_notifier_call_chain(&uchger->uchger_nh, 0, uchger);
+		snprintf(uchger_state, ARRAY_SIZE(uchger_state),
+			 "USB_CHARGER_STATE=%s", "USB_CHARGER_REMOVE");
+		break;
+	default:
+		dev_warn(&uchger->dev, "Unknown USB charger state: %d\n",
+			 state);
+		mutex_unlock(&uchger->lock);
+		return;
+	}
+
+	kobject_uevent_env(&uchger->dev.kobj, KOBJ_CHANGE, envp);
+	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 (WARN(!uchger, "charger can not be NULL"))
+		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.
+ * @gadget - the usb gadget device.
+ * @state - the usb gadget state.
+ */
+int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
+			       unsigned long state)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
+
+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);
+}
+
+/*
+ * 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 (WARN(!uchger, "charger can not be NULL"))
+		return -EINVAL;
+
+	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);
+}
+EXPORT_SYMBOL_GPL(devm_usb_charger_unregister);
+
+/*
+ * usb_charger_register() - Register a new usb charger device
+ * which is created by the usb charger framework.
+ * @parent - the parent device of the new usb charger.
+ * @uchger - the new usb charger device.
+ */
+static int usb_charger_register(struct device *parent,
+				struct usb_charger *uchger)
+{
+	int ret;
+
+	if (WARN(!uchger, "charger can not be NULL"))
+		return -EINVAL;
+
+	uchger->dev.parent = parent;
+	uchger->dev.release = usb_charger_release;
+	uchger->dev.class = usb_charger_class;
+	uchger->dev.groups = usb_charger_groups;
+
+	ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		goto fail_ida;
+
+	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;
+
+	return 0;
+
+fail_register:
+	put_device(&uchger->dev);
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+	uchger->id = -1;
+fail_ida:
+	dev_err(parent, "Failed to register usb charger: %d\n", ret);
+	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;
+}
+EXPORT_SYMBOL_GPL(devm_usb_charger_register);
+
+int usb_charger_init(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger;
+	struct extcon_dev *edev;
+	struct power_supply *psy;
+	int ret;
+
+	uchger = kzalloc(sizeof(struct usb_charger), GFP_KERNEL);
+	if (!uchger)
+		return -ENOMEM;
+
+	uchger->type = UNKNOWN_TYPE;
+	uchger->state = USB_CHARGER_DEFAULT;
+	uchger->id = -1;
+	uchger->sdp_default_cur_change = 0;
+
+	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);
+	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);
+	}
+
+	/* to check if the usb charger is link to a power supply */
+	psy = devm_power_supply_get_by_phandle(ugadget->dev.parent,
+					       "power-supplies");
+	if (!IS_ERR_OR_NULL(psy))
+		uchger->psy = psy;
+	else
+		uchger->psy = NULL;
+
+	/* register a notifier on a usb gadget device */
+	uchger->gadget = ugadget;
+	uchger->old_gadget_state = USB_STATE_NOTATTACHED;
+
+	/* 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);
+
+	kfree(uchger);
+	return ret;
+}
+
+int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+
+static int __init usb_charger_class_init(void)
+{
+	usb_charger_class = class_create(THIS_MODULE, "usb_charger");
+	if (IS_ERR(usb_charger_class)) {
+		pr_err("couldn't create class\n");
+		return PTR_ERR(usb_charger_class);
+	}
+
+	return 0;
+}
+
+static void __exit usb_charger_class_exit(void)
+{
+	class_destroy(usb_charger_class);
+}
+subsys_initcall(usb_charger_class_init);
+module_exit(usb_charger_class_exit);
+
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
+MODULE_DESCRIPTION("USB charger driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/usb/charger.h b/include/linux/usb/charger.h
new file mode 100644
index 0000000..7c3f069
--- /dev/null
+++ b/include/linux/usb/charger.h
@@ -0,0 +1,191 @@
+#ifndef __LINUX_USB_CHARGER_H__
+#define __LINUX_USB_CHARGER_H__
+
+#include <uapi/linux/usb/ch9.h>
+#include <uapi/linux/usb/charger.h>
+
+/* 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 {
+	struct device		dev;
+	struct raw_notifier_head	uchger_nh;
+	/* protect the notifier head and charger */
+	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;
+
+	/* for supporting power supply */
+	struct power_supply	*psy;
+
+	/* user can get charger type by implementing this callback */
+	enum usb_charger_type	(*get_charger_type)(struct usb_charger *);
+	/*
+	 * charger detection method can be implemented if you need to
+	 * manually detect the charger type.
+	 */
+	enum usb_charger_type	(*charger_detect)(struct usb_charger *);
+
+	/* current limitation */
+	struct usb_charger_cur_limit	cur_limit;
+	/* to check if it is needed to change the SDP charger default current */
+	unsigned int		sdp_default_cur_change;
+};
+
+#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 int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+					     enum usb_charger_type type,
+					     unsigned int cur_limit);
+extern int usb_charger_set_current_by_type_lock(struct usb_charger *uchger,
+						enum usb_charger_type type,
+						unsigned int cur_limit);
+extern unsigned int usb_charger_get_current(struct usb_charger *uchger);
+
+extern int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
+				      unsigned long state);
+extern enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger);
+extern int usb_charger_detect_type(struct usb_charger *uchger);
+extern enum usb_charger_state usb_charger_get_state(struct usb_charger *uchger);
+
+extern void devm_usb_charger_unregister(struct device *dev,
+					struct usb_charger *uchger);
+extern int devm_usb_charger_register(struct device *dev,
+				     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 int
+usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+				  enum usb_charger_type type,
+				  unsigned int cur_limit)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_current_by_type_lock(struct usb_charger *uchger,
+				     enum usb_charger_type type,
+				     unsigned int cur_limit)
+{
+	return 0;
+}
+
+static inline unsigned int
+usb_charger_get_current(struct usb_charger *uchger)
+{
+	return 0;
+}
+
+static inline enum usb_charger_type
+usb_charger_get_type(struct usb_charger *uchger)
+{
+	return UNKNOWN_TYPE;
+}
+
+static inline enum usb_charger_state
+usb_charger_get_state(struct usb_charger *uchger)
+{
+	return USB_CHARGER_REMOVE;
+}
+
+static inline int usb_charger_detect_type(struct usb_charger *uchger)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_plug_by_gadget(struct usb_gadget *gadget, unsigned long state)
+{
+	return 0;
+}
+
+static inline void devm_usb_charger_unregister(struct device *dev,
+					       struct usb_charger *uchger)
+{
+}
+
+static inline int devm_usb_charger_register(struct device *dev,
+					    struct usb_charger *uchger)
+{
+	return 0;
+}
+
+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__ */
diff --git a/include/uapi/linux/usb/charger.h b/include/uapi/linux/usb/charger.h
new file mode 100644
index 0000000..3c56ec4
--- /dev/null
+++ b/include/uapi/linux/usb/charger.h
@@ -0,0 +1,31 @@
+/*
+ * This file defines the USB charger type and state that are needed for
+ * USB device APIs.
+ */
+
+#ifndef _UAPI__LINUX_USB_CHARGER_H
+#define _UAPI__LINUX_USB_CHARGER_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,
+};
+
+#endif /* _UAPI__LINUX_USB_CHARGER_H */
-- 
1.7.9.5

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

[Baolin Wang]

For supporting the usb charger, it adds the usb_charger_init() and
usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed,
then the usb charger can do the power things.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Reviewed-by: Li Jun <jun.li@nxp.com>
Tested-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/gadget/udc/udc-core.c |   11 +++++++++++
 include/linux/usb/gadget.h        |   11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 6e8300d..84c098c 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/charger.h>
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -242,6 +243,9 @@ static void usb_gadget_state_work(struct work_struct *work)
 	struct usb_gadget *gadget = work_to_gadget(work);
 	struct usb_udc *udc = gadget->udc;
 
+	/* when the gadget state is changed, then report to USB charger */
+	usb_charger_plug_by_gadget(gadget, gadget->state);
+
 	if (udc)
 		sysfs_notify(&udc->dev.kobj, NULL, "state");
 }
@@ -411,6 +415,10 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 	if (ret)
 		goto err4;
 
+	ret = usb_charger_init(gadget);
+	if (ret)
+		goto err5;
+
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
@@ -431,6 +439,8 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	return 0;
 
+err5:
+	device_del(&udc->dev);
 err4:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
@@ -539,6 +549,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 457651b..40390ec 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -24,6 +24,7 @@
 #include <linux/types.h>
 #include <linux/workqueue.h>
 #include <linux/usb/ch9.h>
+#include <linux/usb/charger.h>
 
 struct usb_ep;
 
@@ -639,6 +640,8 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	/* negotiate the power with the usb charger */
+	struct usb_charger		*charger;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -855,10 +858,18 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget)
  * reporting how much power the device may consume.  For example, this
  * could affect how quickly batteries are recharged.
  *
+ * It will also notify the USB charger how much power the device may
+ * consume if there is a USB charger linking with the gadget.
+ *
  * Returns zero on success, else negative errno.
  */
 static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+	if (gadget->charger)
+		usb_charger_set_cur_limit_by_type(gadget->charger,
+						  gadget->charger->type,
+						  mA);
+
 	if (!gadget->ops->vbus_draw)
 		return -EOPNOTSUPP;
 	return gadget->ops->vbus_draw(gadget, mA);
-- 
1.7.9.5

[PATCH v12 3/4] gadget: Integrate with the usb gadget supporting for usb charger

[Baolin Wang]

When the usb gadget supporting for usb charger is ready, the usb charger
can implement the usb_charger_plug_by_gadget() function and usb_charger_exit()
function by getting 'struct usb_charger' from 'struct gadget'.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Reviewed-by: Li Jun <jun.li@nxp.com>
Tested-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/gadget/udc/charger.c |   39 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
index 7be4c76..949396f 100644
--- a/drivers/usb/gadget/udc/charger.c
+++ b/drivers/usb/gadget/udc/charger.c
@@ -568,6 +568,30 @@ usb_charger_plug_by_extcon(struct notifier_block *nb,
 int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
 			       unsigned long state)
 {
+	struct usb_charger *uchger = gadget->charger;
+	enum usb_charger_state uchger_state;
+
+	if (WARN(!uchger, "charger can not be NULL"))
+		return -EINVAL;
+
+	/*
+	 * 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 0;
 }
 EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
@@ -724,6 +748,7 @@ int usb_charger_init(struct usb_gadget *ugadget)
 
 	/* register a notifier on a usb gadget device */
 	uchger->gadget = ugadget;
+	ugadget->charger = uchger;
 	uchger->old_gadget_state = USB_STATE_NOTATTACHED;
 
 	/* register a new usb charger */
@@ -744,7 +769,19 @@ fail:
 
 int usb_charger_exit(struct usb_gadget *ugadget)
 {
-	return 0;
+	struct usb_charger *uchger = ugadget->charger;
+
+	if (WARN(!uchger, "charger can not be NULL"))
+		return -EINVAL;
+
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB,
+					   &uchger->extcon_nb.nb);
+
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+
+	return usb_charger_unregister(uchger);
 }
 
 static int __init usb_charger_class_init(void)
-- 
1.7.9.5

[PATCH v12 4/4] 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>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Acked-by: Peter Chen <peter.chen@freescale.com>
Acked-by: Sebastian Reichel <sre@kernel.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 7082301..cef1812 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/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 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,8 +647,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:
+	/* put_device on 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);
+		/* Free 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 v12 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> 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. It also supplies the
> notification mechanism to userspace When the usb charger state is changed.
>
> 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.
>
> This patch doesn't yet integrate with the gadget code, so some functions
> which rely on the 'gadget' are not completed, that will be implemented
> in the following patches.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> Reviewed-by: Li Jun <jun.li@nxp.com>
> Tested-by: Li Jun <jun.li@nxp.com>
> ---
>  drivers/usb/gadget/Kconfig       |    7 +
>  drivers/usb/gadget/udc/Makefile  |    1 +
>  drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/charger.h      |  191 ++++++++++
>  include/uapi/linux/usb/charger.h |   31 ++
>  5 files changed, 1000 insertions(+)
>  create mode 100644 drivers/usb/gadget/udc/charger.c
>  create mode 100644 include/linux/usb/charger.h
>  create mode 100644 include/uapi/linux/usb/charger.h
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 2057add..89f4e9b 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
>  	help
>  	   It supports the serial gadget can be used as a console.
>  
> +config USB_CHARGER
> +	bool "USB charger support"

you didn't build test all possibilities, did you?

I have a feeling this won't link if USB_GADGET=m. Can you test that?

> diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
[...]
> +struct class *usb_charger_class;

We already have a UDC class, do we really, really need another class
here?

> +subsys_initcall(usb_charger_class_init);

this should always work as module_init(). Please make sure that's the
case.

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> For supporting the usb charger, it adds the usb_charger_init() and
> usb_charger_exit() functions for usb charger initialization and exit.
>
> It will report to the usb charger when the gadget state is changed,
> then the usb charger can do the power things.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> Reviewed-by: Li Jun <jun.li@nxp.com>
> Tested-by: Li Jun <jun.li@nxp.com>

Before anything, I must say that I really liked this patch. It's
minimaly invasive to udc core and does all the necessary changes. If it
wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class
completely?

>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>  {
> +	if (gadget->charger)

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

-- 
balbi

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

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> @@ -607,8 +647,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);

the fact that you rely on strings and pass them via pdata is an
indication that you don't have enough description of the HW. Seems like
we need to come up with a set of DT properties which tie a charger to a
UDC.

I'm thinking a phandle would be enough?

-- 
balbi

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

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

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

On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
> Baolin Wang <baolin.wang@linaro.org> writes:
> > @@ -607,8 +647,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);

> the fact that you rely on strings and pass them via pdata is an
> indication that you don't have enough description of the HW. Seems like
> we need to come up with a set of DT properties which tie a charger to a
> UDC.

> I'm thinking a phandle would be enough?

The wm831x has no DT support currently.

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

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

[Baolin Wang]

On 21 June 2016 at 18:25, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> 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. It also supplies the
>> notification mechanism to userspace When the usb charger state is changed.
>>
>> 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.
>>
>> This patch doesn't yet integrate with the gadget code, so some functions
>> which rely on the 'gadget' are not completed, that will be implemented
>> in the following patches.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> Reviewed-by: Li Jun <jun.li@nxp.com>
>> Tested-by: Li Jun <jun.li@nxp.com>
>> ---
>>  drivers/usb/gadget/Kconfig       |    7 +
>>  drivers/usb/gadget/udc/Makefile  |    1 +
>>  drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/charger.h      |  191 ++++++++++
>>  include/uapi/linux/usb/charger.h |   31 ++
>>  5 files changed, 1000 insertions(+)
>>  create mode 100644 drivers/usb/gadget/udc/charger.c
>>  create mode 100644 include/linux/usb/charger.h
>>  create mode 100644 include/uapi/linux/usb/charger.h
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 2057add..89f4e9b 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
>>       help
>>          It supports the serial gadget can be used as a console.
>>
>> +config USB_CHARGER
>> +     bool "USB charger support"
>
> you didn't build test all possibilities, did you?
>
> I have a feeling this won't link if USB_GADGET=m. Can you test that?

OK.

>
>> diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
> [...]
>> +struct class *usb_charger_class;
>
> We already have a UDC class, do we really, really need another class
> here?

We want to manage the usb charger devices by this class and one usb
charger device is not equal with one usb device which managed by UDC
class, so can we use UDC class to manage charger devices?
By the way, you also suggested to use the 'class' things instead of
'bus' in previous mail.

>
>> +subsys_initcall(usb_charger_class_init);
>
> this should always work as module_init(). Please make sure that's the
> case.

we should make sure the charger class has been allocated before user
try to add a new gadget to the udc class. So it should be issued
before 'module_init()' (many usb drivers are at module_init level).
Another hand this follows the UDC class allocation.

>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 21 June 2016 at 18:27, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> For supporting the usb charger, it adds the usb_charger_init() and
>> usb_charger_exit() functions for usb charger initialization and exit.
>>
>> It will report to the usb charger when the gadget state is changed,
>> then the usb charger can do the power things.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> Reviewed-by: Li Jun <jun.li@nxp.com>
>> Tested-by: Li Jun <jun.li@nxp.com>
>
> Before anything, I must say that I really liked this patch. It's

Thanks.

> minimaly invasive to udc core and does all the necessary changes. If it
> wasn't for the extra charger class, this would've been perfect.
>
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?

Yeah, I also hope so. But we really want something to manage the
charger devices, do you have any good suggestion to avoid the 'class'
but also can manage the charger devices?

>
>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>  {
>> +     if (gadget->charger)
>
> I guess you could do this check inside
> usb_gadget_set_cur_limit_by_type() itself.

OK.

>
> --
> balbi



-- 
Baolin.wang
Best Regards

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

On 21 June 2016 at 19:03, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>> > @@ -607,8 +647,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);
>
>> the fact that you rely on strings and pass them via pdata is an
>> indication that you don't have enough description of the HW. Seems like
>> we need to come up with a set of DT properties which tie a charger to a
>> UDC.
>
>> I'm thinking a phandle would be enough?
>
> The wm831x has no DT support currently.

Another hand I suppose the usb charger is one virtual device and does
not need be described from DT.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>> Can't you just tie a charger to a UDC and avoid the charger class
>> completely?
>
> Yeah, I also hope so. But we really want something to manage the
> charger devices, do you have any good suggestion to avoid the 'class'
> but also can manage the charger devices?

manage in what way? It seems to me that they don't need to be real
devices, just a handle as part of struct usb_gadget, no?

-- 
balbi

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

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

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


Hi,

Mark Brown <broonie@kernel.org> writes:
> [ Unknown signature status ]
> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>> > @@ -607,8 +647,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);
>
>> the fact that you rely on strings and pass them via pdata is an
>> indication that you don't have enough description of the HW. Seems like
>> we need to come up with a set of DT properties which tie a charger to a
>> UDC.
>
>> I'm thinking a phandle would be enough?
>
> The wm831x has no DT support currently.

okay, perhaps its time to add it.

-- 
balbi

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

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> On 21 June 2016 at 19:03, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> > @@ -607,8 +647,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);
>>
>>> the fact that you rely on strings and pass them via pdata is an
>>> indication that you don't have enough description of the HW. Seems like
>>> we need to come up with a set of DT properties which tie a charger to a
>>> UDC.
>>
>>> I'm thinking a phandle would be enough?
>>
>> The wm831x has no DT support currently.
>
> Another hand I suppose the usb charger is one virtual device and does
> not need be described from DT.

Right, I don't think that should be a device at all. But you can pass a
phandle to the UDC controller and use that to get to struct usb_gadget
from which you could reach ->charger.

-- 
balbi

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

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

On 21 June 2016 at 19:53, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> On 21 June 2016 at 19:03, Mark Brown <broonie@kernel.org> wrote:
>>> On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
>>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> > @@ -607,8 +647,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);
>>>
>>>> the fact that you rely on strings and pass them via pdata is an
>>>> indication that you don't have enough description of the HW. Seems like
>>>> we need to come up with a set of DT properties which tie a charger to a
>>>> UDC.
>>>
>>>> I'm thinking a phandle would be enough?
>>>
>>> The wm831x has no DT support currently.
>>
>> Another hand I suppose the usb charger is one virtual device and does
>> not need be described from DT.
>
> Right, I don't think that should be a device at all. But you can pass a
> phandle to the UDC controller and use that to get to struct usb_gadget
> from which you could reach ->charger.

Ah, make sense.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 21 June 2016 at 19:49, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Can't you just tie a charger to a UDC and avoid the charger class
>>> completely?
>>
>> Yeah, I also hope so. But we really want something to manage the
>> charger devices, do you have any good suggestion to avoid the 'class'
>> but also can manage the charger devices?
>
> manage in what way? It seems to me that they don't need to be real
> devices, just a handle as part of struct usb_gadget, no?

Although charger device is not one real hardware device, we also use
one 'struct device' to describe it in charger.c file. So we should
manage the 'struct device' with one proper way.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>> completely?
>>>
>>> Yeah, I also hope so. But we really want something to manage the
>>> charger devices, do you have any good suggestion to avoid the 'class'
>>> but also can manage the charger devices?
>>
>> manage in what way? It seems to me that they don't need to be real
>> devices, just a handle as part of struct usb_gadget, no?
>
> Although charger device is not one real hardware device, we also use
> one 'struct device' to describe it in charger.c file. So we should
> manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

-- 
balbi

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

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

[Baolin Wang]

On 21 June 2016 at 20:27, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>> completely?
>>>>
>>>> Yeah, I also hope so. But we really want something to manage the
>>>> charger devices, do you have any good suggestion to avoid the 'class'
>>>> but also can manage the charger devices?
>>>
>>> manage in what way? It seems to me that they don't need to be real
>>> devices, just a handle as part of struct usb_gadget, no?
>>
>> Although charger device is not one real hardware device, we also use
>> one 'struct device' to describe it in charger.c file. So we should
>> manage the 'struct device' with one proper way.
>
> that's fine, but why do you think they need a struct device to start with?

We can get/put usb charger and mange usb charger attributes with the
device model if we use a struct device.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>> completely?
>>>>>
>>>>> Yeah, I also hope so. But we really want something to manage the
>>>>> charger devices, do you have any good suggestion to avoid the 'class'
>>>>> but also can manage the charger devices?
>>>>
>>>> manage in what way? It seems to me that they don't need to be real
>>>> devices, just a handle as part of struct usb_gadget, no?
>>>
>>> Although charger device is not one real hardware device, we also use
>>> one 'struct device' to describe it in charger.c file. So we should
>>> manage the 'struct device' with one proper way.
>>
>> that's fine, but why do you think they need a struct device to start with?
>
> We can get/put usb charger and mange usb charger attributes with the
> device model if we use a struct device.

We already have that as part of struct usb_udc. Why don't you just
create a subdirectory called charger which will hold all your
charger-related attributes. That directory will only be created if a
valid ->charger pointer exists.

USB Charging is always tied to a peripheral side controller, anyway.

-- 
balbi

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

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

[Baolin Wang]

On 21 June 2016 at 20:36, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>> completely?
>>>>>>
>>>>>> Yeah, I also hope so. But we really want something to manage the
>>>>>> charger devices, do you have any good suggestion to avoid the 'class'
>>>>>> but also can manage the charger devices?
>>>>>
>>>>> manage in what way? It seems to me that they don't need to be real
>>>>> devices, just a handle as part of struct usb_gadget, no?
>>>>
>>>> Although charger device is not one real hardware device, we also use
>>>> one 'struct device' to describe it in charger.c file. So we should
>>>> manage the 'struct device' with one proper way.
>>>
>>> that's fine, but why do you think they need a struct device to start with?
>>
>> We can get/put usb charger and mange usb charger attributes with the
>> device model if we use a struct device.
>
> We already have that as part of struct usb_udc. Why don't you just
> create a subdirectory called charger which will hold all your
> charger-related attributes. That directory will only be created if a
> valid ->charger pointer exists.

That means we can remove all the device and class things in charger.c
file, right? OK, I try to do that. Thanks.

>
> USB Charging is always tied to a peripheral side controller, anyway.
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>>> completely?
>>>>>>>
>>>>>>> Yeah, I also hope so. But we really want something to manage the
>>>>>>> charger devices, do you have any good suggestion to avoid the 'class'
>>>>>>> but also can manage the charger devices?
>>>>>>
>>>>>> manage in what way? It seems to me that they don't need to be real
>>>>>> devices, just a handle as part of struct usb_gadget, no?
>>>>>
>>>>> Although charger device is not one real hardware device, we also use
>>>>> one 'struct device' to describe it in charger.c file. So we should
>>>>> manage the 'struct device' with one proper way.
>>>>
>>>> that's fine, but why do you think they need a struct device to start with?
>>>
>>> We can get/put usb charger and mange usb charger attributes with the
>>> device model if we use a struct device.
>>
>> We already have that as part of struct usb_udc. Why don't you just
>> create a subdirectory called charger which will hold all your
>> charger-related attributes. That directory will only be created if a
>> valid ->charger pointer exists.
>
> That means we can remove all the device and class things in charger.c
> file, right? OK, I try to do that. Thanks.

right. Keep your charger.c file, because to conditionally compile and
link that to udc-core.ko, but remove all the class initialization and
all of that extra code.

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Felipe Balbi <balbi@kernel.org> writes:
>>>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>>>> completely?
>>>>>>>>
>>>>>>>> Yeah, I also hope so. But we really want something to manage the
>>>>>>>> charger devices, do you have any good suggestion to avoid the 'class'
>>>>>>>> but also can manage the charger devices?
>>>>>>>
>>>>>>> manage in what way? It seems to me that they don't need to be real
>>>>>>> devices, just a handle as part of struct usb_gadget, no?
>>>>>>
>>>>>> Although charger device is not one real hardware device, we also use
>>>>>> one 'struct device' to describe it in charger.c file. So we should
>>>>>> manage the 'struct device' with one proper way.
>>>>>
>>>>> that's fine, but why do you think they need a struct device to start with?
>>>>
>>>> We can get/put usb charger and mange usb charger attributes with the
>>>> device model if we use a struct device.
>>>
>>> We already have that as part of struct usb_udc. Why don't you just
>>> create a subdirectory called charger which will hold all your
>>> charger-related attributes. That directory will only be created if a
>>> valid ->charger pointer exists.
>>
>> That means we can remove all the device and class things in charger.c
>> file, right? OK, I try to do that. Thanks.
>
> right. Keep your charger.c file, because to conditionally compile and
                                          ^
                                       we want   

> link that to udc-core.ko, but remove all the class initialization and
> all of that extra code.
>
> -- 
> balbi

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>> 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. It also supplies the
>>> notification mechanism to userspace When the usb charger state is changed.
>>>
>>> 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.
>>>
>>> This patch doesn't yet integrate with the gadget code, so some functions
>>> which rely on the 'gadget' are not completed, that will be implemented
>>> in the following patches.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>> Tested-by: Li Jun <jun.li@nxp.com>
>>> ---
>>>  drivers/usb/gadget/Kconfig       |    7 +
>>>  drivers/usb/gadget/udc/Makefile  |    1 +
>>>  drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
>>>  include/linux/usb/charger.h      |  191 ++++++++++
>>>  include/uapi/linux/usb/charger.h |   31 ++
>>>  5 files changed, 1000 insertions(+)
>>>  create mode 100644 drivers/usb/gadget/udc/charger.c
>>>  create mode 100644 include/linux/usb/charger.h
>>>  create mode 100644 include/uapi/linux/usb/charger.h
>>>
>>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>>> index 2057add..89f4e9b 100644
>>> --- a/drivers/usb/gadget/Kconfig
>>> +++ b/drivers/usb/gadget/Kconfig
>>> @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
>>>       help
>>>          It supports the serial gadget can be used as a console.
>>>
>>> +config USB_CHARGER
>>> +     bool "USB charger support"
>>
>> you didn't build test all possibilities, did you?
>>
>> I have a feeling this won't link if USB_GADGET=m. Can you test that?
>
> OK.
>
>>
>>> diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
>> [...]
>>> +struct class *usb_charger_class;
>>
>> We already have a UDC class, do we really, really need another class
>> here?
>
> We want to manage the usb charger devices by this class and one usb
> charger device is not equal with one usb device which managed by UDC

Can you explain this statement? If charging is done via a USB peripheral
port, why don't we have a 1:1 mapping between charger and UDC?

> class, so can we use UDC class to manage charger devices?
> By the way, you also suggested to use the 'class' things instead of
> 'bus' in previous mail.

true, I did say that. It seems clearer, however, that we don't need this
virtual device here.

>>> +subsys_initcall(usb_charger_class_init);
>>
>> this should always work as module_init(). Please make sure that's the
>> case.
>
> we should make sure the charger class has been allocated before user
> try to add a new gadget to the udc class. So it should be issued
> before 'module_init()' (many usb drivers are at module_init level).

-EPROBE_DEFER?

-- 
balbi

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

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

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

On Tue, Jun 21, 2016 at 02:50:27PM +0300, Felipe Balbi wrote:
> Mark Brown <broonie@kernel.org> writes:

> > The wm831x has no DT support currently.

> okay, perhaps its time to add it.

The only platform using it would need the DT connector overlays
completing in order to be able to convert to DT.  I'm really not
convinced it's worth the effort either.

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

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

[Baolin Wang]

On 21 June 2016 at 20:53, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>>>> completely?
>>>>>>>>
>>>>>>>> Yeah, I also hope so. But we really want something to manage the
>>>>>>>> charger devices, do you have any good suggestion to avoid the 'class'
>>>>>>>> but also can manage the charger devices?
>>>>>>>
>>>>>>> manage in what way? It seems to me that they don't need to be real
>>>>>>> devices, just a handle as part of struct usb_gadget, no?
>>>>>>
>>>>>> Although charger device is not one real hardware device, we also use
>>>>>> one 'struct device' to describe it in charger.c file. So we should
>>>>>> manage the 'struct device' with one proper way.
>>>>>
>>>>> that's fine, but why do you think they need a struct device to start with?
>>>>
>>>> We can get/put usb charger and mange usb charger attributes with the
>>>> device model if we use a struct device.
>>>
>>> We already have that as part of struct usb_udc. Why don't you just
>>> create a subdirectory called charger which will hold all your
>>> charger-related attributes. That directory will only be created if a
>>> valid ->charger pointer exists.
>>
>> That means we can remove all the device and class things in charger.c
>> file, right? OK, I try to do that. Thanks.
>
> right. Keep your charger.c file, because to conditionally compile and
> link that to udc-core.ko, but remove all the class initialization and
> all of that extra code.

Make sense.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 21 June 2016 at 22:50, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> 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. It also supplies the
>>>> notification mechanism to userspace When the usb charger state is changed.
>>>>
>>>> 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.
>>>>
>>>> This patch doesn't yet integrate with the gadget code, so some functions
>>>> which rely on the 'gadget' are not completed, that will be implemented
>>>> in the following patches.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>> ---
>>>>  drivers/usb/gadget/Kconfig       |    7 +
>>>>  drivers/usb/gadget/udc/Makefile  |    1 +
>>>>  drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/usb/charger.h      |  191 ++++++++++
>>>>  include/uapi/linux/usb/charger.h |   31 ++
>>>>  5 files changed, 1000 insertions(+)
>>>>  create mode 100644 drivers/usb/gadget/udc/charger.c
>>>>  create mode 100644 include/linux/usb/charger.h
>>>>  create mode 100644 include/uapi/linux/usb/charger.h
>>>>
>>>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>>>> index 2057add..89f4e9b 100644
>>>> --- a/drivers/usb/gadget/Kconfig
>>>> +++ b/drivers/usb/gadget/Kconfig
>>>> @@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
>>>>       help
>>>>          It supports the serial gadget can be used as a console.
>>>>
>>>> +config USB_CHARGER
>>>> +     bool "USB charger support"
>>>
>>> you didn't build test all possibilities, did you?
>>>
>>> I have a feeling this won't link if USB_GADGET=m. Can you test that?
>>
>> OK.
>>
>>>
>>>> diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
>>> [...]
>>>> +struct class *usb_charger_class;
>>>
>>> We already have a UDC class, do we really, really need another class
>>> here?
>>
>> We want to manage the usb charger devices by this class and one usb
>> charger device is not equal with one usb device which managed by UDC
>
> Can you explain this statement? If charging is done via a USB peripheral
> port, why don't we have a 1:1 mapping between charger and UDC?

Sorry for confusing, but forget it, I plan to remove the device/class
things as you suggested.

>
>> class, so can we use UDC class to manage charger devices?
>> By the way, you also suggested to use the 'class' things instead of
>> 'bus' in previous mail.
>
> true, I did say that. It seems clearer, however, that we don't need this
> virtual device here.

OK.

>
>>>> +subsys_initcall(usb_charger_class_init);
>>>
>>> this should always work as module_init(). Please make sure that's the
>>> case.
>>
>> we should make sure the charger class has been allocated before user
>> try to add a new gadget to the udc class. So it should be issued
>> before 'module_init()' (many usb drivers are at module_init level).
>
> -EPROBE_DEFER?

OK. But 'EPROBE_DEFER' is not needed if we remove the class things.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi,

On 21 June 2016 at 18:27, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> For supporting the usb charger, it adds the usb_charger_init() and
>> usb_charger_exit() functions for usb charger initialization and exit.
>>
>> It will report to the usb charger when the gadget state is changed,
>> then the usb charger can do the power things.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> Reviewed-by: Li Jun <jun.li@nxp.com>
>> Tested-by: Li Jun <jun.li@nxp.com>
>
> Before anything, I must say that I really liked this patch. It's
> minimaly invasive to udc core and does all the necessary changes. If it
> wasn't for the extra charger class, this would've been perfect.
>
> Can't you just tie a charger to a UDC and avoid the charger class
> completely?
>
>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>  {
>> +     if (gadget->charger)
>
> I guess you could do this check inside
> usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing
usb_gadget_set_cur_limit_by_type(), so I think I should leave the
check here in next new version.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> For supporting the usb charger, it adds the usb_charger_init() and
>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>
>>> It will report to the usb charger when the gadget state is changed,
>>> then the usb charger can do the power things.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>> Tested-by: Li Jun <jun.li@nxp.com>
>>
>> Before anything, I must say that I really liked this patch. It's
>> minimaly invasive to udc core and does all the necessary changes. If it
>> wasn't for the extra charger class, this would've been perfect.
>>
>> Can't you just tie a charger to a UDC and avoid the charger class
>> completely?
>>
>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>  {
>>> +     if (gadget->charger)
>>
>> I guess you could do this check inside
>> usb_gadget_set_cur_limit_by_type() itself.
>
> We will access the 'gadget->charger->type' member when issuing
> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
> check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
{
	struct usb_charger *charger;
        enum usb_charger_type type;

	if (!gadget->charger)
        	return 0;

        charger = gadget->charger;
        type = charger->type;

	return __usb_charger_set_cur_limit(charger, type, mA);
}

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
{
	usb_charger_set_cur_limit(gadget,  mA);

 	if (!gadget->ops->vbus_draw)
 		return -EOPNOTSUPP;
 	return gadget->ops->vbus_draw(gadget, mA);
}

-- 
balbi

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

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

[Baolin Wang]

Hi Felipe,

On 29 June 2016 at 16:20, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>
>>>> It will report to the usb charger when the gadget state is changed,
>>>> then the usb charger can do the power things.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>
>>> Before anything, I must say that I really liked this patch. It's
>>> minimaly invasive to udc core and does all the necessary changes. If it
>>> wasn't for the extra charger class, this would've been perfect.
>>>
>>> Can't you just tie a charger to a UDC and avoid the charger class
>>> completely?
>>>
>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>  {
>>>> +     if (gadget->charger)
>>>
>>> I guess you could do this check inside
>>> usb_gadget_set_cur_limit_by_type() itself.
>>
>> We will access the 'gadget->charger->type' member when issuing
>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>> check here in next new version.
>
> Here's what I mean:
>
> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
> {
>         struct usb_charger *charger;
>         enum usb_charger_type type;
>
>         if (!gadget->charger)
>                 return 0;
>
>         charger = gadget->charger;
>         type = charger->type;
>
>         return __usb_charger_set_cur_limit(charger, type, mA);
> }

But that means we need to export  both 'usb_charger_set_cur_limit()'
function and '__usb_charger_set_cur_limit()' function in charger.c
file. Cause some user may want to set the current limitation by one
charger type parameter (may be not from charger->type), like by
issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
you think about this situation? Thanks.

>
> static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
> {
>         usb_charger_set_cur_limit(gadget,  mA);
>
>         if (!gadget->ops->vbus_draw)
>                 return -EOPNOTSUPP;
>         return gadget->ops->vbus_draw(gadget, mA);
> }
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>
>>>>> It will report to the usb charger when the gadget state is changed,
>>>>> then the usb charger can do the power things.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>
>>>> Before anything, I must say that I really liked this patch. It's
>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>> wasn't for the extra charger class, this would've been perfect.
>>>>
>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>> completely?
>>>>
>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>  {
>>>>> +     if (gadget->charger)
>>>>
>>>> I guess you could do this check inside
>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>
>>> We will access the 'gadget->charger->type' member when issuing
>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>> check here in next new version.
>>
>> Here's what I mean:
>>
>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>> {
>>         struct usb_charger *charger;
>>         enum usb_charger_type type;
>>
>>         if (!gadget->charger)
>>                 return 0;
>>
>>         charger = gadget->charger;
>>         type = charger->type;
>>
>>         return __usb_charger_set_cur_limit(charger, type, mA);
>> }
>
> But that means we need to export  both 'usb_charger_set_cur_limit()'
> function and '__usb_charger_set_cur_limit()' function in charger.c
> file. Cause some user may want to set the current limitation by one
> charger type parameter (may be not from charger->type), like by
> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
> you think about this situation? Thanks.

if we have that requirement, that's totally fine. Just rename
__usb_charger_set_cur_limit() back to
_usb_charger_set_cur_limit_by_type() and expose both. But
set_cur_limit_by_type can assume its arguments are valid at all times.

-- 
balbi

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

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

[Baolin Wang]

On 29 June 2016 at 16:34, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>
>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>> then the usb charger can do the power things.
>>>>>>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>
>>>>> Before anything, I must say that I really liked this patch. It's
>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>
>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>> completely?
>>>>>
>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>  {
>>>>>> +     if (gadget->charger)
>>>>>
>>>>> I guess you could do this check inside
>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>
>>>> We will access the 'gadget->charger->type' member when issuing
>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>> check here in next new version.
>>>
>>> Here's what I mean:
>>>
>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>> {
>>>         struct usb_charger *charger;
>>>         enum usb_charger_type type;
>>>
>>>         if (!gadget->charger)
>>>                 return 0;
>>>
>>>         charger = gadget->charger;
>>>         type = charger->type;
>>>
>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>> }
>>
>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>> function and '__usb_charger_set_cur_limit()' function in charger.c
>> file. Cause some user may want to set the current limitation by one
>> charger type parameter (may be not from charger->type), like by
>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>> you think about this situation? Thanks.
>
> if we have that requirement, that's totally fine. Just rename
> __usb_charger_set_cur_limit() back to
> _usb_charger_set_cur_limit_by_type() and expose both. But
> set_cur_limit_by_type can assume its arguments are valid at all times.

Make sense. I'll fix this issue in v14 version. Thanks.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>>
>>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>>> then the usb charger can do the power things.
>>>>>>>
>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>>
>>>>>> Before anything, I must say that I really liked this patch. It's
>>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>>
>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>> completely?
>>>>>>
>>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>>  {
>>>>>>> +     if (gadget->charger)
>>>>>>
>>>>>> I guess you could do this check inside
>>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>>
>>>>> We will access the 'gadget->charger->type' member when issuing
>>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>>> check here in next new version.
>>>>
>>>> Here's what I mean:
>>>>
>>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>>> {
>>>>         struct usb_charger *charger;
>>>>         enum usb_charger_type type;
>>>>
>>>>         if (!gadget->charger)
>>>>                 return 0;
>>>>
>>>>         charger = gadget->charger;
>>>>         type = charger->type;
>>>>
>>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>>> }
>>>
>>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>>> function and '__usb_charger_set_cur_limit()' function in charger.c
>>> file. Cause some user may want to set the current limitation by one
>>> charger type parameter (may be not from charger->type), like by
>>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>>> you think about this situation? Thanks.
>>
>> if we have that requirement, that's totally fine. Just rename
>> __usb_charger_set_cur_limit() back to
>> _usb_charger_set_cur_limit_by_type() and expose both. But
>> set_cur_limit_by_type can assume its arguments are valid at all times.
>
> Make sense. I'll fix this issue in v14 version. Thanks.

there's one thing bothering me though:

gadget->charger is supposed to be "current detected charger" right? If
we have a single port tied to a single charger, in which case would we
*ever* need to change current limit of any charger type other than
"current charger" ?

IOW, why do we need _set_cur_limit_by_type() exported at all?

-- 
balbi

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

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

[Baolin Wang]

Hi Felipe,

On 29 June 2016 at 20:06, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>>>
>>>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>>>> then the usb charger can do the power things.
>>>>>>>>
>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>>>
>>>>>>> Before anything, I must say that I really liked this patch. It's
>>>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>>>
>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>> completely?
>>>>>>>
>>>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>>>  {
>>>>>>>> +     if (gadget->charger)
>>>>>>>
>>>>>>> I guess you could do this check inside
>>>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>>>
>>>>>> We will access the 'gadget->charger->type' member when issuing
>>>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>>>> check here in next new version.
>>>>>
>>>>> Here's what I mean:
>>>>>
>>>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>>>> {
>>>>>         struct usb_charger *charger;
>>>>>         enum usb_charger_type type;
>>>>>
>>>>>         if (!gadget->charger)
>>>>>                 return 0;
>>>>>
>>>>>         charger = gadget->charger;
>>>>>         type = charger->type;
>>>>>
>>>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>>>> }
>>>>
>>>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>>>> function and '__usb_charger_set_cur_limit()' function in charger.c
>>>> file. Cause some user may want to set the current limitation by one
>>>> charger type parameter (may be not from charger->type), like by
>>>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>>>> you think about this situation? Thanks.
>>>
>>> if we have that requirement, that's totally fine. Just rename
>>> __usb_charger_set_cur_limit() back to
>>> _usb_charger_set_cur_limit_by_type() and expose both. But
>>> set_cur_limit_by_type can assume its arguments are valid at all times.
>>
>> Make sense. I'll fix this issue in v14 version. Thanks.
>
> there's one thing bothering me though:
>
> gadget->charger is supposed to be "current detected charger" right? If
> we have a single port tied to a single charger, in which case would we
> *ever* need to change current limit of any charger type other than
> "current charger" ?

What I mean is user can set the current limit by charger type as what
they want at initial stage. As we know the default current of SDP (not
super speed) is 500 mA, but user can set the current limit of SDP as
450 mA if there are some special on their own platform by issuing
'__usb_charger_set_cur_limit(charger, SDP_TYPE, 450)'.

>
> IOW, why do we need _set_cur_limit_by_type() exported at all?
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>>> For supporting the usb charger, it adds the usb_charger_init() and
>>>>>>>>> usb_charger_exit() functions for usb charger initialization and exit.
>>>>>>>>>
>>>>>>>>> It will report to the usb charger when the gadget state is changed,
>>>>>>>>> then the usb charger can do the power things.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>>>> Reviewed-by: Li Jun <jun.li@nxp.com>
>>>>>>>>> Tested-by: Li Jun <jun.li@nxp.com>
>>>>>>>>
>>>>>>>> Before anything, I must say that I really liked this patch. It's
>>>>>>>> minimaly invasive to udc core and does all the necessary changes. If it
>>>>>>>> wasn't for the extra charger class, this would've been perfect.
>>>>>>>>
>>>>>>>> Can't you just tie a charger to a UDC and avoid the charger class
>>>>>>>> completely?
>>>>>>>>
>>>>>>>>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>>>>>>>>>  {
>>>>>>>>> +     if (gadget->charger)
>>>>>>>>
>>>>>>>> I guess you could do this check inside
>>>>>>>> usb_gadget_set_cur_limit_by_type() itself.
>>>>>>>
>>>>>>> We will access the 'gadget->charger->type' member when issuing
>>>>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the
>>>>>>> check here in next new version.
>>>>>>
>>>>>> Here's what I mean:
>>>>>>
>>>>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)
>>>>>> {
>>>>>>         struct usb_charger *charger;
>>>>>>         enum usb_charger_type type;
>>>>>>
>>>>>>         if (!gadget->charger)
>>>>>>                 return 0;
>>>>>>
>>>>>>         charger = gadget->charger;
>>>>>>         type = charger->type;
>>>>>>
>>>>>>         return __usb_charger_set_cur_limit(charger, type, mA);
>>>>>> }
>>>>>
>>>>> But that means we need to export  both 'usb_charger_set_cur_limit()'
>>>>> function and '__usb_charger_set_cur_limit()' function in charger.c
>>>>> file. Cause some user may want to set the current limitation by one
>>>>> charger type parameter (may be not from charger->type), like by
>>>>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do
>>>>> you think about this situation? Thanks.
>>>>
>>>> if we have that requirement, that's totally fine. Just rename
>>>> __usb_charger_set_cur_limit() back to
>>>> _usb_charger_set_cur_limit_by_type() and expose both. But
>>>> set_cur_limit_by_type can assume its arguments are valid at all times.
>>>
>>> Make sense. I'll fix this issue in v14 version. Thanks.
>>
>> there's one thing bothering me though:
>>
>> gadget->charger is supposed to be "current detected charger" right? If
>> we have a single port tied to a single charger, in which case would we
>> *ever* need to change current limit of any charger type other than
>> "current charger" ?
>
> What I mean is user can set the current limit by charger type as what
> they want at initial stage. As we know the default current of SDP (not
> super speed) is 500 mA, but user can set the current limit of SDP as
> 450 mA if there are some special on their own platform by issuing
> '__usb_charger_set_cur_limit(charger, SDP_TYPE, 450)'.

Oh I see. Odd, but okay

-- 
balbi

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