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

[PATCH v16 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 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     |   69 ++++
 drivers/usb/gadget/Kconfig       |    8 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  780 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c    |   17 +
 include/linux/mfd/wm831x/pdata.h |    3 +
 include/linux/usb/charger.h      |  186 +++++++++
 include/linux/usb/gadget.h       |    3 +
 include/uapi/linux/usb/charger.h |   31 ++
 9 files changed, 1098 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

-- 
1.7.9.5

[PATCH v16 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 |  697 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  186 ++++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 923 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 3c3f31c..acea3f7 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..462faa1
--- /dev/null
+++ b/drivers/usb/gadget/udc/charger.c
@@ -0,0 +1,697 @@
+/*
+ * 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>
+#include <linux/power_supply.h>
+
+/* Default current limit by charger type. */
+#define DEFAULT_SDP_CUR_LIMIT		500
+#define DEFAULT_SDP_CUR_LIMIT_SS	900
+#define DEFAULT_DCP_CUR_LIMIT		1500
+#define DEFAULT_CDP_CUR_LIMIT		1500
+#define DEFAULT_ACA_CUR_LIMIT		1500
+
+static DEFINE_IDA(usb_charger_ida);
+static LIST_HEAD(charger_list);
+static DEFINE_MUTEX(charger_lock);
+
+static unsigned int __usb_charger_get_cur_limit(struct usb_charger *uchger);
+
+static struct usb_charger *dev_to_uchger(struct device *dev)
+{
+	return NULL;
+}
+
+/*
+ * charger_current_show() - Show the charger current limit.
+ */
+static ssize_t charger_current_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+
+	return sprintf(buf, "%u\n", __usb_charger_get_cur_limit(uchger));
+}
+static DEVICE_ATTR_RO(charger_current);
+
+/*
+ * charger_type_show() - Show the charger type.
+ *
+ * It can be SDP/DCP/CDP/ACA type, else for unknown type.
+ */
+static ssize_t charger_type_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	int cnt;
+
+	switch (uchger->type) {
+	case SDP_TYPE:
+		cnt = sprintf(buf, "%s\n", "SDP");
+		break;
+	case DCP_TYPE:
+		cnt = sprintf(buf, "%s\n", "DCP");
+		break;
+	case CDP_TYPE:
+		cnt = sprintf(buf, "%s\n", "CDP");
+		break;
+	case ACA_TYPE:
+		cnt = sprintf(buf, "%s\n", "ACA");
+		break;
+	default:
+		cnt = sprintf(buf, "%s\n", "UNKNOWN");
+		break;
+	}
+
+	return cnt;
+}
+static DEVICE_ATTR_RO(charger_type);
+
+/*
+ * charger_state_show() - Show the charger state.
+ *
+ * Charger state can be present or removed.
+ */
+static ssize_t charger_state_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	struct usb_charger *uchger = dev_to_uchger(dev);
+	int cnt;
+
+	switch (uchger->state) {
+	case USB_CHARGER_PRESENT:
+		cnt = sprintf(buf, "%s\n", "PRESENT");
+		break;
+	case USB_CHARGER_REMOVE:
+		cnt = sprintf(buf, "%s\n", "REMOVE");
+		break;
+	default:
+		cnt = sprintf(buf, "%s\n", "UNKNOWN");
+		break;
+	}
+
+	return cnt;
+}
+static DEVICE_ATTR_RO(charger_state);
+
+static struct attribute *usb_charger_attrs[] = {
+	&dev_attr_charger_current.attr,
+	&dev_attr_charger_type.attr,
+	&dev_attr_charger_state.attr,
+	NULL
+};
+
+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 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 = uchger->type;
+	mutex_unlock(&uchger->lock);
+
+	return type;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get_type);
+
+/*
+ * usb_charger_get_state() - Get the charger state with lock protection.
+ * @uchger - the usb charger 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_get_type_by_others() - 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_by_others(struct usb_charger *uchger)
+{
+	if (uchger->type != UNKNOWN_TYPE)
+		return uchger->type;
+
+	if (uchger->psy) {
+		union power_supply_propval val;
+
+		power_supply_get_property(uchger->psy,
+					  POWER_SUPPLY_PROP_CHARGE_TYPE,
+					  &val);
+		switch (val.intval) {
+		case POWER_SUPPLY_TYPE_USB:
+			uchger->type = SDP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_DCP:
+			uchger->type = DCP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_CDP:
+			uchger->type = CDP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_ACA:
+			uchger->type = ACA_TYPE;
+			break;
+		default:
+			uchger->type = UNKNOWN_TYPE;
+			break;
+		}
+	} else if (uchger->get_charger_type) {
+		uchger->type = uchger->get_charger_type(uchger);
+	} else {
+		uchger->type = UNKNOWN_TYPE;
+	}
+
+	return uchger->type;
+}
+
+/*
+ * __usb_charger_set_cur_limit_by_type() - Set the current limitation
+ * by charger type.
+ * @uchger - the usb charger 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_limit.sdp_cur_limit = cur_limit;
+		uchger->sdp_default_cur_change = 1;
+		break;
+	case DCP_TYPE:
+		uchger->cur_limit.dcp_cur_limit = cur_limit;
+		break;
+	case CDP_TYPE:
+		uchger->cur_limit.cdp_cur_limit	= cur_limit;
+		break;
+	case ACA_TYPE:
+		uchger->cur_limit.aca_cur_limit	= cur_limit;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
+ * usb_charger_notify_work() - Notify users the current has changed by work.
+ * @work - the work instance.
+ */
+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) {
+		usb_charger_get_type_by_others(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+				__usb_charger_get_cur_limit(uchger),
+				uchger);
+	}
+
+	mutex_unlock(&uchger->lock);
+}
+
+/*
+ * 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_cur_limit() - Set the current limitation.
+ * @uchger - the usb charger instance.
+ * @cur_limit_set - the current limitation.
+ */
+int usb_charger_set_cur_limit(struct usb_charger *uchger,
+			      struct usb_charger_cur_limit *cur_limit_set)
+{
+	if (!uchger || !cur_limit_set)
+		return -EINVAL;
+
+	mutex_lock(&uchger->lock);
+	uchger->cur_limit.sdp_cur_limit = cur_limit_set->sdp_cur_limit;
+	uchger->cur_limit.dcp_cur_limit = cur_limit_set->dcp_cur_limit;
+	uchger->cur_limit.cdp_cur_limit = cur_limit_set->cdp_cur_limit;
+	uchger->cur_limit.aca_cur_limit = cur_limit_set->aca_cur_limit;
+	uchger->sdp_default_cur_change = 1;
+	mutex_unlock(&uchger->lock);
+
+	schedule_work(&uchger->work);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit);
+
+/*
+ * __usb_charger_get_cur_limit() - Get the current limitation by
+ * different usb charger type.
+ * @uchger - the usb charger instance.
+ *
+ * return the current limitation to set.
+ */
+static unsigned int
+__usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	enum usb_charger_type uchger_type =
+		usb_charger_get_type_by_others(uchger);
+	unsigned int cur_limit;
+
+	switch (uchger_type) {
+	case SDP_TYPE:
+		/*
+		 * For super speed gadget, the default charger current should
+		 * be 900 mA.
+		 */
+		if (!uchger->sdp_default_cur_change && uchger->gadget &&
+		    uchger->gadget->speed >= USB_SPEED_SUPER) {
+			uchger->cur_limit.sdp_cur_limit =
+				DEFAULT_SDP_CUR_LIMIT_SS;
+
+			uchger->sdp_default_cur_change = 1;
+		}
+
+		cur_limit = uchger->cur_limit.sdp_cur_limit;
+		break;
+	case DCP_TYPE:
+		cur_limit = uchger->cur_limit.dcp_cur_limit;
+		break;
+	case CDP_TYPE:
+		cur_limit = uchger->cur_limit.cdp_cur_limit;
+		break;
+	case ACA_TYPE:
+		cur_limit = uchger->cur_limit.aca_cur_limit;
+		break;
+	default:
+		return 0;
+	}
+
+	return cur_limit;
+}
+
+/*
+ * usb_charger_get_cur_limit() - Get the charger current with lock protection.
+ * @uchger - the usb charger instance.
+ *
+ * Users should get the charger current by this safe API.
+ */
+unsigned int usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	unsigned int cur;
+
+	mutex_lock(&uchger->lock);
+	cur = __usb_charger_get_cur_limit(uchger);
+	mutex_unlock(&uchger->lock);
+
+	return cur;
+}
+EXPORT_SYMBOL_GPL(usb_charger_get_cur_limit);
+
+/*
+ * 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_notifier_others() - 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.
+ */
+static void
+usb_charger_notify_others(struct usb_charger *uchger,
+			  enum usb_charger_state state)
+{
+	char uchger_state[50] = { 0 };
+	char *envp[] = { uchger_state, NULL };
+
+	mutex_lock(&uchger->lock);
+	if (uchger->state == state) {
+		mutex_unlock(&uchger->lock);
+		return;
+	}
+
+	uchger->state = state;
+
+	switch (state) {
+	case USB_CHARGER_PRESENT:
+		usb_charger_get_type_by_others(uchger);
+		raw_notifier_call_chain(&uchger->uchger_nh,
+			__usb_charger_get_cur_limit(uchger),
+			uchger);
+		snprintf(uchger_state, ARRAY_SIZE(uchger_state),
+			 "USB_CHARGER_STATE=%s", "USB_CHARGER_PRESENT");
+		break;
+	case USB_CHARGER_REMOVE:
+		uchger->type = UNKNOWN_TYPE;
+		raw_notifier_call_chain(&uchger->uchger_nh, 0, uchger);
+		snprintf(uchger_state, ARRAY_SIZE(uchger_state),
+			 "USB_CHARGER_STATE=%s", "USB_CHARGER_REMOVE");
+		break;
+	default:
+		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_plug_by_extcon() - The notifier call function which is registered
+ * on the extcon device.
+ * @nb - the notifier block that notified by extcon device.
+ * @state - the extcon device state.
+ * @data - here specify a extcon device.
+ *
+ * return the notify flag.
+ */
+static int
+usb_charger_plug_by_extcon(struct notifier_block *nb,
+			   unsigned long state, void *data)
+{
+	struct usb_charger_nb *extcon_nb =
+		container_of(nb, struct usb_charger_nb, nb);
+	struct usb_charger *uchger = extcon_nb->uchger;
+	enum usb_charger_state uchger_state;
+
+	if (WARN(!uchger, "charger can not be NULL"))
+		return NOTIFY_BAD;
+
+	/*
+	 * Report event to power to setting the current limitation
+	 * for this usb charger when one usb charger is added or removed
+	 * with detecting by extcon device.
+	 */
+	if (state)
+		uchger_state = USB_CHARGER_PRESENT;
+	else
+		uchger_state = USB_CHARGER_REMOVE;
+
+	usb_charger_notify_others(uchger, uchger_state);
+
+	return NOTIFY_OK;
+}
+
+/*
+ * usb_charger_plug_by_gadget() - Set the usb charger current limitation
+ * according to the usb gadget device state.
+ * @gadget - the usb gadget device.
+ * @state - the usb gadget state.
+ */
+int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
+			       unsigned long state)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_plug_by_gadget);
+
+/*
+ * 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);
+		goto fail;
+	}
+
+	mutex_lock(&charger_lock);
+	list_add_tail(&uchger->list, &charger_list);
+	mutex_unlock(&charger_lock);
+
+	return 0;
+
+fail:
+	ida_simple_remove(&usb_charger_ida, uchger->id);
+	uchger->id = -1;
+	return ret;
+}
+
+int usb_charger_init(struct usb_gadget *ugadget)
+{
+	struct usb_charger *uchger;
+	struct extcon_dev *edev;
+	struct power_supply *psy;
+	int ret;
+
+	uchger = kzalloc(sizeof(struct usb_charger), GFP_KERNEL);
+	if (!uchger)
+		return -ENOMEM;
+
+	uchger->type = UNKNOWN_TYPE;
+	uchger->state = USB_CHARGER_DEFAULT;
+	uchger->id = -1;
+	uchger->sdp_default_cur_change = 0;
+
+	uchger->cur_limit.sdp_cur_limit = DEFAULT_SDP_CUR_LIMIT;
+	uchger->cur_limit.dcp_cur_limit = DEFAULT_DCP_CUR_LIMIT;
+	uchger->cur_limit.cdp_cur_limit = DEFAULT_CDP_CUR_LIMIT;
+	uchger->cur_limit.aca_cur_limit = DEFAULT_ACA_CUR_LIMIT;
+
+	mutex_init(&uchger->lock);
+	RAW_INIT_NOTIFIER_HEAD(&uchger->uchger_nh);
+	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;
+		extcon_register_notifier(edev, EXTCON_USB,
+					 &uchger->extcon_nb.nb);
+	}
+
+	/* to check if the usb charger is link to a power supply */
+	psy = devm_power_supply_get_by_phandle(ugadget->dev.parent,
+					       "power-supplies");
+	if (!IS_ERR_OR_NULL(psy))
+		uchger->psy = psy;
+	else
+		uchger->psy = NULL;
+
+	/* register a notifier on a usb gadget device */
+	uchger->gadget = ugadget;
+	uchger->old_gadget_state = USB_STATE_NOTATTACHED;
+
+	/* register a new usb charger */
+	ret = usb_charger_register(uchger);
+	if (ret)
+		goto fail;
+
+	return 0;
+
+fail:
+	if (uchger->extcon_dev) {
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB,
+					   &uchger->extcon_nb.nb);
+	}
+
+	kfree(uchger);
+	return ret;
+}
+
+int usb_charger_exit(struct usb_gadget *ugadget)
+{
+	return 0;
+}
+
+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..fb5b994
--- /dev/null
+++ b/include/linux/usb/charger.h
@@ -0,0 +1,186 @@
+#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 limitation by charger type */
+struct usb_charger_cur_limit {
+	unsigned int sdp_cur_limit;
+	unsigned int dcp_cur_limit;
+	unsigned int cdp_cur_limit;
+	unsigned int aca_cur_limit;
+};
+
+struct usb_charger_nb {
+	struct notifier_block	nb;
+	struct usb_charger	*uchger;
+};
+
+/**
+ * struct usb_charger - 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.
+ * @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.
+ * @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.
+ * @psy: User can get the charger type from power supply if the power supply
+ * supports POWER_SUPPLY_PROP_CHARGE_TYPE property.
+ * @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.
+ * @cur_limit: Describe the current limitation by charger type.
+ * @sdp_default_cur_change: Check if it is needed to change the SDP charger
+ * default current.
+ *
+ * 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 work_struct	work;
+
+	struct extcon_dev	*extcon_dev;
+	struct usb_charger_nb	extcon_nb;
+
+	struct usb_gadget	*gadget;
+	enum usb_device_state	old_gadget_state;
+
+	struct power_supply	*psy;
+
+	enum usb_charger_type	(*get_charger_type)(struct usb_charger *);
+	enum usb_charger_type	(*charger_detect)(struct usb_charger *);
+
+	struct usb_charger_cur_limit	cur_limit;
+	unsigned int		sdp_default_cur_change;
+};
+
+#ifdef CONFIG_USB_CHARGER
+extern struct usb_charger *usb_charger_find_by_name(const char *name);
+
+extern int usb_charger_register_notify(struct usb_charger *uchger,
+				       struct notifier_block *nb);
+extern int usb_charger_unregister_notify(struct usb_charger *uchger,
+					 struct notifier_block *nb);
+
+extern int usb_charger_set_cur_limit(struct usb_charger *uchger,
+				struct usb_charger_cur_limit *cur_limit_set);
+extern int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+					     enum usb_charger_type type,
+					     unsigned int cur_limit);
+extern int usb_charger_set_cur_limit_by_gadget(struct usb_gadget *gadget,
+					       unsigned int cur_limit);
+extern unsigned int usb_charger_get_cur_limit(struct usb_charger *uchger);
+
+extern int usb_charger_plug_by_gadget(struct usb_gadget *gadget,
+				      unsigned long state);
+extern enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger);
+extern int usb_charger_detect_type(struct usb_charger *uchger);
+extern enum usb_charger_state usb_charger_get_state(struct usb_charger *uchger);
+
+extern int usb_charger_init(struct usb_gadget *ugadget);
+extern int usb_charger_exit(struct usb_gadget *ugadget);
+#else
+static inline struct usb_charger *usb_charger_find_by_name(const char *name)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline int
+usb_charger_register_notify(struct usb_charger *uchger,
+			    struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_unregister_notify(struct usb_charger *uchger,
+			      struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_cur_limit(struct usb_charger *uchger,
+			  struct usb_charger_cur_limit *cur_limit_set)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
+				  enum usb_charger_type type,
+				  unsigned int cur_limit)
+{
+	return 0;
+}
+
+static inline int
+usb_charger_set_cur_limit_by_gadget(struct usb_gadget *gadget,
+				    unsigned int cur_limit)
+{
+	return 0;
+}
+
+static inline unsigned int
+usb_charger_get_cur_limit(struct usb_charger *uchger)
+{
+	return 0;
+}
+
+static inline enum usb_charger_type
+usb_charger_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 v16 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 |   17 +++++++++++++++++
 include/linux/usb/gadget.h    |    3 +++
 2 files changed, 20 insertions(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index ff8685e..bdf1874 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>
@@ -578,12 +579,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;
@@ -965,6 +971,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");
 }
@@ -1134,6 +1143,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;
 
@@ -1154,6 +1167,9 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	return 0;
 
+err5:
+	device_del(&udc->dev);
+
 err4:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
@@ -1262,6 +1278,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 612dbdf..c864b51 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.
@@ -385,6 +387,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 v16 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 |   87 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/charger.c b/drivers/usb/gadget/udc/charger.c
index 462faa1..13f53b1 100644
--- a/drivers/usb/gadget/udc/charger.c
+++ b/drivers/usb/gadget/udc/charger.c
@@ -36,7 +36,9 @@ static unsigned int __usb_charger_get_cur_limit(struct usb_charger *uchger);
 
 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;
 }
 
 /*
@@ -325,6 +327,18 @@ static void usb_charger_notify_work(struct work_struct *work)
 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);
@@ -582,11 +596,68 @@ 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 = UNKNOWN_TYPE;
+			usb_charger_get_type_by_others(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_others(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);
+
+	kfree(uchger);
+	return 0;
+}
+
+/*
  * usb_charger_register() - Register a new usb charger.
  * @uchger - the new usb charger instance.
  */
@@ -667,6 +738,7 @@ int usb_charger_init(struct usb_gadget *ugadget)
 
 	/* register a notifier on a usb gadget device */
 	uchger->gadget = ugadget;
+	ugadget->charger = uchger;
 	uchger->old_gadget_state = USB_STATE_NOTATTACHED;
 
 	/* register a new usb charger */
@@ -689,7 +761,18 @@ fail:
 
 int usb_charger_exit(struct usb_gadget *ugadget)
 {
-	return 0;
+	struct usb_charger *uchger = ugadget->charger;
+
+	if (!uchger)
+		return -EINVAL;
+
+	ugadget->charger = NULL;
+	if (uchger->extcon_dev)
+		extcon_unregister_notifier(uchger->extcon_dev,
+					   EXTCON_USB,
+					   &uchger->extcon_nb.nb);
+
+	return usb_charger_unregister(uchger);
 }
 
 MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
-- 
1.7.9.5

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

[Baolin Wang]

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

Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Acked-by: Peter Chen <peter.chen@freescale.com>
Acked-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/power/wm831x_power.c     |   69 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/wm831x/pdata.h |    3 ++
 2 files changed, 72 insertions(+)

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

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

[Baolin Wang]

Hi Felipe,

On 1 August 2016 at 15:09, 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.

Could you please apply this patchset into your 'next' branch if you
have no comments about it? Thank you.

>
> 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     |   69 ++++
>  drivers/usb/gadget/Kconfig       |    8 +
>  drivers/usb/gadget/udc/Makefile  |    1 +
>  drivers/usb/gadget/udc/charger.c |  780 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/udc/core.c    |   17 +
>  include/linux/mfd/wm831x/pdata.h |    3 +
>  include/linux/usb/charger.h      |  186 +++++++++
>  include/linux/usb/gadget.h       |    3 +
>  include/uapi/linux/usb/charger.h |   31 ++
>  9 files changed, 1098 insertions(+)
>  create mode 100644 drivers/usb/gadget/udc/charger.c
>  create mode 100644 include/linux/usb/charger.h
>  create mode 100644 include/uapi/linux/usb/charger.h
>
> --
> 1.7.9.5
>



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Felipe,

On 1 August 2016 at 15:09, Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

Could you please apply this patchset into your 'next' branch if you
have no comments about it? Thank you.

>
> 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     |   69 ++++
>  drivers/usb/gadget/Kconfig       |    8 +
>  drivers/usb/gadget/udc/Makefile  |    1 +
>  drivers/usb/gadget/udc/charger.c |  780 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/udc/core.c    |   17 +
>  include/linux/mfd/wm831x/pdata.h |    3 +
>  include/linux/usb/charger.h      |  186 +++++++++
>  include/linux/usb/gadget.h       |    3 +
>  include/uapi/linux/usb/charger.h |   31 ++
>  9 files changed, 1098 insertions(+)
>  create mode 100644 drivers/usb/gadget/udc/charger.c
>  create mode 100644 include/linux/usb/charger.h
>  create mode 100644 include/uapi/linux/usb/charger.h
>
> --
> 1.7.9.5
>



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Baolin Wang]

Hi Felipe,

On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Felipe,
>
> On 1 August 2016 at 15:09, 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.
>
> Could you please apply this patchset into your 'next' branch if you
> have no comments about it? Thank you.

Since there are no other comments about this patchset for a long time,
could you please apply this patchset? Thanks.

