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

[PATCH v18 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 v17:
 - Remove goto section in usb_charger_register() function.
 - Remove 'extern' in charger.h file.
 - Move the kfree() to usb_charger_exit() function.

Changes since v16:
 - Modify the charger current range with introducing the maximum and minimum
 current.
 - Remove the getting charger type method from power supply.
 - Add the getting charger type method from extcon system.
 - Introduce new usb_charger_get_current() API for users to get the maximum and
 minimum current.
 - Rename some APIs and other optimization.

Changes since v15:
 - Add charger state checking to avoid sending out duplicate notifies to users.
 - Add one work to notify power users the current has been changed.

Changes since v14:
 - Add kernel documentation for struct usb_cahrger.
 - Remove some redundant WARN() functions.

Changes since v13:
 - Remove the charger checking in usb_gadget_vbus_draw() function.
 - Rename some functions in charger.c file.
 - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tags/usb-for-v4.8

Changes since v12:
 - Remove the class and device things.
 - Link usb charger to udc-core.ko.
 - Create one "charger" subdirectory which holds all charger-related attributes.

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):
  usb: gadget: Introduce the usb charger framework
  usb: gadget: Support for the usb charger framework
  usb: gadget: Integrate with the usb gadget supporting for usb charger
  power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c     |   75 ++++
 drivers/usb/gadget/Kconfig       |    8 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  877 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c    |   19 +-
 include/linux/mfd/wm831x/pdata.h |    3 +
 include/linux/usb/charger.h      |  185 ++++++++
 include/linux/usb/gadget.h       |    3 +
 include/uapi/linux/usb/charger.h |   31 ++
 9 files changed, 1201 insertions(+), 1 deletion(-)
 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 v18 1/4] usb: 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       |    8 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  782 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  185 +++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 1007 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 2ea3fc3..7520677 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -134,6 +134,14 @@ config U_SERIAL_CONSOLE
 	help
 	   It supports the serial gadget can be used as a console.
 
+config USB_CHARGER
+	bool "USB charger support"
+	select EXTCON
+	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 98e74ed..ede2351 100644
--- a/drivers/usb/gadget/udc/Makefile
+++ b/drivers/usb/gadget/udc/Makefile
@@ -2,6 +2,7 @@
 CFLAGS_trace.o			:= -I$(src)
 
 udc-core-y			:= core.o trace.o
+udc-core-$(CONFIG_USB_CHARGER)	+= charger.o
 
 #
 # USB peripheral controller drivers
diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
new file mode 100644
index 0000000..d885e86
--- /dev/null
+++ b/drivers/usb/gadget/udc/charger.c
@@ -0,0 +1,782 @@
+/*
+ * charger.c -- USB charger driver
+ *
+ * Copyright (C) 2016 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>
+
+/* Default current range by charger type. */
+#define DEFAULT_SDP_CUR_MIN	2
+#define DEFAULT_SDP_CUR_MAX	500
+#define DEFAULT_SDP_CUR_MIN_SS	150
+#define DEFAULT_SDP_CUR_MAX_SS	900
+#define DEFAULT_DCP_CUR_MIN	500
+#define DEFAULT_DCP_CUR_MAX	5000
+#define DEFAULT_CDP_CUR_MIN	1500
+#define DEFAULT_CDP_CUR_MAX	5000
+#define DEFAULT_ACA_CUR_MIN	1500
+#define DEFAULT_ACA_CUR_MAX	5000
+
+static DEFINE_IDA(usb_charger_ida);
+static LIST_HEAD(charger_list);
+static DEFINE_MUTEX(charger_lock);
+
+static struct usb_charger *dev_to_uchger(struct device *dev)
+{
+	return NULL;
+}
+
+/*
+ * charger_current_show() - Show the charger current.
+ */
+static ssize_t charger_current_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	unsigned int min, max;
+
+	usb_charger_get_current(uchger, &min, &max);
+	return sprintf(buf, "%u-%u\n", min, max);
+}
+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
+};
+
+static const struct attribute_group usb_charger_group = {
+	.name = "charger",
+	.attrs = usb_charger_attrs,
+};
+__ATTRIBUTE_GROUPS(usb_charger);
+
+/*
+ * usb_charger_find_by_name() - Get the usb charger instance by name.
+ * @name - usb charger name.
+ */
+struct usb_charger *usb_charger_find_by_name(const char *name)
+{
+	struct usb_charger *uchger;
+
+	if (WARN(!name, "can't work with NULL name"))
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&charger_lock);
+	list_for_each_entry(uchger, &charger_list, list) {
+		if (!strcmp(uchger->name, name))
+			break;
+	}
+	mutex_unlock(&charger_lock);
+
+	if (WARN(!uchger, "can't find usb charger"))
+		return ERR_PTR(-ENODEV);
+
+	return uchger;
+}
+EXPORT_SYMBOL_GPL(usb_charger_find_by_name);
+
+/*
+ * __usb_charger_get_type() - Get the usb charger type by the callback which
+ * is implemented by users.
+ * @uchger - the usb charger instance.
+ *
+ * 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(struct usb_charger *uchger)
+{
+	if (uchger->type != UNKNOWN_TYPE)
+		return uchger->type;
+
+	if (uchger->get_charger_type)
+		uchger->type = uchger->get_charger_type(uchger);
+	else
+		uchger->type = UNKNOWN_TYPE;
+
+	return uchger->type;
+}
+
+/*
+ * usb_charger_get_type() - get the usb charger type with lock protection.
+ * @uchger - usb charger instance.
+ *
+ * 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 = __usb_charger_get_type(uchger);
+	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 instance.
+ *
+ * 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 instance.
+ *
+ * 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 (!uchger->charger_detect)
+		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_notify_work() - Notify users the current has changed by work.
+ * @work - the work instance.
+ *
+ * Note: When users receive the charger present event, they should check the
+ * charger current by usb_charger_get_current() API.
+ */
+static void usb_charger_notify_work(struct work_struct *work)
+{
+	struct usb_charger *uchger = work_to_charger(work);
+
+	mutex_lock(&uchger->lock);
+	if (uchger->state == USB_CHARGER_PRESENT) {
+		raw_notifier_call_chain(&uchger->uchger_nh,
+					USB_CHARGER_PRESENT,
+					uchger);
+	}
+	mutex_unlock(&uchger->lock);
+}
+
+/*
+ * __usb_charger_set_cur_limit_by_type() - Set the current limitation
+ * by charger type.
+ * @uchger - the usb charger instance.
+ * @type - the usb charger type.
+ * @cur_limit - the current limitation.
+ */
+static int __usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+					       enum usb_charger_type type,
+					       unsigned int cur_limit)
+{
+	switch (type) {
+	case SDP_TYPE:
+		uchger->cur.sdp_max = (cur_limit > DEFAULT_SDP_CUR_MAX) ?
+			DEFAULT_SDP_CUR_MAX : cur_limit;
+		uchger->sdp_default_cur_change = 1;
+		break;
+	case DCP_TYPE:
+		uchger->cur.dcp_max = (cur_limit > DEFAULT_DCP_CUR_MAX) ?
+			DEFAULT_DCP_CUR_MAX : cur_limit;
+		break;
+	case CDP_TYPE:
+		uchger->cur.cdp_max = (cur_limit > DEFAULT_CDP_CUR_MAX) ?
+			DEFAULT_CDP_CUR_MAX : cur_limit;
+		break;
+	case ACA_TYPE:
+		uchger->cur.aca_max = (cur_limit > DEFAULT_ACA_CUR_MAX) ?
+			DEFAULT_ACA_CUR_MAX : cur_limit;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
+ * usb_charger_set_cur_limit_by_gadget() - Set the current limitation from
+ * gadget layer.
+ * @gadget - the usb gadget device.
+ * @cur_limit - the current limitation.
+ *
+ * Note: This function is used in atomic contexts without mutex lock.
+ */
+int usb_charger_set_cur_limit_by_gadget(struct usb_gadget *gadget,
+					unsigned int cur_limit)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_gadget);
+
+/*
+ * usb_charger_set_cur_limit_by_type() - Set the current limitation
+ * by charger type with lock protection.
+ * @uchger - the usb charger instance.
+ * @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_cur_limit_by_type(struct usb_charger *uchger,
+				      enum usb_charger_type type,
+				      unsigned int cur_limit)
+{
+	int ret;
+
+	if (!uchger)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	ret = __usb_charger_set_cur_limit_by_type(uchger, type, cur_limit);
+	mutex_unlock(&uchger->lock);
+
+	schedule_work(&uchger->work);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_type);
+
+/*
+ * usb_charger_set_current() - Set the usb charger current.
+ * @uchger - the usb charger instance.
+ * @values - the current setting values.
+ */
+int usb_charger_set_current(struct usb_charger *uchger,
+			    struct usb_charger_current *values)
+{
+	if (!uchger || !values)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	uchger->cur.sdp_min = values->sdp_min;
+	uchger->cur.sdp_max = (values->sdp_max > DEFAULT_SDP_CUR_MAX) ?
+		DEFAULT_SDP_CUR_MAX : values->sdp_max;
+	uchger->cur.dcp_min = values->dcp_min;
+	uchger->cur.dcp_max = (values->dcp_max > DEFAULT_DCP_CUR_MAX) ?
+		DEFAULT_DCP_CUR_MAX : values->dcp_max;
+	uchger->cur.cdp_min = values->cdp_min;
+	uchger->cur.cdp_max = (values->cdp_max > DEFAULT_CDP_CUR_MAX) ?
+		DEFAULT_CDP_CUR_MAX : values->cdp_max;
+	uchger->cur.aca_min = values->aca_min;
+	uchger->cur.aca_max = (values->aca_max > DEFAULT_ACA_CUR_MAX) ?
+		DEFAULT_ACA_CUR_MAX : values->aca_max;
+	uchger->sdp_default_cur_change = 2;
+	mutex_unlock(&uchger->lock);
+
+	schedule_work(&uchger->work);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_current);
+
+/*
+ * usb_charger_get_current() - Get the charger current with lock protection.
+ * @uchger - the usb charger instance.
+ * @min - return the minimum current.
+ * @max - return the maximum current.
+ *
+ * Users should get the charger current by this safe API.
+ */
+int usb_charger_get_current(struct usb_charger *uchger,
+			    unsigned int *min,
+			    unsigned int *max)
+{
+	enum usb_charger_type type;
+
+	if (!uchger)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	type = __usb_charger_get_type(uchger);
+
+	switch (type) {
+	case SDP_TYPE:
+		/*
+		 * For super speed gadget, the default charger maximum current
+		 * should be 900 mA and the default minimum current should be
+		 * 150mA.
+		 */
+		if (uchger->sdp_default_cur_change != 2 && uchger->gadget &&
+		    uchger->gadget->speed >= USB_SPEED_SUPER) {
+			if (!uchger->sdp_default_cur_change)
+				uchger->cur.sdp_max = DEFAULT_SDP_CUR_MAX_SS;
+			uchger->cur.sdp_min = DEFAULT_SDP_CUR_MIN_SS;
+			uchger->sdp_default_cur_change = 2;
+		}
+
+		*min = uchger->cur.sdp_min;
+		*max = uchger->cur.sdp_max;
+		break;
+	case DCP_TYPE:
+		*min = uchger->cur.dcp_min;
+		*max = uchger->cur.dcp_max;
+		break;
+	case CDP_TYPE:
+		*min = uchger->cur.cdp_min;
+		*max = uchger->cur.cdp_max;
+		break;
+	case ACA_TYPE:
+		*min = uchger->cur.aca_min;
+		*max = uchger->cur.aca_max;
+		break;
+	default:
+		*min = 0;
+		*max = 0;
+		break;
+	}
+	mutex_unlock(&uchger->lock);
+
+	return 0;
+}
+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 instance which is monitored.
+ * @nb - a notifier block to be registered.
+ */
+int usb_charger_register_notify(struct usb_charger *uchger,
+				struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb) {
+		pr_err("Charger or nb can not be NULL.\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&uchger->lock);
+	ret = raw_notifier_chain_register(&uchger->uchger_nh, nb);
+	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 instance which is monitored.
+ * @nb - a notifier block to be unregistered.
+ */
+int usb_charger_unregister_notify(struct usb_charger *uchger,
+				  struct notifier_block *nb)
+{
+	int ret;
+
+	if (!uchger || !nb) {
+		pr_err("Charger or nb can not be NULL.\n");
+		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_notify_state() - It will notify other device registered
+ * on usb charger when the usb charger state is changed.
+ * @uchger - the usb charger instance.
+ * @state - the state of the usb charger.
+ *
+ * Note: When notify the charger present state to power driver, the power driver
+ * should get the current by usb_charger_get_current() API to set current.
+ */
+static void usb_charger_notify_state(struct usb_charger *uchger,
+				     enum usb_charger_state state)
+{
+	char uchger_state[50] = { 0 };
+	char *envp[] = { uchger_state, NULL };
+
+	mutex_lock(&uchger->lock);
+	if (uchger->state == state) {
+		mutex_unlock(&uchger->lock);
+		return;
+	}
+
+	uchger->state = state;
+
+	switch (state) {
+	case USB_CHARGER_PRESENT:
+		__usb_charger_get_type(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+					USB_CHARGER_PRESENT,
+					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,
+					USB_CHARGER_REMOVE,
+					uchger);
+		snprintf(uchger_state, ARRAY_SIZE(uchger_state),
+			 "USB_CHARGER_STATE=%s", "USB_CHARGER_REMOVE");
+		break;
+	default:
+		pr_warn("Unknown USB charger state: %d\n", state);
+		mutex_unlock(&uchger->lock);
+		return;
+	}
+
+	kobject_uevent_env(&uchger->gadget->dev.kobj, KOBJ_CHANGE, envp);
+	mutex_unlock(&uchger->lock);
+}
+
+/*
+ * usb_charger_type_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_type_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;
+	enum usb_charger_type type;
+
+	if (WARN(!uchger, "charger can not be NULL"))
+		return NOTIFY_BAD;
+
+	/* Determine charger type */
+	if (extcon_get_cable_state_(uchger->extcon_dev,
+				    EXTCON_CHG_USB_SDP) > 0) {
+		type = SDP_TYPE;
+		uchger_state = USB_CHARGER_PRESENT;
+	} else if (extcon_get_cable_state_(uchger->extcon_dev,
+					   EXTCON_CHG_USB_CDP) > 0) {
+		type = CDP_TYPE;
+		uchger_state = USB_CHARGER_PRESENT;
+	} else if (extcon_get_cable_state_(uchger->extcon_dev,
+					   EXTCON_CHG_USB_DCP) > 0) {
+		type = DCP_TYPE;
+		uchger_state = USB_CHARGER_PRESENT;
+	} else if (extcon_get_cable_state_(uchger->extcon_dev,
+					   EXTCON_CHG_USB_ACA) > 0) {
+		type = ACA_TYPE;
+		uchger_state = USB_CHARGER_PRESENT;
+	} else {
+		type = UNKNOWN_TYPE;
+		uchger_state = USB_CHARGER_REMOVE;
+	}
+
+	mutex_lock(&uchger->lock);
+	uchger->type = type;
+	mutex_unlock(&uchger->lock);
+
+	usb_charger_notify_state(uchger, uchger_state);
+
+	return NOTIFY_OK;
+}
+
+/*
+ * 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 users 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_state(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);
+
+/*
+ * usb_charger_register() - Register a new usb charger.
+ * @uchger - the new usb charger instance.
+ */
+static int usb_charger_register(struct usb_charger *uchger)
+{
+	int ret;
+
+	ret = ida_simple_get(&usb_charger_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0) {
+		pr_err("Failed to register usb charger: %d\n", ret);
+		return ret;
+	}
+
+	uchger->id = ret;
+	scnprintf(uchger->name, CHARGER_NAME_MAX, "usb-charger.%d", uchger->id);
+
+	ret = sysfs_create_groups(&uchger->gadget->dev.kobj,
+				  usb_charger_groups);
+	if (ret) {
+		pr_err("Failed to create usb charger attributes: %d\n", ret);
+		ida_simple_remove(&usb_charger_ida, uchger->id);
+		uchger->id = -1;
+		return ret;
+	}
+
+	mutex_lock(&charger_lock);
+	list_add_tail(&uchger->list, &charger_list);
+	mutex_unlock(&charger_lock);
+
+	return 0;
+}
+
+int usb_charger_init(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger;
+	struct extcon_dev *edev;
+	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.sdp_min = DEFAULT_SDP_CUR_MIN;
+	uchger->cur.sdp_max = DEFAULT_SDP_CUR_MAX;
+	uchger->cur.dcp_min = DEFAULT_DCP_CUR_MIN;
+	uchger->cur.dcp_max = DEFAULT_DCP_CUR_MAX;
+	uchger->cur.cdp_min = DEFAULT_CDP_CUR_MIN;
+	uchger->cur.cdp_max = DEFAULT_CDP_CUR_MAX;
+	uchger->cur.aca_min = DEFAULT_ACA_CUR_MIN;
+	uchger->cur.aca_max = DEFAULT_ACA_CUR_MAX;
+
+	mutex_init(&uchger->lock);
+	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
+	INIT_WORK(&uchger->work, usb_charger_notify_work);
+
+	/* 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;
+		ret = extcon_register_notifier(edev, EXTCON_USB,
+					       &uchger->extcon_nb.nb);
+		if (ret) {
+			pr_err("Failed to register extcon USB notifier.\n");
+			goto fail_extcon;
+		}
+
+		uchger->extcon_type_nb.nb.notifier_call =
+					usb_charger_type_by_extcon;
+		uchger->extcon_type_nb.uchger = uchger;
+
+		ret = extcon_register_notifier(edev, EXTCON_CHG_USB_SDP,
+					       &uchger->extcon_type_nb.nb);
+		if (ret) {
+			pr_err("Failed to register extcon USB SDP notifier.\n");
+			goto fail_extcon_sdp;
+		}
+
+		ret = extcon_register_notifier(edev, EXTCON_CHG_USB_CDP,
+					       &uchger->extcon_type_nb.nb);
+		if (ret) {
+			pr_err("Failed to register extcon USB CDP notifier.\n");
+			goto fail_extcon_cdp;
+		}
+
+		ret = extcon_register_notifier(edev, EXTCON_CHG_USB_DCP,
+					       &uchger->extcon_type_nb.nb);
+		if (ret) {
+			pr_err("Failed to register extcon USB DCP notifier.\n");
+			goto fail_extcon_dcp;
+		}
+
+		ret = extcon_register_notifier(edev, EXTCON_CHG_USB_ACA,
+					       &uchger->extcon_type_nb.nb);
+		if (ret) {
+			pr_err("Failed to register extcon USB ACA notifier.\n");
+			goto fail_extcon_aca;
+		}
+	}
+
+	uchger->gadget = ugadget;
+	uchger->old_gadget_state = USB_STATE_NOTATTACHED;
+
+	/* Register a new usb charger */
+	ret = usb_charger_register(uchger);
+	if (ret)
+		goto fail_register;
+
+	return 0;
+
+fail_register:
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_ACA,
+				   &uchger->extcon_type_nb.nb);
+fail_extcon_aca:
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_DCP,
+				   &uchger->extcon_type_nb.nb);
+fail_extcon_dcp:
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_CDP,
+				   &uchger->extcon_type_nb.nb);
+fail_extcon_cdp:
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_SDP,
+				   &uchger->extcon_type_nb.nb);
+fail_extcon_sdp:
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_USB,
+				   &uchger->extcon_nb.nb);
+fail_extcon:
+	kfree(uchger);
+
+	return ret;
+}
+
+int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+
+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..1ca97370
--- /dev/null
+++ b/include/linux/usb/charger.h
@@ -0,0 +1,185 @@
+#ifndef __LINUX_USB_CHARGER_H__
+#define __LINUX_USB_CHARGER_H__
+
+#include <uapi/linux/usb/ch9.h>
+#include <uapi/linux/usb/charger.h>
+
+#define CHARGER_NAME_MAX	30
+#define work_to_charger(w)	(container_of((w), struct usb_charger, work))
+
+/* Current range by charger type */
+struct usb_charger_current {
+	unsigned int sdp_min;
+	unsigned int sdp_max;
+	unsigned int dcp_min;
+	unsigned int dcp_max;
+	unsigned int cdp_min;
+	unsigned int cdp_max;
+	unsigned int aca_min;
+	unsigned int aca_max;
+};
+
+struct usb_charger_nb {
+	struct notifier_block	nb;
+	struct usb_charger	*uchger;
+};
+
+/**
+ * struct usb_charger - describe one usb charger
+ * @name: Charger name.
+ * @list: For linking usb charger instances into one global list.
+ * @uchger_nh: Notifier head for users to register.
+ * @lock: Protect the notifier head and charger.
+ * @id: The charger id.
+ * @type: The charger type, it can be SDP_TYPE, DCP_TYPE, CDP_TYPE or
+ * ACA_TYPE.
+ * @state: Indicate the charger state.
+ * @cur: Describe the current range by charger type.
+ * @work: Workqueue to be used for reporting users current has changed.
+ * @extcon_dev: For supporting extcon usb gpio device to detect usb cable
+ * event (plugin or not).
+ * @extcon_nb: Report the events to charger from extcon device.
+ * @extcon_type_nb: Report the charger type from extcon device.
+ * @gadget: One usb charger is always tied to one usb gadget.
+ * @old_gadget_state: Record the previous state of one usb gadget to check if
+ * the gadget state is changed. If gadget state is changed, then notify the
+ * event to charger.
+ * @sdp_default_cur_change: Check if it is needed to change the SDP charger
+ * default maximum current.
+ * @get_charger_type: User can get charger type by implementing this callback.
+ * @charger_detect: Charger detection method can be implemented if you need to
+ * manually detect the charger type.
+ *
+ * Users can set 'get_charger_type' and 'charger_detect' callbacks directly
+ * according to their own requirements. Beyond that, users can not touch
+ * anything else directly in this structure.
+ */
+struct usb_charger {
+	char				name[CHARGER_NAME_MAX];
+	struct list_head		list;
+	struct raw_notifier_head	uchger_nh;
+	struct mutex			lock;
+	int				id;
+	enum usb_charger_type		type;
+	enum usb_charger_state		state;
+	struct usb_charger_current	cur;
+	struct work_struct		work;
+
+	struct extcon_dev		*extcon_dev;
+	struct usb_charger_nb		extcon_nb;
+	struct usb_charger_nb		extcon_type_nb;
+
+	struct usb_gadget		*gadget;
+	enum usb_device_state		old_gadget_state;
+	unsigned int			sdp_default_cur_change;
+
+	enum usb_charger_type	(*get_charger_type)(struct usb_charger *);
+	enum usb_charger_type	(*charger_detect)(struct usb_charger *);
+};
+
+#ifdef CONFIG_USB_CHARGER
+struct usb_charger *usb_charger_find_by_name(const char *name);
+
+int usb_charger_register_notify(struct usb_charger *uchger,
+				struct notifier_block *nb);
+int usb_charger_unregister_notify(struct usb_charger *uchger,
+				  struct notifier_block *nb);
+
+int usb_charger_set_current(struct usb_charger *uchger,
+			    struct usb_charger_current *values);
+int usb_charger_get_current(struct usb_charger *uchger,
+			    unsigned int *min, unsigned int *max);
+
+int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+				      enum usb_charger_type type,
+				      unsigned int cur_limit);
+int usb_charger_set_cur_limit_by_gadget(struct usb_gadget *gadget,
+					unsigned int cur_limit);
+
+int usb_charger_plug_by_gadget(struct usb_gadget *gadget, unsigned long state);
+enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger);
+int usb_charger_detect_type(struct usb_charger *uchger);
+enum usb_charger_state usb_charger_get_state(struct usb_charger *uchger);
+
+int usb_charger_init(struct usb_gadget *ugadget);
+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 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_current(struct usb_charger *uchger,
+					  struct usb_charger_current *values)
+{
+	return 0;
+}
+
+static inline int usb_charger_get_current(struct usb_charger *uchger,
+					  unsigned int *min, unsigned int *max)
+{
+	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_cur_limit_by_gadget(struct usb_gadget *gadget,
+				    unsigned int cur_limit)
+{
+	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 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 v18 2/4] usb: 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/core.c |   19 ++++++++++++++++++-
 include/linux/usb/gadget.h    |    3 +++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 9483489..90df022 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -25,6 +25,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/workqueue.h>
 
+#include <linux/usb/charger.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
@@ -576,12 +577,17 @@ EXPORT_SYMBOL_GPL(usb_gadget_vbus_connect);
  * 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.
  */
 int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
 	int ret = 0;
 
+	usb_charger_set_cur_limit_by_gadget(gadget, mA);
+
 	if (!gadget->ops->vbus_draw) {
 		ret = -EOPNOTSUPP;
 		goto out;
@@ -963,6 +969,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");
 }
@@ -1132,6 +1141,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;
 
@@ -1143,7 +1156,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 			if (ret != -EPROBE_DEFER)
 				list_del(&driver->pending);
 			if (ret)
-				goto err5;
+				goto err6;
 			break;
 		}
 	}