>
>>
>> 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     |   69 ++++
>>  drivers/usb/gadget/Kconfig       |    8 +
>>  drivers/usb/gadget/udc/Makefile  |    1 +
>>  drivers/usb/gadget/udc/charger.c |  780 ++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/gadget/udc/core.c    |   17 +
>>  include/linux/mfd/wm831x/pdata.h |    3 +
>>  include/linux/usb/charger.h      |  186 +++++++++
>>  include/linux/usb/gadget.h       |    3 +
>>  include/uapi/linux/usb/charger.h |   31 ++
>>  9 files changed, 1098 insertions(+)
>>  create mode 100644 drivers/usb/gadget/udc/charger.c
>>  create mode 100644 include/linux/usb/charger.h
>>  create mode 100644 include/uapi/linux/usb/charger.h
>>
>> --
>> 1.7.9.5
>>
>
>
>
> --
> Baolin.wang
> Best Regards



-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Mon, Aug 29 2016, Baolin Wang wrote:

> Hi Felipe,
>
> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>> Hi Felipe,
>>
>> On 1 August 2016 at 15:09, 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.
>>
>> Could you please apply this patchset into your 'next' branch if you
>> have no comments about it? Thank you.
>
> Since there are no other comments about this patchset for a long time,
> could you please apply this patchset? Thanks.

Sorry, I should have replied earlier.  Tim Bird mentioned on the
ksummit-discuss list that there was a frustration with this not making
progress so I decided to contribute what I could now.

I think this patch set is attempting to address an important problem
that needs solving.  However I think it gets some key aspects wrong.
Maybe they can get fixed up after the patchset is upstream, maybe they
should be fixed first - I have no strong opinion on that.

My main complaints involve the detection and handling of the different
charger types - DCP, CDP, ACA etc.
The big-picture requirement here that the PHY will detect the physical
properties of the cable (e.g. resistance to ground on ID) and determine
the type of charger expected.  This information must be communicated to
the PMIC "power_supply" device so it can regulate the power being drawn
through the cable.

The first problem is that there are two different ways that the
distinction between DCP, CDP, ACA etc can be represented in Linux.  They
are cable types in the 'extcon' subsystem, and they are power_supply
types in the 'power_supply' subsystem.  This duplication is confusing.
It is not caused by your patch set, but I believe your patchset needs to
work with the duplication and I think it does so poorly.

In my mind, the power_supply should *not* know about this distinction at
all (except possibly as an advisor attribute simiarly to the current
battery technology attribute).  The other types it knows of are "AC",
"USB", and "BATTERY".  The contrast between these is quite different
From the contrast between DCP, CDP, ACA, which, from the perspective of
the power supply, are almost irrelevant.  Your patchset effectively
examines the power_supply_type of one power_supply, and communicates it
to another.  It isn't clear to me how the first power_supply gets the
information, or what the relationship between the two power_supplies is
meant to be.

It makes much more sense, to me, to utilized the knowledge of this
distinction that extcon provides.  A usb PHY can register an extcon,
declare the sorts of cables that it can detect, and tell the extcon as
cables appear or disappear.  The PMIC power_supply can then register with
that extcon for events and can find out when a cable is attached, and
what sort of cable.
Your usb-charging framework would be well placed to help the
power_supply to find the correct extcon, and possibly even to handle the
registration for events.

Your framework does currently register with extcon, but only listens for
EXTCON_USB cables.  I don't think that cable type is (reliably) reported
when a DCP (for example) is plugged in.

Here there is another problem that is not of your making, but still
needs fixing.  Extcon declares a number of cable types like:

/* USB external connector */
#define EXTCON_USB              1
#define EXTCON_USB_HOST         2

/* Charging external connector */
#define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
#define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
#define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
#define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
#define EXTCON_CHG_USB_FAST     9
#define EXTCON_CHG_USB_SLOW     10

However it doesn't define what those mean, so we are left to guess.
They each correspond to bits in a bitmap, so a cable can have multiple types.
I think the best interpretation is that:

 EXTCON_USB means that the cable carries USB data from a host.
 EXTCON_USB_HOST means that that cable carries USB data to a host.
 EXTCON_CHG_* means that power is available as described in the
 standards.
 (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
 clear).

There is some support for this in the code, but it is not universally
acknowledged.  For a USB charging framework to be genuinely useful, it
must (I think) make sure this issue gets clarified, and the various
cable types used properly.

Once the cable/charger type has been determined, the PMIC power_supply
needs to use this information.  Your current patches make use of this
information in ways that are wrong in two respects.

Firstly, you have made the current limit associated with each cable type
configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
The standard (e.g. BC-1.2) declares what the current limits are.  There
is no reason for those not to be hard coded.

Secondly, you treat each charger type as having a single "cur_limit" and
utilize that limit by telling the PMIC to draw that much current.
Again, this is inconsistent with the specification.
BC-1.2 defines, for each charger type, a minimum and maximum current
level.
The minimum should always be available.   Attempting to draw more than
that (but less that the max) might work or might cause the charger
to shut down, but you can be sure that the charger will respond to the
increased load by first reducing the voltage, and will not shut down
until the voltage has dropped below 2V.
If you try to draw more current than the maximum, then the charger might
shut down before the voltage drops below 2V.

Given this understanding of the current available from the charger,
there are two approaches the PMIC might take.
1/ if the PMIC is able to exercise fine control over the current it
  draws, and if it can monitor the voltage on the charger, then it
  could gradually increase the power being requested until the voltage
  drops below some threshold (e.g. 4.75V), and then (probably) back off
  a little.  It should only increase at most up to the maximum, even if
  the voltage remains high.  It should probably start at zero rather
  than at the minimum.  This allows for lossage elsewhere.

2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
   control of the current requested, then it should request only the
   minimum available current.  Any more is not safe.

So your USB charging framework should communicate the MIN and MAX as
specified in the standard, and allow the PMIC to use that information as
appropriate.
A library routine to support the incremental increase in current drain
would probably be appreciated by PMIC driver writers.


A more details discussion of this problem-space can be found at
   https://lwn.net/Articles/693027/
and an analysis of the functionality currently available in the kernel
(which adds a number of specifics to the above) can be found at
   https://lwn.net/Articles/694062/

NeilBrown



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

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

[NeilBrown]

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

On Mon, Aug 29 2016, Baolin Wang wrote:

> Hi Felipe,
>
> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>> Hi Felipe,
>>
>> On 1 August 2016 at 15:09, 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.
>>
>> Could you please apply this patchset into your 'next' branch if you
>> have no comments about it? Thank you.
>
> Since there are no other comments about this patchset for a long time,
> could you please apply this patchset? Thanks.

Sorry, I should have replied earlier.  Tim Bird mentioned on the
ksummit-discuss list that there was a frustration with this not making
progress so I decided to contribute what I could now.

I think this patch set is attempting to address an important problem
that needs solving.  However I think it gets some key aspects wrong.
Maybe they can get fixed up after the patchset is upstream, maybe they
should be fixed first - I have no strong opinion on that.

My main complaints involve the detection and handling of the different
charger types - DCP, CDP, ACA etc.
The big-picture requirement here that the PHY will detect the physical
properties of the cable (e.g. resistance to ground on ID) and determine
the type of charger expected.  This information must be communicated to
the PMIC "power_supply" device so it can regulate the power being drawn
through the cable.

The first problem is that there are two different ways that the
distinction between DCP, CDP, ACA etc can be represented in Linux.  They
are cable types in the 'extcon' subsystem, and they are power_supply
types in the 'power_supply' subsystem.  This duplication is confusing.
It is not caused by your patch set, but I believe your patchset needs to
work with the duplication and I think it does so poorly.

In my mind, the power_supply should *not* know about this distinction at
all (except possibly as an advisor attribute simiarly to the current
battery technology attribute).  The other types it knows of are "AC",
"USB", and "BATTERY".  The contrast between these is quite different
From the contrast between DCP, CDP, ACA, which, from the perspective of
the power supply, are almost irrelevant.  Your patchset effectively
examines the power_supply_type of one power_supply, and communicates it
to another.  It isn't clear to me how the first power_supply gets the
information, or what the relationship between the two power_supplies is
meant to be.

It makes much more sense, to me, to utilized the knowledge of this
distinction that extcon provides.  A usb PHY can register an extcon,
declare the sorts of cables that it can detect, and tell the extcon as
cables appear or disappear.  The PMIC power_supply can then register with
that extcon for events and can find out when a cable is attached, and
what sort of cable.
Your usb-charging framework would be well placed to help the
power_supply to find the correct extcon, and possibly even to handle the
registration for events.

Your framework does currently register with extcon, but only listens for
EXTCON_USB cables.  I don't think that cable type is (reliably) reported
when a DCP (for example) is plugged in.

Here there is another problem that is not of your making, but still
needs fixing.  Extcon declares a number of cable types like:

/* USB external connector */
#define EXTCON_USB              1
#define EXTCON_USB_HOST         2

/* Charging external connector */
#define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
#define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
#define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
#define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
#define EXTCON_CHG_USB_FAST     9
#define EXTCON_CHG_USB_SLOW     10

However it doesn't define what those mean, so we are left to guess.
They each correspond to bits in a bitmap, so a cable can have multiple types.
I think the best interpretation is that:

 EXTCON_USB means that the cable carries USB data from a host.
 EXTCON_USB_HOST means that that cable carries USB data to a host.
 EXTCON_CHG_* means that power is available as described in the
 standards.
 (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
 clear).

There is some support for this in the code, but it is not universally
acknowledged.  For a USB charging framework to be genuinely useful, it
must (I think) make sure this issue gets clarified, and the various
cable types used properly.

Once the cable/charger type has been determined, the PMIC power_supply
needs to use this information.  Your current patches make use of this
information in ways that are wrong in two respects.

Firstly, you have made the current limit associated with each cable type
configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
The standard (e.g. BC-1.2) declares what the current limits are.  There
is no reason for those not to be hard coded.

Secondly, you treat each charger type as having a single "cur_limit" and
utilize that limit by telling the PMIC to draw that much current.
Again, this is inconsistent with the specification.
BC-1.2 defines, for each charger type, a minimum and maximum current
level.
The minimum should always be available.   Attempting to draw more than
that (but less that the max) might work or might cause the charger
to shut down, but you can be sure that the charger will respond to the
increased load by first reducing the voltage, and will not shut down
until the voltage has dropped below 2V.
If you try to draw more current than the maximum, then the charger might
shut down before the voltage drops below 2V.

Given this understanding of the current available from the charger,
there are two approaches the PMIC might take.
1/ if the PMIC is able to exercise fine control over the current it
  draws, and if it can monitor the voltage on the charger, then it
  could gradually increase the power being requested until the voltage
  drops below some threshold (e.g. 4.75V), and then (probably) back off
  a little.  It should only increase at most up to the maximum, even if
  the voltage remains high.  It should probably start at zero rather
  than at the minimum.  This allows for lossage elsewhere.

2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
   control of the current requested, then it should request only the
   minimum available current.  Any more is not safe.

So your USB charging framework should communicate the MIN and MAX as
specified in the standard, and allow the PMIC to use that information as
appropriate.
A library routine to support the incremental increase in current drain
would probably be appreciated by PMIC driver writers.


A more details discussion of this problem-space can be found at
   https://lwn.net/Articles/693027/
and an analysis of the functionality currently available in the kernel
(which adds a number of specifics to the above) can be found at
   https://lwn.net/Articles/694062/

NeilBrown



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

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

[Felipe Balbi]

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


Hi,

NeilBrown <nfbrown@novell.com> writes:
> Firstly, you have made the current limit associated with each cable type
> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
> The standard (e.g. BC-1.2) declares what the current limits are.  There
> is no reason for those not to be hard coded.

I had raised the same concern WRT configuration current limits.

> Secondly, you treat each charger type as having a single "cur_limit" and
> utilize that limit by telling the PMIC to draw that much current.
> Again, this is inconsistent with the specification.
> BC-1.2 defines, for each charger type, a minimum and maximum current
> level.
> The minimum should always be available.   Attempting to draw more than
> that (but less that the max) might work or might cause the charger
> to shut down, but you can be sure that the charger will respond to the
> increased load by first reducing the voltage, and will not shut down
> until the voltage has dropped below 2V.
> If you try to draw more current than the maximum, then the charger might
> shut down before the voltage drops below 2V.

Very well put :-)

> Given this understanding of the current available from the charger,
> there are two approaches the PMIC might take.
> 1/ if the PMIC is able to exercise fine control over the current it
>   draws, and if it can monitor the voltage on the charger, then it
>   could gradually increase the power being requested until the voltage
>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>   a little.  It should only increase at most up to the maximum, even if
>   the voltage remains high.  It should probably start at zero rather
>   than at the minimum.  This allows for lossage elsewhere.

That's what most charging control SW I've seen in the past ends up
doing. Correct

> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>    control of the current requested, then it should request only the
>    minimum available current.  Any more is not safe.

correct

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

NeilBrown <nfbrown-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org> writes:
> Firstly, you have made the current limit associated with each cable type
> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
> The standard (e.g. BC-1.2) declares what the current limits are.  There
> is no reason for those not to be hard coded.

I had raised the same concern WRT configuration current limits.

> Secondly, you treat each charger type as having a single "cur_limit" and
> utilize that limit by telling the PMIC to draw that much current.
> Again, this is inconsistent with the specification.
> BC-1.2 defines, for each charger type, a minimum and maximum current
> level.
> The minimum should always be available.   Attempting to draw more than
> that (but less that the max) might work or might cause the charger
> to shut down, but you can be sure that the charger will respond to the
> increased load by first reducing the voltage, and will not shut down
> until the voltage has dropped below 2V.
> If you try to draw more current than the maximum, then the charger might
> shut down before the voltage drops below 2V.

Very well put :-)

> Given this understanding of the current available from the charger,
> there are two approaches the PMIC might take.
> 1/ if the PMIC is able to exercise fine control over the current it
>   draws, and if it can monitor the voltage on the charger, then it
>   could gradually increase the power being requested until the voltage
>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>   a little.  It should only increase at most up to the maximum, even if
>   the voltage remains high.  It should probably start at zero rather
>   than at the minimum.  This allows for lossage elsewhere.

That's what most charging control SW I've seen in the past ends up
doing. Correct

> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>    control of the current requested, then it should request only the
>    minimum available current.  Any more is not safe.

correct

-- 
balbi

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

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

[Baolin Wang]

Hi Neil,

On 6 September 2016 at 13:40, NeilBrown <nfbrown@novell.com> wrote:
> On Mon, Aug 29 2016, Baolin Wang wrote:
>
>> Hi Felipe,
>>
>> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> Hi Felipe,
>>>
>>> On 1 August 2016 at 15:09, 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.
>>>
>>> Could you please apply this patchset into your 'next' branch if you
>>> have no comments about it? Thank you.
>>
>> Since there are no other comments about this patchset for a long time,
>> could you please apply this patchset? Thanks.
>

Sorry for the late reply.

> Sorry, I should have replied earlier.  Tim Bird mentioned on the
> ksummit-discuss list that there was a frustration with this not making
> progress so I decided to contribute what I could now.
>
> I think this patch set is attempting to address an important problem
> that needs solving.  However I think it gets some key aspects wrong.
> Maybe they can get fixed up after the patchset is upstream, maybe they
> should be fixed first - I have no strong opinion on that.
>
> My main complaints involve the detection and handling of the different
> charger types - DCP, CDP, ACA etc.
> The big-picture requirement here that the PHY will detect the physical
> properties of the cable (e.g. resistance to ground on ID) and determine
> the type of charger expected.  This information must be communicated to
> the PMIC "power_supply" device so it can regulate the power being drawn
> through the cable.
>
> The first problem is that there are two different ways that the
> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
> are cable types in the 'extcon' subsystem, and they are power_supply
> types in the 'power_supply' subsystem.  This duplication is confusing.
> It is not caused by your patch set, but I believe your patchset needs to
> work with the duplication and I think it does so poorly.

Now the usb charger will not get charger type from 'extcon' subsystem,
we get the charger type from 'power_supply' and calllback
'get_charger_type' for users.

>
> In my mind, the power_supply should *not* know about this distinction at
> all (except possibly as an advisor attribute simiarly to the current
> battery technology attribute).  The other types it knows of are "AC",
> "USB", and "BATTERY".  The contrast between these is quite different
> From the contrast between DCP, CDP, ACA, which, from the perspective of
> the power supply, are almost irrelevant.  Your patchset effectively
> examines the power_supply_type of one power_supply, and communicates it
> to another.  It isn't clear to me how the first power_supply gets the
> information, or what the relationship between the two power_supplies is
> meant to be.

We just get the charger type from the power supply which can get the
charger type from register or something else, and the usb charger did
nothing for this power supply. In some platform, the charger type is
get in power supply driver, thus we should link this power supply to
get the charger type when USB cable is plugin. If you don't get
charger type from power supply driver, then it does not need to link
this power supply phandle.

>
> It makes much more sense, to me, to utilized the knowledge of this
> distinction that extcon provides.  A usb PHY can register an extcon,
> declare the sorts of cables that it can detect, and tell the extcon as
> cables appear or disappear.  The PMIC power_supply can then register with
> that extcon for events and can find out when a cable is attached, and
> what sort of cable.
> Your usb-charging framework would be well placed to help the
> power_supply to find the correct extcon, and possibly even to handle the
> registration for events.
>
> Your framework does currently register with extcon, but only listens for
> EXTCON_USB cables.  I don't think that cable type is (reliably) reported
> when a DCP (for example) is plugged in.

Here we just listen the plugin/out events from extcon, if one cable
plugin it will report to usb charger.

>
> Here there is another problem that is not of your making, but still
> needs fixing.  Extcon declares a number of cable types like:
>
> /* USB external connector */
> #define EXTCON_USB              1
> #define EXTCON_USB_HOST         2
>
> /* Charging external connector */
> #define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
> #define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
> #define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
> #define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
> #define EXTCON_CHG_USB_FAST     9
> #define EXTCON_CHG_USB_SLOW     10
>
> However it doesn't define what those mean, so we are left to guess.
> They each correspond to bits in a bitmap, so a cable can have multiple types.
> I think the best interpretation is that:
>
>  EXTCON_USB means that the cable carries USB data from a host.
>  EXTCON_USB_HOST means that that cable carries USB data to a host.
>  EXTCON_CHG_* means that power is available as described in the
>  standards.
>  (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
>  clear).
>
> There is some support for this in the code, but it is not universally
> acknowledged.  For a USB charging framework to be genuinely useful, it
> must (I think) make sure this issue gets clarified, and the various
> cable types used properly.

Here I agree with you, now the usb charger does not listen the charger
type from extcon which need to be fixed.

>
> Once the cable/charger type has been determined, the PMIC power_supply
> needs to use this information.  Your current patches make use of this
> information in ways that are wrong in two respects.
>
> Firstly, you have made the current limit associated with each cable type
> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
> The standard (e.g. BC-1.2) declares what the current limits are.  There
> is no reason for those not to be hard coded.

Yes, but you must think about some special cases on some platforms.
Users may need to change the current in some situations, thus we
should export one API for users to change the current. (I think you
misunderstand the current limit here, that is the current for power
driver  to draw).

>
> Secondly, you treat each charger type as having a single "cur_limit" and
> utilize that limit by telling the PMIC to draw that much current.

This "cur_limit" is set by user or USB configuration what they want,
usb charger just tells the power driver how much current need to draw.

> Again, this is inconsistent with the specification.
> BC-1.2 defines, for each charger type, a minimum and maximum current
> level.
> The minimum should always be available.   Attempting to draw more than
> that (but less that the max) might work or might cause the charger
> to shut down, but you can be sure that the charger will respond to the
> increased load by first reducing the voltage, and will not shut down
> until the voltage has dropped below 2V.
> If you try to draw more current than the maximum, then the charger might
> shut down before the voltage drops below 2V.
>
> Given this understanding of the current available from the charger,
> there are two approaches the PMIC might take.
> 1/ if the PMIC is able to exercise fine control over the current it
>   draws, and if it can monitor the voltage on the charger, then it
>   could gradually increase the power being requested until the voltage
>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>   a little.  It should only increase at most up to the maximum, even if
>   the voltage remains high.  It should probably start at zero rather
>   than at the minimum.  This allows for lossage elsewhere.
>
> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>    control of the current requested, then it should request only the
>    minimum available current.  Any more is not safe.

Make sense.

>
> So your USB charging framework should communicate the MIN and MAX as
> specified in the standard, and allow the PMIC to use that information as
> appropriate.
> A library routine to support the incremental increase in current drain
> would probably be appreciated by PMIC driver writers.

I should say again, the current is set by user or USB configuration
what they want, USB charger just tells the power driver how much
current need to draw. But we can add some checking if the setting
current is in the right range.

Thanks for your comments.

>
>
> A more details discussion of this problem-space can be found at
>    https://lwn.net/Articles/693027/
> and an analysis of the functionality currently available in the kernel
> (which adds a number of specifics to the above) can be found at
>    https://lwn.net/Articles/694062/
>
> NeilBrown
>
>



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Neil,

On 6 September 2016 at 13:40, NeilBrown <nfbrown@novell.com> wrote:
> On Mon, Aug 29 2016, Baolin Wang wrote:
>
>> Hi Felipe,
>>
>> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> Hi Felipe,
>>>
>>> On 1 August 2016 at 15:09, 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.
>>>
>>> Could you please apply this patchset into your 'next' branch if you
>>> have no comments about it? Thank you.
>>
>> Since there are no other comments about this patchset for a long time,
>> could you please apply this patchset? Thanks.
>

Sorry for the late reply.

> Sorry, I should have replied earlier.  Tim Bird mentioned on the
> ksummit-discuss list that there was a frustration with this not making
> progress so I decided to contribute what I could now.
>
> I think this patch set is attempting to address an important problem
> that needs solving.  However I think it gets some key aspects wrong.
> Maybe they can get fixed up after the patchset is upstream, maybe they
> should be fixed first - I have no strong opinion on that.
>
> My main complaints involve the detection and handling of the different
> charger types - DCP, CDP, ACA etc.
> The big-picture requirement here that the PHY will detect the physical
> properties of the cable (e.g. resistance to ground on ID) and determine
> the type of charger expected.  This information must be communicated to
> the PMIC "power_supply" device so it can regulate the power being drawn
> through the cable.
>
> The first problem is that there are two different ways that the
> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
> are cable types in the 'extcon' subsystem, and they are power_supply
> types in the 'power_supply' subsystem.  This duplication is confusing.
> It is not caused by your patch set, but I believe your patchset needs to
> work with the duplication and I think it does so poorly.