@@ -1152,6 +1165,9 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	return 0;
 
+err6:
+	usb_charger_exit(gadget);
+
 err5:
 	device_del(&udc->dev);
 
@@ -1263,6 +1279,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 8e81f9e..82a0d63 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>
 
 #define UDC_TRACE_STR_MAX	512
 
@@ -328,6 +329,7 @@ struct usb_gadget_ops {
  * @in_epnum: last used in ep number
  * @mA: last set mA value
  * @otg_caps: OTG capabilities of this gadget.
+ * @charger: Negotiate the power with the usb charger.
  * @sg_supported: true if we can handle scatter-gather
  * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
  *	gadget driver must provide a USB OTG descriptor.
@@ -387,6 +389,7 @@ struct usb_gadget {
 	unsigned			in_epnum;
 	unsigned			mA;
 	struct usb_otg_caps		*otg_caps;
+	struct usb_charger		*charger;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
-- 
1.7.9.5

[PATCH v18 3/4] usb: 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, usb_charger_exit()
function and dev_to_uchger() 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 |   97 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
index d885e86..56aee28 100644
--- a/drivers/usb/gadget/udc/charger.c
+++ b/drivers/usb/gadget/udc/charger.c
@@ -38,7 +38,9 @@ static DEFINE_MUTEX(charger_lock);
 
 static struct usb_charger *dev_to_uchger(struct device *dev)
 {
-	return NULL;
+	struct usb_gadget *gadget = container_of(dev, struct usb_gadget, dev);
+
+	return gadget->charger;
 }
 
 /*
@@ -308,6 +310,18 @@ static int __usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
 int usb_charger_set_cur_limit_by_gadget(struct usb_gadget *gadget,
 					unsigned int cur_limit)
 {
+	struct usb_charger *uchger = gadget->charger;
+	int ret;
+
+	if (!uchger)
+		return -EINVAL;
+
+	ret = __usb_charger_set_cur_limit_by_type(uchger, uchger->type,
+						  cur_limit);
+	if (ret)
+		return ret;
+
+	schedule_work(&uchger->work);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_gadget);
@@ -625,11 +639,66 @@ static int 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) {
+			mutex_lock(&uchger->lock);
+
+			/*
+			 * Need check the charger type to make sure the usb
+			 * cable is removed, in case it just changes the usb
+			 * function with configfs.
+			 */
+			uchger->type = __usb_charger_get_type(uchger);
+			if (uchger->type != UNKNOWN_TYPE) {
+				mutex_unlock(&uchger->lock);
+				return 0;
+			}
+
+			mutex_unlock(&uchger->lock);
+			uchger_state = USB_CHARGER_REMOVE;
+		} else {
+			uchger_state = USB_CHARGER_DEFAULT;
+		}
+
+		usb_charger_notify_state(uchger, uchger_state);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
 
 /*
+ * usb_charger_unregister() - Unregister a usb charger.
+ * @uchger - the usb charger to be unregistered.
+ */
+static int usb_charger_unregister(struct usb_charger *uchger)
+{
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+	sysfs_remove_groups(&uchger->gadget->dev.kobj, usb_charger_groups);
+
+	mutex_lock(&charger_lock);
+	list_del(&uchger->list);
+	mutex_unlock(&charger_lock);
+
+	return 0;
+}
+
+/*
  * usb_charger_register() - Register a new usb charger.
  * @uchger - the new usb charger instance.
  */
@@ -737,6 +806,7 @@ int usb_charger_init(struct usb_gadget *ugadget)
 	}
 
 	uchger->gadget = ugadget;
+	ugadget->charger = uchger;
 	uchger->old_gadget_state = USB_STATE_NOTATTACHED;
 
 	/* Register a new usb charger */
@@ -774,6 +844,31 @@ fail_extcon:
 
 int usb_charger_exit(struct usb_gadget *ugadget)
 {
+	struct usb_charger *uchger = ugadget->charger;
+
+	if (!uchger)
+		return -EINVAL;
+
+	usb_charger_unregister(uchger);
+	ugadget->charger = NULL;
+
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_USB,
+				   &uchger->extcon_nb.nb);
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_SDP,
+				   &uchger->extcon_type_nb.nb);
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_CDP,
+				   &uchger->extcon_type_nb.nb);
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_DCP,
+				   &uchger->extcon_type_nb.nb);
+	extcon_unregister_notifier(uchger->extcon_dev,
+				   EXTCON_CHG_USB_ACA,
+				   &uchger->extcon_type_nb.nb);
+
+	kfree(uchger);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH v18 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     |   75 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/wm831x/pdata.h |    3 ++
 2 files changed, 78 insertions(+)