Now the usb charger will not get charger type from 'extcon' subsystem,
we get the charger type from 'power_supply' and calllback
'get_charger_type' for users.

>
> In my mind, the power_supply should *not* know about this distinction at
> all (except possibly as an advisor attribute simiarly to the current
> battery technology attribute).  The other types it knows of are "AC",
> "USB", and "BATTERY".  The contrast between these is quite different
> From the contrast between DCP, CDP, ACA, which, from the perspective of
> the power supply, are almost irrelevant.  Your patchset effectively
> examines the power_supply_type of one power_supply, and communicates it
> to another.  It isn't clear to me how the first power_supply gets the
> information, or what the relationship between the two power_supplies is
> meant to be.

We just get the charger type from the power supply which can get the
charger type from register or something else, and the usb charger did
nothing for this power supply. In some platform, the charger type is
get in power supply driver, thus we should link this power supply to
get the charger type when USB cable is plugin. If you don't get
charger type from power supply driver, then it does not need to link
this power supply phandle.

>
> It makes much more sense, to me, to utilized the knowledge of this
> distinction that extcon provides.  A usb PHY can register an extcon,
> declare the sorts of cables that it can detect, and tell the extcon as
> cables appear or disappear.  The PMIC power_supply can then register with
> that extcon for events and can find out when a cable is attached, and
> what sort of cable.
> Your usb-charging framework would be well placed to help the
> power_supply to find the correct extcon, and possibly even to handle the
> registration for events.
>
> Your framework does currently register with extcon, but only listens for
> EXTCON_USB cables.  I don't think that cable type is (reliably) reported
> when a DCP (for example) is plugged in.

Here we just listen the plugin/out events from extcon, if one cable
plugin it will report to usb charger.

>
> Here there is another problem that is not of your making, but still
> needs fixing.  Extcon declares a number of cable types like:
>
> /* USB external connector */
> #define EXTCON_USB              1
> #define EXTCON_USB_HOST         2
>
> /* Charging external connector */
> #define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
> #define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
> #define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
> #define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
> #define EXTCON_CHG_USB_FAST     9
> #define EXTCON_CHG_USB_SLOW     10
>
> However it doesn't define what those mean, so we are left to guess.
> They each correspond to bits in a bitmap, so a cable can have multiple types.
> I think the best interpretation is that:
>
>  EXTCON_USB means that the cable carries USB data from a host.
>  EXTCON_USB_HOST means that that cable carries USB data to a host.
>  EXTCON_CHG_* means that power is available as described in the
>  standards.
>  (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
>  clear).
>
> There is some support for this in the code, but it is not universally
> acknowledged.  For a USB charging framework to be genuinely useful, it
> must (I think) make sure this issue gets clarified, and the various
> cable types used properly.

Here I agree with you, now the usb charger does not listen the charger
type from extcon which need to be fixed.

>
> Once the cable/charger type has been determined, the PMIC power_supply
> needs to use this information.  Your current patches make use of this
> information in ways that are wrong in two respects.
>
> Firstly, you have made the current limit associated with each cable type
> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
> The standard (e.g. BC-1.2) declares what the current limits are.  There
> is no reason for those not to be hard coded.

Yes, but you must think about some special cases on some platforms.
Users may need to change the current in some situations, thus we
should export one API for users to change the current. (I think you
misunderstand the current limit here, that is the current for power
driver  to draw).

>
> Secondly, you treat each charger type as having a single "cur_limit" and
> utilize that limit by telling the PMIC to draw that much current.

This "cur_limit" is set by user or USB configuration what they want,
usb charger just tells the power driver how much current need to draw.

> Again, this is inconsistent with the specification.
> BC-1.2 defines, for each charger type, a minimum and maximum current
> level.
> The minimum should always be available.   Attempting to draw more than
> that (but less that the max) might work or might cause the charger
> to shut down, but you can be sure that the charger will respond to the
> increased load by first reducing the voltage, and will not shut down
> until the voltage has dropped below 2V.
> If you try to draw more current than the maximum, then the charger might
> shut down before the voltage drops below 2V.
>
> Given this understanding of the current available from the charger,
> there are two approaches the PMIC might take.
> 1/ if the PMIC is able to exercise fine control over the current it
>   draws, and if it can monitor the voltage on the charger, then it
>   could gradually increase the power being requested until the voltage
>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>   a little.  It should only increase at most up to the maximum, even if
>   the voltage remains high.  It should probably start at zero rather
>   than at the minimum.  This allows for lossage elsewhere.
>
> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>    control of the current requested, then it should request only the
>    minimum available current.  Any more is not safe.

Make sense.

>
> So your USB charging framework should communicate the MIN and MAX as
> specified in the standard, and allow the PMIC to use that information as
> appropriate.
> A library routine to support the incremental increase in current drain
> would probably be appreciated by PMIC driver writers.

I should say again, the current is set by user or USB configuration
what they want, USB charger just tells the power driver how much
current need to draw. But we can add some checking if the setting
current is in the right range.

Thanks for your comments.

>
>
> A more details discussion of this problem-space can be found at
>    https://lwn.net/Articles/693027/
> and an analysis of the functionality currently available in the kernel
> (which adds a number of specifics to the above) can be found at
>    https://lwn.net/Articles/694062/
>
> NeilBrown
>
>



-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Thu, Sep 08 2016, Baolin Wang wrote:

> Hi Neil,
>
> On 6 September 2016 at 13:40, NeilBrown <nfbrown@novell.com> wrote:
>> On Mon, Aug 29 2016, Baolin Wang wrote:
>>
>>> Hi Felipe,
>>>
>>> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>> Hi Felipe,
>>>>
>>>> On 1 August 2016 at 15:09, 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.
>>>>
>>>> Could you please apply this patchset into your 'next' branch if you
>>>> have no comments about it? Thank you.
>>>
>>> Since there are no other comments about this patchset for a long time,
>>> could you please apply this patchset? Thanks.
>>
>
> Sorry for the late reply.
>
>> Sorry, I should have replied earlier.  Tim Bird mentioned on the
>> ksummit-discuss list that there was a frustration with this not making
>> progress so I decided to contribute what I could now.
>>
>> I think this patch set is attempting to address an important problem
>> that needs solving.  However I think it gets some key aspects wrong.
>> Maybe they can get fixed up after the patchset is upstream, maybe they
>> should be fixed first - I have no strong opinion on that.
>>
>> My main complaints involve the detection and handling of the different
>> charger types - DCP, CDP, ACA etc.
>> The big-picture requirement here that the PHY will detect the physical
>> properties of the cable (e.g. resistance to ground on ID) and determine
>> the type of charger expected.  This information must be communicated to
>> the PMIC "power_supply" device so it can regulate the power being drawn
>> through the cable.
>>
>> The first problem is that there are two different ways that the
>> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
>> are cable types in the 'extcon' subsystem, and they are power_supply
>> types in the 'power_supply' subsystem.  This duplication is confusing.
>> It is not caused by your patch set, but I believe your patchset needs to
>> work with the duplication and I think it does so poorly.
>
> Now the usb charger will not get charger type from 'extcon' subsystem,
> we get the charger type from 'power_supply' and calllback
> 'get_charger_type' for users.

I understand this.  I think it is wrong because, in general, the
power_supply doesn't know what the charger_type is (it might know it is
USB, but most don't know which sort of USB).  The PHY knows that, not
the power_supply.


>
>>
>> In my mind, the power_supply should *not* know about this distinction at
>> all (except possibly as an advisor attribute simiarly to the current
>> battery technology attribute).  The other types it knows of are "AC",
>> "USB", and "BATTERY".  The contrast between these is quite different
>> From the contrast between DCP, CDP, ACA, which, from the perspective of
>> the power supply, are almost irrelevant.  Your patchset effectively
>> examines the power_supply_type of one power_supply, and communicates it
>> to another.  It isn't clear to me how the first power_supply gets the
>> information, or what the relationship between the two power_supplies is
>> meant to be.
>
> We just get the charger type from the power supply which can get the
> charger type from register or something else,

But that register would be part of the PHY, not part of the charger.
Having the power_supply driver reading the PHY register might work for
some hardware, but is not a general solution.


>                                                and the usb charger did
> nothing for this power supply. In some platform, the charger type is
> get in power supply driver, thus we should link this power supply to
> get the charger type when USB cable is plugin. If you don't get
> charger type from power supply driver, then it does not need to link
> this power supply phandle.
>
>>
>> It makes much more sense, to me, to utilized the knowledge of this
>> distinction that extcon provides.  A usb PHY can register an extcon,
>> declare the sorts of cables that it can detect, and tell the extcon as
>> cables appear or disappear.  The PMIC power_supply can then register with
>> that extcon for events and can find out when a cable is attached, and
>> what sort of cable.
>> Your usb-charging framework would be well placed to help the
>> power_supply to find the correct extcon, and possibly even to handle the
>> registration for events.
>>
>> Your framework does currently register with extcon, but only listens for
>> EXTCON_USB cables.  I don't think that cable type is (reliably) reported
>> when a DCP (for example) is plugged in.
>
> Here we just listen the plugin/out events from extcon, if one cable
> plugin it will report to usb charger.

As far as I can tell there are no generic "plugin/out events from extcon".
There are only events for a specific cable being plugged or unplugged.
So you need to listed for each different type of cable that you are
interested in.

If it detects a "EXTCON_CHG_USB_ACA plugged" event, then it makes sense
to use that event to tell the power supply that the current profile for
an ACA is available.


>
>>
>> Here there is another problem that is not of your making, but still
>> needs fixing.  Extcon declares a number of cable types like:
>>
>> /* USB external connector */
>> #define EXTCON_USB              1
>> #define EXTCON_USB_HOST         2
>>
>> /* Charging external connector */
>> #define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
>> #define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
>> #define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
>> #define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
>> #define EXTCON_CHG_USB_FAST     9
>> #define EXTCON_CHG_USB_SLOW     10
>>
>> However it doesn't define what those mean, so we are left to guess.
>> They each correspond to bits in a bitmap, so a cable can have multiple types.
>> I think the best interpretation is that:
>>
>>  EXTCON_USB means that the cable carries USB data from a host.
>>  EXTCON_USB_HOST means that that cable carries USB data to a host.
>>  EXTCON_CHG_* means that power is available as described in the
>>  standards.
>>  (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
>>  clear).
>>
>> There is some support for this in the code, but it is not universally
>> acknowledged.  For a USB charging framework to be genuinely useful, it
>> must (I think) make sure this issue gets clarified, and the various
>> cable types used properly.
>
> Here I agree with you, now the usb charger does not listen the charger
> type from extcon which need to be fixed.
>
>>
>> Once the cable/charger type has been determined, the PMIC power_supply
>> needs to use this information.  Your current patches make use of this
>> information in ways that are wrong in two respects.
>>
>> Firstly, you have made the current limit associated with each cable type
>> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
>> The standard (e.g. BC-1.2) declares what the current limits are.  There
>> is no reason for those not to be hard coded.
>
> Yes, but you must think about some special cases on some platforms.
> Users may need to change the current in some situations, thus we
> should export one API for users to change the current. (I think you
> misunderstand the current limit here, that is the current for power
> driver  to draw).

Can you be specific about these "special cases" please?
I cannot think of any.

And what sort of "user" might use that API?  I don't think having an app
on your phone that allows you to set the max-current would be a good
idea, so you must be thinking of some other sort of user.

>
>>
>> Secondly, you treat each charger type as having a single "cur_limit" and
>> utilize that limit by telling the PMIC to draw that much current.
>
> This "cur_limit" is set by user or USB configuration what they want,
> usb charger just tells the power driver how much current need to draw.

You don't tell the power driver "how much current it needs to draw".
You tell it "how much current it is safe to draw".

A power driver cannot draw current that isn't available, and it cannot
draw current that the system as a whole has no use for.
The task of the power driver is to make sure the system doesn't draw
more power than is available, else the voltage will drop and the power
supply might shut off.

>
>> Again, this is inconsistent with the specification.
>> BC-1.2 defines, for each charger type, a minimum and maximum current
>> level.
>> The minimum should always be available.   Attempting to draw more than
>> that (but less that the max) might work or might cause the charger
>> to shut down, but you can be sure that the charger will respond to the
>> increased load by first reducing the voltage, and will not shut down
>> until the voltage has dropped below 2V.
>> If you try to draw more current than the maximum, then the charger might
>> shut down before the voltage drops below 2V.
>>
>> Given this understanding of the current available from the charger,
>> there are two approaches the PMIC might take.
>> 1/ if the PMIC is able to exercise fine control over the current it
>>   draws, and if it can monitor the voltage on the charger, then it
>>   could gradually increase the power being requested until the voltage
>>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>>   a little.  It should only increase at most up to the maximum, even if
>>   the voltage remains high.  It should probably start at zero rather
>>   than at the minimum.  This allows for lossage elsewhere.
>>
>> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>>    control of the current requested, then it should request only the
>>    minimum available current.  Any more is not safe.
>
> Make sense.
>
>>
>> So your USB charging framework should communicate the MIN and MAX as
>> specified in the standard, and allow the PMIC to use that information as
>> appropriate.
>> A library routine to support the incremental increase in current drain
>> would probably be appreciated by PMIC driver writers.
>
> I should say again, the current is set by user or USB configuration
> what they want, USB charger just tells the power driver how much
> current need to draw. But we can add some checking if the setting
> current is in the right range.

The USB configuration tells the Host how much power it needs.  If the
host enables the configuration, that implicitly say that much power is
available, so the power_driver can safely request it.  I'm mostly happy
wit that aspect of your patch set.

For a dedicated charger there is no USB configuration.  The electrical
properties of the cable provide some information about how the charger
will behave, but they only guarantee a minimum.  If you want to use more
than the minimum, you have to do so carefully and watch the voltage level.
You cannot sensible let the user tell you how much current the charger
can provide, because they'll probably get it wrong and the charger will
just shut down.

Thanks,
NeilBrown


>
> Thanks for your comments.
>
>>
>>
>> A more details discussion of this problem-space can be found at
>>    https://lwn.net/Articles/693027/
>> and an analysis of the functionality currently available in the kernel
>> (which adds a number of specifics to the above) can be found at
>>    https://lwn.net/Articles/694062/
>>
>> NeilBrown
>>
>>
>
>
>
> -- 
> Baolin.wang
> Best Regards

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

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

[NeilBrown]

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

On Thu, Sep 08 2016, Baolin Wang wrote:

> Hi Neil,
>
> On 6 September 2016 at 13:40, NeilBrown <nfbrown@novell.com> wrote:
>> On Mon, Aug 29 2016, Baolin Wang wrote:
>>
>>> Hi Felipe,
>>>
>>> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>> Hi Felipe,
>>>>
>>>> On 1 August 2016 at 15:09, 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.
>>>>
>>>> Could you please apply this patchset into your 'next' branch if you
>>>> have no comments about it? Thank you.
>>>
>>> Since there are no other comments about this patchset for a long time,
>>> could you please apply this patchset? Thanks.
>>
>
> Sorry for the late reply.
>
>> Sorry, I should have replied earlier.  Tim Bird mentioned on the
>> ksummit-discuss list that there was a frustration with this not making
>> progress so I decided to contribute what I could now.
>>
>> I think this patch set is attempting to address an important problem
>> that needs solving.  However I think it gets some key aspects wrong.
>> Maybe they can get fixed up after the patchset is upstream, maybe they
>> should be fixed first - I have no strong opinion on that.
>>
>> My main complaints involve the detection and handling of the different
>> charger types - DCP, CDP, ACA etc.
>> The big-picture requirement here that the PHY will detect the physical
>> properties of the cable (e.g. resistance to ground on ID) and determine
>> the type of charger expected.  This information must be communicated to
>> the PMIC "power_supply" device so it can regulate the power being drawn
>> through the cable.
>>
>> The first problem is that there are two different ways that the
>> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
>> are cable types in the 'extcon' subsystem, and they are power_supply
>> types in the 'power_supply' subsystem.  This duplication is confusing.
>> It is not caused by your patch set, but I believe your patchset needs to
>> work with the duplication and I think it does so poorly.
>
> Now the usb charger will not get charger type from 'extcon' subsystem,
> we get the charger type from 'power_supply' and calllback
> 'get_charger_type' for users.

I understand this.  I think it is wrong because, in general, the
power_supply doesn't know what the charger_type is (it might know it is
USB, but most don't know which sort of USB).  The PHY knows that, not
the power_supply.


>
>>
>> In my mind, the power_supply should *not* know about this distinction at
>> all (except possibly as an advisor attribute simiarly to the current
>> battery technology attribute).  The other types it knows of are "AC",
>> "USB", and "BATTERY".  The contrast between these is quite different
>> From the contrast between DCP, CDP, ACA, which, from the perspective of
>> the power supply, are almost irrelevant.  Your patchset effectively
>> examines the power_supply_type of one power_supply, and communicates it
>> to another.  It isn't clear to me how the first power_supply gets the
>> information, or what the relationship between the two power_supplies is
>> meant to be.
>
> We just get the charger type from the power supply which can get the
> charger type from register or something else,

But that register would be part of the PHY, not part of the charger.
Having the power_supply driver reading the PHY register might work for
some hardware, but is not a general solution.


>                                                and the usb charger did
> nothing for this power supply. In some platform, the charger type is
> get in power supply driver, thus we should link this power supply to
> get the charger type when USB cable is plugin. If you don't get
> charger type from power supply driver, then it does not need to link
> this power supply phandle.
>
>>
>> It makes much more sense, to me, to utilized the knowledge of this
>> distinction that extcon provides.  A usb PHY can register an extcon,
>> declare the sorts of cables that it can detect, and tell the extcon as
>> cables appear or disappear.  The PMIC power_supply can then register with
>> that extcon for events and can find out when a cable is attached, and
>> what sort of cable.
>> Your usb-charging framework would be well placed to help the
>> power_supply to find the correct extcon, and possibly even to handle the
>> registration for events.
>>
>> Your framework does currently register with extcon, but only listens for
>> EXTCON_USB cables.  I don't think that cable type is (reliably) reported
>> when a DCP (for example) is plugged in.
>
> Here we just listen the plugin/out events from extcon, if one cable
> plugin it will report to usb charger.

As far as I can tell there are no generic "plugin/out events from extcon".
There are only events for a specific cable being plugged or unplugged.
So you need to listed for each different type of cable that you are
interested in.

If it detects a "EXTCON_CHG_USB_ACA plugged" event, then it makes sense
to use that event to tell the power supply that the current profile for
an ACA is available.


>
>>
>> Here there is another problem that is not of your making, but still
>> needs fixing.  Extcon declares a number of cable types like:
>>
>> /* USB external connector */
>> #define EXTCON_USB              1
>> #define EXTCON_USB_HOST         2
>>
>> /* Charging external connector */
>> #define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
>> #define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
>> #define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
>> #define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
>> #define EXTCON_CHG_USB_FAST     9
>> #define EXTCON_CHG_USB_SLOW     10
>>
>> However it doesn't define what those mean, so we are left to guess.
>> They each correspond to bits in a bitmap, so a cable can have multiple types.
>> I think the best interpretation is that:
>>
>>  EXTCON_USB means that the cable carries USB data from a host.
>>  EXTCON_USB_HOST means that that cable carries USB data to a host.
>>  EXTCON_CHG_* means that power is available as described in the
>>  standards.
>>  (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
>>  clear).
>>
>> There is some support for this in the code, but it is not universally
>> acknowledged.  For a USB charging framework to be genuinely useful, it
>> must (I think) make sure this issue gets clarified, and the various
>> cable types used properly.
>
> Here I agree with you, now the usb charger does not listen the charger
> type from extcon which need to be fixed.
>
>>
>> Once the cable/charger type has been determined, the PMIC power_supply
>> needs to use this information.  Your current patches make use of this
>> information in ways that are wrong in two respects.
>>
>> Firstly, you have made the current limit associated with each cable type
>> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
>> The standard (e.g. BC-1.2) declares what the current limits are.  There
>> is no reason for those not to be hard coded.
>
> Yes, but you must think about some special cases on some platforms.
> Users may need to change the current in some situations, thus we
> should export one API for users to change the current. (I think you
> misunderstand the current limit here, that is the current for power
> driver  to draw).

Can you be specific about these "special cases" please?
I cannot think of any.

And what sort of "user" might use that API?  I don't think having an app
on your phone that allows you to set the max-current would be a good
idea, so you must be thinking of some other sort of user.

>
>>
>> Secondly, you treat each charger type as having a single "cur_limit" and
>> utilize that limit by telling the PMIC to draw that much current.
>
> This "cur_limit" is set by user or USB configuration what they want,
> usb charger just tells the power driver how much current need to draw.

You don't tell the power driver "how much current it needs to draw".
You tell it "how much current it is safe to draw".

A power driver cannot draw current that isn't available, and it cannot
draw current that the system as a whole has no use for.
The task of the power driver is to make sure the system doesn't draw
more power than is available, else the voltage will drop and the power
supply might shut off.