diff --git a/drivers/power/wm831x_power.c b/drivers/power/wm831x_power.c
index 7082301..2bcd775 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,49 @@ 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 state, void *data)
+{
+	struct wm831x_power *wm831x_power = container_of(nb,
+							 struct wm831x_power,
+							 usb_notify);
+	struct usb_charger *uchger = (struct usb_charger *)data;
+	unsigned int i, best, min, limit;
+
+	if (state == USB_CHARGER_PRESENT)
+		usb_charger_get_current(uchger, &min, &limit);
+	else
+		limit = 0;
+
+	/* 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 +653,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 +706,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 v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

Hi Felipe,

On 19 October 2016 at 10:37, Baolin Wang <baolin.wang@linaro.org> wrote:
> Currently the Linux kernel does not provide any standard integration of this
> feature that integrates the USB subsystem with the system power regulation
> provided by PMICs meaning that either vendors must add this in their kernels
> or USB gadget devices based on Linux (such as mobile phones) may not behave
> as they should. 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 v17:
>  - Remove goto section in usb_charger_register() function.
>  - Remove 'extern' in charger.h file.
>  - Move the kfree() to usb_charger_exit() function.
>
> Changes since v16:
>  - Modify the charger current range with introducing the maximum and minimum
>  current.
>  - Remove the getting charger type method from power supply.
>  - Add the getting charger type method from extcon system.
>  - Introduce new usb_charger_get_current() API for users to get the maximum and
>  minimum current.
>  - Rename some APIs and other optimization.
>
> Changes since v15:
>  - Add charger state checking to avoid sending out duplicate notifies to users.
>  - Add one work to notify power users the current has been changed.
>
> Changes since v14:
>  - Add kernel documentation for struct usb_cahrger.
>  - Remove some redundant WARN() functions.
>
> Changes since v13:
>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>  - Rename some functions in charger.c file.
>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tags/usb-for-v4.8
>
> Changes since v12:
>  - Remove the class and device things.
>  - Link usb charger to udc-core.ko.
>  - Create one "charger" subdirectory which holds all charger-related attributes.
>
> 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.

Could you apply this patchset into your branch if there are no other
comments? Thanks.

>
> Baolin Wang (4):
>   usb: gadget: Introduce the usb charger framework
>   usb: gadget: Support for the usb charger framework
>   usb: gadget: Integrate with the usb gadget supporting for usb charger
>   power: wm831x_power: Support USB charger current limit management
>
>  drivers/power/wm831x_power.c     |   75 ++++
>  drivers/usb/gadget/Kconfig       |    8 +
>  drivers/usb/gadget/udc/Makefile  |    1 +
>  drivers/usb/gadget/udc/charger.c |  877 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/udc/core.c    |   19 +-
>  include/linux/mfd/wm831x/pdata.h |    3 +
>  include/linux/usb/charger.h      |  185 ++++++++
>  include/linux/usb/gadget.h       |    3 +
>  include/uapi/linux/usb/charger.h |   31 ++
>  9 files changed, 1201 insertions(+), 1 deletion(-)
>  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
>



-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Thu, Oct 27 2016, Baolin Wang wrote:

> Hi Felipe,
>
> On 19 October 2016 at 10:37, Baolin Wang <baolin.wang@linaro.org> wrote:
>> Currently the Linux kernel does not provide any standard integration of this
>> feature that integrates the USB subsystem with the system power regulation
>> provided by PMICs meaning that either vendors must add this in their kernels
>> or USB gadget devices based on Linux (such as mobile phones) may not behave
>> as they should. 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 v17:
>>  - Remove goto section in usb_charger_register() function.
>>  - Remove 'extern' in charger.h file.
>>  - Move the kfree() to usb_charger_exit() function.
>>
>> Changes since v16:
>>  - Modify the charger current range with introducing the maximum and minimum
>>  current.
>>  - Remove the getting charger type method from power supply.
>>  - Add the getting charger type method from extcon system.
>>  - Introduce new usb_charger_get_current() API for users to get the maximum and
>>  minimum current.
>>  - Rename some APIs and other optimization.
>>
>> Changes since v15:
>>  - Add charger state checking to avoid sending out duplicate notifies to users.
>>  - Add one work to notify power users the current has been changed.
>>
>> Changes since v14:
>>  - Add kernel documentation for struct usb_cahrger.
>>  - Remove some redundant WARN() functions.
>>
>> Changes since v13:
>>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>>  - Rename some functions in charger.c file.
>>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tags/usb-for-v4.8
>>
>> Changes since v12:
>>  - Remove the class and device things.
>>  - Link usb charger to udc-core.ko.
>>  - Create one "charger" subdirectory which holds all charger-related attributes.
>>
>> 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.
>
> Could you apply this patchset into your branch if there are no other
> comments? Thanks.

Some of my previous comments are still outstanding.  You seem to have
just brushed them off without apparently understanding.
And no-one else seems to care enough to try to bridge the gap...

Let me try again.

1/ I think we agreed that it doesn't make sense for there to be
 two chargers registered in a system.
 However usb_charger_register() still allows that, and assigns
 and arbitrary name to each based on discovery order.
 This *cannot* make sense.

2/ Why do you have usb_charger_set_current()??
  No code ever calls it.
  This updates the min and max current which are defined in a
  standard.  It never makes sense to change the min and max
  for a particular cable type.

3/ usb_charger_notify_state() does nothing if the state doesn't change.
  When the extcon detects an SDP, it will be called to set the state
  to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
  it happened to be before, which is probably wrong.
  When after USB negotiation completes,
  usb_charger_set_cur_limit_by_gadget()
  will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
  again, but with a new current.  This will be ignored, as the state is
  already USB_CHARGER_PRESENT.

 (as an aside
 +enum usb_charger_state {
 +	USB_CHARGER_DEFAULT,
 +	USB_CHARGER_PRESENT,
 +	USB_CHARGER_REMOVE,
 +};

 looks odd.  It should probably by
    USB_CHARGER_UNKNOWN
    USB_CHARGER_PRESENT
    USB_CHARGER_ABSENT

 "REMOVE" isn't a state.  "REMOVED" might be.
 )

4/ I still strongly object to the ->get_charger_type() interface.
 You previously said:

     No. User can implement the get_charger_type() method to access the
     PMIC registers to get the charger type, which is one very common method.

 I suggest that if the PMIC registers report the charger type, then the
 PMIC driver should register an EXTCON and report the charger type
 through that.  Then the information would be directly available to
 user-space, and the usb-charger framework would have a single uniform
 mechanism for being told the cable type.

 Related:  I don't like charger_type_show().  I don't think
 the usb-charger should export that information to user-space because
 extcon already does that, and duplication is confusing and pointless.

 And I just noticed you have a ->charger_detect() too, which seems
 identical to ->get_charger_type().  There is no documentation
 explaining the difference.

5/ There is no convincing example usage of this framework.
  wm8931x_power.c just scratches the surface.
  If it is so good, it should be easy to convert a lot of other
  drivers over to it.  If you did that it would be much easier
  to see how it works and what the strengths/weaknesses were.


NeilBrown

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

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

[Baolin Wang]

Hi,

On 28 October 2016 at 06:00, NeilBrown <neilb@suse.com> wrote:
> On Thu, Oct 27 2016, Baolin Wang wrote:
>
>> Hi Felipe,
>>
>> On 19 October 2016 at 10:37, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> Currently the Linux kernel does not provide any standard integration of this
>>> feature that integrates the USB subsystem with the system power regulation
>>> provided by PMICs meaning that either vendors must add this in their kernels
>>> or USB gadget devices based on Linux (such as mobile phones) may not behave
>>> as they should. 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 v17:
>>>  - Remove goto section in usb_charger_register() function.
>>>  - Remove 'extern' in charger.h file.
>>>  - Move the kfree() to usb_charger_exit() function.
>>>
>>> Changes since v16:
>>>  - Modify the charger current range with introducing the maximum and minimum
>>>  current.
>>>  - Remove the getting charger type method from power supply.
>>>  - Add the getting charger type method from extcon system.
>>>  - Introduce new usb_charger_get_current() API for users to get the maximum and
>>>  minimum current.
>>>  - Rename some APIs and other optimization.
>>>
>>> Changes since v15:
>>>  - Add charger state checking to avoid sending out duplicate notifies to users.
>>>  - Add one work to notify power users the current has been changed.
>>>
>>> Changes since v14:
>>>  - Add kernel documentation for struct usb_cahrger.
>>>  - Remove some redundant WARN() functions.
>>>
>>> Changes since v13:
>>>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>>>  - Rename some functions in charger.c file.
>>>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tags/usb-for-v4.8
>>>
>>> Changes since v12:
>>>  - Remove the class and device things.
>>>  - Link usb charger to udc-core.ko.
>>>  - Create one "charger" subdirectory which holds all charger-related attributes.
>>>
>>> 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.
>>
>> Could you apply this patchset into your branch if there are no other
>> comments? Thanks.
>
> Some of my previous comments are still outstanding.  You seem to have
> just brushed them off without apparently understanding.

I am very appreciate for your comments, and I've explained your
comments but you did not reply me......

> And no-one else seems to care enough to try to bridge the gap...
>
> Let me try again.
>
> 1/ I think we agreed that it doesn't make sense for there to be
>  two chargers registered in a system.

Yes, until now...

>  However usb_charger_register() still allows that, and assigns
>  and arbitrary name to each based on discovery order.
>  This *cannot* make sense.

Fine, I can change that to allow only one charger to register.

>
> 2/ Why do you have usb_charger_set_current()??
>   No code ever calls it.
>   This updates the min and max current which are defined in a
>   standard.  It never makes sense to change the min and max
>   for a particular cable type.

Mark, do we have some scenarios which want to change the current
limitation? If not, okay, I agree with you to remove this function.

>
> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>   When the extcon detects an SDP, it will be called to set the state
>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>   it happened to be before, which is probably wrong.

Sorry, I did not get your points here, could you please explain it explicitly?

>   When after USB negotiation completes,
>   usb_charger_set_cur_limit_by_gadget()
>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>   again, but with a new current.  This will be ignored, as the state is
>   already USB_CHARGER_PRESENT.

No, we will notify the user the current has been changed by one work.

>
>  (as an aside
>  +enum usb_charger_state {
>  +      USB_CHARGER_DEFAULT,
>  +      USB_CHARGER_PRESENT,
>  +      USB_CHARGER_REMOVE,
>  +};
>
>  looks odd.  It should probably by
>     USB_CHARGER_UNKNOWN
>     USB_CHARGER_PRESENT
>     USB_CHARGER_ABSENT
>
>  "REMOVE" isn't a state.  "REMOVED" might be.
>  )

Sure.

>
> 4/ I still strongly object to the ->get_charger_type() interface.
>  You previously said:
>
>      No. User can implement the get_charger_type() method to access the
>      PMIC registers to get the charger type, which is one very common method.
>
>  I suggest that if the PMIC registers report the charger type, then the
>  PMIC driver should register an EXTCON and report the charger type
>  through that.  Then the information would be directly available to
>  user-space, and the usb-charger framework would have a single uniform
>  mechanism for being told the cable type.

We just access only one PMIC register to get the charger type, which
is no need add one driver for that and there are no any events for
extcon. Some sample code in power driver can be like below:

enum usb_charger_type pmic_get_charger_type(struct usb_charger *charger)
{
    enum usb_charger_type type;
    u32 val;

    regmap(reg_map, PMIC_CHARGER_STATUS, &val);

    /* change val to 'enum usb_charger_type' type ... */
    return type;
}

->get_charger_type() = pmic_get_charger_type;

>
>  Related:  I don't like charger_type_show().  I don't think
>  the usb-charger should export that information to user-space because
>  extcon already does that, and duplication is confusing and pointless.

I think we should combine all charger related information into one
place for user. Moreover if we don't get charger type from extcon, we
should also need one place to export the charger type.

>
>  And I just noticed you have a ->charger_detect() too, which seems
>  identical to ->get_charger_type().  There is no documentation
>  explaining the difference.

I think the kernel doc have explained that, but I like to explain it
again. Since we can detect the charger by software or hardware (like
PMIC), if you need to detect your charger type by software, then you
can implement this callback, you can refer to Jun's patch:
http://www.spinics.net/lists/linux-usb/msg139808.html

>
> 5/ There is no convincing example usage of this framework.
>   wm8931x_power.c just scratches the surface.
>   If it is so good, it should be easy to convert a lot of other
>   drivers over to it.  If you did that it would be much easier
>   to see how it works and what the strengths/weaknesses were.

Jun have send out one patchset[1] based on my patchset, and he tested
mypatchset. Thanks for your comments.
[1]http://www.spinics.net/lists/linux-usb/msg139809.html


-- 
Baolin.wang
Best Regards

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

[Mark Brown]

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

On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
> On 28 October 2016 at 06:00, NeilBrown <neilb@suse.com> wrote:

> > 1/ I think we agreed that it doesn't make sense for there to be
> >  two chargers registered in a system.

> Yes, until now...

> >  However usb_charger_register() still allows that, and assigns
> >  and arbitrary name to each based on discovery order.
> >  This *cannot* make sense.

> Fine, I can change that to allow only one charger to register.

Yeah, it's a reasonable change.  I'm not sure the prior discussion was
100% conclusive on the issue (I remember there being some debate about
leaving things there to avoid any need for future refactoring to touch
the interface).

> > 2/ Why do you have usb_charger_set_current()??
> >   No code ever calls it.
> >   This updates the min and max current which are defined in a
> >   standard.  It never makes sense to change the min and max
> >   for a particular cable type.

> Mark, do we have some scenarios which want to change the current
> limitation? If not, okay, I agree with you to remove this function.

I'm not aware of any, we can always add it back if the need arises.

> >  Related:  I don't like charger_type_show().  I don't think
> >  the usb-charger should export that information to user-space because
> >  extcon already does that, and duplication is confusing and pointless.

> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

I had also thought there was some software negotation as well as the
physical charger in cases where the device is plugged into an active
host?  I could be wrong.

> > 5/ There is no convincing example usage of this framework.
> >   wm8931x_power.c just scratches the surface.
> >   If it is so good, it should be easy to convert a lot of other
> >   drivers over to it.  If you did that it would be much easier
> >   to see how it works and what the strengths/weaknesses were.

> Jun have send out one patchset[1] based on my patchset, and he tested
> mypatchset. Thanks for your comments.
> [1]http://www.spinics.net/lists/linux-usb/msg139809.html

I think it's a good idea to pick up Jun's patches into your patch set,
that way Jun doesn't need to rebase and it might help with review of
your patches too.

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

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

[Baolin Wang]

On 29 October 2016 at 01:03, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
>> On 28 October 2016 at 06:00, NeilBrown <neilb@suse.com> wrote:
>
>> > 1/ I think we agreed that it doesn't make sense for there to be
>> >  two chargers registered in a system.
>
>> Yes, until now...
>
>> >  However usb_charger_register() still allows that, and assigns
>> >  and arbitrary name to each based on discovery order.
>> >  This *cannot* make sense.
>
>> Fine, I can change that to allow only one charger to register.
>
> Yeah, it's a reasonable change.  I'm not sure the prior discussion was
> 100% conclusive on the issue (I remember there being some debate about
> leaving things there to avoid any need for future refactoring to touch
> the interface).

I think we should leave these things to avoid refactoring in future.

>
>> > 2/ Why do you have usb_charger_set_current()??
>> >   No code ever calls it.
>> >   This updates the min and max current which are defined in a
>> >   standard.  It never makes sense to change the min and max
>> >   for a particular cable type.
>
>> Mark, do we have some scenarios which want to change the current
>> limitation? If not, okay, I agree with you to remove this function.
>
> I'm not aware of any, we can always add it back if the need arises.

OK.

>
>> >  Related:  I don't like charger_type_show().  I don't think
>> >  the usb-charger should export that information to user-space because
>> >  extcon already does that, and duplication is confusing and pointless.
>
>> I think we should combine all charger related information into one
>> place for user. Moreover if we don't get charger type from extcon, we
>> should also need one place to export the charger type.
>
> I had also thought there was some software negotation as well as the
> physical charger in cases where the device is plugged into an active
> host?  I could be wrong.
>
>> > 5/ There is no convincing example usage of this framework.
>> >   wm8931x_power.c just scratches the surface.
>> >   If it is so good, it should be easy to convert a lot of other
>> >   drivers over to it.  If you did that it would be much easier
>> >   to see how it works and what the strengths/weaknesses were.
>
>> Jun have send out one patchset[1] based on my patchset, and he tested
>> mypatchset. Thanks for your comments.
>> [1]http://www.spinics.net/lists/linux-usb/msg139809.html
>
> I think it's a good idea to pick up Jun's patches into your patch set,
> that way Jun doesn't need to rebase and it might help with review of
> your patches too.

Yes, I think so. I will ask for Jun's help.


-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Fri, Oct 28 2016, Baolin Wang wrote:

>>
>> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>>   When the extcon detects an SDP, it will be called to set the state
>>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>>   it happened to be before, which is probably wrong.
>
> Sorry, I did not get your points here, could you please explain it explicitly?

usb_charger_get_current() is used to get the min/max current that is
supported.
In the case that an SDP (non-super-speed) has been detected it will
report the values sdp_min and sdp_max.  Ignoring usb_charger_set_current(),
sdp_max is set
 - to DEFAULT_SDP_CUR_MAX (500) at initializaion
 - to cur_limit when usb_charger_set_cur_limit_by_gadget() is called
     which happens after USB negotiation, once an allowed vbus_draw is
     negotiated.

This means that the first time you plug in an SDP cable, the reported
max will be 500, even though nothing has been negotiated.   The maximum
before negotiation is much less than that  - I don't remember exactly
how much.

If negotiation completes, the sdp_max will be set to whatever was
negotiated.  Maybe 200mA.
If you unplug, and then plug another SDP cable in, the sdp_max will
still be 200mA - different from the first time, but still not correct.
It will remain incorrect until (and unless) USB negotiation completes.

>
>>   When after USB negotiation completes,
>>   usb_charger_set_cur_limit_by_gadget()
>>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>>   again, but with a new current.  This will be ignored, as the state is
>>   already USB_CHARGER_PRESENT.
>
> No, we will notify the user the current has been changed by one work.

I can see no evidence in the code to justify this assertion, and you
didn't even try to provide any.

>
>>
>> 4/ I still strongly object to the ->get_charger_type() interface.
>>  You previously said:
>>
>>      No. User can implement the get_charger_type() method to access the
>>      PMIC registers to get the charger type, which is one very common method.
>>
>>  I suggest that if the PMIC registers report the charger type, then the
>>  PMIC driver should register an EXTCON and report the charger type
>>  through that.  Then the information would be directly available to
>>  user-space, and the usb-charger framework would have a single uniform
>>  mechanism for being told the cable type.
>
> We just access only one PMIC register to get the charger type, which
> is no need add one driver for that and there are no any events for
> extcon. Some sample code in power driver can be like below:

If there are no events, then how do you know when a charger has been
plugged in?  Do you poll?

In any case, one of the major values provided by using an OS like Linux
is uniform interfaces.  If a device can detect what sort of cable is
inserted, then that information should be presented as an EXTCON.

>>
>>  Related:  I don't like charger_type_show().  I don't think
>>  the usb-charger should export that information to user-space because
>>  extcon already does that, and duplication is confusing and pointless.
>
> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

Yes and no.

Certainly a uniform consistent interface should be presented.
"a usb charger" is not the right abstraction.  It is not a thing that
should have independent existence.  To everybody else in the world, a
"usb charger" in a box that you plug into the wall, and which has a
cable to plug into your device.  It is not part of the device itself.
In general, you cannot point to any component in a device that is the
"usb charger" so it isn't clear that Linux should even know about a "usb
charger".

There is a battery-charger which can take whatever current is available
and feed it to the battery.  It may well be appropriate for user-space
to have fine control of the current that this uses quite independently
of whatever is plugged in (I have a board which can get the current via
USB or via a more direct connection).

There is also a USB PHY which can detect when a cable is plugged in
(possibly just because 5V appears on VBUS) and can usually detect some
details of the cable.  It should report, via the EXTCON interface, the
presence and type of the cable.

Maybe these are all in the one integrated circuit, maybe not. On the
board I have, the one IC includes the USB phy, the battery charger, the
audio codec, some regulators, some GPIOs and other stuff.  We have
separate drivers for each logical component, unified by an "mfd" driver.

From the interface design perspective, the number of ICs doesn't matter
at all.  The interface presented, both within the kernel and out to
user-space, should be consistent.  Every USB port should present with an
EXTCON, and if it can detect types of cables, those cable types should
be visible in the extcon.

>
>>
>>  And I just noticed you have a ->charger_detect() too, which seems
>>  identical to ->get_charger_type().  There is no documentation
>>  explaining the difference.
>
> I think the kernel doc have explained that, but I like to explain it
> again. Since we can detect the charger by software or hardware (like
> PMIC), if you need to detect your charger type by software, then you
> can implement this callback, you can refer to Jun's patch:
> http://www.spinics.net/lists/linux-usb/msg139808.html

The kernel-doc says:

 + * @get_charger_type: User can get charger type by implementing this callback.
 + * @charger_detect: Charger detection method can be implemented if you need to
 + * manually detect the charger type.

To me, that doesn't say anything useful.  What is the difference between
"get charger type" and "manually detect the charger type" ??

I don't want to have to refer to some extra set of patches to guess how
something is supposed to work.  It needs to be clearly and
unambiguously documented.

However, I'm very quickly losing interest in this whole debate, partly
because it misses a key point.  As I have said, I strongly feel that
resolving the current inconsistencies in the use of
usb_register_notifier() is an important preliminary to resolve this
issue, as that is *already* sometimes used to communication available
current.