>
>> Again, this is inconsistent with the specification.
>> BC-1.2 defines, for each charger type, a minimum and maximum current
>> level.
>> The minimum should always be available.   Attempting to draw more than
>> that (but less that the max) might work or might cause the charger
>> to shut down, but you can be sure that the charger will respond to the
>> increased load by first reducing the voltage, and will not shut down
>> until the voltage has dropped below 2V.
>> If you try to draw more current than the maximum, then the charger might
>> shut down before the voltage drops below 2V.
>>
>> Given this understanding of the current available from the charger,
>> there are two approaches the PMIC might take.
>> 1/ if the PMIC is able to exercise fine control over the current it
>>   draws, and if it can monitor the voltage on the charger, then it
>>   could gradually increase the power being requested until the voltage
>>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>>   a little.  It should only increase at most up to the maximum, even if
>>   the voltage remains high.  It should probably start at zero rather
>>   than at the minimum.  This allows for lossage elsewhere.
>>
>> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>>    control of the current requested, then it should request only the
>>    minimum available current.  Any more is not safe.
>
> Make sense.
>
>>
>> So your USB charging framework should communicate the MIN and MAX as
>> specified in the standard, and allow the PMIC to use that information as
>> appropriate.
>> A library routine to support the incremental increase in current drain
>> would probably be appreciated by PMIC driver writers.
>
> I should say again, the current is set by user or USB configuration
> what they want, USB charger just tells the power driver how much
> current need to draw. But we can add some checking if the setting
> current is in the right range.

The USB configuration tells the Host how much power it needs.  If the
host enables the configuration, that implicitly say that much power is
available, so the power_driver can safely request it.  I'm mostly happy
wit that aspect of your patch set.

For a dedicated charger there is no USB configuration.  The electrical
properties of the cable provide some information about how the charger
will behave, but they only guarantee a minimum.  If you want to use more
than the minimum, you have to do so carefully and watch the voltage level.
You cannot sensible let the user tell you how much current the charger
can provide, because they'll probably get it wrong and the charger will
just shut down.

Thanks,
NeilBrown


>
> Thanks for your comments.
>
>>
>>
>> A more details discussion of this problem-space can be found at
>>    https://lwn.net/Articles/693027/
>> and an analysis of the functionality currently available in the kernel
>> (which adds a number of specifics to the above) can be found at
>>    https://lwn.net/Articles/694062/
>>
>> NeilBrown
>>
>>
>
>
>
> -- 
> Baolin.wang
> Best Regards

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

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

[Baolin Wang]

On 8 September 2016 at 15:31, NeilBrown <neilb@suse.com> wrote:
> On Thu, Sep 08 2016, Baolin Wang wrote:
>
>> Hi Neil,
>>
>> On 6 September 2016 at 13:40, NeilBrown <nfbrown@novell.com> wrote:
>>> On Mon, Aug 29 2016, Baolin Wang wrote:
>>>
>>>> Hi Felipe,
>>>>
>>>> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>> Hi Felipe,
>>>>>
>>>>> On 1 August 2016 at 15:09, 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.
>>>>>
>>>>> Could you please apply this patchset into your 'next' branch if you
>>>>> have no comments about it? Thank you.
>>>>
>>>> Since there are no other comments about this patchset for a long time,
>>>> could you please apply this patchset? Thanks.
>>>
>>
>> Sorry for the late reply.
>>
>>> Sorry, I should have replied earlier.  Tim Bird mentioned on the
>>> ksummit-discuss list that there was a frustration with this not making
>>> progress so I decided to contribute what I could now.
>>>
>>> I think this patch set is attempting to address an important problem
>>> that needs solving.  However I think it gets some key aspects wrong.
>>> Maybe they can get fixed up after the patchset is upstream, maybe they
>>> should be fixed first - I have no strong opinion on that.
>>>
>>> My main complaints involve the detection and handling of the different
>>> charger types - DCP, CDP, ACA etc.
>>> The big-picture requirement here that the PHY will detect the physical
>>> properties of the cable (e.g. resistance to ground on ID) and determine
>>> the type of charger expected.  This information must be communicated to
>>> the PMIC "power_supply" device so it can regulate the power being drawn
>>> through the cable.
>>>
>>> The first problem is that there are two different ways that the
>>> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
>>> are cable types in the 'extcon' subsystem, and they are power_supply
>>> types in the 'power_supply' subsystem.  This duplication is confusing.
>>> It is not caused by your patch set, but I believe your patchset needs to
>>> work with the duplication and I think it does so poorly.
>>
>> Now the usb charger will not get charger type from 'extcon' subsystem,
>> we get the charger type from 'power_supply' and calllback
>> 'get_charger_type' for users.
>
> I understand this.  I think it is wrong because, in general, the
> power_supply doesn't know what the charger_type is (it might know it is
> USB, but most don't know which sort of USB).  The PHY knows that, not
> the power_supply.

I don't think so. Now many platforms will detect the charger type by
PMIC hardware, and we can get the charger type by PMIC hardware
register. Then power supply driver can access the PMIC register to get
the charger type. Here USB charger just considers if the accessing the
PMIC register to get charger type is implemented in power supply, it
is optional depending on what your platform designed.

>>
>>>
>>> In my mind, the power_supply should *not* know about this distinction at
>>> all (except possibly as an advisor attribute simiarly to the current
>>> battery technology attribute).  The other types it knows of are "AC",
>>> "USB", and "BATTERY".  The contrast between these is quite different
>>> From the contrast between DCP, CDP, ACA, which, from the perspective of
>>> the power supply, are almost irrelevant.  Your patchset effectively
>>> examines the power_supply_type of one power_supply, and communicates it
>>> to another.  It isn't clear to me how the first power_supply gets the
>>> information, or what the relationship between the two power_supplies is
>>> meant to be.
>>
>> We just get the charger type from the power supply which can get the
>> charger type from register or something else,
>
> But that register would be part of the PHY, not part of the charger.
> Having the power_supply driver reading the PHY register might work for
> some hardware, but is not a general solution.

Not only PHY. Like I said, the charger type detection can be done by
PMIC hardware, power supply can get the charger type by accessing PMIC
registers.

>>                                                and the usb charger did
>> nothing for this power supply. In some platform, the charger type is
>> get in power supply driver, thus we should link this power supply to
>> get the charger type when USB cable is plugin. If you don't get
>> charger type from power supply driver, then it does not need to link
>> this power supply phandle.
>>
>>>
>>> It makes much more sense, to me, to utilized the knowledge of this
>>> distinction that extcon provides.  A usb PHY can register an extcon,
>>> declare the sorts of cables that it can detect, and tell the extcon as
>>> cables appear or disappear.  The PMIC power_supply can then register with
>>> that extcon for events and can find out when a cable is attached, and
>>> what sort of cable.
>>> Your usb-charging framework would be well placed to help the
>>> power_supply to find the correct extcon, and possibly even to handle the
>>> registration for events.
>>>
>>> Your framework does currently register with extcon, but only listens for
>>> EXTCON_USB cables.  I don't think that cable type is (reliably) reported
>>> when a DCP (for example) is plugged in.
>>
>> Here we just listen the plugin/out events from extcon, if one cable
>> plugin it will report to usb charger.
>
> As far as I can tell there are no generic "plugin/out events from extcon".
> There are only events for a specific cable being plugged or unplugged.

Yeah, that is what I meaning. Now we just listen plugged or unplugged
events from extcon.

> So you need to listed for each different type of cable that you are
> interested in.
>
> If it detects a "EXTCON_CHG_USB_ACA plugged" event, then it makes sense
> to use that event to tell the power supply that the current profile for
> an ACA is available.

Yes, I understand. Now USB charger doses not listen for each different
type of cable, which need to add.

>>
>>>
>>> Here there is another problem that is not of your making, but still
>>> needs fixing.  Extcon declares a number of cable types like:
>>>
>>> /* USB external connector */
>>> #define EXTCON_USB              1
>>> #define EXTCON_USB_HOST         2
>>>
>>> /* Charging external connector */
>>> #define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
>>> #define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
>>> #define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
>>> #define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
>>> #define EXTCON_CHG_USB_FAST     9
>>> #define EXTCON_CHG_USB_SLOW     10
>>>
>>> However it doesn't define what those mean, so we are left to guess.
>>> They each correspond to bits in a bitmap, so a cable can have multiple types.
>>> I think the best interpretation is that:
>>>
>>>  EXTCON_USB means that the cable carries USB data from a host.
>>>  EXTCON_USB_HOST means that that cable carries USB data to a host.
>>>  EXTCON_CHG_* means that power is available as described in the
>>>  standards.
>>>  (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
>>>  clear).
>>>
>>> There is some support for this in the code, but it is not universally
>>> acknowledged.  For a USB charging framework to be genuinely useful, it
>>> must (I think) make sure this issue gets clarified, and the various
>>> cable types used properly.
>>
>> Here I agree with you, now the usb charger does not listen the charger
>> type from extcon which need to be fixed.
>>
>>>
>>> Once the cable/charger type has been determined, the PMIC power_supply
>>> needs to use this information.  Your current patches make use of this
>>> information in ways that are wrong in two respects.
>>>
>>> Firstly, you have made the current limit associated with each cable type
>>> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
>>> The standard (e.g. BC-1.2) declares what the current limits are.  There
>>> is no reason for those not to be hard coded.
>>
>> Yes, but you must think about some special cases on some platforms.
>> Users may need to change the current in some situations, thus we
>> should export one API for users to change the current. (I think you
>> misunderstand the current limit here, that is the current for power
>> driver  to draw).
>
> Can you be specific about these "special cases" please?
> I cannot think of any.

Suppose the USB configuration requests 100mA, then we should set the
USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
funtion, then notify this to power driver.

>
> And what sort of "user" might use that API?  I don't think having an app
> on your phone that allows you to set the max-current would be a good
> idea, so you must be thinking of some other sort of user.
>
>>
>>>
>>> Secondly, you treat each charger type as having a single "cur_limit" and
>>> utilize that limit by telling the PMIC to draw that much current.
>>
>> This "cur_limit" is set by user or USB configuration what they want,
>> usb charger just tells the power driver how much current need to draw.
>
> You don't tell the power driver "how much current it needs to draw".
> You tell it "how much current it is safe to draw".
>
> A power driver cannot draw current that isn't available, and it cannot
> draw current that the system as a whole has no use for.
> The task of the power driver is to make sure the system doesn't draw
> more power than is available, else the voltage will drop and the power
> supply might shut off.

How much current in USB charger is set by USB configuration or power
users, but yes, we can check if the current is available in USB
charger. If user set one unavailable we can ignore that setting.

>
>>
>>> Again, this is inconsistent with the specification.
>>> BC-1.2 defines, for each charger type, a minimum and maximum current
>>> level.
>>> The minimum should always be available.   Attempting to draw more than
>>> that (but less that the max) might work or might cause the charger
>>> to shut down, but you can be sure that the charger will respond to the
>>> increased load by first reducing the voltage, and will not shut down
>>> until the voltage has dropped below 2V.
>>> If you try to draw more current than the maximum, then the charger might
>>> shut down before the voltage drops below 2V.
>>>
>>> Given this understanding of the current available from the charger,
>>> there are two approaches the PMIC might take.
>>> 1/ if the PMIC is able to exercise fine control over the current it
>>>   draws, and if it can monitor the voltage on the charger, then it
>>>   could gradually increase the power being requested until the voltage
>>>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>>>   a little.  It should only increase at most up to the maximum, even if
>>>   the voltage remains high.  It should probably start at zero rather
>>>   than at the minimum.  This allows for lossage elsewhere.
>>>
>>> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>>>    control of the current requested, then it should request only the
>>>    minimum available current.  Any more is not safe.
>>
>> Make sense.
>>
>>>
>>> So your USB charging framework should communicate the MIN and MAX as
>>> specified in the standard, and allow the PMIC to use that information as
>>> appropriate.
>>> A library routine to support the incremental increase in current drain
>>> would probably be appreciated by PMIC driver writers.
>>
>> I should say again, the current is set by user or USB configuration
>> what they want, USB charger just tells the power driver how much
>> current need to draw. But we can add some checking if the setting
>> current is in the right range.
>
> The USB configuration tells the Host how much power it needs.  If the
> host enables the configuration, that implicitly say that much power is
> available, so the power_driver can safely request it.  I'm mostly happy
> wit that aspect of your patch set.

OK.

>
> For a dedicated charger there is no USB configuration.  The electrical
> properties of the cable provide some information about how the charger
> will behave, but they only guarantee a minimum.  If you want to use more
> than the minimum, you have to do so carefully and watch the voltage level.
> You cannot sensible let the user tell you how much current the charger
> can provide, because they'll probably get it wrong and the charger will
> just shut down.

Current validation is need in USB charger to guarantee the current is
available. If user set one unavailable current, we can ignore it or
change to the minimum, right? Thanks.

>
> Thanks,
> NeilBrown
>
>
>>
>> Thanks for your comments.
>>
>>>
>>>
>>> A more details discussion of this problem-space can be found at
>>>    https://lwn.net/Articles/693027/
>>> and an analysis of the functionality currently available in the kernel
>>> (which adds a number of specifics to the above) can be found at
>>>    https://lwn.net/Articles/694062/
>>>
>>> NeilBrown
>>>
>>>
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 8 September 2016 at 15:31, NeilBrown <neilb@suse.com> wrote:
> On Thu, Sep 08 2016, Baolin Wang wrote:
>
>> Hi Neil,
>>
>> On 6 September 2016 at 13:40, NeilBrown <nfbrown@novell.com> wrote:
>>> On Mon, Aug 29 2016, Baolin Wang wrote:
>>>
>>>> Hi Felipe,
>>>>
>>>> On 11 August 2016 at 11:14, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>> Hi Felipe,
>>>>>
>>>>> On 1 August 2016 at 15:09, 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.
>>>>>
>>>>> Could you please apply this patchset into your 'next' branch if you
>>>>> have no comments about it? Thank you.
>>>>
>>>> Since there are no other comments about this patchset for a long time,
>>>> could you please apply this patchset? Thanks.
>>>
>>
>> Sorry for the late reply.
>>
>>> Sorry, I should have replied earlier.  Tim Bird mentioned on the
>>> ksummit-discuss list that there was a frustration with this not making
>>> progress so I decided to contribute what I could now.
>>>
>>> I think this patch set is attempting to address an important problem
>>> that needs solving.  However I think it gets some key aspects wrong.
>>> Maybe they can get fixed up after the patchset is upstream, maybe they
>>> should be fixed first - I have no strong opinion on that.
>>>
>>> My main complaints involve the detection and handling of the different
>>> charger types - DCP, CDP, ACA etc.
>>> The big-picture requirement here that the PHY will detect the physical
>>> properties of the cable (e.g. resistance to ground on ID) and determine
>>> the type of charger expected.  This information must be communicated to
>>> the PMIC "power_supply" device so it can regulate the power being drawn
>>> through the cable.
>>>
>>> The first problem is that there are two different ways that the
>>> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
>>> are cable types in the 'extcon' subsystem, and they are power_supply
>>> types in the 'power_supply' subsystem.  This duplication is confusing.
>>> It is not caused by your patch set, but I believe your patchset needs to
>>> work with the duplication and I think it does so poorly.
>>
>> Now the usb charger will not get charger type from 'extcon' subsystem,
>> we get the charger type from 'power_supply' and calllback
>> 'get_charger_type' for users.
>
> I understand this.  I think it is wrong because, in general, the
> power_supply doesn't know what the charger_type is (it might know it is
> USB, but most don't know which sort of USB).  The PHY knows that, not
> the power_supply.

I don't think so. Now many platforms will detect the charger type by
PMIC hardware, and we can get the charger type by PMIC hardware
register. Then power supply driver can access the PMIC register to get
the charger type. Here USB charger just considers if the accessing the
PMIC register to get charger type is implemented in power supply, it
is optional depending on what your platform designed.

>>
>>>
>>> In my mind, the power_supply should *not* know about this distinction at
>>> all (except possibly as an advisor attribute simiarly to the current
>>> battery technology attribute).  The other types it knows of are "AC",
>>> "USB", and "BATTERY".  The contrast between these is quite different
>>> From the contrast between DCP, CDP, ACA, which, from the perspective of
>>> the power supply, are almost irrelevant.  Your patchset effectively
>>> examines the power_supply_type of one power_supply, and communicates it
>>> to another.  It isn't clear to me how the first power_supply gets the
>>> information, or what the relationship between the two power_supplies is
>>> meant to be.
>>
>> We just get the charger type from the power supply which can get the
>> charger type from register or something else,
>
> But that register would be part of the PHY, not part of the charger.
> Having the power_supply driver reading the PHY register might work for
> some hardware, but is not a general solution.

Not only PHY. Like I said, the charger type detection can be done by
PMIC hardware, power supply can get the charger type by accessing PMIC
registers.

>>                                                and the usb charger did
>> nothing for this power supply. In some platform, the charger type is
>> get in power supply driver, thus we should link this power supply to
>> get the charger type when USB cable is plugin. If you don't get
>> charger type from power supply driver, then it does not need to link
>> this power supply phandle.
>>
>>>
>>> It makes much more sense, to me, to utilized the knowledge of this
>>> distinction that extcon provides.  A usb PHY can register an extcon,
>>> declare the sorts of cables that it can detect, and tell the extcon as
>>> cables appear or disappear.  The PMIC power_supply can then register with
>>> that extcon for events and can find out when a cable is attached, and
>>> what sort of cable.
>>> Your usb-charging framework would be well placed to help the
>>> power_supply to find the correct extcon, and possibly even to handle the
>>> registration for events.
>>>
>>> Your framework does currently register with extcon, but only listens for
>>> EXTCON_USB cables.  I don't think that cable type is (reliably) reported
>>> when a DCP (for example) is plugged in.
>>
>> Here we just listen the plugin/out events from extcon, if one cable
>> plugin it will report to usb charger.
>
> As far as I can tell there are no generic "plugin/out events from extcon".
> There are only events for a specific cable being plugged or unplugged.

Yeah, that is what I meaning. Now we just listen plugged or unplugged
events from extcon.

> So you need to listed for each different type of cable that you are
> interested in.
>
> If it detects a "EXTCON_CHG_USB_ACA plugged" event, then it makes sense
> to use that event to tell the power supply that the current profile for
> an ACA is available.

Yes, I understand. Now USB charger doses not listen for each different
type of cable, which need to add.

>>
>>>
>>> Here there is another problem that is not of your making, but still
>>> needs fixing.  Extcon declares a number of cable types like:
>>>
>>> /* USB external connector */
>>> #define EXTCON_USB              1
>>> #define EXTCON_USB_HOST         2
>>>
>>> /* Charging external connector */
>>> #define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
>>> #define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
>>> #define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
>>> #define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
>>> #define EXTCON_CHG_USB_FAST     9
>>> #define EXTCON_CHG_USB_SLOW     10
>>>
>>> However it doesn't define what those mean, so we are left to guess.
>>> They each correspond to bits in a bitmap, so a cable can have multiple types.
>>> I think the best interpretation is that:
>>>
>>>  EXTCON_USB means that the cable carries USB data from a host.
>>>  EXTCON_USB_HOST means that that cable carries USB data to a host.
>>>  EXTCON_CHG_* means that power is available as described in the
>>>  standards.
>>>  (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all
>>>  clear).
>>>
>>> There is some support for this in the code, but it is not universally
>>> acknowledged.  For a USB charging framework to be genuinely useful, it
>>> must (I think) make sure this issue gets clarified, and the various
>>> cable types used properly.
>>
>> Here I agree with you, now the usb charger does not listen the charger
>> type from extcon which need to be fixed.
>>
>>>
>>> Once the cable/charger type has been determined, the PMIC power_supply
>>> needs to use this information.  Your current patches make use of this
>>> information in ways that are wrong in two respects.
>>>
>>> Firstly, you have made the current limit associated with each cable type
>>> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
>>> The standard (e.g. BC-1.2) declares what the current limits are.  There
>>> is no reason for those not to be hard coded.
>>
>> Yes, but you must think about some special cases on some platforms.
>> Users may need to change the current in some situations, thus we
>> should export one API for users to change the current. (I think you
>> misunderstand the current limit here, that is the current for power
>> driver  to draw).
>
> Can you be specific about these "special cases" please?
> I cannot think of any.

Suppose the USB configuration requests 100mA, then we should set the
USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
funtion, then notify this to power driver.

>
> And what sort of "user" might use that API?  I don't think having an app
> on your phone that allows you to set the max-current would be a good
> idea, so you must be thinking of some other sort of user.
>
>>
>>>
>>> Secondly, you treat each charger type as having a single "cur_limit" and
>>> utilize that limit by telling the PMIC to draw that much current.
>>
>> This "cur_limit" is set by user or USB configuration what they want,
>> usb charger just tells the power driver how much current need to draw.
>
> You don't tell the power driver "how much current it needs to draw".
> You tell it "how much current it is safe to draw".
>
> A power driver cannot draw current that isn't available, and it cannot
> draw current that the system as a whole has no use for.
> The task of the power driver is to make sure the system doesn't draw
> more power than is available, else the voltage will drop and the power
> supply might shut off.

How much current in USB charger is set by USB configuration or power
users, but yes, we can check if the current is available in USB
charger. If user set one unavailable we can ignore that setting.

>
>>
>>> Again, this is inconsistent with the specification.
>>> BC-1.2 defines, for each charger type, a minimum and maximum current
>>> level.
>>> The minimum should always be available.   Attempting to draw more than
>>> that (but less that the max) might work or might cause the charger
>>> to shut down, but you can be sure that the charger will respond to the
>>> increased load by first reducing the voltage, and will not shut down
>>> until the voltage has dropped below 2V.
>>> If you try to draw more current than the maximum, then the charger might
>>> shut down before the voltage drops below 2V.
>>>
>>> Given this understanding of the current available from the charger,
>>> there are two approaches the PMIC might take.
>>> 1/ if the PMIC is able to exercise fine control over the current it
>>>   draws, and if it can monitor the voltage on the charger, then it
>>>   could gradually increase the power being requested until the voltage
>>>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>>>   a little.  It should only increase at most up to the maximum, even if
>>>   the voltage remains high.  It should probably start at zero rather
>>>   than at the minimum.  This allows for lossage elsewhere.
>>>
>>> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>>>    control of the current requested, then it should request only the
>>>    minimum available current.  Any more is not safe.
>>
>> Make sense.
>>
>>>
>>> So your USB charging framework should communicate the MIN and MAX as
>>> specified in the standard, and allow the PMIC to use that information as
>>> appropriate.
>>> A library routine to support the incremental increase in current drain
>>> would probably be appreciated by PMIC driver writers.
>>
>> I should say again, the current is set by user or USB configuration
>> what they want, USB charger just tells the power driver how much
>> current need to draw. But we can add some checking if the setting
>> current is in the right range.
>
> The USB configuration tells the Host how much power it needs.  If the
> host enables the configuration, that implicitly say that much power is
> available, so the power_driver can safely request it.  I'm mostly happy
> wit that aspect of your patch set.