So I won't be responding on this topic any further until I see a genuine
attempt to understand and resolve the inconsistencies with
usb_register_notifier().

NeilBrown

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

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

[Baolin Wang]

Hi,

On 31 October 2016 at 08:00, NeilBrown <neilb@suse.com> wrote:
> On Fri, Oct 28 2016, Baolin Wang wrote:
>
>>>
>>> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>>>   When the extcon detects an SDP, it will be called to set the state
>>>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>>>   it happened to be before, which is probably wrong.
>>
>> Sorry, I did not get your points here, could you please explain it explicitly?
>
> usb_charger_get_current() is used to get the min/max current that is
> supported.
> In the case that an SDP (non-super-speed) has been detected it will
> report the values sdp_min and sdp_max.  Ignoring usb_charger_set_current(),
> sdp_max is set
>  - to DEFAULT_SDP_CUR_MAX (500) at initializaion
>  - to cur_limit when usb_charger_set_cur_limit_by_gadget() is called
>      which happens after USB negotiation, once an allowed vbus_draw is
>      negotiated.
>
> This means that the first time you plug in an SDP cable, the reported
> max will be 500, even though nothing has been negotiated.   The maximum
> before negotiation is much less than that  - I don't remember exactly
> how much.
>
> If negotiation completes, the sdp_max will be set to whatever was
> negotiated.  Maybe 200mA.
> If you unplug, and then plug another SDP cable in, the sdp_max will
> still be 200mA - different from the first time, but still not correct.
> It will remain incorrect until (and unless) USB negotiation completes.

Yes. I need some modification to reset current to default values when
cable unplugged.

>
>>
>>>   When after USB negotiation completes,
>>>   usb_charger_set_cur_limit_by_gadget()
>>>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>>>   again, but with a new current.  This will be ignored, as the state is
>>>   already USB_CHARGER_PRESENT.
>>
>> No, we will notify the user the current has been changed by one work.
>
> I can see no evidence in the code to justify this assertion, and you
> didn't even try to provide any.

We have one work to notify the user the current has been changed,
please see usb_charger_notify_work().

>
>>
>>>
>>> 4/ I still strongly object to the ->get_charger_type() interface.
>>>  You previously said:
>>>
>>>      No. User can implement the get_charger_type() method to access the
>>>      PMIC registers to get the charger type, which is one very common method.
>>>
>>>  I suggest that if the PMIC registers report the charger type, then the
>>>  PMIC driver should register an EXTCON and report the charger type
>>>  through that.  Then the information would be directly available to
>>>  user-space, and the usb-charger framework would have a single uniform
>>>  mechanism for being told the cable type.
>>
>> We just access only one PMIC register to get the charger type, which
>> is no need add one driver for that and there are no any events for
>> extcon. Some sample code in power driver can be like below:
>
> If there are no events, then how do you know when a charger has been
> plugged in?  Do you poll?

We just monitor the plug-in and plug-out events by extcon,  and get
the charger type by accessing PMIC registers when one cable is
plugged.

>
> In any case, one of the major values provided by using an OS like Linux
> is uniform interfaces.  If a device can detect what sort of cable is
> inserted, then that information should be presented as an EXTCON.

Fine. I can remove this callback. We can add it if we need it in future.

>
>>>
>>>  Related:  I don't like charger_type_show().  I don't think
>>>  the usb-charger should export that information to user-space because
>>>  extcon already does that, and duplication is confusing and pointless.
>>
>> I think we should combine all charger related information into one
>> place for user. Moreover if we don't get charger type from extcon, we
>> should also need one place to export the charger type.
>
> Yes and no.
>
> Certainly a uniform consistent interface should be presented.
> "a usb charger" is not the right abstraction.  It is not a thing that
> should have independent existence.  To everybody else in the world, a
> "usb charger" in a box that you plug into the wall, and which has a
> cable to plug into your device.  It is not part of the device itself.
> In general, you cannot point to any component in a device that is the
> "usb charger" so it isn't clear that Linux should even know about a "usb
> charger".

Yes,  we agree that 'usb charger' is not one actual device, and 'usb
charger' is depended on gadget device. Moreover these charger
information is associated with actual gadget device, not virtual usb
charger device.

>
> There is a battery-charger which can take whatever current is available
> and feed it to the battery.  It may well be appropriate for user-space
> to have fine control of the current that this uses quite independently
> of whatever is plugged in (I have a board which can get the current via
> USB or via a more direct connection).
>
> There is also a USB PHY which can detect when a cable is plugged in
> (possibly just because 5V appears on VBUS) and can usually detect some
> details of the cable.  It should report, via the EXTCON interface, the
> presence and type of the cable.
>
> Maybe these are all in the one integrated circuit, maybe not. On the
> board I have, the one IC includes the USB phy, the battery charger, the
> audio codec, some regulators, some GPIOs and other stuff.  We have
> separate drivers for each logical component, unified by an "mfd" driver.
>
> From the interface design perspective, the number of ICs doesn't matter
> at all.  The interface presented, both within the kernel and out to
> user-space, should be consistent.  Every USB port should present with an
> EXTCON, and if it can detect types of cables, those cable types should
> be visible in the extcon.
>
>>
>>>
>>>  And I just noticed you have a ->charger_detect() too, which seems
>>>  identical to ->get_charger_type().  There is no documentation
>>>  explaining the difference.
>>
>> I think the kernel doc have explained that, but I like to explain it
>> again. Since we can detect the charger by software or hardware (like
>> PMIC), if you need to detect your charger type by software, then you
>> can implement this callback, you can refer to Jun's patch:
>> http://www.spinics.net/lists/linux-usb/msg139808.html
>
> The kernel-doc says:
>
>  + * @get_charger_type: User can get charger type by implementing this callback.
>  + * @charger_detect: Charger detection method can be implemented if you need to
>  + * manually detect the charger type.
>
> To me, that doesn't say anything useful.  What is the difference between
> "get charger type" and "manually detect the charger type" ??
>
> I don't want to have to refer to some extra set of patches to guess how
> something is supposed to work.  It needs to be clearly and
> unambiguously documented.

OK. I will update the kernel doc to make it clear.

>
> However, I'm very quickly losing interest in this whole debate, partly
> because it misses a key point.  As I have said, I strongly feel that
> resolving the current inconsistencies in the use of
> usb_register_notifier() is an important preliminary to resolve this
> issue, as that is *already* sometimes used to communication available
> current.
>
> So I won't be responding on this topic any further until I see a genuine
> attempt to understand and resolve the inconsistencies with
> usb_register_notifier().

Any better solution?

-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Tue, Nov 01 2016, Baolin Wang wrote:


>> So I won't be responding on this topic any further until I see a genuine
>> attempt to understand and resolve the inconsistencies with
>> usb_register_notifier().
>
> Any better solution?

I'm not sure exactly what you are asking, so I'll assume you are asking
the question I want to answer :-)

1/ Liase with the extcon developers to resolve the inconsistencies
  with USB connector types.
  e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
  which both seem to suggest a standard downstream port.  There is no
  documentation describing how these relate, and no consistent practice
  to copy.
  I suspect the intention is that
    EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
    the cable, while EXTCON_CHG_USB* indicate the power capabilities of
    the cable. 
    So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
    while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
    would normally appear with EXTCON_USB_HOST (I think).
  Some drivers follow this model, particularly extcon-max14577.c
  but it is not consistent.

  This policy should be well documented and possibly existing drivers
  should be updated to follow it.

  At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
  and EXTCON_CHG_USB_FAST.  These names don't mean much.
  They were recently removed from drivers/power/axp288_charger.c
  which is good, but are still used in drivers/extcon/extcon-max*
  Possibly they should be changed to names from the standard, or
  possibly they should be renamed to identify the current they are
  expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A

2/ Change all usb phys to register an extcon and to send appropriate
   notifications.  Many already do, but I don't think it is universal.
   It is probable that the extcon should be registered using common code
   instead of each phy driver having its own
   extcon_get_edev_by_phandle()
   or whatever.
   If the usb phy driver needs to look at battery charger registers to
   know what sort of cable was connected (which I believe is the case
   for the chips you are interested in), then it should do that.

3/ Currently some USB controllers discover that a cable was connected by
   listening on an extcon, and some by registering for a usb_notifier
   (described below) ... though there seem to only be 2 left which do that.
   Now that all USB phys send connection information via extcon (see 2),
   the USB controllers should be changed to all find out about the cable
   using extcon.

4/ struct usb_phy contains:
        /* for notification of usb_phy_events */
        struct atomic_notifier_head     notifier;

  This is used inconsistently.  Sometimes the argument passed
  is NULL, sometimes it is a pointer to 'vbus_draw' - the current
  limited negotiated via USB, sometimes it is a pointer the the gadget
  though as far as I can tell, that last one is never used.

  This should be changed to be consistent.  This notifier is no longer
  needed to tell the USB controller that a cable was connected (extcon
  now does that, see 3) so it is only used to communicate the
  'vbus_draw' information.
  So it should be changed to *only* send a notification when vbus_draw
  is known, and it should carry that information.
  This should probably be done in common code, and removed
  from individual drivers.

5/ Now that all cable connection notifications are sent over extcon and
   all vbus_draw notifications are sent over the usb_phy notifier, write
   some support code that a power supply client can use to be told what
   power is available.
   e.g. a battery charger driver would call:
       register_power_client(.....)
   or similar, providing a phandle (or similar) for the usb phy and a
   function to call back when the available current changes (or maybe a
   work_struct containing the function pointer)

   register_power_client() would then register with extcon and separately
   with the usb_phy notifier.  When the different events arrive it
   calculates what ranges of currents are expected and calls the
   call-back function with those details.

6/ Any battery charger that needs to know the available current can now
   call register_power_client() and get the information delivered.

NeilBrown

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

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

[Baolin Wang]

On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:
>
>
>>> So I won't be responding on this topic any further until I see a genuine
>>> attempt to understand and resolve the inconsistencies with
>>> usb_register_notifier().
>>
>> Any better solution?
>
> I'm not sure exactly what you are asking, so I'll assume you are asking
> the question I want to answer :-)
>
> 1/ Liase with the extcon developers to resolve the inconsistencies
>   with USB connector types.
>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>   which both seem to suggest a standard downstream port.  There is no
>   documentation describing how these relate, and no consistent practice
>   to copy.
>   I suspect the intention is that
>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>     the cable.
>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>     would normally appear with EXTCON_USB_HOST (I think).
>   Some drivers follow this model, particularly extcon-max14577.c
>   but it is not consistent.
>
>   This policy should be well documented and possibly existing drivers
>   should be updated to follow it.
>
>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>   They were recently removed from drivers/power/axp288_charger.c
>   which is good, but are still used in drivers/extcon/extcon-max*
>   Possibly they should be changed to names from the standard, or
>   possibly they should be renamed to identify the current they are
>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>
> 2/ Change all usb phys to register an extcon and to send appropriate
>    notifications.  Many already do, but I don't think it is universal.
>    It is probable that the extcon should be registered using common code
>    instead of each phy driver having its own
>    extcon_get_edev_by_phandle()
>    or whatever.
>    If the usb phy driver needs to look at battery charger registers to
>    know what sort of cable was connected (which I believe is the case
>    for the chips you are interested in), then it should do that.
>
> 3/ Currently some USB controllers discover that a cable was connected by
>    listening on an extcon, and some by registering for a usb_notifier
>    (described below) ... though there seem to only be 2 left which do that.
>    Now that all USB phys send connection information via extcon (see 2),
>    the USB controllers should be changed to all find out about the cable
>    using extcon.
>
> 4/ struct usb_phy contains:
>         /* for notification of usb_phy_events */
>         struct atomic_notifier_head     notifier;
>
>   This is used inconsistently.  Sometimes the argument passed
>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>   limited negotiated via USB, sometimes it is a pointer the the gadget
>   though as far as I can tell, that last one is never used.
>
>   This should be changed to be consistent.  This notifier is no longer
>   needed to tell the USB controller that a cable was connected (extcon
>   now does that, see 3) so it is only used to communicate the
>   'vbus_draw' information.
>   So it should be changed to *only* send a notification when vbus_draw
>   is known, and it should carry that information.
>   This should probably be done in common code, and removed
>   from individual drivers.
>
> 5/ Now that all cable connection notifications are sent over extcon and
>    all vbus_draw notifications are sent over the usb_phy notifier, write
>    some support code that a power supply client can use to be told what
>    power is available.
>    e.g. a battery charger driver would call:
>        register_power_client(.....)
>    or similar, providing a phandle (or similar) for the usb phy and a
>    function to call back when the available current changes (or maybe a
>    work_struct containing the function pointer)
>
>    register_power_client() would then register with extcon and separately
>    with the usb_phy notifier.  When the different events arrive it
>    calculates what ranges of currents are expected and calls the
>    call-back function with those details.
>
> 6/ Any battery charger that needs to know the available current can now
>    call register_power_client() and get the information delivered.

I agree with your most opinions, but these are optimization. Firstly I
think we should upstream the USB charger driver. What I want to ask is
how can we notify power driver if we don't set the
usb_register_notifier(), then I think you give the answer is: power
driver can register by 'struct usb_phy->notifier'. But why usb phy
should notify the power driver how much current should be drawn, and I
still think we should notify the current in usb charger driver which
is better, and do not need to notify current for power driver in usb
phy driver.

-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Mon, Nov 07 2016, Baolin Wang wrote:

> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>> On Tue, Nov 01 2016, Baolin Wang wrote:
>
> I agree with your most opinions, but these are optimization. 

I see them as bug fixes, not optimizations.

>                                                              Firstly I
> think we should upstream the USB charger driver.

I think you missed the point.  The point is that we don't *need* your
"USB charger driver" because all the infrastructure we need is *already*
present in the kernel.  It is buggy and not used uniformly, and could
usefully be polished and improved.  But the structure is already
present.

If everyone just added new infrastructure when they didn't like, or
didn't understand, what was already present, the kernel would become
like the Mad Hatter's tea party, full of dirty dishes.

>                                                  What I want to ask is
> how can we notify power driver if we don't set the
> usb_register_notifier(), then I think you give the answer is: power
> driver can register by 'struct usb_phy->notifier'. But why usb phy
> should notify the power driver how much current should be drawn, and I
> still think we should notify the current in usb charger driver which
> is better, and do not need to notify current for power driver in usb
> phy driver.

I accept that it isn't clear that the phy *should* be involved in
communicating the negotiated power availability, but nor is it clear
that it shouldn't.  The power does travel through the physical
interface, so physically it plays a role.

But more importantly, it already *does* get told (in some cases).
There is an interface "usb_phy_set_power()" which exists explicitly to
tell the phy what power has been negotiated.  Given that infrastructure
exists and works, it make sense to use it.

If you think it is a broken design and should be removed, then fine:
make a case for that.  Examine the history.  Make sure you know why it
is there (or make sure that information cannot be found), and then
present a case as to why it should be removed and replaced with
something else.  But don't just leave it there and pretend it doesn't
exist and create something similar-but-different and hope people will
know why yours is better.  That way lies madness.

NeilBrown

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

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

[Baolin Wang]

Hi

On 8 November 2016 at 04:36, NeilBrown <neilb@suse.com> wrote:
> On Mon, Nov 07 2016, Baolin Wang wrote:
>
>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>
>> I agree with your most opinions, but these are optimization.
>
> I see them as bug fixes, not optimizations.
>
>>                                                              Firstly I
>> think we should upstream the USB charger driver.
>
> I think you missed the point.  The point is that we don't *need* your
> "USB charger driver" because all the infrastructure we need is *already*
> present in the kernel.  It is buggy and not used uniformly, and could
> usefully be polished and improved.  But the structure is already
> present.
>
> If everyone just added new infrastructure when they didn't like, or
> didn't understand, what was already present, the kernel would become
> like the Mad Hatter's tea party, full of dirty dishes.
>
>>                                                  What I want to ask is
>> how can we notify power driver if we don't set the
>> usb_register_notifier(), then I think you give the answer is: power
>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>> should notify the power driver how much current should be drawn, and I
>> still think we should notify the current in usb charger driver which
>> is better, and do not need to notify current for power driver in usb
>> phy driver.
>
> I accept that it isn't clear that the phy *should* be involved in
> communicating the negotiated power availability, but nor is it clear
> that it shouldn't.  The power does travel through the physical
> interface, so physically it plays a role.
>
> But more importantly, it already *does* get told (in some cases).
> There is an interface "usb_phy_set_power()" which exists explicitly to
> tell the phy what power has been negotiated.  Given that infrastructure
> exists and works, it make sense to use it.
>
> If you think it is a broken design and should be removed, then fine:
> make a case for that.  Examine the history.  Make sure you know why it
> is there (or make sure that information cannot be found), and then
> present a case as to why it should be removed and replaced with
> something else.  But don't just leave it there and pretend it doesn't
> exist and create something similar-but-different and hope people will
> know why yours is better.  That way lies madness.

Like Peter said, it is not only PHY can detect the USB charger type,
which means there are other places can detect the charger type.
Second, some controller need to detect the charger type manually which
USB phy did not support. Third, it did not handle what current should
be drawn in USB phy. Fourth, we need integrate all charger plugin/out
event in one framework, not from extcon, maybe type-c in future. In a
word, we need one standard integration of this feature we need, though
like you said we should do some clean up or fix to make it better.

-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Thu, Nov 10 2016, Baolin Wang wrote:

> Hi
>
> On 8 November 2016 at 04:36, NeilBrown <neilb@suse.com> wrote:
>> On Mon, Nov 07 2016, Baolin Wang wrote:
>>
>>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>
>>> I agree with your most opinions, but these are optimization.
>>
>> I see them as bug fixes, not optimizations.
>>
>>>                                                              Firstly I
>>> think we should upstream the USB charger driver.
>>
>> I think you missed the point.  The point is that we don't *need* your
>> "USB charger driver" because all the infrastructure we need is *already*
>> present in the kernel.  It is buggy and not used uniformly, and could
>> usefully be polished and improved.  But the structure is already
>> present.
>>
>> If everyone just added new infrastructure when they didn't like, or
>> didn't understand, what was already present, the kernel would become
>> like the Mad Hatter's tea party, full of dirty dishes.
>>
>>>                                                  What I want to ask is
>>> how can we notify power driver if we don't set the
>>> usb_register_notifier(), then I think you give the answer is: power
>>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>>> should notify the power driver how much current should be drawn, and I
>>> still think we should notify the current in usb charger driver which
>>> is better, and do not need to notify current for power driver in usb
>>> phy driver.
>>
>> I accept that it isn't clear that the phy *should* be involved in
>> communicating the negotiated power availability, but nor is it clear
>> that it shouldn't.  The power does travel through the physical
>> interface, so physically it plays a role.
>>
>> But more importantly, it already *does* get told (in some cases).
>> There is an interface "usb_phy_set_power()" which exists explicitly to
>> tell the phy what power has been negotiated.  Given that infrastructure
>> exists and works, it make sense to use it.
>>
>> If you think it is a broken design and should be removed, then fine:
>> make a case for that.  Examine the history.  Make sure you know why it
>> is there (or make sure that information cannot be found), and then
>> present a case as to why it should be removed and replaced with
>> something else.  But don't just leave it there and pretend it doesn't
>> exist and create something similar-but-different and hope people will
>> know why yours is better.  That way lies madness.
>
> Like Peter said, it is not only PHY can detect the USB charger type,
> which means there are other places can detect the charger type.

If I understand Peter's example correctly, it shows a configuration
where the USB PHysical interface is partly implemented in the SOC and
partly in the PMIC.  I appreciate that would make it more challenging to
implement a PHY driver, but there is no reason it should impact anything
outside of the PHY.


> Second, some controller need to detect the charger type manually which
> USB phy did not support.

"manually" is an odd term to use in this context.
I think you mean that to detect the charger type you need to issue some
command to the hardware and wait for it to respond, then assess the
response.
That isn't at all surprising.  The charger type is detected by measuring
resistance between ID and GND, which may require setting up a potential
and activating ADCs to measure the voltage.  This can all be done
internally to the phy driver.
Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for
twl4030, though it never got upstream).
The code for the imx7d does look more complex, but not intrinsically
different.

> Third, it did not handle what current should
> be drawn in USB phy.

The standards define that.  The extcon just reports the cable type.
Certainly it would be sensible to provide a library function to
translate from cable type to current range.  You don't need a new
subsystem to do that, just a library function.

> Fourth, we need integrate all charger plugin/out
> event in one framework, not from extcon, maybe type-c in future.

Why not extcon?  Given that a charger is connected by an external
connector, extcon seems like exactly the right thing to use.

Obviously extcon doesn't report the current that was negotiated, but
that is best kept separate.  The battery charger can be advised of the
available current either via extcon or separately via the usb
subsystem.  Don't conflate the two.


>  In a
> word, we need one standard integration of this feature we need, though
> like you said we should do some clean up or fix to make it better.

But really, I'm not the person you need to convince.  I'm just a vaguely
interested bystander who is rapidly losing interest.  You need to
convince a maintainer, but they have so far shown remarkably little
interest.  I don't know why, but I'd guess that reviewing a complex new
subsystem isn't much fun.  Reviewing and applying clear bugfixes and
incremental improvements is much easier and more enjoyable.  But that is
just a guess.

NeilBrown

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

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

[Mark Brown]

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

On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
> On Thu, Nov 10 2016, Baolin Wang wrote:

> > Fourth, we need integrate all charger plugin/out
> > event in one framework, not from extcon, maybe type-c in future.

> Why not extcon?  Given that a charger is connected by an external
> connector, extcon seems like exactly the right thing to use.

> Obviously extcon doesn't report the current that was negotiated, but
> that is best kept separate.  The battery charger can be advised of the
> available current either via extcon or separately via the usb
> subsystem.  Don't conflate the two.

Conflating the two seems like the whole point here.  We're looking for
something that sits between the power supply code and the USB code and
tells the power supply code what it's allowed to do which is the result
of a combination of physical cable detection and USB protocol.  It seems
reasonable that extcon drivers ought to be part of this but it doesn't
seem like they are the whole story.

> > word, we need one standard integration of this feature we need, though
> > like you said we should do some clean up or fix to make it better.

> But really, I'm not the person you need to convince.  I'm just a vaguely
> interested bystander who is rapidly losing interest.  You need to
> convince a maintainer, but they have so far shown remarkably little
> interest.  I don't know why, but I'd guess that reviewing a complex new
> subsystem isn't much fun.  Reviewing and applying clear bugfixes and
> incremental improvements is much easier and more enjoyable.  But that is
> just a guess.

OTOH having someone else having reviewed might help them apply something
they don't care about.

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

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

[NeilBrown]

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

On Mon, Nov 14 2016, Mark Brown wrote:

> On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 10 2016, Baolin Wang wrote:
>
>> > Fourth, we need integrate all charger plugin/out
>> > event in one framework, not from extcon, maybe type-c in future.
>
>> Why not extcon?  Given that a charger is connected by an external
>> connector, extcon seems like exactly the right thing to use.
>
>> Obviously extcon doesn't report the current that was negotiated, but
>> that is best kept separate.  The battery charger can be advised of the
>> available current either via extcon or separately via the usb
>> subsystem.  Don't conflate the two.
>
> Conflating the two seems like the whole point here.  We're looking for
> something that sits between the power supply code and the USB code and
> tells the power supply code what it's allowed to do which is the result
> of a combination of physical cable detection and USB protocol.  It seems
> reasonable that extcon drivers ought to be part of this but it doesn't
> seem like they are the whole story.

I don't think "between the power supply code and the USB code" is where
this thing sits. I think it sits inside the power-supply driver.
We already have extcon which sits between the phy and the power_supply
code, and the usb_notifier which sits between the USB code and the
power supply code.  We don't need another go-between.

If we have extcon able to deliver reliable information about cable type,
and if with have the usb notifier able to deliver reliable information
about negotiated current, and if the power supply manager is able to
register with the correct extcon and the correct usb notifier, then the
power supply manager *could* handle all the notifications and make the
correct determinations and set the current limits itself.  All this
could be done entirely internally, without the help of any new
subsystem.
Do you agree?

Clearly doing it that way would result in lots of code duplication and
would mean that each driver probably gets its own private set of bugs,
but it would be possible.  Just undesirable.

So yes, it makes perfect to provide common code which handles the
registrations, and captures the events, and translates the different
events into current levels and feeds those back to the driver.  This
isn't some new subsystem, this is just a resource, provided by a
library, that power drivers can allocate and initialize if the want to.

To quote myself:

> 5/ Now that all cable connection notifications are sent over extcon and
>    all vbus_draw notifications are sent over the usb_phy notifier, write
>    some support code that a power supply client can use to be told what
>    power is available.
>    e.g. a battery charger driver would call:
>        register_power_client(.....)
>    or similar, providing a phandle (or similar) for the usb phy and a
>    function to call back when the available current changes (or maybe a
>    work_struct containing the function pointer)
> 
>    register_power_client() would then register with extcon and separately
>    with the usb_phy notifier.  When the different events arrive it
>    calculates what ranges of currents are expected and calls the
>    call-back function with those details.

NeilBrown

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

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

[Peter Chen]

On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
> On Mon, Nov 14 2016, Mark Brown wrote:
> 
> > On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
> >> On Thu, Nov 10 2016, Baolin Wang wrote:
> >
> >> > Fourth, we need integrate all charger plugin/out
> >> > event in one framework, not from extcon, maybe type-c in future.
> >
> >> Why not extcon?  Given that a charger is connected by an external
> >> connector, extcon seems like exactly the right thing to use.
> >
> >> Obviously extcon doesn't report the current that was negotiated, but
> >> that is best kept separate.  The battery charger can be advised of the
> >> available current either via extcon or separately via the usb
> >> subsystem.  Don't conflate the two.
> >
> > Conflating the two seems like the whole point here.  We're looking for
> > something that sits between the power supply code and the USB code and
> > tells the power supply code what it's allowed to do which is the result
> > of a combination of physical cable detection and USB protocol.  It seems
> > reasonable that extcon drivers ought to be part of this but it doesn't
> > seem like they are the whole story.
> 
> I don't think "between the power supply code and the USB code" is where
> this thing sits. I think it sits inside the power-supply driver.
> We already have extcon which sits between the phy and the power_supply
> code, and the usb_notifier which sits between the USB code and the
> power supply code.  We don't need another go-between.
> 
> If we have extcon able to deliver reliable information about cable type,
> and if with have the usb notifier able to deliver reliable information
> about negotiated current, and if the power supply manager is able to
> register with the correct extcon and the correct usb notifier, then the
> power supply manager *could* handle all the notifications and make the
> correct determinations and set the current limits itself.  All this
> could be done entirely internally, without the help of any new
> subsystem.
> Do you agree?

Through the USB gadget/phy framework (usb_gadget.vbus_draw->usb_phy.set_power)
we can get the USB bus information when the device connects SDP, but the
enum usb_phy_events lacks some events like bus suspend (2mA), and bus
speed (high/super speed, 500mA vs 900mA). Besides many USB PHYs use
generic PHY driver now, it is lack of above event and related notifier.

About getting cable type, the key points are detect vbus and negotiate
the charger type, these two stuffs are much different among platforms.
Extcon has charger type definition, it is good, we can use it.
But it needs the device which has charger detection function as extcon
device too, and at meanwhile, this device needs to have vbus detect
function, most pmic devices are suitable for that, but not for USB PHY.

Asssume wm831x as a power client, according your suggestion, does its
design like below?
At dts, it needs to be described like below:
&wm831x {
	...
	phy-dev = <&usb_phy>;
	extcon-dev = <&extcon>;
	...
}
And at wm831x driver, it gets information through extcon-dev and phy-dev
notifier, and it needs knowledge about current limit for specific
cable type, but these information are from USB (Charger) specification.

Your suggestion is trying use current notifications to get the
information for power client, this patch set is trying to keep
these two notifications at an new framework, and power client
gets refined notification from this new framework.

The biggest problem I concern about your solution is extcon device, it may
not be an universal solution, does current frameworks have a way to
get cable type (usb charger type)? If not, we may need to have a new
framework.

-- 

Best Regards,
Peter Chen

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

[Mark Brown]

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

On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
> On Mon, Nov 14 2016, Mark Brown wrote:

> > Conflating the two seems like the whole point here.  We're looking for
> > something that sits between the power supply code and the USB code and
> > tells the power supply code what it's allowed to do which is the result
> > of a combination of physical cable detection and USB protocol.  It seems
> > reasonable that extcon drivers ought to be part of this but it doesn't
> > seem like they are the whole story.

> I don't think "between the power supply code and the USB code" is where
> this thing sits. I think it sits inside the power-supply driver.
> We already have extcon which sits between the phy and the power_supply
> code, and the usb_notifier which sits between the USB code and the
> power supply code.  We don't need another go-between.

...

> correct determinations and set the current limits itself.  All this
> could be done entirely internally, without the help of any new
> subsystem.
> Do you agree?

> Clearly doing it that way would result in lots of code duplication and
> would mean that each driver probably gets its own private set of bugs,
> but it would be possible.  Just undesirable.

I think this is the key here - the fact that it's technically possible
to implement doesn't really matter if it's sufficiently fiddly and non
obvious that nobody is actually joining everything up (bits are done
intermittently but not as a whole as far as I can see).

> So yes, it makes perfect to provide common code which handles the
> registrations, and captures the events, and translates the different
> events into current levels and feeds those back to the driver.  This
> isn't some new subsystem, this is just a resource, provided by a
> library, that power drivers can allocate and initialize if the want to.

To me that's pretty much what's being done here, the code just happens
to sit in USB instead but fundamentally it's just a blob of helper code,
you could replace the notifier with a callback if that's the big concern
here.

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

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

[NeilBrown]

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

On Thu, Nov 17 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
>> On Mon, Nov 14 2016, Mark Brown wrote:
>
>> > Conflating the two seems like the whole point here.  We're looking for
>> > something that sits between the power supply code and the USB code and
>> > tells the power supply code what it's allowed to do which is the result
>> > of a combination of physical cable detection and USB protocol.  It seems
>> > reasonable that extcon drivers ought to be part of this but it doesn't
>> > seem like they are the whole story.
>
>> I don't think "between the power supply code and the USB code" is where
>> this thing sits. I think it sits inside the power-supply driver.
>> We already have extcon which sits between the phy and the power_supply
>> code, and the usb_notifier which sits between the USB code and the
>> power supply code.  We don't need another go-between.
>
> ...
>
>> correct determinations and set the current limits itself.  All this
>> could be done entirely internally, without the help of any new
>> subsystem.
>> Do you agree?
>
>> Clearly doing it that way would result in lots of code duplication and
>> would mean that each driver probably gets its own private set of bugs,
>> but it would be possible.  Just undesirable.
>
> I think this is the key here - the fact that it's technically possible
> to implement doesn't really matter if it's sufficiently fiddly and non
> obvious that nobody is actually joining everything up (bits are done
> intermittently but not as a whole as far as I can see).
>
>> So yes, it makes perfect to provide common code which handles the
>> registrations, and captures the events, and translates the different
>> events into current levels and feeds those back to the driver.  This
>> isn't some new subsystem, this is just a resource, provided by a
>> library, that power drivers can allocate and initialize if the want to.
>
> To me that's pretty much what's being done here, the code just happens
> to sit in USB instead but fundamentally it's just a blob of helper code,
> you could replace the notifier with a callback if that's the big concern
> here.

It is a lot more than "just a blob of helper code".  It duplicates
existing infrastructure instead of fixing and using the
infrastructure.... but I've said all this before.  Repeatedly.

NeilBrown

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

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

[Mark Brown]

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

On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
> On Thu, Nov 17 2016, Mark Brown wrote:

> > To me that's pretty much what's being done here, the code just happens
> > to sit in USB instead but fundamentally it's just a blob of helper code,
> > you could replace the notifier with a callback if that's the big concern
> > here.

> It is a lot more than "just a blob of helper code".  It duplicates
> existing infrastructure instead of fixing and using the
> infrastructure.... but I've said all this before.  Repeatedly.

My read on that is that the question of what we want to be responsible
for aggregating the information about what power the system is allowed
to draw from a given USB port hasn't been resolved yet and that apart
from that you're fairly close.  It seems to me like that's really what
the difference between your two positions is.  Fixing the existing
notifiers implies that things have to be aggregated in the power supply
drivers but Baolin is proposing doing that in the USB code instead.  It
does seem at least worth considering if that's a good idea since the
current situation doesn't look terribly thought through.

There are a whole bunch of things that need to be sorted out whatever
the decision is like the extcon related cleanups you mentioned in your
mail the other day (steps 1 and 2), it seems like those could be moved
forwards independently.

By the way it occurred to me recently that we have a use case for
multiple USB ports that could supply power - USB C.  Things with more
than one port like things in a laptop form factor are going to want to
be able to use all of them interchangably for power support (likely only
one at a time, at least initially, but still more than one port).

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

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

[NeilBrown]

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

On Tue, Nov 22 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 17 2016, Mark Brown wrote:
>
>> > To me that's pretty much what's being done here, the code just happens
>> > to sit in USB instead but fundamentally it's just a blob of helper code,
>> > you could replace the notifier with a callback if that's the big concern
>> > here.
>
>> It is a lot more than "just a blob of helper code".  It duplicates
>> existing infrastructure instead of fixing and using the
>> infrastructure.... but I've said all this before.  Repeatedly.
>
> My read on that is that the question of what we want to be responsible
> for aggregating the information about what power the system is allowed
> to draw from a given USB port hasn't been resolved yet and that apart
> from that you're fairly close.  It seems to me like that's really what
> the difference between your two positions is.  Fixing the existing
> notifiers implies that things have to be aggregated in the power supply
> drivers but Baolin is proposing doing that in the USB code instead.  It
> does seem at least worth considering if that's a good idea since the
> current situation doesn't look terribly thought through.

I agree that the question of where the responsibility for information
aggregation lies is open for discussion. If fact all details on how
things should work are always open for discussion.
I don't agree that this is the main different between our positions,
though I can see how you might get that impression.