OK.

>
> For a dedicated charger there is no USB configuration.  The electrical
> properties of the cable provide some information about how the charger
> will behave, but they only guarantee a minimum.  If you want to use more
> than the minimum, you have to do so carefully and watch the voltage level.
> You cannot sensible let the user tell you how much current the charger
> can provide, because they'll probably get it wrong and the charger will
> just shut down.

Current validation is need in USB charger to guarantee the current is
available. If user set one unavailable current, we can ignore it or
change to the minimum, right? Thanks.

>
> Thanks,
> NeilBrown
>
>
>>
>> Thanks for your comments.
>>
>>>
>>>
>>> A more details discussion of this problem-space can be found at
>>>    https://lwn.net/Articles/693027/
>>> and an analysis of the functionality currently available in the kernel
>>> (which adds a number of specifics to the above) can be found at
>>>    https://lwn.net/Articles/694062/
>>>
>>> NeilBrown
>>>
>>>
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards

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

[NeilBrown]

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

On Thu, Sep 08 2016, Baolin Wang wrote:

> On 8 September 2016 at 15:31, NeilBrown <neilb@suse.com> wrote:
>> On Thu, Sep 08 2016, Baolin Wang wrote:
>>>
>>> Now the usb charger will not get charger type from 'extcon' subsystem,
>>> we get the charger type from 'power_supply' and calllback
>>> 'get_charger_type' for users.
>>
>> I understand this.  I think it is wrong because, in general, the
>> power_supply doesn't know what the charger_type is (it might know it is
>> USB, but most don't know which sort of USB).  The PHY knows that, not
>> the power_supply.
>
> I don't think so. Now many platforms will detect the charger type by
> PMIC hardware, and we can get the charger type by PMIC hardware
> register. Then power supply driver can access the PMIC register to get
> the charger type. Here USB charger just considers if the accessing the
> PMIC register to get charger type is implemented in power supply, it
> is optional depending on what your platform designed.
>

In practice, the USB PHY and the Power manager will often be in the same
IC (the PMIC) so the driver for one could look at the registers for the
other.
But there is no guarantee that the hardware works like that.  It is
best to create a generally design.
Conceptually, the PHY is separate from the power manager and a solution
which recognises that will be more universal.

If the power manager can always just look at that phy registers to know
what sort of charger is connected, why does you framework need to work
with charger types at all?

>>>
>>> Yes, but you must think about some special cases on some platforms.
>>> Users may need to change the current in some situations, thus we
>>> should export one API for users to change the current. (I think you
>>> misunderstand the current limit here, that is the current for power
>>> driver  to draw).
>>
>> Can you be specific about these "special cases" please?
>> I cannot think of any.
>
> Suppose the USB configuration requests 100mA, then we should set the
> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
> funtion, then notify this to power driver.

ahh.... I had missed something there.  It's a while since I looked
closely at these patches.

Only.... this usage of usb_charger_set_cur_limit_by_type() is really
nonsensical.

If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
the number negotiated with the USB configuration is not relevant and
should be ignored.  There is a guaranteed minimum which is at least the
maximum that *can* be negotiated.

It is only when the cable appears to be a SDP (standard downstream
port) that the usb-config negotiation is relevant.  That is because the
minimum guaranteed for SDP is only 100mA.

NeilBrown

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

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

[NeilBrown]

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

On Thu, Sep 08 2016, Baolin Wang wrote:

> On 8 September 2016 at 15:31, NeilBrown <neilb@suse.com> wrote:
>> On Thu, Sep 08 2016, Baolin Wang wrote:
>>>
>>> Now the usb charger will not get charger type from 'extcon' subsystem,
>>> we get the charger type from 'power_supply' and calllback
>>> 'get_charger_type' for users.
>>
>> I understand this.  I think it is wrong because, in general, the
>> power_supply doesn't know what the charger_type is (it might know it is
>> USB, but most don't know which sort of USB).  The PHY knows that, not
>> the power_supply.
>
> I don't think so. Now many platforms will detect the charger type by
> PMIC hardware, and we can get the charger type by PMIC hardware
> register. Then power supply driver can access the PMIC register to get
> the charger type. Here USB charger just considers if the accessing the
> PMIC register to get charger type is implemented in power supply, it
> is optional depending on what your platform designed.
>

In practice, the USB PHY and the Power manager will often be in the same
IC (the PMIC) so the driver for one could look at the registers for the
other.
But there is no guarantee that the hardware works like that.  It is
best to create a generally design.
Conceptually, the PHY is separate from the power manager and a solution
which recognises that will be more universal.

If the power manager can always just look at that phy registers to know
what sort of charger is connected, why does you framework need to work
with charger types at all?

>>>
>>> Yes, but you must think about some special cases on some platforms.
>>> Users may need to change the current in some situations, thus we
>>> should export one API for users to change the current. (I think you
>>> misunderstand the current limit here, that is the current for power
>>> driver  to draw).
>>
>> Can you be specific about these "special cases" please?
>> I cannot think of any.
>
> Suppose the USB configuration requests 100mA, then we should set the
> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
> funtion, then notify this to power driver.

ahh.... I had missed something there.  It's a while since I looked
closely at these patches.

Only.... this usage of usb_charger_set_cur_limit_by_type() is really
nonsensical.

If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
the number negotiated with the USB configuration is not relevant and
should be ignored.  There is a guaranteed minimum which is at least the
maximum that *can* be negotiated.

It is only when the cable appears to be a SDP (standard downstream
port) that the usb-config negotiation is relevant.  That is because the
minimum guaranteed for SDP is only 100mA.

NeilBrown

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

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

[Baolin Wang]

On 9 September 2016 at 07:13, NeilBrown <neilb@suse.com> wrote:
> On Thu, Sep 08 2016, Baolin Wang wrote:
>
>> On 8 September 2016 at 15:31, NeilBrown <neilb@suse.com> wrote:
>>> On Thu, Sep 08 2016, Baolin Wang wrote:
>>>>
>>>> Now the usb charger will not get charger type from 'extcon' subsystem,
>>>> we get the charger type from 'power_supply' and calllback
>>>> 'get_charger_type' for users.
>>>
>>> I understand this.  I think it is wrong because, in general, the
>>> power_supply doesn't know what the charger_type is (it might know it is
>>> USB, but most don't know which sort of USB).  The PHY knows that, not
>>> the power_supply.
>>
>> I don't think so. Now many platforms will detect the charger type by
>> PMIC hardware, and we can get the charger type by PMIC hardware
>> register. Then power supply driver can access the PMIC register to get
>> the charger type. Here USB charger just considers if the accessing the
>> PMIC register to get charger type is implemented in power supply, it
>> is optional depending on what your platform designed.
>>
>
> In practice, the USB PHY and the Power manager will often be in the same
> IC (the PMIC) so the driver for one could look at the registers for the
> other.
> But there is no guarantee that the hardware works like that.  It is
> best to create a generally design.

Yes, we hope to create one generally design, so we need to consider
this situation: the power supply getting the charger type by accessing
PMIC registers. The registers which save the charger type are not
always belong to the USB PHY, may be just some registers on PMIC.

Now in mainline kernel, there are 3 methods can get the charger type
which need to integrate with USB charger framework:
1. power supply
2. extcon (need to add as you suggested)
3. others (by 'get_charger_type' callback of USB charger)

> Conceptually, the PHY is separate from the power manager and a solution
> which recognises that will be more universal.
>
> If the power manager can always just look at that phy registers to know
> what sort of charger is connected, why does you framework need to work
> with charger types at all?
>
>>>>
>>>> Yes, but you must think about some special cases on some platforms.
>>>> Users may need to change the current in some situations, thus we
>>>> should export one API for users to change the current. (I think you
>>>> misunderstand the current limit here, that is the current for power
>>>> driver  to draw).
>>>
>>> Can you be specific about these "special cases" please?
>>> I cannot think of any.
>>
>> Suppose the USB configuration requests 100mA, then we should set the
>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
>> funtion, then notify this to power driver.
>
> ahh.... I had missed something there.  It's a while since I looked
> closely at these patches.
>
> Only.... this usage of usb_charger_set_cur_limit_by_type() is really
> nonsensical.
>
> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
> the number negotiated with the USB configuration is not relevant and
> should be ignored.  There is a guaranteed minimum which is at least the
> maximum that *can* be negotiated.

Yes. If it is not relevant, we will no't set the current from USB
configuration. Just when your charger type is SDP and the USB
enumeration is done, we can get the USB configuration from host to set
current.

>
> It is only when the cable appears to be a SDP (standard downstream
> port) that the usb-config negotiation is relevant.  That is because the
> minimum guaranteed for SDP is only 100mA.
>
> NeilBrown



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 9 September 2016 at 07:13, NeilBrown <neilb@suse.com> wrote:
> On Thu, Sep 08 2016, Baolin Wang wrote:
>
>> On 8 September 2016 at 15:31, NeilBrown <neilb@suse.com> wrote:
>>> On Thu, Sep 08 2016, Baolin Wang wrote:
>>>>
>>>> Now the usb charger will not get charger type from 'extcon' subsystem,
>>>> we get the charger type from 'power_supply' and calllback
>>>> 'get_charger_type' for users.
>>>
>>> I understand this.  I think it is wrong because, in general, the
>>> power_supply doesn't know what the charger_type is (it might know it is
>>> USB, but most don't know which sort of USB).  The PHY knows that, not
>>> the power_supply.
>>
>> I don't think so. Now many platforms will detect the charger type by
>> PMIC hardware, and we can get the charger type by PMIC hardware
>> register. Then power supply driver can access the PMIC register to get
>> the charger type. Here USB charger just considers if the accessing the
>> PMIC register to get charger type is implemented in power supply, it
>> is optional depending on what your platform designed.
>>
>
> In practice, the USB PHY and the Power manager will often be in the same
> IC (the PMIC) so the driver for one could look at the registers for the
> other.
> But there is no guarantee that the hardware works like that.  It is
> best to create a generally design.

Yes, we hope to create one generally design, so we need to consider
this situation: the power supply getting the charger type by accessing
PMIC registers. The registers which save the charger type are not
always belong to the USB PHY, may be just some registers on PMIC.

Now in mainline kernel, there are 3 methods can get the charger type
which need to integrate with USB charger framework:
1. power supply
2. extcon (need to add as you suggested)
3. others (by 'get_charger_type' callback of USB charger)

> Conceptually, the PHY is separate from the power manager and a solution
> which recognises that will be more universal.
>
> If the power manager can always just look at that phy registers to know
> what sort of charger is connected, why does you framework need to work
> with charger types at all?
>
>>>>
>>>> Yes, but you must think about some special cases on some platforms.
>>>> Users may need to change the current in some situations, thus we
>>>> should export one API for users to change the current. (I think you
>>>> misunderstand the current limit here, that is the current for power
>>>> driver  to draw).
>>>
>>> Can you be specific about these "special cases" please?
>>> I cannot think of any.
>>
>> Suppose the USB configuration requests 100mA, then we should set the
>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
>> funtion, then notify this to power driver.
>
> ahh.... I had missed something there.  It's a while since I looked
> closely at these patches.
>
> Only.... this usage of usb_charger_set_cur_limit_by_type() is really
> nonsensical.
>
> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
> the number negotiated with the USB configuration is not relevant and
> should be ignored.  There is a guaranteed minimum which is at least the
> maximum that *can* be negotiated.

Yes. If it is not relevant, we will no't set the current from USB
configuration. Just when your charger type is SDP and the USB
enumeration is done, we can get the USB configuration from host to set
current.

>
> It is only when the cable appears to be a SDP (standard downstream
> port) that the usb-config negotiation is relevant.  That is because the
> minimum guaranteed for SDP is only 100mA.
>
> NeilBrown



-- 
Baolin.wang
Best Regards

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

[Mark Brown]

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

On Fri, Sep 09, 2016 at 09:13:31AM +1000, NeilBrown wrote:

> Conceptually, the PHY is separate from the power manager and a solution
> which recognises that will be more universal.

The wm831x driver in the patch series is an example of such hardware -
it is purely a power manager, it has no USB PHY hardware at all.  It's a
current limiter intended to sit in line with the USB power lines to
ensure the system doesn't go over the maximum current draw (and also
integrates with the power source selection logic the chip has to pick
the best available power source for the system).

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

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

[Mark Brown]

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

On Fri, Sep 09, 2016 at 09:13:31AM +1000, NeilBrown wrote:

> Conceptually, the PHY is separate from the power manager and a solution
> which recognises that will be more universal.

The wm831x driver in the patch series is an example of such hardware -
it is purely a power manager, it has no USB PHY hardware at all.  It's a
current limiter intended to sit in line with the USB power lines to
ensure the system doesn't go over the maximum current draw (and also
integrates with the power source selection logic the chip has to pick
the best available power source for the system).

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

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

[NeilBrown]

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

On Fri, Sep 09 2016, Baolin Wang wrote:

>>
>> In practice, the USB PHY and the Power manager will often be in the same
>> IC (the PMIC) so the driver for one could look at the registers for the
>> other.
>> But there is no guarantee that the hardware works like that.  It is
>> best to create a generally design.
>
> Yes, we hope to create one generally design, so we need to consider
> this situation: the power supply getting the charger type by accessing
> PMIC registers. The registers which save the charger type are not
> always belong to the USB PHY, may be just some registers on PMIC.

If the power_supply can directly detect the type of charger, then it
doesn't need any usb-charger-infrastructure to tell it.  It can handle
current selection entirely internally.
Surely the only interesting case for a framework to address is the one
where the power_supply cannot directly detect the charger type.

>
> Now in mainline kernel, there are 3 methods can get the charger type
> which need to integrate with USB charger framework:
> 1. power supply
> 2. extcon (need to add as you suggested)
> 3. others (by 'get_charger_type' callback of USB charger)

There is no "get_charger_type" now in the mainline kernel.

>>>
>>> Suppose the USB configuration requests 100mA, then we should set the
>>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
>>> funtion, then notify this to power driver.
>>
>> ahh.... I had missed something there.  It's a while since I looked
>> closely at these patches.
>>
>> Only.... this usage of usb_charger_set_cur_limit_by_type() is really
>> nonsensical.
>>
>> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
>> the number negotiated with the USB configuration is not relevant and
>> should be ignored.  There is a guaranteed minimum which is at least the
>> maximum that *can* be negotiated.
>
> Yes. If it is not relevant, we will no't set the current from USB
> configuration. Just when your charger type is SDP and the USB
> enumeration is done, we can get the USB configuration from host to set
> current.

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
Your patch passes that to usb_charger_set_cur_limit_by_type()
which calls __usb_charger_set_cur_limit_by_type() which will set the
cur_limit for whichever type uchger->type currently is.

So when it is not relevant, your code *does* set some current limit.

NeilBrown

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

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

[NeilBrown]

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

On Fri, Sep 09 2016, Baolin Wang wrote:

>>
>> In practice, the USB PHY and the Power manager will often be in the same
>> IC (the PMIC) so the driver for one could look at the registers for the
>> other.
>> But there is no guarantee that the hardware works like that.  It is
>> best to create a generally design.
>
> Yes, we hope to create one generally design, so we need to consider
> this situation: the power supply getting the charger type by accessing
> PMIC registers. The registers which save the charger type are not
> always belong to the USB PHY, may be just some registers on PMIC.

If the power_supply can directly detect the type of charger, then it
doesn't need any usb-charger-infrastructure to tell it.  It can handle
current selection entirely internally.
Surely the only interesting case for a framework to address is the one
where the power_supply cannot directly detect the charger type.

>
> Now in mainline kernel, there are 3 methods can get the charger type
> which need to integrate with USB charger framework:
> 1. power supply
> 2. extcon (need to add as you suggested)
> 3. others (by 'get_charger_type' callback of USB charger)

There is no "get_charger_type" now in the mainline kernel.

>>>
>>> Suppose the USB configuration requests 100mA, then we should set the
>>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
>>> funtion, then notify this to power driver.
>>
>> ahh.... I had missed something there.  It's a while since I looked
>> closely at these patches.
>>
>> Only.... this usage of usb_charger_set_cur_limit_by_type() is really
>> nonsensical.
>>
>> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
>> the number negotiated with the USB configuration is not relevant and
>> should be ignored.  There is a guaranteed minimum which is at least the
>> maximum that *can* be negotiated.
>
> Yes. If it is not relevant, we will no't set the current from USB
> configuration. Just when your charger type is SDP and the USB
> enumeration is done, we can get the USB configuration from host to set
> current.

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
Your patch passes that to usb_charger_set_cur_limit_by_type()
which calls __usb_charger_set_cur_limit_by_type() which will set the
cur_limit for whichever type uchger->type currently is.

So when it is not relevant, your code *does* set some current limit.

NeilBrown

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

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

[NeilBrown]

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

On Fri, Sep 09 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Fri, Sep 09, 2016 at 09:13:31AM +1000, NeilBrown wrote:
>
>> Conceptually, the PHY is separate from the power manager and a solution
>> which recognises that will be more universal.
>
> The wm831x driver in the patch series is an example of such hardware -
> it is purely a power manager, it has no USB PHY hardware at all.  It's a
> current limiter intended to sit in line with the USB power lines to
> ensure the system doesn't go over the maximum current draw (and also
> integrates with the power source selection logic the chip has to pick
> the best available power source for the system).

It might be instructive to look at exactly what happens with this
device.
The "probe" routine calls

 +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);

Presumably wm831x_pdata is initialised by a board file?
I strongly suspect it is initialized to  "usb-charger.0" because the
names given to usb chargers are "usb-charger.%d" in discovery order.
I don't see this being at all useful if there is ever more than one
usb-charger.
Do you?

The probe function then registers for charger notifications.  When they
arrive, wm831x_usb_limit_change() will set the highest supported limit
which is less than the notified "limit".  So presumably that "limit"
must be the minimum guaranteed by the charger type.

Let's see when the notification is called...
->uchgr_nh is sent a notification from usb_charger_notify_others()
with (in the "charger is present" case) the value of
   __usb_charger_get_cur_limit(uchger)
which just pulls values out of the cur_limit structure, based on the
type, reported by
		usb_charger_get_type_by_others(uchger);
(The default values in this structure are not the minimums guaranteed
by the charger types, they are generally higher.  So this could easily
result in the charger shutting down).

usb_charger_get_type_by_others()  has two ways to get the charger
type, which it then caches in uchger->type until the charger is removed.

If there is a uchger->psy power supply, then the
  POWER_SUPPLY_PROP_CHARGE_TYPE
property is used...  Oh, that is weird.
The valid values for that property are:

enum {
        POWER_SUPPLY_CHARGE_TYPE_UNKNOWN = 0,
        POWER_SUPPLY_CHARGE_TYPE_NONE,
        POWER_SUPPLY_CHARGE_TYPE_TRICKLE,
        POWER_SUPPLY_CHARGE_TYPE_FAST,
};

but the code in usb_charger_get_type_by_others() compares it against:
 +		case POWER_SUPPLY_TYPE_USB:
 +		case POWER_SUPPLY_TYPE_USB_DCP:
 +		case POWER_SUPPLY_TYPE_USB_CDP:
 +		case POWER_SUPPLY_TYPE_USB_ACA:

Presumably that it just a bug and it was meant to request the
  POWER_SUPPLY_PROP_TYPE ??
I wonder how much testing was done on this code?

Anyway, assuming it is meant to request the TYPE, where is that set?
The new code doesn't set it at all.
Only three existing power supplies set any of
  POWER_SUPPLY_TYPE_USB_*
axp288_charger.c  gpio-charger.c isp1704_charger.c
As I wrote in https://lwn.net/Articles/694062/
axp288_charger.c is broken and cannot possibly work.
gpio-charger.c configures the type at boot-time so it cannot sensibly
detect a newly plugged in charger (how does that make any sense)
and isp1704_charger.c peeks in the usb registers (ULPI) to work out
the charger type.

None of these set the  POWER_SUPPLY_PROP_TYPE in a useful way, so why
would usb_charger_get_type_by_others() want to use that property?

Maybe it really does want to use POWER_SUPPLY_PROP_CHARGE_TYPE?
Quite a few chargers set that.  It would be a challenge to map names
like "TRICKLE" and "FAST" into mA values for a current limiter though.
My hardware knowledge is running out here.. I see wm8350_power.c reports
that property, but I don't know how that device fits into the picture.
With the patch, that driver would request that property from somewhere
else(?) and also report it.  That is kind-a strange.

The other possible source for the charger type is a call to
   uchger->get_charger_type()

There is no get_charger_type() function anywhere in the patchset.  No code
ever sets that field in the uchger.

So how can this wm831x driver actually find out what sort of charger is
connected and so set the power limit?  I just don't see this working *at*
*all*.

NeilBrown

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

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

[NeilBrown]

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

On Fri, Sep 09 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Fri, Sep 09, 2016 at 09:13:31AM +1000, NeilBrown wrote:
>
>> Conceptually, the PHY is separate from the power manager and a solution
>> which recognises that will be more universal.
>
> The wm831x driver in the patch series is an example of such hardware -
> it is purely a power manager, it has no USB PHY hardware at all.  It's a
> current limiter intended to sit in line with the USB power lines to
> ensure the system doesn't go over the maximum current draw (and also
> integrates with the power source selection logic the chip has to pick
> the best available power source for the system).

It might be instructive to look at exactly what happens with this
device.
The "probe" routine calls

 +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);