I don't agree that "Fixing the existing notifiers implies that things
have to be aggregated in the power supply drivers".  That would be the
simplest way to fix them, but not the only way.  You could fix them so
that the usb driver sends notifications instead of the phy, and you
could fix them so that the information contained in the notification
being a power range instead the current ad-hoc inconsistent set of
information.

You could even fix them so they look *exactly* like the notifiers that
Baolin is proposing.  This is my key point.  It is not the end result
that I particularly object to (though I do object to some details).  It
is the process of getting to the end result that I don't like.  If the
current system doesn't work and something different is needed, then the
correct thing to do is to transform the existing system into something
new that works better.  This should be a clear series of steps.  Each
one makes a small, well defined, change.  Maybe it adds a new feature.
Maybe it refactors code without changing functionality.  Maybe it fixes a
bug.  Maybe it moves the responsibility for an action from *here* to
*there*.  Each step is clearly explained and convincingly justified.
This doesn't have to be a long justification.  Sometimes a single
sentence or a short paragraph is plenty.  Sometimes the change is so
obviously an improvement that no explanation is necessary.

Changing one step at a time make it easier to ensure that no
functionality is lost and that each change is actually needed.

If the patch series fixed the existing code and transformed it into
something better and more powerful, then it would be a lot easier to
have sensible discussions about individual patches, about which there
might be disagreement.

>
> There are a whole bunch of things that need to be sorted out whatever
> the decision is like the extcon related cleanups you mentioned in your
> mail the other day (steps 1 and 2), it seems like those could be moved
> forwards independently.

Agreed.

>
> By the way it occurred to me recently that we have a use case for
> multiple USB ports that could supply power - USB C.  Things with more
> than one port like things in a laptop form factor are going to want to
> be able to use all of them interchangably for power support (likely only
> one at a time, at least initially, but still more than one port).

Thanks!  It is so much easier to discusses these sorts of things where
there is a concrete reference point.

I wonder if each port would have its own current regulator, or if there
would be a single central one.  Maybe there would be a switch that
allowed power to flow in only from one port.
Would each "charger" need to get notifications from "its" port, or would
there be a single "charger" which got notifications from the
power-switch, which in-turn got notifications from each port?
Or would the battery-charger driver want to register with every port,
receive notifications, and be able to turn each one on or off?
Without concrete details of actual hardware, we can only guess.

But I think here my key point got lost too, in part because it was hard
to refer to an actual instance.
My point was that in the present patch set, the "usb charger" is given
a name which is dependant on discovery order, and only supports
lookup-by-name.  This cannot work.
If each USB-C port registers a usb_charger, they would be
"usb-charger.0", "usb-charger.1", "usb-charger.2", with no reliable
correlation between name and physical location.
If they supported lookup by phy-name or lookup-by-active (i.e. "find me
any usb-charger which has power available"), or look up by some other
attribute, then discover-order naming could work.  But the only
lookup-mechanism is by-name, and the names aren't reliably stable.  So
the name/lookup system proposed cannot possibly do anything useful
with more than one usb_charger.

Thanks,
NeilBrown

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

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

[Mark Brown]

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

On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:

> I agree that the question of where the responsibility for information
> aggregation lies is open for discussion. If fact all details on how
> things should work are always open for discussion.
> I don't agree that this is the main different between our positions,
> though I can see how you might get that impression.

> You could even fix them so they look *exactly* like the notifiers that
> Baolin is proposing.  This is my key point.  It is not the end result
> that I particularly object to (though I do object to some details).  It

Ah, OK.  This really hadn't been at all clear - both Baolin and I had
the impression that the it was both that were blockers for you.  What
were the details here?

> is the process of getting to the end result that I don't like.  If the
> current system doesn't work and something different is needed, then the
> correct thing to do is to transform the existing system into something
> new that works better.  This should be a clear series of steps.  Each

Sometimes there's something to be said for working out what we want
things to look like before setting out to make these gradual
refactorings and sometimes the refactorings are just more pain than
they're worth, especially when they go across subsystems.  In this case
I do worry about the cross subsystem aspect causing hassle, it may be
more practical to do anything that represents an interface change by
adding the new interface, converting the users to it and then removing
the old interface.

At the very least the series should grow to incorporate conversion of
the existing users though.  Baolin, I think this does need adding to the
series but probably best to think about how to do it - some of Neil's
suggestions for incremental steps do seem like they should be useful
for organizing things here, probably we can get some things that can be
done internally within individual drivers merged while everything else
is under discussion.

> But I think here my key point got lost too, in part because it was hard
> to refer to an actual instance.
> My point was that in the present patch set, the "usb charger" is given
> a name which is dependant on discovery order, and only supports
> lookup-by-name.  This cannot work.

There's two bits here: one is the way names are assigned and the other
is the lookup by name.  I agree that the lookup by name isn't
particularly useful as things stand, that could just be dropped until
some naming mechanism is added.  We'd be more likely to use phandles in
DT systems, I don't know what ACPI systems would look like but I guess
it'd be something similar.

> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
> any usb-charger which has power available"), or look up by some other
> attribute, then discover-order naming could work.  But the only
> lookup-mechanism is by-name, and the names aren't reliably stable.  So
> the name/lookup system proposed cannot possibly do anything useful
> with more than one usb_charger.

Baolin, I think adding a DT binding and lookup mechanism makes sense
here - do you agree?

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

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

[NeilBrown]

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

On Sat, Nov 26 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:
>
>> I agree that the question of where the responsibility for information
>> aggregation lies is open for discussion. If fact all details on how
>> things should work are always open for discussion.
>> I don't agree that this is the main different between our positions,
>> though I can see how you might get that impression.
>
>> You could even fix them so they look *exactly* like the notifiers that
>> Baolin is proposing.  This is my key point.  It is not the end result
>> that I particularly object to (though I do object to some details).  It
>
> Ah, OK.  This really hadn't been at all clear - both Baolin and I had
> the impression that the it was both that were blockers for you.  What
> were the details here?

I don't really like the idea of a separate "usb charger" object.  It
looks too much like a "midlayer" and they tend to get in the way.  But
if a convincing case could be made that changing from the current design
to that aspect of the proposed design brings measurable benefits, then I
would certainly assess that case on its merits.  No such case was made,
and the patchset didn't seem to even acknowledge the existing design.

When I said "I do object to some details" it was details of the end
result, not details of what took responsibility of information
aggregation (in case that wasn't clear).  Those details were everything
that duplicated existing functionality, or ignored existing
functionality, or was simply unworkable.  e.g. the lack of proper
integration with extcon, the new sysfs attributes, the name-lookup
mechanism.  Probably others.

>
>> is the process of getting to the end result that I don't like.  If the
>> current system doesn't work and something different is needed, then the
>> correct thing to do is to transform the existing system into something
>> new that works better.  This should be a clear series of steps.  Each
>
> Sometimes there's something to be said for working out what we want
> things to look like before setting out to make these gradual
> refactorings and sometimes the refactorings are just more pain than
> they're worth, especially when they go across subsystems.  In this case
> I do worry about the cross subsystem aspect causing hassle, it may be
> more practical to do anything that represents an interface change by
> adding the new interface, converting the users to it and then removing
> the old interface.

Yes, you need a clear vision of the goal.  You also need a clear vision
of the starting point. There was no evidence of the latter.
Yes, sometimes you need to create a new thing and transition users over,
then discard the old.  There was no discarding of the old.

>
> At the very least the series should grow to incorporate conversion of
> the existing users though.  Baolin, I think this does need adding to the
> series but probably best to think about how to do it - some of Neil's
> suggestions for incremental steps do seem like they should be useful
> for organizing things here, probably we can get some things that can be
> done internally within individual drivers merged while everything else
> is under discussion.

I would be very encouraged to see those simple things done first!
Seeing the series grow isn't much fun, but seeing preliminary work land
certainly is.

>
>> But I think here my key point got lost too, in part because it was hard
>> to refer to an actual instance.
>> My point was that in the present patch set, the "usb charger" is given
>> a name which is dependant on discovery order, and only supports
>> lookup-by-name.  This cannot work.
>
> There's two bits here: one is the way names are assigned and the other
> is the lookup by name.  I agree that the lookup by name isn't
> particularly useful as things stand, that could just be dropped until
> some naming mechanism is added.  We'd be more likely to use phandles in
> DT systems, I don't know what ACPI systems would look like but I guess
> it'd be something similar.
>
>> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
>> any usb-charger which has power available"), or look up by some other
>> attribute, then discover-order naming could work.  But the only
>> lookup-mechanism is by-name, and the names aren't reliably stable.  So
>> the name/lookup system proposed cannot possibly do anything useful
>> with more than one usb_charger.
>
> Baolin, I think adding a DT binding and lookup mechanism makes sense
> here - do you agree?

We already have a lookup mechanism for a battery charger to find the phy
that it gets current from: devm_usb_get_phy_by_phandle() (or even
devm_usb_get_phy() if there is known to only be one phy).  We would need
a case to be made that the existing mechanism cannot be used before we
consider "adding a DT binding and lookup mechanism".

Thanks,
NeilBrown

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

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

[Baolin Wang]

On 25 November 2016 at 21:00, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:
>
>> I agree that the question of where the responsibility for information
>> aggregation lies is open for discussion. If fact all details on how
>> things should work are always open for discussion.
>> I don't agree that this is the main different between our positions,
>> though I can see how you might get that impression.
>
>> You could even fix them so they look *exactly* like the notifiers that
>> Baolin is proposing.  This is my key point.  It is not the end result
>> that I particularly object to (though I do object to some details).  It
>
> Ah, OK.  This really hadn't been at all clear - both Baolin and I had
> the impression that the it was both that were blockers for you.  What
> were the details here?
>
>> is the process of getting to the end result that I don't like.  If the
>> current system doesn't work and something different is needed, then the
>> correct thing to do is to transform the existing system into something
>> new that works better.  This should be a clear series of steps.  Each
>
> Sometimes there's something to be said for working out what we want
> things to look like before setting out to make these gradual
> refactorings and sometimes the refactorings are just more pain than
> they're worth, especially when they go across subsystems.  In this case
> I do worry about the cross subsystem aspect causing hassle, it may be
> more practical to do anything that represents an interface change by
> adding the new interface, converting the users to it and then removing
> the old interface.
>
> At the very least the series should grow to incorporate conversion of
> the existing users though.  Baolin, I think this does need adding to the
> series but probably best to think about how to do it - some of Neil's
> suggestions for incremental steps do seem like they should be useful
> for organizing things here, probably we can get some things that can be
> done internally within individual drivers merged while everything else
> is under discussion.

OK. I will think about how to incorporate conversion of the existing
users according to Neil's suggestion.

>
>> But I think here my key point got lost too, in part because it was hard
>> to refer to an actual instance.
>> My point was that in the present patch set, the "usb charger" is given
>> a name which is dependant on discovery order, and only supports
>> lookup-by-name.  This cannot work.
>
> There's two bits here: one is the way names are assigned and the other
> is the lookup by name.  I agree that the lookup by name isn't
> particularly useful as things stand, that could just be dropped until
> some naming mechanism is added.  We'd be more likely to use phandles in
> DT systems, I don't know what ACPI systems would look like but I guess
> it'd be something similar.
>
>> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
>> any usb-charger which has power available"), or look up by some other
>> attribute, then discover-order naming could work.  But the only
>> lookup-mechanism is by-name, and the names aren't reliably stable.  So
>> the name/lookup system proposed cannot possibly do anything useful
>> with more than one usb_charger.
>
> Baolin, I think adding a DT binding and lookup mechanism makes sense
> here - do you agree?

Yes, I agree. But 'usb charger' is one virtual device and we described
nothing in DT about 'usb charger'. But as you and Neil said, we need
one usful method to look up the USB charger. I will think about that.
Thanks.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 14 November 2016 at 12:21, NeilBrown <neilb@suse.com> wrote:
> On Thu, Nov 10 2016, Baolin Wang wrote:
>
>> Hi
>>
>> On 8 November 2016 at 04:36, NeilBrown <neilb@suse.com> wrote:
>>> On Mon, Nov 07 2016, Baolin Wang wrote:
>>>
>>>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>
>>>> I agree with your most opinions, but these are optimization.
>>>
>>> I see them as bug fixes, not optimizations.
>>>
>>>>                                                              Firstly I
>>>> think we should upstream the USB charger driver.
>>>
>>> I think you missed the point.  The point is that we don't *need* your
>>> "USB charger driver" because all the infrastructure we need is *already*
>>> present in the kernel.  It is buggy and not used uniformly, and could
>>> usefully be polished and improved.  But the structure is already
>>> present.
>>>
>>> If everyone just added new infrastructure when they didn't like, or
>>> didn't understand, what was already present, the kernel would become
>>> like the Mad Hatter's tea party, full of dirty dishes.
>>>
>>>>                                                  What I want to ask is
>>>> how can we notify power driver if we don't set the
>>>> usb_register_notifier(), then I think you give the answer is: power
>>>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>>>> should notify the power driver how much current should be drawn, and I
>>>> still think we should notify the current in usb charger driver which
>>>> is better, and do not need to notify current for power driver in usb
>>>> phy driver.
>>>
>>> I accept that it isn't clear that the phy *should* be involved in
>>> communicating the negotiated power availability, but nor is it clear
>>> that it shouldn't.  The power does travel through the physical
>>> interface, so physically it plays a role.
>>>
>>> But more importantly, it already *does* get told (in some cases).
>>> There is an interface "usb_phy_set_power()" which exists explicitly to
>>> tell the phy what power has been negotiated.  Given that infrastructure
>>> exists and works, it make sense to use it.
>>>
>>> If you think it is a broken design and should be removed, then fine:
>>> make a case for that.  Examine the history.  Make sure you know why it
>>> is there (or make sure that information cannot be found), and then
>>> present a case as to why it should be removed and replaced with
>>> something else.  But don't just leave it there and pretend it doesn't
>>> exist and create something similar-but-different and hope people will
>>> know why yours is better.  That way lies madness.
>>
>> Like Peter said, it is not only PHY can detect the USB charger type,
>> which means there are other places can detect the charger type.
>
> If I understand Peter's example correctly, it shows a configuration
> where the USB PHysical interface is partly implemented in the SOC and
> partly in the PMIC.  I appreciate that would make it more challenging to
> implement a PHY driver, but there is no reason it should impact anything
> outside of the PHY.

Like Peter's example, it need to use controller register to pull up dp
to begin the secondary detection, which is not belonged to phy driver
and I don't think it is suitable we implemented these in phy driver.

>
>> Second, some controller need to detect the charger type manually which
>> USB phy did not support.
>
> "manually" is an odd term to use in this context.

Sorry for the confusing.

> I think you mean that to detect the charger type you need to issue some
> command to the hardware and wait for it to respond, then assess the
> response.

Yes.

> That isn't at all surprising.  The charger type is detected by measuring
> resistance between ID and GND, which may require setting up a potential
> and activating ADCs to measure the voltage.  This can all be done
> internally to the phy driver.
> Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for
> twl4030, though it never got upstream).
> The code for the imx7d does look more complex, but not intrinsically
> different.

But you should implement these in every phy driver, why not one
standard framework?

>
>> Third, it did not handle what current should
>> be drawn in USB phy.
>
> The standards define that.  The extcon just reports the cable type.
> Certainly it would be sensible to provide a library function to
> translate from cable type to current range.  You don't need a new
> subsystem to do that, just a library function.

I don't think the extcon should handle current things. For example,
the extcon can not know the gadget speed, which is used to change the
default current values for super speed gadget.

>
>> Fourth, we need integrate all charger plugin/out
>> event in one framework, not from extcon, maybe type-c in future.
>
> Why not extcon?  Given that a charger is connected by an external
> connector, extcon seems like exactly the right thing to use.

My mistake, what I mean is not only from extcon, maybe from other
places in future.

>
> Obviously extcon doesn't report the current that was negotiated, but
> that is best kept separate.  The battery charger can be advised of the
> available current either via extcon or separately via the usb
> subsystem.  Don't conflate the two.
>
>
>>  In a
>> word, we need one standard integration of this feature we need, though
>> like you said we should do some clean up or fix to make it better.
>
> But really, I'm not the person you need to convince.  I'm just a vaguely
> interested bystander who is rapidly losing interest.  You need to
> convince a maintainer, but they have so far shown remarkably little
> interest.  I don't know why, but I'd guess that reviewing a complex new
> subsystem isn't much fun.  Reviewing and applying clear bugfixes and
> incremental improvements is much easier and more enjoyable.  But that is
> just a guess.

Maybe you missed previous comments, and we had a lot of discussion
about this patchset. Also Felipe had reviewed this patchset with some
suggestion.

-- 
Baolin.wang
Best Regards

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

[Peter Chen]

On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:
> 
> 
> >> So I won't be responding on this topic any further until I see a genuine
> >> attempt to understand and resolve the inconsistencies with
> >> usb_register_notifier().
> >
> > Any better solution?
> 
> I'm not sure exactly what you are asking, so I'll assume you are asking
> the question I want to answer :-)
> 
> 1/ Liase with the extcon developers to resolve the inconsistencies
>   with USB connector types.
>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>   which both seem to suggest a standard downstream port.  There is no
>   documentation describing how these relate, and no consistent practice
>   to copy.
>   I suspect the intention is that
>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>     the cable. 
>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>     would normally appear with EXTCON_USB_HOST (I think).
>   Some drivers follow this model, particularly extcon-max14577.c
>   but it is not consistent.
> 
>   This policy should be well documented and possibly existing drivers
>   should be updated to follow it.
> 
>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>   They were recently removed from drivers/power/axp288_charger.c
>   which is good, but are still used in drivers/extcon/extcon-max*
>   Possibly they should be changed to names from the standard, or
>   possibly they should be renamed to identify the current they are
>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
> 
> 2/ Change all usb phys to register an extcon and to send appropriate
>    notifications.  Many already do, but I don't think it is universal.
>    It is probable that the extcon should be registered using common code
>    instead of each phy driver having its own
>    extcon_get_edev_by_phandle()
>    or whatever.
>    If the usb phy driver needs to look at battery charger registers to
>    know what sort of cable was connected (which I believe is the case
>    for the chips you are interested in), then it should do that.

Not only USB PHY to register an extcon, but also for the drivers which
can detect USB charger type, it may be USB controller driver, USB type-c
driver, pmic driver, and these drivers may not have an extcon device
since the internal part can finish the vbus detect.

-- 

Best Regards,
Peter Chen

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

[NeilBrown]

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

On Tue, Nov 08 2016, Peter Chen wrote:

> On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote:
>> 
>> 
>> 
>> 2/ Change all usb phys to register an extcon and to send appropriate
>>    notifications.  Many already do, but I don't think it is universal.
>>    It is probable that the extcon should be registered using common code
>>    instead of each phy driver having its own
>>    extcon_get_edev_by_phandle()
>>    or whatever.
>>    If the usb phy driver needs to look at battery charger registers to
>>    know what sort of cable was connected (which I believe is the case
>>    for the chips you are interested in), then it should do that.
>
> Not only USB PHY to register an extcon, but also for the drivers which
> can detect USB charger type, it may be USB controller driver, USB type-c
> driver, pmic driver, and these drivers may not have an extcon device
> since the internal part can finish the vbus detect.

Can you point to an example of the sort of hardware/driver you are
referring to, preferably in the mainline kernel.  Concrete examples make
this sort of thing much easier to understand.

I think I would argue that if some piece of hardware can detect the USB
charger type, then that piece of hardware is part of the "USB PHY", even
if the hardware manufacturer has chosen to partition the register set in
a way which doesn't make that obvious.

NeilBrown

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

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

[Peter Chen]

On Wed, Nov 09, 2016 at 07:38:36AM +1100, NeilBrown wrote:
> On Tue, Nov 08 2016, Peter Chen wrote:
> 
> > On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote:
> >> 
> >> 
> >> 
> >> 2/ Change all usb phys to register an extcon and to send appropriate
> >>    notifications.  Many already do, but I don't think it is universal.
> >>    It is probable that the extcon should be registered using common code
> >>    instead of each phy driver having its own
> >>    extcon_get_edev_by_phandle()
> >>    or whatever.
> >>    If the usb phy driver needs to look at battery charger registers to
> >>    know what sort of cable was connected (which I believe is the case
> >>    for the chips you are interested in), then it should do that.
> >
> > Not only USB PHY to register an extcon, but also for the drivers which
> > can detect USB charger type, it may be USB controller driver, USB type-c
> > driver, pmic driver, and these drivers may not have an extcon device
> > since the internal part can finish the vbus detect.
> 
> Can you point to an example of the sort of hardware/driver you are
> referring to, preferably in the mainline kernel.  Concrete examples make
> this sort of thing much easier to understand.
> 

Eg, the nxp/fsl USB charger detector part, the register used to begin
the detection is in USB controller register region, and we also need to
use controller register to pull up dp to begin the secondary detect.

Since there is no USB charger framework, the code is not in mainline.
It finished an upstream version based on patch set in this thread [1]

Some SoCs uses chipidea IP doesn't need to use extcon to detect plugging
in/out, since the vbus pin connects to SoC's vbus pin.

The pmic has abilities to charger detection, eg, drivers/mfd/axp20x.c
and drivers/extcon/extcon-axp288.c.

> I think I would argue that if some piece of hardware can detect the USB
> charger type, then that piece of hardware is part of the "USB PHY", even
> if the hardware manufacturer has chosen to partition the register set in
> a way which doesn't make that obvious.

I agree with that the charger detect hardware is in the USB PHY part,
but some USB PHY's control is integrated in USB controller (eg, some
controller uses drivers/usb/phy/phy-generic.c as its PHY driver), so
we have to use controller register to finish some PHY function, eg
charger detection.

[1] https://www.spinics.net/lists/linux-usb/msg139425.html

-- 

Best Regards,
Peter Chen

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

[Baolin Wang]

Hi Neil,

On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:
>
>
>>> So I won't be responding on this topic any further until I see a genuine
>>> attempt to understand and resolve the inconsistencies with
>>> usb_register_notifier().
>>
>> Any better solution?
>
> I'm not sure exactly what you are asking, so I'll assume you are asking
> the question I want to answer :-)
>
> 1/ Liase with the extcon developers to resolve the inconsistencies
>   with USB connector types.
>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>   which both seem to suggest a standard downstream port.  There is no
>   documentation describing how these relate, and no consistent practice
>   to copy.
>   I suspect the intention is that
>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>     the cable.
>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>     would normally appear with EXTCON_USB_HOST (I think).
>   Some drivers follow this model, particularly extcon-max14577.c
>   but it is not consistent.
>
>   This policy should be well documented and possibly existing drivers
>   should be updated to follow it.
>
>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>   They were recently removed from drivers/power/axp288_charger.c
>   which is good, but are still used in drivers/extcon/extcon-max*
>   Possibly they should be changed to names from the standard, or
>   possibly they should be renamed to identify the current they are
>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A

Now I am creating the new patchset with fixing and converting exist drivers.

I did some investigation about EXTCON subsystem. From your suggestion:
1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
---- After checking, now all extcon drivers were following this rule.

2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to change.

3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
---- There are no model that shows the slow charger should be 500mA
and fast charger is 1A. (In extcon-max77693.c file, the fast charger
can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
I think.

>From above investigation, I did not find anything need to be changed
now. What do you think? Thanks.

>
> 2/ Change all usb phys to register an extcon and to send appropriate
>    notifications.  Many already do, but I don't think it is universal.
>    It is probable that the extcon should be registered using common code
>    instead of each phy driver having its own
>    extcon_get_edev_by_phandle()
>    or whatever.
>    If the usb phy driver needs to look at battery charger registers to
>    know what sort of cable was connected (which I believe is the case
>    for the chips you are interested in), then it should do that.
>
> 3/ Currently some USB controllers discover that a cable was connected by
>    listening on an extcon, and some by registering for a usb_notifier
>    (described below) ... though there seem to only be 2 left which do that.
>    Now that all USB phys send connection information via extcon (see 2),
>    the USB controllers should be changed to all find out about the cable
>    using extcon.
>
> 4/ struct usb_phy contains:
>         /* for notification of usb_phy_events */
>         struct atomic_notifier_head     notifier;
>
>   This is used inconsistently.  Sometimes the argument passed
>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>   limited negotiated via USB, sometimes it is a pointer the the gadget
>   though as far as I can tell, that last one is never used.
>
>   This should be changed to be consistent.  This notifier is no longer
>   needed to tell the USB controller that a cable was connected (extcon
>   now does that, see 3) so it is only used to communicate the
>   'vbus_draw' information.
>   So it should be changed to *only* send a notification when vbus_draw
>   is known, and it should carry that information.
>   This should probably be done in common code, and removed
>   from individual drivers.
>
> 5/ Now that all cable connection notifications are sent over extcon and
>    all vbus_draw notifications are sent over the usb_phy notifier, write
>    some support code that a power supply client can use to be told what
>    power is available.
>    e.g. a battery charger driver would call:
>        register_power_client(.....)
>    or similar, providing a phandle (or similar) for the usb phy and a
>    function to call back when the available current changes (or maybe a
>    work_struct containing the function pointer)
>
>    register_power_client() would then register with extcon and separately
>    with the usb_phy notifier.  When the different events arrive it
>    calculates what ranges of currents are expected and calls the
>    call-back function with those details.
>
> 6/ Any battery charger that needs to know the available current can now
>    call register_power_client() and get the information delivered.
>
> NeilBrown



-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Tue, Dec 20 2016, Baolin Wang wrote:

> Hi Neil,
>
> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>
>>
>>>> So I won't be responding on this topic any further until I see a genuine
>>>> attempt to understand and resolve the inconsistencies with
>>>> usb_register_notifier().
>>>
>>> Any better solution?
>>
>> I'm not sure exactly what you are asking, so I'll assume you are asking
>> the question I want to answer :-)
>>
>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>   with USB connector types.
>>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>   which both seem to suggest a standard downstream port.  There is no
>>   documentation describing how these relate, and no consistent practice
>>   to copy.
>>   I suspect the intention is that
>>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>     the cable.
>>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>     would normally appear with EXTCON_USB_HOST (I think).
>>   Some drivers follow this model, particularly extcon-max14577.c
>>   but it is not consistent.
>>
>>   This policy should be well documented and possibly existing drivers
>>   should be updated to follow it.
>>
>>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>>   They were recently removed from drivers/power/axp288_charger.c
>>   which is good, but are still used in drivers/extcon/extcon-max*
>>   Possibly they should be changed to names from the standard, or
>>   possibly they should be renamed to identify the current they are
>>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>
> Now I am creating the new patchset with fixing and converting exist drivers.

Thanks!

>
> I did some investigation about EXTCON subsystem. From your suggestion:
> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
> ---- After checking, now all extcon drivers were following this rule.

what about extcon-axp288.c ?
axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
never sets EXTCON_USB.
Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.

>
> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
> change.

Agreed.

>
> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
> ---- There are no model that shows the slow charger should be 500mA
> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
> I think.

Leaving the names a SLOW/FAST is less useful as those names don't *mean*
anything.
The only place where the cable types are registered are in
 extcon-max{14577,77693,77843,8997}.c

In each case, the code strongly suggests that the meaning is that "slow"
means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).

With names like "fast" and "slow", any common changer framework cannot
make use of these cable types as the name doesn't mean anything.
If the names were changed to 500MA/1A, then common code could reasonably
assume how much current can safely be drawn from each.

>
> From above investigation, I did not find anything need to be changed
> now. What do you think? Thanks.
>

As above, I think changes are needed.

I particularly highlight:

>>   This policy should be well documented and possibly existing drivers
>>   should be updated to follow it.

The documentation is key.  A usb charger framework needs to depend on
correct information from extcon, and currently there is no clear
documentation on how a USB phy should signal the charger type.
There isn't a lot to say: primarily that both the EXTCON_USB* and
EXTCON_CGH_USB* types should be provided together.  Each does not imply
the other in any way.  But it still needs to be documented.

Thanks,
NeilBrown


>>
>> 2/ Change all usb phys to register an extcon and to send appropriate
>>    notifications.  Many already do, but I don't think it is universal.
>>    It is probable that the extcon should be registered using common code
>>    instead of each phy driver having its own
>>    extcon_get_edev_by_phandle()
>>    or whatever.
>>    If the usb phy driver needs to look at battery charger registers to
>>    know what sort of cable was connected (which I believe is the case
>>    for the chips you are interested in), then it should do that.
>>
>> 3/ Currently some USB controllers discover that a cable was connected by
>>    listening on an extcon, and some by registering for a usb_notifier
>>    (described below) ... though there seem to only be 2 left which do that.
>>    Now that all USB phys send connection information via extcon (see 2),
>>    the USB controllers should be changed to all find out about the cable
>>    using extcon.
>>
>> 4/ struct usb_phy contains:
>>         /* for notification of usb_phy_events */
>>         struct atomic_notifier_head     notifier;
>>
>>   This is used inconsistently.  Sometimes the argument passed
>>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>>   limited negotiated via USB, sometimes it is a pointer the the gadget
>>   though as far as I can tell, that last one is never used.
>>
>>   This should be changed to be consistent.  This notifier is no longer
>>   needed to tell the USB controller that a cable was connected (extcon
>>   now does that, see 3) so it is only used to communicate the
>>   'vbus_draw' information.
>>   So it should be changed to *only* send a notification when vbus_draw
>>   is known, and it should carry that information.
>>   This should probably be done in common code, and removed
>>   from individual drivers.
>>
>> 5/ Now that all cable connection notifications are sent over extcon and
>>    all vbus_draw notifications are sent over the usb_phy notifier, write
>>    some support code that a power supply client can use to be told what
>>    power is available.
>>    e.g. a battery charger driver would call:
>>        register_power_client(.....)
>>    or similar, providing a phandle (or similar) for the usb phy and a
>>    function to call back when the available current changes (or maybe a
>>    work_struct containing the function pointer)
>>
>>    register_power_client() would then register with extcon and separately
>>    with the usb_phy notifier.  When the different events arrive it
>>    calculates what ranges of currents are expected and calls the
>>    call-back function with those details.
>>
>> 6/ Any battery charger that needs to know the available current can now
>>    call register_power_client() and get the information delivered.
>>
>> NeilBrown
>
>
>
> -- 
> Baolin.wang
> Best Regards

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

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

[Baolin Wang]

Hi,

On 21 December 2016 at 06:07, NeilBrown <neilb@suse.com> wrote:
> On Tue, Dec 20 2016, Baolin Wang wrote:
>
>> Hi Neil,
>>
>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>
>>>
>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>> attempt to understand and resolve the inconsistencies with
>>>>> usb_register_notifier().
>>>>
>>>> Any better solution?
>>>
>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>> the question I want to answer :-)
>>>
>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>   with USB connector types.
>>>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>   which both seem to suggest a standard downstream port.  There is no
>>>   documentation describing how these relate, and no consistent practice
>>>   to copy.
>>>   I suspect the intention is that
>>>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>     the cable.
>>>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>     would normally appear with EXTCON_USB_HOST (I think).
>>>   Some drivers follow this model, particularly extcon-max14577.c
>>>   but it is not consistent.
>>>
>>>   This policy should be well documented and possibly existing drivers
>>>   should be updated to follow it.
>>>
>>>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>>>   They were recently removed from drivers/power/axp288_charger.c
>>>   which is good, but are still used in drivers/extcon/extcon-max*
>>>   Possibly they should be changed to names from the standard, or
>>>   possibly they should be renamed to identify the current they are
>>>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>
>> Now I am creating the new patchset with fixing and converting exist drivers.
>
> Thanks!
>
>>
>> I did some investigation about EXTCON subsystem. From your suggestion:
>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>> ---- After checking, now all extcon drivers were following this rule.
>
> what about extcon-axp288.c ?
> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
> never sets EXTCON_USB.
> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.

Ha, sorry, I missed these 2 files, and I will fix them.

>
>>
>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>> change.
>
> Agreed.
>
>>
>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>> ---- There are no model that shows the slow charger should be 500mA
>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>> I think.
>
> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
> anything.
> The only place where the cable types are registered are in
>  extcon-max{14577,77693,77843,8997}.c
>
> In each case, the code strongly suggests that the meaning is that "slow"
> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>
> With names like "fast" and "slow", any common changer framework cannot
> make use of these cable types as the name doesn't mean anything.
> If the names were changed to 500MA/1A, then common code could reasonably
> assume how much current can safely be drawn from each.

As I know, some fast charger can be drawn 5A, then do we need another
macro named 5A? then will introduce more macros in future, I am not
true this is helpful.

>
>>
>> From above investigation, I did not find anything need to be changed
>> now. What do you think? Thanks.
>>
>
> As above, I think changes are needed.
>
> I particularly highlight:
>
>>>   This policy should be well documented and possibly existing drivers
>>>   should be updated to follow it.
>
> The documentation is key.  A usb charger framework needs to depend on
> correct information from extcon, and currently there is no clear
> documentation on how a USB phy should signal the charger type.
> There isn't a lot to say: primarily that both the EXTCON_USB* and
> EXTCON_CGH_USB* types should be provided together.  Each does not imply
> the other in any way.  But it still needs to be documented.

Yes, I will try to add some documentation for this. Thanks.