Presumably wm831x_pdata is initialised by a board file?
I strongly suspect it is initialized to  "usb-charger.0" because the
names given to usb chargers are "usb-charger.%d" in discovery order.
I don't see this being at all useful if there is ever more than one
usb-charger.
Do you?

The probe function then registers for charger notifications.  When they
arrive, wm831x_usb_limit_change() will set the highest supported limit
which is less than the notified "limit".  So presumably that "limit"
must be the minimum guaranteed by the charger type.

Let's see when the notification is called...
->uchgr_nh is sent a notification from usb_charger_notify_others()
with (in the "charger is present" case) the value of
   __usb_charger_get_cur_limit(uchger)
which just pulls values out of the cur_limit structure, based on the
type, reported by
		usb_charger_get_type_by_others(uchger);
(The default values in this structure are not the minimums guaranteed
by the charger types, they are generally higher.  So this could easily
result in the charger shutting down).

usb_charger_get_type_by_others()  has two ways to get the charger
type, which it then caches in uchger->type until the charger is removed.

If there is a uchger->psy power supply, then the
  POWER_SUPPLY_PROP_CHARGE_TYPE
property is used...  Oh, that is weird.
The valid values for that property are:

enum {
        POWER_SUPPLY_CHARGE_TYPE_UNKNOWN = 0,
        POWER_SUPPLY_CHARGE_TYPE_NONE,
        POWER_SUPPLY_CHARGE_TYPE_TRICKLE,
        POWER_SUPPLY_CHARGE_TYPE_FAST,
};

but the code in usb_charger_get_type_by_others() compares it against:
 +		case POWER_SUPPLY_TYPE_USB:
 +		case POWER_SUPPLY_TYPE_USB_DCP:
 +		case POWER_SUPPLY_TYPE_USB_CDP:
 +		case POWER_SUPPLY_TYPE_USB_ACA:

Presumably that it just a bug and it was meant to request the
  POWER_SUPPLY_PROP_TYPE ??
I wonder how much testing was done on this code?

Anyway, assuming it is meant to request the TYPE, where is that set?
The new code doesn't set it at all.
Only three existing power supplies set any of
  POWER_SUPPLY_TYPE_USB_*
axp288_charger.c  gpio-charger.c isp1704_charger.c
As I wrote in https://lwn.net/Articles/694062/
axp288_charger.c is broken and cannot possibly work.
gpio-charger.c configures the type at boot-time so it cannot sensibly
detect a newly plugged in charger (how does that make any sense)
and isp1704_charger.c peeks in the usb registers (ULPI) to work out
the charger type.

None of these set the  POWER_SUPPLY_PROP_TYPE in a useful way, so why
would usb_charger_get_type_by_others() want to use that property?

Maybe it really does want to use POWER_SUPPLY_PROP_CHARGE_TYPE?
Quite a few chargers set that.  It would be a challenge to map names
like "TRICKLE" and "FAST" into mA values for a current limiter though.
My hardware knowledge is running out here.. I see wm8350_power.c reports
that property, but I don't know how that device fits into the picture.
With the patch, that driver would request that property from somewhere
else(?) and also report it.  That is kind-a strange.

The other possible source for the charger type is a call to
   uchger->get_charger_type()

There is no get_charger_type() function anywhere in the patchset.  No code
ever sets that field in the uchger.

So how can this wm831x driver actually find out what sort of charger is
connected and so set the power limit?  I just don't see this working *at*
*all*.

NeilBrown

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

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

[Mark Brown]

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

On Sat, Sep 10, 2016 at 07:57:26AM +1000, NeilBrown wrote:
> On Fri, Sep 09 2016, Mark Brown wrote:

> > The wm831x driver in the patch series is an example of such hardware -
> > it is purely a power manager, it has no USB PHY hardware at all.  It's a

> The "probe" routine calls

>  +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);

> Presumably wm831x_pdata is initialised by a board file?

Yes.

> I strongly suspect it is initialized to  "usb-charger.0" because the
> names given to usb chargers are "usb-charger.%d" in discovery order.
> I don't see this being at all useful if there is ever more than one
> usb-charger.
> Do you?

It's no worse than any other board file situation - if someone has that
problem they get to fix it.

> So how can this wm831x driver actually find out what sort of charger is
> connected and so set the power limit?  I just don't see this working *at*
> *all*.

The whole point from the point of view of the wm831x driver is that it
just wants something to tell it how much current it's allowed to draw, I
appreciate that doesn't change your analysis of the bit in the middle
but the consumer driver bit seems fine here.

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

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

[Mark Brown]

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

On Sat, Sep 10, 2016 at 07:57:26AM +1000, NeilBrown wrote:
> On Fri, Sep 09 2016, Mark Brown wrote:

> > The wm831x driver in the patch series is an example of such hardware -
> > it is purely a power manager, it has no USB PHY hardware at all.  It's a

> The "probe" routine calls

>  +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);

> Presumably wm831x_pdata is initialised by a board file?

Yes.

> I strongly suspect it is initialized to  "usb-charger.0" because the
> names given to usb chargers are "usb-charger.%d" in discovery order.
> I don't see this being at all useful if there is ever more than one
> usb-charger.
> Do you?

It's no worse than any other board file situation - if someone has that
problem they get to fix it.

> So how can this wm831x driver actually find out what sort of charger is
> connected and so set the power limit?  I just don't see this working *at*
> *all*.

The whole point from the point of view of the wm831x driver is that it
just wants something to tell it how much current it's allowed to draw, I
appreciate that doesn't change your analysis of the bit in the middle
but the consumer driver bit seems fine here.

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

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

[NeilBrown]

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

On Mon, Sep 12 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Sat, Sep 10, 2016 at 07:57:26AM +1000, NeilBrown wrote:
>> On Fri, Sep 09 2016, Mark Brown wrote:
>
>> > The wm831x driver in the patch series is an example of such hardware -
>> > it is purely a power manager, it has no USB PHY hardware at all.  It's a
>
>> The "probe" routine calls
>
>>  +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);
>
>> Presumably wm831x_pdata is initialised by a board file?
>
> Yes.
>
>> I strongly suspect it is initialized to  "usb-charger.0" because the
>> names given to usb chargers are "usb-charger.%d" in discovery order.
>> I don't see this being at all useful if there is ever more than one
>> usb-charger.
>> Do you?
>
> It's no worse than any other board file situation - if someone has that
> problem they get to fix it.

My point is that the present design does not appear to scale beyond a
single USB power supply (as if there were two, they would be named in
discovery order, which is not reliably stable).

Your point is, I think, that when someone actually cares about that lack
of scaling, they can fix it.

I am perfectly happy with that approach.  However if the code doesn't
scale beyond one charger, it shouldn't pretend that it does.
i.e.
  Don't have "usb_charger_find_by_name()", just a global "usb_charger"
  (or similar).
  The first charger to register gets to be the "usb_charger".  The
  second one gets an error.
I could be quite happy with that sort of interface.

>
>> So how can this wm831x driver actually find out what sort of charger is
>> connected and so set the power limit?  I just don't see this working *at*
>> *all*.
>
> The whole point from the point of view of the wm831x driver is that it
> just wants something to tell it how much current it's allowed to draw, I
> appreciate that doesn't change your analysis of the bit in the middle
> but the consumer driver bit seems fine here.

Yes, the wm831x driver probably does the right thing.
Other drivers might want to know not only the minimum they are allowed
to draw, but also the maximum they should try even if they are carefully
monitoring the voltage.
So wm831x is doing the right thing with the wrong interface.  Maybe you
can describe that as "fine".

NeilBrown

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

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

[NeilBrown]

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

On Mon, Sep 12 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Sat, Sep 10, 2016 at 07:57:26AM +1000, NeilBrown wrote:
>> On Fri, Sep 09 2016, Mark Brown wrote:
>
>> > The wm831x driver in the patch series is an example of such hardware -
>> > it is purely a power manager, it has no USB PHY hardware at all.  It's a
>
>> The "probe" routine calls
>
>>  +			usb_charger_find_by_name(wm831x_pdata->usb_gadget);
>
>> Presumably wm831x_pdata is initialised by a board file?
>
> Yes.
>
>> I strongly suspect it is initialized to  "usb-charger.0" because the
>> names given to usb chargers are "usb-charger.%d" in discovery order.
>> I don't see this being at all useful if there is ever more than one
>> usb-charger.
>> Do you?
>
> It's no worse than any other board file situation - if someone has that
> problem they get to fix it.

My point is that the present design does not appear to scale beyond a
single USB power supply (as if there were two, they would be named in
discovery order, which is not reliably stable).

Your point is, I think, that when someone actually cares about that lack
of scaling, they can fix it.

I am perfectly happy with that approach.  However if the code doesn't
scale beyond one charger, it shouldn't pretend that it does.
i.e.
  Don't have "usb_charger_find_by_name()", just a global "usb_charger"
  (or similar).
  The first charger to register gets to be the "usb_charger".  The
  second one gets an error.
I could be quite happy with that sort of interface.

>
>> So how can this wm831x driver actually find out what sort of charger is
>> connected and so set the power limit?  I just don't see this working *at*
>> *all*.
>
> The whole point from the point of view of the wm831x driver is that it
> just wants something to tell it how much current it's allowed to draw, I
> appreciate that doesn't change your analysis of the bit in the middle
> but the consumer driver bit seems fine here.

Yes, the wm831x driver probably does the right thing.
Other drivers might want to know not only the minimum they are allowed
to draw, but also the maximum they should try even if they are carefully
monitoring the voltage.
So wm831x is doing the right thing with the wrong interface.  Maybe you
can describe that as "fine".

NeilBrown

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

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

[Mark Brown]

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

On Mon, Sep 12, 2016 at 03:27:18PM +0200, NeilBrown wrote:
> On Mon, Sep 12 2016, Mark Brown wrote:

> > It's no worse than any other board file situation - if someone has that
> > problem they get to fix it.

> My point is that the present design does not appear to scale beyond a
> single USB power supply (as if there were two, they would be named in
> discovery order, which is not reliably stable).

For the practical purposes of people making systems (as opposed to
upstream where this is likely to get most use) it pretty much is.
Though quite how many systems have multiple chargers is itself also a
question.

> Your point is, I think, that when someone actually cares about that lack
> of scaling, they can fix it.

Yes.

> I am perfectly happy with that approach.  However if the code doesn't
> scale beyond one charger, it shouldn't pretend that it does.
> i.e.
>   Don't have "usb_charger_find_by_name()", just a global "usb_charger"
>   (or similar).
>   The first charger to register gets to be the "usb_charger".  The
>   second one gets an error.
> I could be quite happy with that sort of interface.

Well, a fairly standard way of extending would be to allow the explicit
assignment of names to chargers so this'd avoid such churn.

> > The whole point from the point of view of the wm831x driver is that it
> > just wants something to tell it how much current it's allowed to draw, I
> > appreciate that doesn't change your analysis of the bit in the middle
> > but the consumer driver bit seems fine here.

> Yes, the wm831x driver probably does the right thing.
> Other drivers might want to know not only the minimum they are allowed
> to draw, but also the maximum they should try even if they are carefully
> monitoring the voltage.
> So wm831x is doing the right thing with the wrong interface.  Maybe you
> can describe that as "fine".

That's not actually 100% clear to me - for what the wm831x is doing it
probably *does* want the higher limit.  This is a system inflow limit
(as it should be for this), at least the charger will adapt to voltage
variations though other users in the system are much less likely to do
so.

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

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

[Mark Brown]

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

On Mon, Sep 12, 2016 at 03:27:18PM +0200, NeilBrown wrote:
> On Mon, Sep 12 2016, Mark Brown wrote:

> > It's no worse than any other board file situation - if someone has that
> > problem they get to fix it.

> My point is that the present design does not appear to scale beyond a
> single USB power supply (as if there were two, they would be named in
> discovery order, which is not reliably stable).

For the practical purposes of people making systems (as opposed to
upstream where this is likely to get most use) it pretty much is.
Though quite how many systems have multiple chargers is itself also a
question.

> Your point is, I think, that when someone actually cares about that lack
> of scaling, they can fix it.

Yes.

> I am perfectly happy with that approach.  However if the code doesn't
> scale beyond one charger, it shouldn't pretend that it does.
> i.e.
>   Don't have "usb_charger_find_by_name()", just a global "usb_charger"
>   (or similar).
>   The first charger to register gets to be the "usb_charger".  The
>   second one gets an error.
> I could be quite happy with that sort of interface.

Well, a fairly standard way of extending would be to allow the explicit
assignment of names to chargers so this'd avoid such churn.

> > The whole point from the point of view of the wm831x driver is that it
> > just wants something to tell it how much current it's allowed to draw, I
> > appreciate that doesn't change your analysis of the bit in the middle
> > but the consumer driver bit seems fine here.

> Yes, the wm831x driver probably does the right thing.
> Other drivers might want to know not only the minimum they are allowed
> to draw, but also the maximum they should try even if they are carefully
> monitoring the voltage.
> So wm831x is doing the right thing with the wrong interface.  Maybe you
> can describe that as "fine".

That's not actually 100% clear to me - for what the wm831x is doing it
probably *does* want the higher limit.  This is a system inflow limit
(as it should be for this), at least the charger will adapt to voltage
variations though other users in the system are much less likely to do
so.

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

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

[NeilBrown]

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

On Mon, Sep 12 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Mon, Sep 12, 2016 at 03:27:18PM +0200, NeilBrown wrote:
>> On Mon, Sep 12 2016, Mark Brown wrote:
>
>> > It's no worse than any other board file situation - if someone has that
>> > problem they get to fix it.
>
>> My point is that the present design does not appear to scale beyond a
>> single USB power supply (as if there were two, they would be named in
>> discovery order, which is not reliably stable).
>
> For the practical purposes of people making systems (as opposed to
> upstream where this is likely to get most use) it pretty much is.
> Though quite how many systems have multiple chargers is itself also a
> question.
>
>> Your point is, I think, that when someone actually cares about that lack
>> of scaling, they can fix it.
>
> Yes.
>
>> I am perfectly happy with that approach.  However if the code doesn't
>> scale beyond one charger, it shouldn't pretend that it does.
>> i.e.
>>   Don't have "usb_charger_find_by_name()", just a global "usb_charger"
>>   (or similar).
>>   The first charger to register gets to be the "usb_charger".  The
>>   second one gets an error.
>> I could be quite happy with that sort of interface.
>
> Well, a fairly standard way of extending would be to allow the explicit
> assignment of names to chargers so this'd avoid such churn.

Sure, that might work.  I'm just against a design that obviously cannot
work.


>
>> > The whole point from the point of view of the wm831x driver is that it
>> > just wants something to tell it how much current it's allowed to draw, I
>> > appreciate that doesn't change your analysis of the bit in the middle
>> > but the consumer driver bit seems fine here.
>
>> Yes, the wm831x driver probably does the right thing.
>> Other drivers might want to know not only the minimum they are allowed
>> to draw, but also the maximum they should try even if they are carefully
>> monitoring the voltage.
>> So wm831x is doing the right thing with the wrong interface.  Maybe you
>> can describe that as "fine".
>
> That's not actually 100% clear to me - for what the wm831x is doing it
> probably *does* want the higher limit.  This is a system inflow limit
> (as it should be for this), at least the charger will adapt to voltage
> variations though other users in the system are much less likely to do
> so.

Interesting ... I hadn't considered that possibility.

As long as the current remains below the maximum, the charger will
reduce the voltage towards 2V as load increases.  Somewhere before it
gets there, the system will not be able to make use of the power as the
voltage will be too low to be usable. So that will naturally limit the
current being drawn.

Not having very much electrical engineering background, I cannot say for
sure what will happen, but it seems likely that once the voltage drops
much below 4.75V, the charger won't be operating at peak efficiency,
which would be a waste.
I can easily imagine that the hardware would switch off at some voltage
level, rather than just making do with what is there.
So I'm skeptical of this approach, but I'm open to being corrected by
someone more knowledgeable than I.

Looking at it from a different perspective, according to the patch set,
the limits that wm831x is able to impose are:

 +	0,
 +	2,
 +	100,
 +	500,
 +	900,
 +	1500,
 +	1800,
 +	550,

These are, from the battery charger spec, minimums rather than maximums.
e.g. a CDP provides at least 1500, and as much as 5000.  So it seems
that the wm831x was designed to be told the minimum guaranteed available.
But that is circumstantial evidence and might be misleading.

NeilBrown

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

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

[NeilBrown]

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

On Mon, Sep 12 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Mon, Sep 12, 2016 at 03:27:18PM +0200, NeilBrown wrote:
>> On Mon, Sep 12 2016, Mark Brown wrote:
>
>> > It's no worse than any other board file situation - if someone has that
>> > problem they get to fix it.
>
>> My point is that the present design does not appear to scale beyond a
>> single USB power supply (as if there were two, they would be named in
>> discovery order, which is not reliably stable).
>
> For the practical purposes of people making systems (as opposed to
> upstream where this is likely to get most use) it pretty much is.
> Though quite how many systems have multiple chargers is itself also a
> question.
>
>> Your point is, I think, that when someone actually cares about that lack
>> of scaling, they can fix it.
>
> Yes.
>
>> I am perfectly happy with that approach.  However if the code doesn't
>> scale beyond one charger, it shouldn't pretend that it does.
>> i.e.
>>   Don't have "usb_charger_find_by_name()", just a global "usb_charger"
>>   (or similar).
>>   The first charger to register gets to be the "usb_charger".  The
>>   second one gets an error.
>> I could be quite happy with that sort of interface.
>
> Well, a fairly standard way of extending would be to allow the explicit
> assignment of names to chargers so this'd avoid such churn.

Sure, that might work.  I'm just against a design that obviously cannot
work.


>
>> > The whole point from the point of view of the wm831x driver is that it
>> > just wants something to tell it how much current it's allowed to draw, I
>> > appreciate that doesn't change your analysis of the bit in the middle
>> > but the consumer driver bit seems fine here.
>
>> Yes, the wm831x driver probably does the right thing.
>> Other drivers might want to know not only the minimum they are allowed
>> to draw, but also the maximum they should try even if they are carefully
>> monitoring the voltage.
>> So wm831x is doing the right thing with the wrong interface.  Maybe you
>> can describe that as "fine".
>
> That's not actually 100% clear to me - for what the wm831x is doing it
> probably *does* want the higher limit.  This is a system inflow limit
> (as it should be for this), at least the charger will adapt to voltage
> variations though other users in the system are much less likely to do
> so.

Interesting ... I hadn't considered that possibility.

As long as the current remains below the maximum, the charger will
reduce the voltage towards 2V as load increases.  Somewhere before it
gets there, the system will not be able to make use of the power as the
voltage will be too low to be usable. So that will naturally limit the
current being drawn.

Not having very much electrical engineering background, I cannot say for
sure what will happen, but it seems likely that once the voltage drops
much below 4.75V, the charger won't be operating at peak efficiency,
which would be a waste.
I can easily imagine that the hardware would switch off at some voltage
level, rather than just making do with what is there.
So I'm skeptical of this approach, but I'm open to being corrected by
someone more knowledgeable than I.

Looking at it from a different perspective, according to the patch set,
the limits that wm831x is able to impose are:

 +	0,
 +	2,
 +	100,
 +	500,
 +	900,
 +	1500,
 +	1800,
 +	550,

These are, from the battery charger spec, minimums rather than maximums.
e.g. a CDP provides at least 1500, and as much as 5000.  So it seems
that the wm831x was designed to be told the minimum guaranteed available.
But that is circumstantial evidence and might be misleading.

NeilBrown

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

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

[Mark Brown]

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

On Tue, Sep 13, 2016 at 10:00:28AM +0200, NeilBrown wrote:
> On Mon, Sep 12 2016, Mark Brown wrote:

> > That's not actually 100% clear to me - for what the wm831x is doing it
> > probably *does* want the higher limit.  This is a system inflow limit
> > (as it should be for this), at least the charger will adapt to voltage
> > variations though other users in the system are much less likely to do
> > so.

> Not having very much electrical engineering background, I cannot say for
> sure what will happen, but it seems likely that once the voltage drops
> much below 4.75V, the charger won't be operating at peak efficiency,
> which would be a waste.
> I can easily imagine that the hardware would switch off at some voltage
> level, rather than just making do with what is there.
> So I'm skeptical of this approach, but I'm open to being corrected by
> someone more knowledgeable than I.

Yes, the idea is that the charger will back off charging and stop
entirely if the rest of the system is consuming too much power to allow
it to continue effectively.  The same thing happens with wall power, if
a wall supply isn't able to power the charger (eg, because the rest of
the system is running flat out) it'll have to cope with that.

> Looking at it from a different perspective, according to the patch set,
> the limits that wm831x is able to impose are:

>  +	0,
>  +	2,
>  +	100,
>  +	500,
>  +	900,
>  +	1500,
>  +	1800,
>  +	550,

> These are, from the battery charger spec, minimums rather than maximums.
> e.g. a CDP provides at least 1500, and as much as 5000.  So it seems
> that the wm831x was designed to be told the minimum guaranteed available.
> But that is circumstantial evidence and might be misleading.

AIUI this is conservatisim in the system design - another way of reading
a spec with a range like this is that the consumer should aim for the
lower limit, the provider should aim for the upper limit and that way if
either of them has issues with tolerances things will still work out.
It predates wide availability of CDP so I wouldn't be surprised if that
bit were just an oversight.  Like I say this bit of hardware is totally
separate to the battery charger.

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

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

[Mark Brown]

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

On Tue, Sep 13, 2016 at 10:00:28AM +0200, NeilBrown wrote:
> On Mon, Sep 12 2016, Mark Brown wrote:

> > That's not actually 100% clear to me - for what the wm831x is doing it
> > probably *does* want the higher limit.  This is a system inflow limit
> > (as it should be for this), at least the charger will adapt to voltage
> > variations though other users in the system are much less likely to do
> > so.