>
> Thanks,
> NeilBrown
>
>
>>>
>>> 2/ Change all usb phys to register an extcon and to send appropriate
>>>    notifications.  Many already do, but I don't think it is universal.
>>>    It is probable that the extcon should be registered using common code
>>>    instead of each phy driver having its own
>>>    extcon_get_edev_by_phandle()
>>>    or whatever.
>>>    If the usb phy driver needs to look at battery charger registers to
>>>    know what sort of cable was connected (which I believe is the case
>>>    for the chips you are interested in), then it should do that.
>>>
>>> 3/ Currently some USB controllers discover that a cable was connected by
>>>    listening on an extcon, and some by registering for a usb_notifier
>>>    (described below) ... though there seem to only be 2 left which do that.
>>>    Now that all USB phys send connection information via extcon (see 2),
>>>    the USB controllers should be changed to all find out about the cable
>>>    using extcon.
>>>
>>> 4/ struct usb_phy contains:
>>>         /* for notification of usb_phy_events */
>>>         struct atomic_notifier_head     notifier;
>>>
>>>   This is used inconsistently.  Sometimes the argument passed
>>>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>>>   limited negotiated via USB, sometimes it is a pointer the the gadget
>>>   though as far as I can tell, that last one is never used.
>>>
>>>   This should be changed to be consistent.  This notifier is no longer
>>>   needed to tell the USB controller that a cable was connected (extcon
>>>   now does that, see 3) so it is only used to communicate the
>>>   'vbus_draw' information.
>>>   So it should be changed to *only* send a notification when vbus_draw
>>>   is known, and it should carry that information.
>>>   This should probably be done in common code, and removed
>>>   from individual drivers.
>>>
>>> 5/ Now that all cable connection notifications are sent over extcon and
>>>    all vbus_draw notifications are sent over the usb_phy notifier, write
>>>    some support code that a power supply client can use to be told what
>>>    power is available.
>>>    e.g. a battery charger driver would call:
>>>        register_power_client(.....)
>>>    or similar, providing a phandle (or similar) for the usb phy and a
>>>    function to call back when the available current changes (or maybe a
>>>    work_struct containing the function pointer)
>>>
>>>    register_power_client() would then register with extcon and separately
>>>    with the usb_phy notifier.  When the different events arrive it
>>>    calculates what ranges of currents are expected and calls the
>>>    call-back function with those details.
>>>
>>> 6/ Any battery charger that needs to know the available current can now
>>>    call register_power_client() and get the information delivered.
>>>
>>> NeilBrown
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Wed, Dec 21 2016, Baolin Wang wrote:

> Hi,
>
> On 21 December 2016 at 06:07, NeilBrown <neilb@suse.com> wrote:
>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>
>>> Hi Neil,
>>>
>>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>
>>>>
>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>> usb_register_notifier().
>>>>>
>>>>> Any better solution?
>>>>
>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>> the question I want to answer :-)
>>>>
>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>   with USB connector types.
>>>>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>   which both seem to suggest a standard downstream port.  There is no
>>>>   documentation describing how these relate, and no consistent practice
>>>>   to copy.
>>>>   I suspect the intention is that
>>>>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>     the cable.
>>>>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>     would normally appear with EXTCON_USB_HOST (I think).
>>>>   Some drivers follow this model, particularly extcon-max14577.c
>>>>   but it is not consistent.
>>>>
>>>>   This policy should be well documented and possibly existing drivers
>>>>   should be updated to follow it.
>>>>
>>>>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>>>>   They were recently removed from drivers/power/axp288_charger.c
>>>>   which is good, but are still used in drivers/extcon/extcon-max*
>>>>   Possibly they should be changed to names from the standard, or
>>>>   possibly they should be renamed to identify the current they are
>>>>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>
>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>
>> Thanks!
>>
>>>
>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>> ---- After checking, now all extcon drivers were following this rule.
>>
>> what about extcon-axp288.c ?
>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>> never sets EXTCON_USB.
>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>
> Ha, sorry, I missed these 2 files, and I will fix them.
>
>>
>>>
>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>> change.
>>
>> Agreed.
>>
>>>
>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>> ---- There are no model that shows the slow charger should be 500mA
>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>> I think.
>>
>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>> anything.
>> The only place where the cable types are registered are in
>>  extcon-max{14577,77693,77843,8997}.c
>>
>> In each case, the code strongly suggests that the meaning is that "slow"
>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>
>> With names like "fast" and "slow", any common changer framework cannot
>> make use of these cable types as the name doesn't mean anything.
>> If the names were changed to 500MA/1A, then common code could reasonably
>> assume how much current can safely be drawn from each.
>
> As I know, some fast charger can be drawn 5A, then do we need another
> macro named 5A? then will introduce more macros in future, I am not
> true this is helpful.

It isn't really a question of what the charger can provide.  It is a
question of what the cable reports to the phy.

Unfortunately I cannot find any datasheets on line to verify how these
devices work, but the code seems to suggest that they can detect and
report special charger types that provide 500mA and 1A respectively.
If the hardware exists that reports the functionality, it make sense to
use it.  Hopefully new hardware will follow the USB BC spec, so further
special cases won't be needed.

Thanks,
NeilBrown

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

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

[Baolin Wang]

On 21 December 2016 at 11:48, NeilBrown <neilb@suse.com> wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> Hi,
>>
>> On 21 December 2016 at 06:07, NeilBrown <neilb@suse.com> wrote:
>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>
>>>> Hi Neil,
>>>>
>>>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>>
>>>>>
>>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>>> usb_register_notifier().
>>>>>>
>>>>>> Any better solution?
>>>>>
>>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>>> the question I want to answer :-)
>>>>>
>>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>>   with USB connector types.
>>>>>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>>   which both seem to suggest a standard downstream port.  There is no
>>>>>   documentation describing how these relate, and no consistent practice
>>>>>   to copy.
>>>>>   I suspect the intention is that
>>>>>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>>     the cable.
>>>>>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>>     would normally appear with EXTCON_USB_HOST (I think).
>>>>>   Some drivers follow this model, particularly extcon-max14577.c
>>>>>   but it is not consistent.
>>>>>
>>>>>   This policy should be well documented and possibly existing drivers
>>>>>   should be updated to follow it.
>>>>>
>>>>>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>>>>>   They were recently removed from drivers/power/axp288_charger.c
>>>>>   which is good, but are still used in drivers/extcon/extcon-max*
>>>>>   Possibly they should be changed to names from the standard, or
>>>>>   possibly they should be renamed to identify the current they are
>>>>>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>>
>>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>>
>>> Thanks!
>>>
>>>>
>>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>>> ---- After checking, now all extcon drivers were following this rule.
>>>
>>> what about extcon-axp288.c ?
>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>> never sets EXTCON_USB.
>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>
>> Ha, sorry, I missed these 2 files, and I will fix them.
>>
>>>
>>>>
>>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>>> change.
>>>
>>> Agreed.
>>>
>>>>
>>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>>> ---- There are no model that shows the slow charger should be 500mA
>>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>>> I think.
>>>
>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>> anything.
>>> The only place where the cable types are registered are in
>>>  extcon-max{14577,77693,77843,8997}.c
>>>
>>> In each case, the code strongly suggests that the meaning is that "slow"
>>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>>
>>> With names like "fast" and "slow", any common changer framework cannot
>>> make use of these cable types as the name doesn't mean anything.
>>> If the names were changed to 500MA/1A, then common code could reasonably
>>> assume how much current can safely be drawn from each.
>>
>> As I know, some fast charger can be drawn 5A, then do we need another
>> macro named 5A? then will introduce more macros in future, I am not
>> true this is helpful.
>
> It isn't really a question of what the charger can provide.  It is a
> question of what the cable reports to the phy.

Yes, there is no spec to describe fast/slow charger type and how much
current fast/slow charger can provide. Maybe some fast charger can
provide 1A/2A, others can provide 5A, which depends on users'
platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
charger can provide 1.5A on user's platform, will it report the fast
charger type by EXTCON_CHG_USB_1A on user's platform (but it can
provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
maintainer has applied them).

>
> Unfortunately I cannot find any datasheets on line to verify how these
> devices work, but the code seems to suggest that they can detect and
> report special charger types that provide 500mA and 1A respectively.
> If the hardware exists that reports the functionality, it make sense to
> use it.  Hopefully new hardware will follow the USB BC spec, so further
> special cases won't be needed.
>
> Thanks,
> NeilBrown



-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Wed, Dec 21 2016, Baolin Wang wrote:

> On 21 December 2016 at 11:48, NeilBrown <neilb@suse.com> wrote:
>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>
>>> Hi,
>>>
>>> On 21 December 2016 at 06:07, NeilBrown <neilb@suse.com> wrote:
>>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>>
>>>>> Hi Neil,
>>>>>
>>>>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>>>
>>>>>>
>>>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>>>> usb_register_notifier().
>>>>>>>
>>>>>>> Any better solution?
>>>>>>
>>>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>>>> the question I want to answer :-)
>>>>>>
>>>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>>>   with USB connector types.
>>>>>>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>>>   which both seem to suggest a standard downstream port.  There is no
>>>>>>   documentation describing how these relate, and no consistent practice
>>>>>>   to copy.
>>>>>>   I suspect the intention is that
>>>>>>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>>>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>>>     the cable.
>>>>>>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>>>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>>>     would normally appear with EXTCON_USB_HOST (I think).
>>>>>>   Some drivers follow this model, particularly extcon-max14577.c
>>>>>>   but it is not consistent.
>>>>>>
>>>>>>   This policy should be well documented and possibly existing drivers
>>>>>>   should be updated to follow it.
>>>>>>
>>>>>>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>>>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>>>>>>   They were recently removed from drivers/power/axp288_charger.c
>>>>>>   which is good, but are still used in drivers/extcon/extcon-max*
>>>>>>   Possibly they should be changed to names from the standard, or
>>>>>>   possibly they should be renamed to identify the current they are
>>>>>>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>>>
>>>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>>>> ---- After checking, now all extcon drivers were following this rule.
>>>>
>>>> what about extcon-axp288.c ?
>>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>>> never sets EXTCON_USB.
>>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>>
>>> Ha, sorry, I missed these 2 files, and I will fix them.
>>>
>>>>
>>>>>
>>>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>>>> change.
>>>>
>>>> Agreed.
>>>>
>>>>>
>>>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>>>> ---- There are no model that shows the slow charger should be 500mA
>>>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>>>> I think.
>>>>
>>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>>> anything.
>>>> The only place where the cable types are registered are in
>>>>  extcon-max{14577,77693,77843,8997}.c
>>>>
>>>> In each case, the code strongly suggests that the meaning is that "slow"
>>>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>>>
>>>> With names like "fast" and "slow", any common changer framework cannot
>>>> make use of these cable types as the name doesn't mean anything.
>>>> If the names were changed to 500MA/1A, then common code could reasonably
>>>> assume how much current can safely be drawn from each.
>>>
>>> As I know, some fast charger can be drawn 5A, then do we need another
>>> macro named 5A? then will introduce more macros in future, I am not
>>> true this is helpful.
>>
>> It isn't really a question of what the charger can provide.  It is a
>> question of what the cable reports to the phy.
>
> Yes, there is no spec to describe fast/slow charger type and how much
> current fast/slow charger can provide. Maybe some fast charger can
> provide 1A/2A, others can provide 5A, which depends on users'
> platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
> charger can provide 1.5A on user's platform, will it report the fast
> charger type by EXTCON_CHG_USB_1A on user's platform (but it can
> provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
> they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
> maintainer has applied them).

I said "It isn't really a question of what the charger can provide."
and you are still taking about "and some fast charger can provide 1.5A".
So it seems like you didn't read, or you didn't understand what I wrote.
I'll try again.

Ignore the chargers.  COMPLETELY.  This not about chargers AT ALL.
This is about cables and the information that the cable reports.

Some how, and I cannot find the details, these MAXIM devices can report
things like
	case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
and
	case MAX14577_CHARGER_TYPE_SPECIAL_1A:
and
	case MAX77693_CHARGER_TYPE_APPLE_500MA:
and
	case MAX77693_CHARGER_TYPE_APPLE_1A_2A:

If this information is to be useful to the USB battery charging
infrastructure, then it must be communicated unambiguously.  Reporting
"EXTCON_CHG_USB_SLOW" or "..._FAST" doesn't have an obvious unambiguous
meaning.

If it were documented somewhere that
  A cable of type EXTCON_CHG_USB_SLOW can provide at least 500mA of current
  at 5V.  A cable of type EXTCON_CHG_USB_FAST can provide at least 1A of
  current at 5V.
when those cable types could be used by the USB battery chargers.
If we just changed the names, then we wouldn't really need to document
it and the intention would be obvious.  Self-documenting names are
better where possible.

So yes, we could leave it as it is a decide not to fix this bug.  But
then I would then argue strongly against any attempt for the USB battery
charging infrastructure to do anything with EXTCON_CHG_USB_SLOW and
EXTCON_CHG_USB_FAST.  If you are going to fix this thing, you may as
well fix it properly.

NeilBrown


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

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

[Baolin Wang]

On 22 December 2016 at 07:47, NeilBrown <neilb@suse.com> wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> On 21 December 2016 at 11:48, NeilBrown <neilb@suse.com> wrote:
>>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>>
>>>> Hi,
>>>>
>>>> On 21 December 2016 at 06:07, NeilBrown <neilb@suse.com> wrote:
>>>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>>>
>>>>>> Hi Neil,
>>>>>>
>>>>>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
>>>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>>>>> usb_register_notifier().
>>>>>>>>
>>>>>>>> Any better solution?
>>>>>>>
>>>>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>>>>> the question I want to answer :-)
>>>>>>>
>>>>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>>>>   with USB connector types.
>>>>>>>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>>>>   which both seem to suggest a standard downstream port.  There is no
>>>>>>>   documentation describing how these relate, and no consistent practice
>>>>>>>   to copy.
>>>>>>>   I suspect the intention is that
>>>>>>>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>>>>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>>>>     the cable.
>>>>>>>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>>>>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>>>>     would normally appear with EXTCON_USB_HOST (I think).
>>>>>>>   Some drivers follow this model, particularly extcon-max14577.c
>>>>>>>   but it is not consistent.
>>>>>>>
>>>>>>>   This policy should be well documented and possibly existing drivers
>>>>>>>   should be updated to follow it.
>>>>>>>
>>>>>>>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>>>>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>>>>>>>   They were recently removed from drivers/power/axp288_charger.c
>>>>>>>   which is good, but are still used in drivers/extcon/extcon-max*
>>>>>>>   Possibly they should be changed to names from the standard, or
>>>>>>>   possibly they should be renamed to identify the current they are
>>>>>>>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>>>>
>>>>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>>
>>>>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>>>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>>>>> ---- After checking, now all extcon drivers were following this rule.
>>>>>
>>>>> what about extcon-axp288.c ?
>>>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>>>> never sets EXTCON_USB.
>>>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>>>
>>>> Ha, sorry, I missed these 2 files, and I will fix them.
>>>>
>>>>>
>>>>>>
>>>>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>>>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>>>>> change.
>>>>>
>>>>> Agreed.
>>>>>
>>>>>>
>>>>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>>>>> ---- There are no model that shows the slow charger should be 500mA
>>>>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>>>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>>>>> I think.
>>>>>
>>>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>>>> anything.
>>>>> The only place where the cable types are registered are in
>>>>>  extcon-max{14577,77693,77843,8997}.c
>>>>>
>>>>> In each case, the code strongly suggests that the meaning is that "slow"
>>>>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>>>>
>>>>> With names like "fast" and "slow", any common changer framework cannot
>>>>> make use of these cable types as the name doesn't mean anything.
>>>>> If the names were changed to 500MA/1A, then common code could reasonably
>>>>> assume how much current can safely be drawn from each.
>>>>
>>>> As I know, some fast charger can be drawn 5A, then do we need another
>>>> macro named 5A? then will introduce more macros in future, I am not
>>>> true this is helpful.
>>>
>>> It isn't really a question of what the charger can provide.  It is a
>>> question of what the cable reports to the phy.
>>
>> Yes, there is no spec to describe fast/slow charger type and how much
>> current fast/slow charger can provide. Maybe some fast charger can
>> provide 1A/2A, others can provide 5A, which depends on users'
>> platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
>> charger can provide 1.5A on user's platform, will it report the fast
>> charger type by EXTCON_CHG_USB_1A on user's platform (but it can
>> provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
>> they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
>> maintainer has applied them).
>
> I said "It isn't really a question of what the charger can provide."
> and you are still taking about "and some fast charger can provide 1.5A".
> So it seems like you didn't read, or you didn't understand what I wrote.
> I'll try again.
>
> Ignore the chargers.  COMPLETELY.  This not about chargers AT ALL.
> This is about cables and the information that the cable reports.
>
> Some how, and I cannot find the details, these MAXIM devices can report
> things like
>         case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
> and
>         case MAX14577_CHARGER_TYPE_SPECIAL_1A:
> and
>         case MAX77693_CHARGER_TYPE_APPLE_500MA:
> and
>         case MAX77693_CHARGER_TYPE_APPLE_1A_2A:
>
> If this information is to be useful to the USB battery charging
> infrastructure, then it must be communicated unambiguously.  Reporting
> "EXTCON_CHG_USB_SLOW" or "..._FAST" doesn't have an obvious unambiguous
> meaning.
>
> If it were documented somewhere that
>   A cable of type EXTCON_CHG_USB_SLOW can provide at least 500mA of current
>   at 5V.  A cable of type EXTCON_CHG_USB_FAST can provide at least 1A of
>   current at 5V.
> when those cable types could be used by the USB battery chargers.
> If we just changed the names, then we wouldn't really need to document
> it and the intention would be obvious.  Self-documenting names are
> better where possible.

Fine, I will submit one patch to document EXTCON_CHG_USB_SLOW/FAST as
you suggested.

>
> So yes, we could leave it as it is a decide not to fix this bug.  But
> then I would then argue strongly against any attempt for the USB battery
> charging infrastructure to do anything with EXTCON_CHG_USB_SLOW and
> EXTCON_CHG_USB_FAST.  If you are going to fix this thing, you may as
> well fix it properly.
>
> NeilBrown
>



-- 
Baolin.wang
Best Regards