> Not having very much electrical engineering background, I cannot say for
> sure what will happen, but it seems likely that once the voltage drops
> much below 4.75V, the charger won't be operating at peak efficiency,
> which would be a waste.
> I can easily imagine that the hardware would switch off at some voltage
> level, rather than just making do with what is there.
> So I'm skeptical of this approach, but I'm open to being corrected by
> someone more knowledgeable than I.

Yes, the idea is that the charger will back off charging and stop
entirely if the rest of the system is consuming too much power to allow
it to continue effectively.  The same thing happens with wall power, if
a wall supply isn't able to power the charger (eg, because the rest of
the system is running flat out) it'll have to cope with that.

> Looking at it from a different perspective, according to the patch set,
> the limits that wm831x is able to impose are:

>  +	0,
>  +	2,
>  +	100,
>  +	500,
>  +	900,
>  +	1500,
>  +	1800,
>  +	550,

> These are, from the battery charger spec, minimums rather than maximums.
> e.g. a CDP provides at least 1500, and as much as 5000.  So it seems
> that the wm831x was designed to be told the minimum guaranteed available.
> But that is circumstantial evidence and might be misleading.

AIUI this is conservatisim in the system design - another way of reading
a spec with a range like this is that the consumer should aim for the
lower limit, the provider should aim for the upper limit and that way if
either of them has issues with tolerances things will still work out.
It predates wide availability of CDP so I wouldn't be surprised if that
bit were just an oversight.  Like I say this bit of hardware is totally
separate to the battery charger.

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

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

[NeilBrown]

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

On Wed, Sep 14 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Sep 13, 2016 at 10:00:28AM +0200, NeilBrown wrote:
>> On Mon, Sep 12 2016, Mark Brown wrote:
>
>> > That's not actually 100% clear to me - for what the wm831x is doing it
>> > probably *does* want the higher limit.  This is a system inflow limit
>> > (as it should be for this), at least the charger will adapt to voltage
>> > variations though other users in the system are much less likely to do
>> > so.
>
>> Not having very much electrical engineering background, I cannot say for
>> sure what will happen, but it seems likely that once the voltage drops
>> much below 4.75V, the charger won't be operating at peak efficiency,
>> which would be a waste.
>> I can easily imagine that the hardware would switch off at some voltage
>> level, rather than just making do with what is there.
>> So I'm skeptical of this approach, but I'm open to being corrected by
>> someone more knowledgeable than I.
>
> Yes, the idea is that the charger will back off charging and stop
> entirely if the rest of the system is consuming too much power to allow
> it to continue effectively.  The same thing happens with wall power, if
> a wall supply isn't able to power the charger (eg, because the rest of
> the system is running flat out) it'll have to cope with that.

Maybe you are correct.  I don't find your argument convincing, but maybe
that is because I don't want to...
Some facts though:
 1/ I had a report once from someone whose device stopped charging
   because it was pulling more current than the charger could supply.
   The voltage dropped below the 3.5V (I think) that the battery
   charging hardware needed, so it switched off.  It wouldn't switch
   back on again until explicitly told too.  It would then overload the
   charger again and switch off.
   Changing the code to put a lower limit on the current allowed the
   battery to be charged.  So empirical evidence suggests that the
   lower number should be used.

 2/ I hoped that
      Battery Charging Specification
      Revision 1.2
      December 7, 2010

    would say something definite, but I cannot find it.
    However,  "note 1" to "Table 5-2 Currents" says:
     
        1) The maximum current is for safety reasons, as per USB 2.0 section 7.2.1.2.1.
     
    Which seems to say the maximum is just for safety, implying that the
    minimum is the important value.

 3/  Felipe Balbi <balbi@kernel.org>  appears to agree with my
   perspective.
      http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1224904.html
   does argument-by-authority work?


>
>> Looking at it from a different perspective, according to the patch set,
>> the limits that wm831x is able to impose are:
>
>>  +	0,
>>  +	2,
>>  +	100,
>>  +	500,
>>  +	900,
>>  +	1500,
>>  +	1800,
>>  +	550,
>
>> These are, from the battery charger spec, minimums rather than maximums.
>> e.g. a CDP provides at least 1500, and as much as 5000.  So it seems
>> that the wm831x was designed to be told the minimum guaranteed available.
>> But that is circumstantial evidence and might be misleading.
>
> AIUI this is conservatisim in the system design - another way of reading
> a spec with a range like this is that the consumer should aim for the
> lower limit, the provider should aim for the upper limit and that way if
> either of them has issues with tolerances things will still work out.
> It predates wide availability of CDP so I wouldn't be surprised if that
> bit were just an oversight.  Like I say this bit of hardware is totally
> separate to the battery charger.

Your last sentence is interesting .... I would reply "of course".
All the code we are talking about is only tangentially related to battery
charging.  It is about how much current can safely be pulled from a USB
port.  What that current is used for is a completely separate question.

NeilBrown

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

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

[NeilBrown]

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

On Wed, Sep 14 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Sep 13, 2016 at 10:00:28AM +0200, NeilBrown wrote:
>> On Mon, Sep 12 2016, Mark Brown wrote:
>
>> > That's not actually 100% clear to me - for what the wm831x is doing it
>> > probably *does* want the higher limit.  This is a system inflow limit
>> > (as it should be for this), at least the charger will adapt to voltage
>> > variations though other users in the system are much less likely to do
>> > so.
>
>> Not having very much electrical engineering background, I cannot say for
>> sure what will happen, but it seems likely that once the voltage drops
>> much below 4.75V, the charger won't be operating at peak efficiency,
>> which would be a waste.
>> I can easily imagine that the hardware would switch off at some voltage
>> level, rather than just making do with what is there.
>> So I'm skeptical of this approach, but I'm open to being corrected by
>> someone more knowledgeable than I.
>
> Yes, the idea is that the charger will back off charging and stop
> entirely if the rest of the system is consuming too much power to allow
> it to continue effectively.  The same thing happens with wall power, if
> a wall supply isn't able to power the charger (eg, because the rest of
> the system is running flat out) it'll have to cope with that.

Maybe you are correct.  I don't find your argument convincing, but maybe
that is because I don't want to...
Some facts though:
 1/ I had a report once from someone whose device stopped charging
   because it was pulling more current than the charger could supply.
   The voltage dropped below the 3.5V (I think) that the battery
   charging hardware needed, so it switched off.  It wouldn't switch
   back on again until explicitly told too.  It would then overload the
   charger again and switch off.
   Changing the code to put a lower limit on the current allowed the
   battery to be charged.  So empirical evidence suggests that the
   lower number should be used.

 2/ I hoped that
      Battery Charging Specification
      Revision 1.2
      December 7, 2010

    would say something definite, but I cannot find it.
    However,  "note 1" to "Table 5-2 Currents" says:
     
        1) The maximum current is for safety reasons, as per USB 2.0 section 7.2.1.2.1.
     
    Which seems to say the maximum is just for safety, implying that the
    minimum is the important value.

 3/  Felipe Balbi <balbi@kernel.org>  appears to agree with my
   perspective.
      http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1224904.html
   does argument-by-authority work?


>
>> Looking at it from a different perspective, according to the patch set,
>> the limits that wm831x is able to impose are:
>
>>  +	0,
>>  +	2,
>>  +	100,
>>  +	500,
>>  +	900,
>>  +	1500,
>>  +	1800,
>>  +	550,
>
>> These are, from the battery charger spec, minimums rather than maximums.
>> e.g. a CDP provides at least 1500, and as much as 5000.  So it seems
>> that the wm831x was designed to be told the minimum guaranteed available.
>> But that is circumstantial evidence and might be misleading.
>
> AIUI this is conservatisim in the system design - another way of reading
> a spec with a range like this is that the consumer should aim for the
> lower limit, the provider should aim for the upper limit and that way if
> either of them has issues with tolerances things will still work out.
> It predates wide availability of CDP so I wouldn't be surprised if that
> bit were just an oversight.  Like I say this bit of hardware is totally
> separate to the battery charger.

Your last sentence is interesting .... I would reply "of course".
All the code we are talking about is only tangentially related to battery
charging.  It is about how much current can safely be pulled from a USB
port.  What that current is used for is a completely separate question.

NeilBrown

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

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

[Mark Brown]

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

On Wed, Sep 14, 2016 at 04:11:58PM +0200, NeilBrown wrote:
> On Wed, Sep 14 2016, Mark Brown wrote:

> > Yes, the idea is that the charger will back off charging and stop
> > entirely if the rest of the system is consuming too much power to allow
> > it to continue effectively.  The same thing happens with wall power, if
> > a wall supply isn't able to power the charger (eg, because the rest of
> > the system is running flat out) it'll have to cope with that.

> Maybe you are correct.  I don't find your argument convincing, but maybe
> that is because I don't want to...

There's a *huge* variation in how chargers are designed, some are
designed to be dumb and won't function without software while the wm831x
is more at the opposite end of the spectrum and will quite happily run
all the charging and power source selection logic with no software
intervention at all - the parameters it uses can be changed at runtime
but that's about it.  Software implementations are obviously more
flexible but hardware implementations can be more responsive to changes
in system state like drooping supplies and aren't vulnerable to things
like software lockups.

>  1/ I had a report once from someone whose device stopped charging
>  because it was pulling more current than the charger could supply.
>  The voltage dropped below the 3.5V (I think) that the battery
>  charging hardware needed, so it switched off.  It wouldn't switch
>  back on again until explicitly told too.  It would then overload the
>  charger again and switch off.

That's just one charger's algorithm though, other options are available.

>     Which seems to say the maximum is just for safety, implying that the
>     minimum is the important value.

This is what I was saying about a sensible reading being for the supply
and consumer side to directly target the maximum and minimum limits
respectively (though for the battery charger spec it's a bit different
as the range is so wide).

>  3/  Felipe Balbi <balbi@kernel.org>  appears to agree with my
>    perspective.
>       http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1224904.html
>    does argument-by-authority work?

TI do a lot of the more software managed chargers (which I suspect are
the main thing Felipe will have looked at) if that's what you're
referring to here?  The device is implementing pretty much the algorithm
you're describing in that e-mail so I'm a bit confused as to what you're
saying here.

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

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

[Mark Brown]

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

On Wed, Sep 14, 2016 at 04:11:58PM +0200, NeilBrown wrote:
> On Wed, Sep 14 2016, Mark Brown wrote:

> > Yes, the idea is that the charger will back off charging and stop
> > entirely if the rest of the system is consuming too much power to allow
> > it to continue effectively.  The same thing happens with wall power, if
> > a wall supply isn't able to power the charger (eg, because the rest of
> > the system is running flat out) it'll have to cope with that.

> Maybe you are correct.  I don't find your argument convincing, but maybe
> that is because I don't want to...

There's a *huge* variation in how chargers are designed, some are
designed to be dumb and won't function without software while the wm831x
is more at the opposite end of the spectrum and will quite happily run
all the charging and power source selection logic with no software
intervention at all - the parameters it uses can be changed at runtime
but that's about it.  Software implementations are obviously more
flexible but hardware implementations can be more responsive to changes
in system state like drooping supplies and aren't vulnerable to things
like software lockups.

>  1/ I had a report once from someone whose device stopped charging
>  because it was pulling more current than the charger could supply.
>  The voltage dropped below the 3.5V (I think) that the battery
>  charging hardware needed, so it switched off.  It wouldn't switch
>  back on again until explicitly told too.  It would then overload the
>  charger again and switch off.

That's just one charger's algorithm though, other options are available.

>     Which seems to say the maximum is just for safety, implying that the
>     minimum is the important value.

This is what I was saying about a sensible reading being for the supply
and consumer side to directly target the maximum and minimum limits
respectively (though for the battery charger spec it's a bit different
as the range is so wide).

>  3/  Felipe Balbi <balbi@kernel.org>  appears to agree with my
>    perspective.
>       http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1224904.html
>    does argument-by-authority work?

TI do a lot of the more software managed chargers (which I suspect are
the main thing Felipe will have looked at) if that's what you're
referring to here?  The device is implementing pretty much the algorithm
you're describing in that e-mail so I'm a bit confused as to what you're
saying here.

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

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

[NeilBrown]

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

On Wed, Sep 14 2016, Mark Brown wrote:

> TI do a lot of the more software managed chargers (which I suspect are
> the main thing Felipe will have looked at) if that's what you're
> referring to here?  The device is implementing pretty much the algorithm
> you're describing in that e-mail so I'm a bit confused as to what you're
> saying here.

Ah.... my mistake, sorry.
When earlier you said:
>                                                                 It's a
> current limiter intended to sit in line with the USB power lines to
> ensure the system doesn't go over the maximum current draw (and also
> integrates with the power source selection logic the chip has to pick
> the best available power source for the system).

I assumed that all it did was limit the current to number given.
If it also limits the current to ensure that voltage doesn't drop
unduly, then I agree with your assertion that it just needs to be told
the upper limit.
I hope you'll agree that other drivers might need to know the lower
limit, so reporting both to all drivers is sensible.

Thanks,
NeilBrown

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

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

[NeilBrown]

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

On Wed, Sep 14 2016, Mark Brown wrote:

> TI do a lot of the more software managed chargers (which I suspect are
> the main thing Felipe will have looked at) if that's what you're
> referring to here?  The device is implementing pretty much the algorithm
> you're describing in that e-mail so I'm a bit confused as to what you're
> saying here.

Ah.... my mistake, sorry.
When earlier you said:
>                                                                 It's a
> current limiter intended to sit in line with the USB power lines to
> ensure the system doesn't go over the maximum current draw (and also
> integrates with the power source selection logic the chip has to pick
> the best available power source for the system).

I assumed that all it did was limit the current to number given.
If it also limits the current to ensure that voltage doesn't drop
unduly, then I agree with your assertion that it just needs to be told
the upper limit.
I hope you'll agree that other drivers might need to know the lower
limit, so reporting both to all drivers is sensible.

Thanks,
NeilBrown

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

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

[Mark Brown]

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

On Wed, Sep 14, 2016 at 07:50:00PM +0200, NeilBrown wrote:
> On Wed, Sep 14 2016, Mark Brown wrote:

> Ah.... my mistake, sorry.
> When earlier you said:

> >                                                                 It's a
> > current limiter intended to sit in line with the USB power lines to

> I assumed that all it did was limit the current to number given.
> If it also limits the current to ensure that voltage doesn't drop
> unduly, then I agree with your assertion that it just needs to be told
> the upper limit.

Oh, I see the gap here - the USB specific bit is only a current limiter
but it works in concert with other bits of the system that try to stop
the voltage from whatever supply is in use from dropping and can't be
used independently of them.  That's why I wasn't clear in what I said!

> I hope you'll agree that other drivers might need to know the lower
> limit, so reporting both to all drivers is sensible.

Yes, absolutely.

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

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

[Mark Brown]

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

On Wed, Sep 14, 2016 at 07:50:00PM +0200, NeilBrown wrote:
> On Wed, Sep 14 2016, Mark Brown wrote:

> Ah.... my mistake, sorry.
> When earlier you said:

> >                                                                 It's a
> > current limiter intended to sit in line with the USB power lines to

> I assumed that all it did was limit the current to number given.
> If it also limits the current to ensure that voltage doesn't drop
> unduly, then I agree with your assertion that it just needs to be told
> the upper limit.

Oh, I see the gap here - the USB specific bit is only a current limiter
but it works in concert with other bits of the system that try to stop
the voltage from whatever supply is in use from dropping and can't be
used independently of them.  That's why I wasn't clear in what I said!

> I hope you'll agree that other drivers might need to know the lower
> limit, so reporting both to all drivers is sensible.

Yes, absolutely.

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

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

[Pavel Machek]

Hi!

> > That's not actually 100% clear to me - for what the wm831x is doing it
> > probably *does* want the higher limit.  This is a system inflow limit
> > (as it should be for this), at least the charger will adapt to voltage
> > variations though other users in the system are much less likely to do
> > so.
> 
> Interesting ... I hadn't considered that possibility.
> 
> As long as the current remains below the maximum, the charger will
> reduce the voltage towards 2V as load increases.  Somewhere before it
> gets there, the system will not be able to make use of the power as the
> voltage will be too low to be usable. So that will naturally limit the
> current being drawn.
> 
> Not having very much electrical engineering background, I cannot say for
> sure what will happen, but it seems likely that once the voltage drops
> much below 4.75V, the charger won't be operating at peak efficiency,
> which would be a waste.
> I can easily imagine that the hardware would switch off at some voltage
> level, rather than just making do with what is there.
> So I'm skeptical of this approach, but I'm open to being corrected by
> someone more knowledgeable than I.

Devices I seen charge down to ~4.2V. This is useful thing to play
with:

dx.com: 406496
1" USB Current & Voltage Detector Tester Meter w/ Red LED Display -
Blue

Best regards,
								Pavel

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

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

[Pavel Machek]

Hi!

> > That's not actually 100% clear to me - for what the wm831x is doing it
> > probably *does* want the higher limit.  This is a system inflow limit
> > (as it should be for this), at least the charger will adapt to voltage
> > variations though other users in the system are much less likely to do
> > so.
> 
> Interesting ... I hadn't considered that possibility.
> 
> As long as the current remains below the maximum, the charger will
> reduce the voltage towards 2V as load increases.  Somewhere before it
> gets there, the system will not be able to make use of the power as the
> voltage will be too low to be usable. So that will naturally limit the
> current being drawn.
> 
> Not having very much electrical engineering background, I cannot say for
> sure what will happen, but it seems likely that once the voltage drops
> much below 4.75V, the charger won't be operating at peak efficiency,
> which would be a waste.
> I can easily imagine that the hardware would switch off at some voltage
> level, rather than just making do with what is there.
> So I'm skeptical of this approach, but I'm open to being corrected by
> someone more knowledgeable than I.

Devices I seen charge down to ~4.2V. This is useful thing to play
with:

dx.com: 406496
1" USB Current & Voltage Detector Tester Meter w/ Red LED Display -
Blue

Best regards,
								Pavel

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

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

[Baolin Wang]

Hi Neil,

Sorry for late reply due yo my holiday.

On 10 September 2016 at 05:19, NeilBrown <neilb@suse.com> wrote:
> On Fri, Sep 09 2016, Baolin Wang wrote:
>
>>>
>>> In practice, the USB PHY and the Power manager will often be in the same
>>> IC (the PMIC) so the driver for one could look at the registers for the
>>> other.
>>> But there is no guarantee that the hardware works like that.  It is
>>> best to create a generally design.
>>
>> Yes, we hope to create one generally design, so we need to consider
>> this situation: the power supply getting the charger type by accessing
>> PMIC registers. The registers which save the charger type are not
>> always belong to the USB PHY, may be just some registers on PMIC.
>
> If the power_supply can directly detect the type of charger, then it
> doesn't need any usb-charger-infrastructure to tell it.  It can handle
> current selection entirely internally.
> Surely the only interesting case for a framework to address is the one
> where the power_supply cannot directly detect the charger type.

But power supply also need one plugged or unplugged event to set
current from USB charger framework. Another hand, considering one
generic framework, since power supply support API (e.g.:
power_supply_get_property()) to get charger type for others, we should
integrate power supply into USB charger framework.

>
>>
>> Now in mainline kernel, there are 3 methods can get the charger type
>> which need to integrate with USB charger framework:
>> 1. power supply
>> 2. extcon (need to add as you suggested)
>> 3. others (by 'get_charger_type' callback of USB charger)
>
> There is no "get_charger_type" now in the mainline kernel.

We want to create one generic framework, thus we should consider some
users want to implement the function to get charger type by accessing
PMIC registers or else.

>
>>>>
>>>> Suppose the USB configuration requests 100mA, then we should set the
>>>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
>>>> funtion, then notify this to power driver.
>>>
>>> ahh.... I had missed something there.  It's a while since I looked
>>> closely at these patches.
>>>
>>> Only.... this usage of usb_charger_set_cur_limit_by_type() is really
>>> nonsensical.
>>>
>>> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
>>> the number negotiated with the USB configuration is not relevant and
>>> should be ignored.  There is a guaranteed minimum which is at least the
>>> maximum that *can* be negotiated.
>>
>> Yes. If it is not relevant, we will no't set the current from USB
>> configuration. Just when your charger type is SDP and the USB
>> enumeration is done, we can get the USB configuration from host to set
>> current.
>
> But you do!
> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
> Your patch passes that to usb_charger_set_cur_limit_by_type()
> which calls __usb_charger_set_cur_limit_by_type() which will set the
> cur_limit for whichever type uchger->type currently is.
>
> So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number
from the USB configuration ), it will not do the USB enumeration, then
no USB configuration from host to set current.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Neil,

Sorry for late reply due yo my holiday.

On 10 September 2016 at 05:19, NeilBrown <neilb@suse.com> wrote:
> On Fri, Sep 09 2016, Baolin Wang wrote:
>
>>>
>>> In practice, the USB PHY and the Power manager will often be in the same
>>> IC (the PMIC) so the driver for one could look at the registers for the
>>> other.
>>> But there is no guarantee that the hardware works like that.  It is
>>> best to create a generally design.
>>
>> Yes, we hope to create one generally design, so we need to consider
>> this situation: the power supply getting the charger type by accessing
>> PMIC registers. The registers which save the charger type are not
>> always belong to the USB PHY, may be just some registers on PMIC.
>
> If the power_supply can directly detect the type of charger, then it
> doesn't need any usb-charger-infrastructure to tell it.  It can handle
> current selection entirely internally.
> Surely the only interesting case for a framework to address is the one
> where the power_supply cannot directly detect the charger type.

But power supply also need one plugged or unplugged event to set
current from USB charger framework. Another hand, considering one
generic framework, since power supply support API (e.g.:
power_supply_get_property()) to get charger type for others, we should
integrate power supply into USB charger framework.

>
>>
>> Now in mainline kernel, there are 3 methods can get the charger type
>> which need to integrate with USB charger framework:
>> 1. power supply
>> 2. extcon (need to add as you suggested)
>> 3. others (by 'get_charger_type' callback of USB charger)
>
> There is no "get_charger_type" now in the mainline kernel.

We want to create one generic framework, thus we should consider some
users want to implement the function to get charger type by accessing
PMIC registers or else.

>
>>>>
>>>> Suppose the USB configuration requests 100mA, then we should set the
>>>> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
>>>> funtion, then notify this to power driver.
>>>
>>> ahh.... I had missed something there.  It's a while since I looked
>>> closely at these patches.
>>>
>>> Only.... this usage of usb_charger_set_cur_limit_by_type() is really
>>> nonsensical.
>>>
>>> If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
>>> the number negotiated with the USB configuration is not relevant and
>>> should be ignored.  There is a guaranteed minimum which is at least the
>>> maximum that *can* be negotiated.
>>
>> Yes. If it is not relevant, we will no't set the current from USB
>> configuration. Just when your charger type is SDP and the USB
>> enumeration is done, we can get the USB configuration from host to set
>> current.
>
> But you do!
> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
> Your patch passes that to usb_charger_set_cur_limit_by_type()
> which calls __usb_charger_set_cur_limit_by_type() which will set the
> cur_limit for whichever type uchger->type currently is.
>
> So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number
from the USB configuration ), it will not do the USB enumeration, then
no USB configuration from host to set current.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Felipe,

>> But you do!
>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>> cur_limit for whichever type uchger->type currently is.
>>
>> So when it is not relevant, your code *does* set some current limit.
>
> Suppose the charger type is DCP(it is not relevant to the mA number
> from the USB configuration ), it will not do the USB enumeration, then
> no USB configuration from host to set current.

>From the talking, there are some issues (thanks for Neil's comments)
need to be fixed as below:
1. Need to add the method getting charger type from extcon subsystem.
2. Need to remove the method getting charger type from power supply.
3. There are still some different views about reporting the maximum
current or minimum current to power driver.

Now the current v16 patchset can work well on my Spreadtrum platform
and Jun's NXP platform, if you like to apply this patchset then I can
send out new patches to fix above issues. If you don't like that, I
can send out new version patchset to fix above issues. Could you  give
me some suggestions what should I do next step? Thanks.

>
> --
> Baolin.wang
> Best Regards



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Felipe,

>> But you do!
>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>> cur_limit for whichever type uchger->type currently is.
>>
>> So when it is not relevant, your code *does* set some current limit.
>
> Suppose the charger type is DCP(it is not relevant to the mA number
> from the USB configuration ), it will not do the USB enumeration, then
> no USB configuration from host to set current.

>From the talking, there are some issues (thanks for Neil's comments)
need to be fixed as below:
1. Need to add the method getting charger type from extcon subsystem.
2. Need to remove the method getting charger type from power supply.
3. There are still some different views about reporting the maximum
current or minimum current to power driver.

Now the current v16 patchset can work well on my Spreadtrum platform
and Jun's NXP platform, if you like to apply this patchset then I can
send out new patches to fix above issues. If you don't like that, I
can send out new version patchset to fix above issues. Could you  give
me some suggestions what should I do next step? Thanks.

>
> --
> Baolin.wang
> Best Regards



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi Baolin,

Baolin Wang <baolin.wang@linaro.org> writes:
>>> But you do!
>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>> cur_limit for whichever type uchger->type currently is.
>>>
>>> So when it is not relevant, your code *does* set some current limit.
>>
>> Suppose the charger type is DCP(it is not relevant to the mA number
>> from the USB configuration ), it will not do the USB enumeration, then
>> no USB configuration from host to set current.
>
> From the talking, there are some issues (thanks for Neil's comments)
> need to be fixed as below:
> 1. Need to add the method getting charger type from extcon subsystem.
> 2. Need to remove the method getting charger type from power supply.
> 3. There are still some different views about reporting the maximum
> current or minimum current to power driver.
>
> Now the current v16 patchset can work well on my Spreadtrum platform
> and Jun's NXP platform, if you like to apply this patchset then I can
> send out new patches to fix above issues. If you don't like that, I
> can send out new version patchset to fix above issues. Could you  give
> me some suggestions what should I do next step? Thanks.

Merge window just opened, nothing will happen for about 2 weeks. How
about you send a new version after merge window closes and we go from
there? Fixing 1 and 2 is needed. 3 we need to consider more
carefully. Perhaps report both minimum and maximum somehow?

Neil, comments?

-- 
balbi

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

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

[Felipe Balbi]

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


Hi Baolin,

Baolin Wang <baolin.wang@linaro.org> writes:
>>> But you do!
>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>> cur_limit for whichever type uchger->type currently is.
>>>
>>> So when it is not relevant, your code *does* set some current limit.
>>
>> Suppose the charger type is DCP(it is not relevant to the mA number
>> from the USB configuration ), it will not do the USB enumeration, then
>> no USB configuration from host to set current.
>
> From the talking, there are some issues (thanks for Neil's comments)
> need to be fixed as below:
> 1. Need to add the method getting charger type from extcon subsystem.
> 2. Need to remove the method getting charger type from power supply.
> 3. There are still some different views about reporting the maximum
> current or minimum current to power driver.
>
> Now the current v16 patchset can work well on my Spreadtrum platform
> and Jun's NXP platform, if you like to apply this patchset then I can
> send out new patches to fix above issues. If you don't like that, I
> can send out new version patchset to fix above issues. Could you  give
> me some suggestions what should I do next step? Thanks.

Merge window just opened, nothing will happen for about 2 weeks. How
about you send a new version after merge window closes and we go from
there? Fixing 1 and 2 is needed. 3 we need to consider more
carefully. Perhaps report both minimum and maximum somehow?

Neil, comments?

-- 
balbi

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

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

[Baolin Wang]

Hi Felipe,

On 5 October 2016 at 15:47, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi Baolin,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> But you do!
>>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>>> cur_limit for whichever type uchger->type currently is.
>>>>
>>>> So when it is not relevant, your code *does* set some current limit.
>>>
>>> Suppose the charger type is DCP(it is not relevant to the mA number
>>> from the USB configuration ), it will not do the USB enumeration, then
>>> no USB configuration from host to set current.
>>
>> From the talking, there are some issues (thanks for Neil's comments)
>> need to be fixed as below:
>> 1. Need to add the method getting charger type from extcon subsystem.
>> 2. Need to remove the method getting charger type from power supply.
>> 3. There are still some different views about reporting the maximum
>> current or minimum current to power driver.
>>
>> Now the current v16 patchset can work well on my Spreadtrum platform
>> and Jun's NXP platform, if you like to apply this patchset then I can
>> send out new patches to fix above issues. If you don't like that, I
>> can send out new version patchset to fix above issues. Could you  give
>> me some suggestions what should I do next step? Thanks.
>
> Merge window just opened, nothing will happen for about 2 weeks. How
> about you send a new version after merge window closes and we go from
> there? Fixing 1 and 2 is needed. 3 we need to consider more

Sure. I will send out the new version with fixing these issues. Thanks.

> carefully. Perhaps report both minimum and maximum somehow?
>
> Neil, comments?
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Felipe,

On 5 October 2016 at 15:47, Felipe Balbi <balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Hi Baolin,
>
> Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>>>> But you do!
>>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>>> cur_limit for whichever type uchger->type currently is.
>>>>
>>>> So when it is not relevant, your code *does* set some current limit.
>>>
>>> Suppose the charger type is DCP(it is not relevant to the mA number
>>> from the USB configuration ), it will not do the USB enumeration, then
>>> no USB configuration from host to set current.
>>
>> From the talking, there are some issues (thanks for Neil's comments)
>> need to be fixed as below:
>> 1. Need to add the method getting charger type from extcon subsystem.
>> 2. Need to remove the method getting charger type from power supply.
>> 3. There are still some different views about reporting the maximum
>> current or minimum current to power driver.
>>
>> Now the current v16 patchset can work well on my Spreadtrum platform
>> and Jun's NXP platform, if you like to apply this patchset then I can
>> send out new patches to fix above issues. If you don't like that, I
>> can send out new version patchset to fix above issues. Could you  give
>> me some suggestions what should I do next step? Thanks.
>
> Merge window just opened, nothing will happen for about 2 weeks. How
> about you send a new version after merge window closes and we go from
> there? Fixing 1 and 2 is needed. 3 we need to consider more

Sure. I will send out the new version with fixing these issues. Thanks.

> carefully. Perhaps report both minimum and maximum somehow?
>
> Neil, comments?
>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[NeilBrown]

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

On Wed, Oct 05 2016, Felipe Balbi wrote:

> Hi Baolin,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> But you do!
>>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>>> cur_limit for whichever type uchger->type currently is.
>>>>
>>>> So when it is not relevant, your code *does* set some current limit.
>>>
>>> Suppose the charger type is DCP(it is not relevant to the mA number
>>> from the USB configuration ), it will not do the USB enumeration, then
>>> no USB configuration from host to set current.
>>
>> From the talking, there are some issues (thanks for Neil's comments)
>> need to be fixed as below:
>> 1. Need to add the method getting charger type from extcon subsystem.
>> 2. Need to remove the method getting charger type from power supply.
>> 3. There are still some different views about reporting the maximum
>> current or minimum current to power driver.
>>
>> Now the current v16 patchset can work well on my Spreadtrum platform
>> and Jun's NXP platform, if you like to apply this patchset then I can

I'm really curious how much testing this has had.  Have you actually
plugged in different cable types (SDP DCP DCP ACA) and has each one been
detected correctly?  Because I cannot see how that could happen with the
code you have posted.

>> send out new patches to fix above issues. If you don't like that, I
>> can send out new version patchset to fix above issues. Could you  give
>> me some suggestions what should I do next step? Thanks.
>
> Merge window just opened, nothing will happen for about 2 weeks. How
> about you send a new version after merge window closes and we go from
> there? Fixing 1 and 2 is needed. 3 we need to consider more
> carefully. Perhaps report both minimum and maximum somehow?
>
> Neil, comments?

This probably seems a bit harsh, but I really think the current patchset
should be discarded and the the project started again with a clear
vision of what is required.  What we currently have is too confused.

To respond to the points:
>> 1. Need to add the method getting charger type from extcon subsystem.

Yes.  This should be the only way to get the charger type.

>> 2. Need to remove the method getting charger type from power supply.

Also need to remove the ->get_charger_type() method as there is no
credible use-case for this.

>> 3. There are still some different views about reporting the maximum
>> current or minimum current to power driver.

I think those were resolved.  There was some confusion over whether a
particular power manager wanted to be told the maximum or the minimum,
but I think both have a clear use case in different hardware.

Also: We don't want another notifier_chain.  The usb_notifier combined
with the extcon notifier are sufficient.  Possibly it would be sensible
to replace the usb notifier with a new new notifier chain, but don't add
something without first cleaning up what is there.

Also: resolve the question of whether it could ever make sense to have
 more than one "usb_charger" in a system.  If it doesn't, make it an
 obvious singleton.  If it does, make it clear how the correct
 usb_charger is chosen.

Also: think very carefully before exposing any details through sysfs.
  Some of the details are already visible, either in sys/class/extcon
  or sys/class/power_supply.  Don't duplicate without good reason.


NeilBrown



>
> -- 
> balbi

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

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

[NeilBrown]

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

On Wed, Oct 05 2016, Felipe Balbi wrote:

> Hi Baolin,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> But you do!
>>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>>> cur_limit for whichever type uchger->type currently is.
>>>>
>>>> So when it is not relevant, your code *does* set some current limit.
>>>
>>> Suppose the charger type is DCP(it is not relevant to the mA number
>>> from the USB configuration ), it will not do the USB enumeration, then
>>> no USB configuration from host to set current.
>>
>> From the talking, there are some issues (thanks for Neil's comments)
>> need to be fixed as below:
>> 1. Need to add the method getting charger type from extcon subsystem.
>> 2. Need to remove the method getting charger type from power supply.
>> 3. There are still some different views about reporting the maximum
>> current or minimum current to power driver.
>>
>> Now the current v16 patchset can work well on my Spreadtrum platform
>> and Jun's NXP platform, if you like to apply this patchset then I can

I'm really curious how much testing this has had.  Have you actually
plugged in different cable types (SDP DCP DCP ACA) and has each one been
detected correctly?  Because I cannot see how that could happen with the
code you have posted.

>> send out new patches to fix above issues. If you don't like that, I
>> can send out new version patchset to fix above issues. Could you  give
>> me some suggestions what should I do next step? Thanks.
>
> Merge window just opened, nothing will happen for about 2 weeks. How
> about you send a new version after merge window closes and we go from
> there? Fixing 1 and 2 is needed. 3 we need to consider more
> carefully. Perhaps report both minimum and maximum somehow?
>
> Neil, comments?

This probably seems a bit harsh, but I really think the current patchset
should be discarded and the the project started again with a clear
vision of what is required.  What we currently have is too confused.

To respond to the points:
>> 1. Need to add the method getting charger type from extcon subsystem.

Yes.  This should be the only way to get the charger type.

>> 2. Need to remove the method getting charger type from power supply.

Also need to remove the ->get_charger_type() method as there is no
credible use-case for this.

>> 3. There are still some different views about reporting the maximum
>> current or minimum current to power driver.

I think those were resolved.  There was some confusion over whether a
particular power manager wanted to be told the maximum or the minimum,
but I think both have a clear use case in different hardware.

Also: We don't want another notifier_chain.  The usb_notifier combined
with the extcon notifier are sufficient.  Possibly it would be sensible
to replace the usb notifier with a new new notifier chain, but don't add
something without first cleaning up what is there.

Also: resolve the question of whether it could ever make sense to have
 more than one "usb_charger" in a system.  If it doesn't, make it an
 obvious singleton.  If it does, make it clear how the correct
 usb_charger is chosen.

Also: think very carefully before exposing any details through sysfs.
  Some of the details are already visible, either in sys/class/extcon
  or sys/class/power_supply.  Don't duplicate without good reason.


NeilBrown



>
> -- 
> balbi

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

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

[Baolin Wang]

Hi Neil,

On 5 October 2016 at 18:44, NeilBrown <neilb@suse.com> wrote:
> On Wed, Oct 05 2016, Felipe Balbi wrote:
>
>> Hi Baolin,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> But you do!
>>>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>>>> cur_limit for whichever type uchger->type currently is.
>>>>>
>>>>> So when it is not relevant, your code *does* set some current limit.
>>>>
>>>> Suppose the charger type is DCP(it is not relevant to the mA number
>>>> from the USB configuration ), it will not do the USB enumeration, then
>>>> no USB configuration from host to set current.
>>>
>>> From the talking, there are some issues (thanks for Neil's comments)
>>> need to be fixed as below:
>>> 1. Need to add the method getting charger type from extcon subsystem.
>>> 2. Need to remove the method getting charger type from power supply.
>>> 3. There are still some different views about reporting the maximum
>>> current or minimum current to power driver.
>>>
>>> Now the current v16 patchset can work well on my Spreadtrum platform
>>> and Jun's NXP platform, if you like to apply this patchset then I can
>
> I'm really curious how much testing this has had.  Have you actually
> plugged in different cable types (SDP DCP DCP ACA) and has each one been
> detected correctly?  Because I cannot see how that could happen with the
> code you have posted.

I transplanted the USB charger framework to our Spreadtrum platform
with implementing the 'get_charger_type' callback to get the charger
type in power driver. Cause we get the charger type from accessing the
PMIC registers not from USB PHY.

>
>>> send out new patches to fix above issues. If you don't like that, I
>>> can send out new version patchset to fix above issues. Could you  give
>>> me some suggestions what should I do next step? Thanks.
>>
>> Merge window just opened, nothing will happen for about 2 weeks. How
>> about you send a new version after merge window closes and we go from
>> there? Fixing 1 and 2 is needed. 3 we need to consider more
>> carefully. Perhaps report both minimum and maximum somehow?
>>
>> Neil, comments?
>
> This probably seems a bit harsh, but I really think the current patchset
> should be discarded and the the project started again with a clear
> vision of what is required.  What we currently have is too confused.

Probably not. Now the USB charger framework tried to integrate all
different charger plugged/unplugged events, and all different charger
type getting methods, then noticed the plugged/unplugged events and
charger current to power driver, which I think that is what USB
charger should really do. Moreover, this patchset is reviewed and
helped by many people (thanks Felipe, Greg, Mark, Peter and Jun), I
really hope I can make it better to upstream.

>
> To respond to the points:
>>> 1. Need to add the method getting charger type from extcon subsystem.
>
> Yes.  This should be the only way to get the charger type.

Not really. Like I said, some platform's charger detection is done by
hardware not USB PHY, thus we can get the charger type from PMIC
hardware registers.

>
>>> 2. Need to remove the method getting charger type from power supply.
>
> Also need to remove the ->get_charger_type() method as there is no
> credible use-case for this.

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.

>
>>> 3. There are still some different views about reporting the maximum
>>> current or minimum current to power driver.
>
> I think those were resolved.  There was some confusion over whether a
> particular power manager wanted to be told the maximum or the minimum,
> but I think both have a clear use case in different hardware.

So, seems I should report both minimum and maximum.

>
> Also: We don't want another notifier_chain.  The usb_notifier combined
> with the extcon notifier are sufficient.  Possibly it would be sensible
> to replace the usb notifier with a new new notifier chain, but don't add
> something without first cleaning up what is there.

USB charger is one virtual device not one actual hardware device, we
should not mess it together with usb_notifier or extcon notifier.

>
> Also: resolve the question of whether it could ever make sense to have
>  more than one "usb_charger" in a system.  If it doesn't, make it an
>  obvious singleton.  If it does, make it clear how the correct
>  usb_charger is chosen.

Usually only one USB charger in one system, I have not seen more than
one charger in a system.

>
> Also: think very carefully before exposing any details through sysfs.
>   Some of the details are already visible, either in sys/class/extcon
>   or sys/class/power_supply.  Don't duplicate without good reason.

I think now the current/state/type attributes are enough, which are
USB chargger needed. Thanks.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Neil,

On 5 October 2016 at 18:44, NeilBrown <neilb@suse.com> wrote:
> On Wed, Oct 05 2016, Felipe Balbi wrote:
>
>> Hi Baolin,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> But you do!
>>>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw.
>>>>> Your patch passes that to usb_charger_set_cur_limit_by_type()
>>>>> which calls __usb_charger_set_cur_limit_by_type() which will set the
>>>>> cur_limit for whichever type uchger->type currently is.
>>>>>
>>>>> So when it is not relevant, your code *does* set some current limit.
>>>>
>>>> Suppose the charger type is DCP(it is not relevant to the mA number
>>>> from the USB configuration ), it will not do the USB enumeration, then
>>>> no USB configuration from host to set current.
>>>
>>> From the talking, there are some issues (thanks for Neil's comments)
>>> need to be fixed as below:
>>> 1. Need to add the method getting charger type from extcon subsystem.
>>> 2. Need to remove the method getting charger type from power supply.
>>> 3. There are still some different views about reporting the maximum
>>> current or minimum current to power driver.
>>>
>>> Now the current v16 patchset can work well on my Spreadtrum platform
>>> and Jun's NXP platform, if you like to apply this patchset then I can
>
> I'm really curious how much testing this has had.  Have you actually
> plugged in different cable types (SDP DCP DCP ACA) and has each one been
> detected correctly?  Because I cannot see how that could happen with the
> code you have posted.

I transplanted the USB charger framework to our Spreadtrum platform
with implementing the 'get_charger_type' callback to get the charger
type in power driver. Cause we get the charger type from accessing the
PMIC registers not from USB PHY.

>
>>> send out new patches to fix above issues. If you don't like that, I
>>> can send out new version patchset to fix above issues. Could you  give
>>> me some suggestions what should I do next step? Thanks.
>>
>> Merge window just opened, nothing will happen for about 2 weeks. How
>> about you send a new version after merge window closes and we go from
>> there? Fixing 1 and 2 is needed. 3 we need to consider more
>> carefully. Perhaps report both minimum and maximum somehow?
>>
>> Neil, comments?
>
> This probably seems a bit harsh, but I really think the current patchset
> should be discarded and the the project started again with a clear
> vision of what is required.  What we currently have is too confused.

Probably not. Now the USB charger framework tried to integrate all
different charger plugged/unplugged events, and all different charger
type getting methods, then noticed the plugged/unplugged events and
charger current to power driver, which I think that is what USB
charger should really do. Moreover, this patchset is reviewed and
helped by many people (thanks Felipe, Greg, Mark, Peter and Jun), I
really hope I can make it better to upstream.

>
> To respond to the points:
>>> 1. Need to add the method getting charger type from extcon subsystem.
>
> Yes.  This should be the only way to get the charger type.

Not really. Like I said, some platform's charger detection is done by
hardware not USB PHY, thus we can get the charger type from PMIC
hardware registers.

>
>>> 2. Need to remove the method getting charger type from power supply.
>
> Also need to remove the ->get_charger_type() method as there is no
> credible use-case for this.

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.

>
>>> 3. There are still some different views about reporting the maximum
>>> current or minimum current to power driver.
>
> I think those were resolved.  There was some confusion over whether a
> particular power manager wanted to be told the maximum or the minimum,
> but I think both have a clear use case in different hardware.

So, seems I should report both minimum and maximum.

>
> Also: We don't want another notifier_chain.  The usb_notifier combined
> with the extcon notifier are sufficient.  Possibly it would be sensible
> to replace the usb notifier with a new new notifier chain, but don't add
> something without first cleaning up what is there.

USB charger is one virtual device not one actual hardware device, we
should not mess it together with usb_notifier or extcon notifier.

>
> Also: resolve the question of whether it could ever make sense to have
>  more than one "usb_charger" in a system.  If it doesn't, make it an
>  obvious singleton.  If it does, make it clear how the correct
>  usb_charger is chosen.

Usually only one USB charger in one system, I have not seen more than
one charger in a system.

>
> Also: think very carefully before exposing any details through sysfs.
>   Some of the details are already visible, either in sys/class/extcon
>   or sys/class/power_supply.  Don't duplicate without good reason.

I think now the current/state/type attributes are enough, which are
USB chargger needed. Thanks.

-- 
Baolin.wang
Best Regards